gh-100372: Use BIO_eof to detect EOF for SSL_FILETYPE_ASN1#100373
Conversation
In PEM, we need to parse until error and then suppress PEM_R_NO_START_LINE, because PEM allows arbitrary leading and trailing data. DER, however, does not. Parsing until error and suppressing ASN1_R_HEADER_TOO_LONG doesn't quite work because that error also covers some cases that should be rejected. Instead, check BIO_eof early and stop the loop that way.
|
@tiran This PR look reasonable? Anything missing on my end? |
|
I don't think this is a security issue, or at least not serious enough to backport to security-only releases. Do you disagree, @davidben, or is there a security angle I'm missing? I'm not sure if this should be backported to 3.11/3.10 either. It's a bug, but it doesn't feel important enough to backport and risk breaking users who rely on the old broken behaviour. |
|
Status check is done, and it's a success ✅. |
Nah, can't think of any security angle. Just generally improving behavior and reducing dependency on OpenSSL error codes. (Conditioning on OpenSSL error codes can be a bit messy. Sometimes you have to, like the PEM case here, but other times the error codes don't correspond enough to clear, stable conditions to condition on. :-( ) |
…honGH-100373) In PEM, we need to parse until error and then suppress `PEM_R_NO_START_LINE`, because PEM allows arbitrary leading and trailing data. DER, however, does not. Parsing until error and suppressing `ASN1_R_HEADER_TOO_LONG` doesn't quite work because that error also covers some cases that should be rejected. Instead, check `BIO_eof` early and stop the loop that way. Automerge-Triggered-By: GH:Yhg1s
…honGH-100373) In PEM, we need to parse until error and then suppress `PEM_R_NO_START_LINE`, because PEM allows arbitrary leading and trailing data. DER, however, does not. Parsing until error and suppressing `ASN1_R_HEADER_TOO_LONG` doesn't quite work because that error also covers some cases that should be rejected. Instead, check `BIO_eof` early and stop the loop that way. Automerge-Triggered-By: GH:Yhg1s
FWIW, the fact that this was not backported is now affecting 3.10/3.11 versions together with the newest set of openssl versions (e.g. 3.6.3, 3.5.7): conda-forge/openssl-feedstock#237 The patch backports cleanly to 3.10 & 3.11 (and fixes the regression), which is what I'm looking towards doing as a fix for this in conda-forge. |
encukou
added
needs backport to 3.10
|
Thanks @davidben for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11. |
|
Thanks @davidben for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10. |
|
GH-153311 is a backport of this pull request to the 3.11 branch. |
|
GH-153312 is a backport of this pull request to the 3.10 branch. |
|
It now fails on macOS on GitHub Actions; I made backports to run the CI. It's up to the RMs. Not a security issue; it can make an OpenSSL upgrade break working code. |
|
Should be backported to 3.9 too, as it fails on CentOS 9 after openssl update. |
7 participants
In PEM, we need to parse until error and then suppress
PEM_R_NO_START_LINE, because PEM allows arbitrary leading and trailing data. DER, however, does not. Parsing until error and suppressingASN1_R_HEADER_TOO_LONGdoesn't quite work because that error also covers some cases that should be rejected.Instead, check
BIO_eofearly and stop the loop that way.Automerge-Triggered-By: GH:Yhg1s