chip-cert tool: Fix OpenSSL Object Reuse and Double-Free #24166
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Don't rely on d2i_X509 object reuse and fix double-free The chip-cert tool is relying on OpenSSL's "object reuse" mode in d2i_X509. d2i_X509 has a very bizarre type signature: X509 *d2i_X509(X509 **out, const unsigned char **inp, long len); The safest way to call this function is to pass NULL into out. The function then straightforwardly hands you a new X509 on success, or NULL on error. However, if out and *out are both NULL, OpenSSL tries to reuse the existing X509 object. This does not work, particular not in the way that chip-cert uses it. When d2i_X509 fails, even in this mode, it will free what's at *out and set *out to NULL. So when ReadCert's d2i_X509 call fails, it will silently free the cert parameter. But the caller doesn't know this and will double-free it!
|
PR #24166: Size comparison from e101730 to 0758a0f Increases (9 builds for bl702, nrfconnect, psoc6, qpg, telink)
Decreases (4 builds for bl602, k32w, telink)
Full report (53 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
arkq
approved these changes
Dec 21, 2022
Minor correction (this was my fault; I had a typo in my description). This should have read:
|
|
Also worth noting, here's OpenSSL's documentation that discourages the object reuse mode.
https://www.openssl.org/docs/man1.1.1/man3/d2i_X509.html In addition to being slightly misleading with the error case, as described here, it can go wrong if the original object has some cached values in it. On the BoringSSL side, we're thinking of making it always free the old object and write in a fresh object, which is safer and less error-prone, but it will require a change like this one. |
bzbarsky-apple
approved these changes
Dec 22, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed #24165
Don't rely on d2i_X509 object reuse and fix double-free
The chip-cert tool is relying on OpenSSL's "object reuse" mode in d2i_X509. d2i_X509 has a very bizarre type signature:
X509 *d2i_X509(X509 **out, const unsigned char **inp, long len);
The safest way to call this function is to pass NULL into out. The function then straightforwardly hands you a new X509 on success, or NULL on error. However, if out and *out are both NULL, OpenSSL tries to reuse the existing X509 object.
This does not work, particular not in the way that chip-cert uses it. When d2i_X509 fails, even in this mode, it will free what's at *out and set *out to NULL. So when ReadCert's d2i_X509 call fails, it will silently free the cert parameter. But the caller doesn't know this and will double-free it!