fixes #11920 raise a clean Python error on DSA signing failure due to nilpotent - #11921
Conversation
|
@davidben question for you (since this testcase is from BoringSSL), this isn't a case of key invalidity that should be caught on key load, right? Catching it on signing is the only thing that makes sense? (That's what BoringSSL appears to do, so I assume its correct.) |
1a000fd to
b396b02
Compare
|
@hannob it looks like OpenSSL 3.0 just hangs on these :-( I'm not sure what the right solution is there. Probably OpenSSL ought to backport the fix, unsure if it's CVE worthy... |
|
Ok, looks like the fix was backported in openssl/openssl@5a53d73, but never made its way into Ubunut. Great. |
|
Actually... I reported this years ago to OpenSSL as a security issue, and initially, they didn't consider this fix-worthy at all. They said that malformed private keys aren't part of their threat-model. |
|
I'm sure all the CDNs that accept keys from their users love that. |
Well, to say whether something is "correct" presumes a baseline level of well-definedness and importance that DSA does not have. 😄 First, it is not an invalid DSA key per se, but an invalid DSA group. NIST calls them "domain parameters". It's like picking arbitrary coefficients for an ECC key, and then finding that those coefficients do not meet the mathematical preconditions of any of the algorithms and things go into space. Now, what is the correct handling for invalid domain parameters? Well, the correct way to handle domain parameters is to agree on values ahead of time so that, when it comes down to code, they're hard-coded in the system anyway. Unlike key material, domain parameters are not meant to vary, and they often have preconditions that are expensive to check in key import or use (e.g. primality). But that's not how DSA ended up being used. People treated the domain parameters as part of the key, even though the expectations on the preconditons are very different. And so now we have to worry about how all our algorithms behave when their preconditions fail. Here, when p is composite, the Z/pZ "field" (it's not a field) has nontrivial nilpotent elements. Pick one of those as the generator and g^k will almost always be zero. DSA signing will then not succeed. So, what do you do? Well, we pretended it was part of the key, so ideally we'd check it as part of key import, not signing. But preconditions for domain parameters are often hard to check, and correct use of DSA would only use trusted domain parameters, so it's expensive. The other option is to say, well, if you pass invalid domain parameters, we're not promising any kind of cryptographic soundness (because none of the preconditions for the system hold), or much of anything. Just that the function won't infinite loop or hit language-level UB or something. It will cleanly output something garbage, or cleanly fail. This is a mess, and if DSA mattered, it would perhaps be worth a whole process to revamp the API, migrate from arbitrary parameters to a set of named, pre-defined parameters. But DSA is only decreasing in relevance, so we just went with the second option and added an iteration limit. (Oh, and then on top of all these, there's the mess around when you check preconditions for keys in the first place because OpenSSL's APIs are not very good about this. I think we've talked about this a bit with |
|
Well, I am once again wishing we would just drop DSA. OpenSSL also does not
expose a `DSA_check_key` function at all, so that's off the table.
Interestingly, LibreSSL does reject these on load. Which is nice for them.
Which leaves us with the actual relevant choice: Do we just ignore this for
a while, do we simply skip these tests on OpenSSL from Ubuntu, or do we try
to get the Ubuntu fix backported?
…On Fri, Nov 8, 2024 at 12:26 PM David Benjamin ***@***.***> wrote:
@davidben <https://github.com/davidben> question for you (since this
testcase is from BoringSSL), this isn't a case of key invalidity that
should be caught on key load, right? Catching it on signing is the only
thing that makes sense? (That's what BoringSSL appears to do, so I assume
its correct.)
Well, to say whether something is "correct" presumes a baseline level of
well-definedness and importance that DSA does not have. 😄
First, it is not an invalid DSA *key* per se, but an invalid DSA *group*.
NIST calls them "domain parameters". It's like picking arbitrary
coefficients for an ECC key, and then finding that those coefficients do
not meet the mathematical preconditions of any of the algorithms and things
go into space <https://openssl-library.org/news/secadv/20220315.txt>.
Now, what is the correct handling for invalid domain parameters? Well, the
*correct* way to handle domain parameters is to agree on values ahead of
time so that, when it comes down to code, they're hard-coded in the system
anyway. Unlike key material, domain parameters are not meant to vary, and
they often have preconditions that are expensive to check in key import or
use (e.g. primality).
But that's not how DSA ended up being used. People treated the domain
parameters as part of the key, even though the expectations on the
preconditons are very different. And so now we have to worry about how all
our algorithms behave when their preconditions fail. Here, when p is
composite, the Z/pZ "field" (it's not a field) has nontrivial nilpotent
elements. Pick one of those as the generator and g^k will almost always be
zero. DSA signing will then not succeed.
So, what do you do? Well, we pretended it was part of the key, so ideally
we'd check it as part of key import, not signing. But preconditions for
domain parameters are often hard to check, and correct use of DSA would
only use trusted domain parameters, so it's expensive. The other option is
to say, well, if you pass invalid domain parameters, we're not promising
any kind of cryptographic soundness (because none of the preconditions for
the system hold), or much of anything. Just that the function won't
infinite loop or hit language-level UB or something. It will cleanly output
*something* garbage, or cleanly fail.
This is a mess, and if DSA mattered, it would perhaps be worth a whole
process to revamp the API, migrate from arbitrary parameters to a set of
named, pre-defined parameters. But DSA is only decreasing in relevance, so
we just went with the second option and added an iteration limit.
(Oh, and then on top of all these, there's the mess around when you check
preconditions for keys in the first place because OpenSSL's APIs are not
very good about this. I think we've talked about this a bit with
RSA_check_key and such. But at least importing from a file does give
*some* place for the library to assemble all this. For RSA, I have plans
to rework things in BoringSSL to make that better. DSA... well, DSA is not
really relevant.)
—
Reply to this email directly, view it on GitHub
<#11921 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBFG2QX3XDUSWGEA2OLZ7TX3TAVCNFSM6AAAAABRNUBNC6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRVGM3DANRQGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
|
Yeah, I think LibreSSL opted to stick a primality check in there. I take it you are testing against older OpenSSLs that still infinite loop? (Nitpick: The fix wasn't specifically for nilpotent "generators". It was to cap the number of iterations and eventually give up. It was a while ago, so I forget if we ended up finding other types of invalid inputs that also tickled this.) |
|
Yeah, specifically the version of OpenSSL 3.0 that's in Ubuntu 22.04
does not carry the patch for this.
…On Fri, Nov 8, 2024 at 2:46 PM David Benjamin ***@***.***> wrote:
Yeah, I think LibreSSL opted to stick a primality check in there. I take it you are testing against older OpenSSLs that still infinite loop?
(Nitpick: The fix wasn't specifically for nilpotent "generators". It was to cap the number of iterations and eventually give up. It was a while ago, so I forget if we ended up finding other types of invalid inputs that also tickled this.)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
…e to nilpotent
4 participants
No description provided.