Created attachment 3183 [details] proposed patch Some encrypted PEM keys stopped working from OpenSSH with the new OpenSSL 1.1.0i, while they still can be used from OpenSSL. The example key is attached to the Red Hat bugzilla [1]. After some analysis done by Tomas, there is a change in OpenSSL that supports zero-length passprahse, that are passed by default to the PEM decryption methods. In a rare case, the padding is decrypted successfully with this passprahse, the garbage is passed further which results in fail in OpenSSH. The correct solution is to create a password callback that returns -1 to overwrite the default passphrase callback as attached in the patch. With this patch, the ssh tools correctly ask for the passphrase, rather than failing with invalid key error. For more information, see the attached bugzilla [1]. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1632902
Created attachment 3184 [details] patch v2 Ok, the solution is a bit more complicated. We need to shield also the zero-lenght passphrases and otherwise pass the passprase from userdata. With the attached patch I am able to decrypt existing keys as well as these broken ones without any issues.
Why is this not being considered a bug in OpenSSL?
Because zero length encryption keys are technically possible and so it is not a bug to try to decrypt encrypted data with zero length keys. Basically openssh here depended on undocumented behavior of openssl and this patch makes it not to depend on it anymore.
It might be undocumented behaviour of OpenSSL (given the state of the OpenSSL documentation, especially historically, most behaviours are), but it's been a stable one for >20 years and is demonstrably being depended upon by consumers of the API.
I do not disagree that such change on a stable release cannot be seen as functionality change breaking API/ABI. On the other hand if such change was done on release which does not promise to keep the API/ABI stable it would definitely be acceptable anyway. So I do not see why this patch should not be applied regardless of whether the openssl change in patchlevel release is OK or not.
FWIW I don't see the new OpenSSL behaviour documented either. Are you sure it's an intentioned change? I'm sure OpenSSH isn't the only application that depended on the old behaviour. Wouldn't it be more sensible for OpenSSL to deal with zero-length passwords internally; by checking for a plaintext key first and then attempting to decrypt with a zero-length password?
What OpenSSH does is very specific - it depends on returning some particular error codes when zero length password is passed to OpenSSL key loading function to distinguish between invalid keys vs. encrypted keys. I would be very surprised this would be done elsewhere unless they actually copied the key loading logic from OpenSSH. Anyway I can report to OpenSSL that this change introduced OpenSSH regression and see what they have to say about it.
Btw. this is the issue that triggered this change in OpenSSL: https://github.com/openssl/openssl/issues/4716
OpenSSL issue: https://github.com/openssl/openssl/issues/7355
Does the patch attached to bug 2901 avoid this?
(In reply to Damien Miller from comment #10) > Does the patch attached to bug 2901 avoid this? You can try that yourself with the key attached to the bug referenced in the bug description. It does not solve the problem. Running the same test with your patch gives to following: $ SSH_AUTH_SOCK= ./ssh -i invalid_key -F /dev/null localhost error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag error:0907B00D:PEM routines:PEM_read_bio_PrivateKey:ASN1 lib Load key "invalid_key": invalid format The reason why is that had_passphrase is 0 (passphrase is zero-length) and therefore we return SSH_ERR_INVALID_FORMAT from the translate_libcrypto_error().
Created attachment 3190 [details] tweaked patch; avoid -Wsign-compare problem
(In reply to Damien Miller from comment #12) > Created attachment 3190 [details] > tweaked patch; avoid -Wsign-compare problem This looks like wrong patch (this one is from #2912).
Created attachment 3192 [details] correct diff
This has been committed and will be in OpenSSH 7.9
closing resolved bugs as of 8.6p1 release