Fix parsing EC private keys with EC parameters #535
Conversation
The logic used with OpenSSL 1.1.1 allows for parsing private keys with EC parameters in the PEM format. The OpenSSL 3.0 logic doesn't allow for this and breaks loading these types of private keys. The parsing loop here is changed to continue parsing PEM blocks also if it succeeds to parse a PEM block, like for EC parameters. We want to ensure that all valid PEM blocks are loaded, not only the first one. Combined with it also trying to parse non valid PEM bits, it means we always want to continue parsing until we hit EOF on the BIO, or if we no longer see progress. These two checks were already implemented, so all that's needed is to unconditionally try to grab all PEM blocks.
|
I realized there's also another bug. If the data is only the EC parameters, we don't trigger a parser error since the private key will be a partial filed structure with the type setup correctly, but no public nor private key data is then present. So it returns an unusable I think |
It's possible that an EC key is parsed with just parameters, but it's not usable at that point as a valid public or private key. So we error in that case for now.
I've looked more at this and it only seems possible with EC keys, so I added a validation check. But not sure if that's desired, so also fine to drop that if that's better then. |
|
Looks like #527 is somewhat related, as that would provide easier ways of checking if it's a valid public or private key compared to what is done here. |
|
Interesting. Is it a commonly used format? I didn't think of a case where the input contains two decodable PEM blocks into OpenSSL::PKey::PKey. (Note that in the example input, Technically speaking, the result of decoding It is expected itself that ruby/openssl now decodes |
|
The current change in #527 appears to revert the behavior, as it will scan input string repeatedly attempting to decode it as private key -> public key -> parameters, in order, but this is more of a side-effect. I had no choice due to the lack of openssl/openssl#9467 in OpenSSL 3.0. |
I think it's fairly standard, the simplest I imagine people do regularly put that in a file then and expect it to work when it's being loaded. It also does work with the OpenSSL 1.1.1 implementation. Our problem is that we deal with user provided keys in this situation so handling different formats including a format with EC parameters makes things more robust. Otherwise we'd have to filter out EC parameters manually before we pass it to Ruby OpenSSL for systems on OpenSSL 3.0. |
Ah, if that also (somewhat accidentally) fixes the behavior, I'm all for that fix. Probably still good to add a test for this to avoid a future regression then? |
Hmm. I didn't know about this behavior. This seems odd to me and it is even inconsistent with the DSA counterpart I agree the format should work as it did in previous versions of ruby/openssl. |
Scan through the input for a private key, then fallback to generic decoder. OpenSSL 3.0's OSSL_DECODER supports encoded key parameters. The PEM header "-----BEGIN EC PARAMETERS-----" is used by one of such encoding formats. While this is useful for OpenSSL::PKey::PKey, an edge case has been discovered. The openssl CLI command line "openssl ecparam -genkey" prints two PEM blocks in a row, one for EC parameters and another for the private key. Feeding the whole output into OSSL_DECODER results in only the first PEM block, the key parameters, being decoded. Previously, ruby/openssl did not support decoding key parameters and it would decode the private key PEM block instead. While the new behavior is technically correct, "openssl ecparam -genkey" is so widely used that ruby/openssl does not want to break existing applications. Fixes ruby#535
|
Closing in favor of #540 |
…enkey" output Scan through the input for a private key, then fallback to generic decoder. OpenSSL 3.0's OSSL_DECODER supports encoded key parameters. The PEM header "-----BEGIN EC PARAMETERS-----" is used by one of such encoding formats. While this is useful for OpenSSL::PKey::PKey, an edge case has been discovered. The openssl CLI command line "openssl ecparam -genkey" prints two PEM blocks in a row, one for EC parameters and another for the private key. Feeding the whole output into OSSL_DECODER results in only the first PEM block, the key parameters, being decoded. Previously, ruby/openssl did not support decoding key parameters and it would decode the private key PEM block instead. While the new behavior is technically correct, "openssl ecparam -genkey" is so widely used that ruby/openssl does not want to break existing applications. Fixes ruby/openssl#535 ruby/openssl@d486c82833
…enkey" output Scan through the input for a private key, then fallback to generic decoder. OpenSSL 3.0's OSSL_DECODER supports encoded key parameters. The PEM header "-----BEGIN EC PARAMETERS-----" is used by one of such encoding formats. While this is useful for OpenSSL::PKey::PKey, an edge case has been discovered. The openssl CLI command line "openssl ecparam -genkey" prints two PEM blocks in a row, one for EC parameters and another for the private key. Feeding the whole output into OSSL_DECODER results in only the first PEM block, the key parameters, being decoded. Previously, ruby/openssl did not support decoding key parameters and it would decode the private key PEM block instead. While the new behavior is technically correct, "openssl ecparam -genkey" is so widely used that ruby/openssl does not want to break existing applications. Fixes ruby/openssl#535 ruby/openssl@d486c82833
The logic used with OpenSSL 1.1.1 allows for parsing private keys with EC parameters in the PEM format. The OpenSSL 3.0 logic doesn't allow for this and breaks loading these types of private keys.
The parsing loop here is changed to continue parsing PEM blocks also if it succeeds to parse a PEM block, like for EC parameters. We want to ensure that all valid PEM blocks are loaded, not only the first one.
Combined with it also trying to parse non valid PEM bits, it means we always want to continue parsing until we hit EOF on the BIO, or if we no longer see progress. These two checks were already implemented, so all that's needed is to unconditionally try to grab all PEM blocks.