Backport 2.16: Remove Extraneous bytes from buffer post pem write #3935
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
paul-elliott-arm
added
bug
needs-review
2 tasks
gilles-peskine-arm
added
needs-work
and removed
needs-backports
paul-elliott-arm
force-pushed
the
fix_pem_write_2_16
branch
from
3812af3 to
0951782
Compare
gilles-peskine-arm
previously approved these changes
Dec 7, 2020
gabor-mezei-arm
removed
the
needs-reviewer
paul-elliott-arm
force-pushed
the
fix_pem_write_2_16
branch
from
0951782 to
ce0ab1a
Compare
In order to remove large buffers from the stack, the der data is written into the same buffer that the pem is eventually written into, however although the pem data is zero terminated, there is now data left in the buffer after the zero termination, which can cause mbedtls_x509_crt_parse to fail to parse the same buffer if passed back in. Patches also applied to mbedtls_pk_write_pubkey_pem, and mbedtls_pk_write_key_pem, which use similar methods of writing der data to the same buffer, and tests modified to hopefully catch any future regression on this. Signed-off-by: Paul Elliott <[email protected]>
paul-elliott-arm
force-pushed
the
fix_pem_write_2_16
branch
from
ce0ab1a to
319b593
Compare
gabor-mezei-arm
approved these changes
Dec 8, 2020
gilles-peskine-arm
approved these changes
Dec 8, 2020
gilles-peskine-arm
added
approved
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.
Description
In order to remove some big (4k) buffers from being created on the stack, the output buffer was re-used - in this case the raw der data is written to the buffer prior to being base64 encoded into an allocated buffer, and then overwritten with the pem data. However, even though the pem data is zero terminated, usually der data will remain in the buffer after the terminator.
Should this buffer get passed into mbedtls_x509_crt_parse() then it will likely fail to parse, as it decides whether or not a buffer is pem by checking the last byte of the buffer for being zero - if it isn't zero, it will attempt to parse as der, which will obviously fail.
This just ensures the buffer is cleaned to avoid the regression, there may be future work around this subject to make the area safer and tests to ensure this doesn't happen again.
This is a backport of #3898
Status
READY
Steps to test or reproduce
See #3682