DsaTest.testModifiedSignatures doesn't report errors

[bbc2]

DsaTest.testModifiedSignatures calls testVectors (source) with isValidDER: false and isValidBER: true. In such a case, errors++ will only be called if verifier.verify raises an exception different from SignatureException. As a result, non-DER signatures that are accepted by the provider (Sun or BouncyCastle) are not reported by this Wycheproof test.

For instance, the last signature in MODIFIED_SIGNATURES (source) seems to still be accepted as valid by the Sun provider on Java 1.8.0u162.

Can you confirm this? It looks like the if-else structure in testVectors (source) could be extended to account for those non-DER signatures that are accepted and verified by the provider.

[thaidn]

Thanks for your bug report.

Below is a reply from bleichen who can't access GitHub for now.

The signature that is accepted by the SUN provider is a legacy signature.
The provider is deliberately accepting the signature to be compatible with
some legacy implementation that formatted DSA signatures incorrectly.
I don't know what this legacy implementation is or whether it is still in use.
Since it is possible to generate a valid signature from the invalid one
by fixing the ASN format this does not introduce a vulnerability.

We are currently working on switching the tests to a new version that
is using a JSON file for the test vectors. Each test vector has a status
that is either "valid", "invalid" or "acceptable". The expectation is that
a library accepts all signatures that are marked as "valid", rejects
all signatures marked as "invalid" and is free to accept or reject
signatures marked as "acceptable". The signature discussed above
is marked as "acceptable".

BTW, it still makes sense to test vectors marked as "acceptable", because
they should of course not crash an implementation. For Java implementations
we additionally expect for example that verify either returns a boolean or
throws a SignatureException. E.g., so that other exceptions are a clear sign
that there is a bug in the code or configuration.

[bleichen]

Sorry for the late reply.

It is probably possible to do some stricter checking now. When I wrote the first tests there
were too many libraries that did not care about ASN parsing, and so BER vs DER issues
had to be ignored just so that the bugs did not pass unnoticed.
I'm also working on documenting the test vectors better, so that it should be at least
possible to ignore test vectors for those cases where an implementer disagrees.

[bbc2]

Thanks for the explanation. That makes sense so I think the issue is solved for me. It would still be useful if, in the future, those accepted modified signatures could optionally be reported (like for avoiding malleability issues) but the tests are already very useful as they are.

[bleichen]

We are working on such a scheme. Though the details are still sketchy. The test vectors will be generally in JSON format, since that makes it easier to use them for other languages than just Java. These test vectors include comments and additionally a list of flags. There will be flags for BER encodings that are not DER or signature malleability. If it is easy enough to silence flaws that are not exploitable then it might also be possible to set stricter defaults.

…

On Wed, Mar 7, 2018 at 9:29 PM, Bertrand Bonnefoy-Claudet < ***@***.***> wrote: Thanks for the explanation. That makes sense so I think the issue is solved for me. It would still be useful if, in the future, those accepted modified signatures could optionally be reported (like for avoiding malleability issues) but the tests are already very useful as they are. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#44 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYx_3r0CLt96RiQhapWnKz4NVU8za-2Lks5tcEMUgaJpZM4SCIH1> .

[thaidn]

@bbc2 check out the flags in https://github.com/google/wycheproof/blob/master/testvectors/ecdsa_secp256r1_sha256_test.json and other JSON files.

There might be future changes, but I guess this should enough if you want to skip certain malleability tests.

Please reopen if you have any further comments or questions.