Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Distinguish ECDSA malleability? #70

Open
gmaxwell opened this issue Apr 12, 2019 · 12 comments
Open

Distinguish ECDSA malleability? #70

gmaxwell opened this issue Apr 12, 2019 · 12 comments

Comments

@gmaxwell
Copy link

Libsecp256k1 implements a variant of ECDSA used by the Bitcoin system (and many other things now) which eliminates malleability by requiring s to be in the lower half of its range. Callers that need compatible (but vulnerable...) behaviour are instructed by the documentation to call a provided normalize function on signatures before validating them.

Someone opened an issue reporting that secp256k1 ecdsa sha256 test case 218 fails. I confirmed that this failure is due exclusively to this restriction.

You can expect other secp256k1 implementations to implement similar things as this behaviour is required for consensus consistency with Bitcoin, so it might be useful to avoid gratuitously triggering this incompatibility, and flag tests that necessarily do so (e.g. because they are intentionally trying to use high S values).

Thanks for the interesting work. Although we have very extensive testing already, I think a diverse test cases created by a different set of minds will be a valuable addition to our testing arsenal.

You may be interested in a different test strategy, implemented by our "exhaustive" tests based on the observation the virtually all code in an ECC library is independent of b in the curve equation (usually only point decompression/validation depends on it). We use this fact to allow a test harness to build an alternative version of the library that operates using a different curve equation on a small sgroup (e.g. of size 43) where we can do things like exhaustively test group operations. We've shown this test to be effective at detecting some real but otherwise incredibly hard to detect bugs, such as cases of incorrectly handled group law incompleteness. As a result it might be interesting to publish vectors for these easily tested degenerate groups.

@bleichen
Copy link
Contributor

Is there an RFC or other documentation defining the requirements?

I.e., instead of trying to mix schemes, I'd rather generate separate test vectors for
"Bitcoin-ECDSA". If the signatures are DER encoded then I'd also expect that
all BER alternatives must be rejected.

Almost all of the libraries that I'm testing use special case code for the main
curves. Hence test vectors with special cases need to be included for all the curves.
In a lot of cases, I generate such test vectors by starting with the
edge case point addition and then compute corresponding keys and signatures.
I'll look into adding test with degenerate curves for other cases.

@real-or-random
Copy link

real-or-random commented Apr 17, 2019

There are two restrictions enforced by libsecp256k1:

edit: Maybe "Bitcoin-ECDSA" isn't the best name for it, because this is a tricky story. There is a discrepancy between what signatures are allowed in the Bitcoin blockchain, what signatures will be relayed in the P2P network by different implementations of Bitcoin and what signatures will be produced by those different implementations. (All due to historical reasons because OpenSSL was not strict about the things mentioned above). I'd suggest calling it "libsecp256k1-ECDSA" or something.

@bleichen
Copy link
Contributor

bleichen commented Apr 18, 2019 via email

@real-or-random
Copy link

Two minor notes:

Funnily, #65 reports that wycheproof helped to discover a signature malleability due to high S values in EdDSA.

I had edited my previous comment to add a paragraph about naming. I assume you missed that paragraph (because you replied via email.)

@gmaxwell
Copy link
Author

No objection to calling it whatever you like, but you should be aware that the considerations extent outside Bitcoin, e.g. OpenSSL certificate blacklisting is vulnerable due to this malleability (I can take a valid ECDSA using certificate and make another one which is also valid but has a different hash). So it would be reasonable to expect totally bitcoin unrelated systems to adopt an equivalent countermeasure -- though potentially they might adopt a different tiebreaker, there are several options. We favoured this one because it was appeared the simplest for calling software to implement as a wrapper around a weaker signer/verifier.

@briansmith
Copy link

I can take a valid ECDSA using certificate and make another one which is also valid but has a different hash

There are multiple ways of doing that. Blacklisting certificates that way doesn't work in general, and doing that should be considered a bug with potentially serious security consequences.

@gmaxwell
Copy link
Author

gmaxwell commented May 31, 2019

Maybe, but OpenSSL issued a CVE and a fix for evasion of hash based blacklisted based on using BER extensions in signatures: CVE-2014-8275. Use of ECDSA malleability appears to be an almost equally powerful way to exploit the same vulnerability, it's only weaker in that there are only two possible hashes.

@real-or-random
Copy link

Thanks for the pointers. I'm generating a separate file with test vectors for "Bitcoin-ECDSA". This makes it easier to specify how an implementation should behave.

Just a friendly reminder. Has there been progress on this?

@bleichen
Copy link
Contributor

bleichen commented Oct 18, 2021 via email

@real-or-random
Copy link

Thanks, that's nice to hear! I think then libsecp256k1 would be the first thing to test against. Maybe bitcoin-core/secp256k1#609 helps as a starter.

Or I believe we could also help by providing an integration here. From what I understand, this repo targets Java but the test vectors were used also for C projects. How did other C libraries use the test vectors in practice?

@real-or-random
Copy link

Is there any update on this?

@real-or-random
Copy link

I believe this has been solved by fcee28b, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants