Remove Extraneous bytes from buffer post pem write #3898
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
paul-elliott-arm
added
bug
component-x509
needs-ci
There was a problem hiding this comment.
I agree with this approach. It needs:
- Same thing with CSR.
- In unit tests, after calling
mbedtls_x509write_crt_pem, check that the trail of the buffer is zeroed out. Make sure that there is at least one unit test where the output buffer is larger than the output data. - Unit tests for the other functions with the same pattern (
mbedtls_pk_write_pubkey_pem,mbedtls_pk_write_key_pem,mbedtls_x509write_csr_pem). - Changelog entry.
paul-elliott-arm
force-pushed
the
fix_pem_write
branch
from
16b0c32 to
c018547
Compare
gilles-peskine-arm
requested changes
Dec 2, 2020
paul-elliott-arm
force-pushed
the
fix_pem_write
branch
from
c018547 to
6b6376d
Compare
gilles-peskine-arm
requested changes
Dec 2, 2020
ChangeLog.d/clean_pem_buffers.txt
Outdated
| @@ -0,0 +1,8 @@ | |||
| Bugfix | |||
| * As an optimisation to stop large buffers being written to the stack the | |||
| raw der data was instead written to the same buffer that the PEM data | |||
There was a problem hiding this comment.
DER also needs to be in all caps.
paul-elliott-arm
force-pushed
the
fix_pem_write
branch
from
6b6376d to
7385f10
Compare
paul-elliott-arm
added
needs-backports
gilles-peskine-arm
requested changes
Dec 3, 2020
ChangeLog.d/clean_pem_buffers.txt
Outdated
| bytes. This guarantees that the corresponding parsing function can read | ||
| the buffer back, which was the case for mbedtls_x509write_{crt,csr}_pem | ||
| until this property was inadvertently broken in Mbed TLS 2.19.0. | ||
| Fixes #3682. |
There was a problem hiding this comment.
Please ensure that tests/scripts/all.sh -k check_files check_changelog passes
There was a problem hiding this comment.
Missing newline at end. Fixed.
paul-elliott-arm
force-pushed
the
fix_pem_write
branch
from
7385f10 to
b3e9d2d
Compare
gilles-peskine-arm
previously approved these changes
Dec 7, 2020
gabor-mezei-arm
removed
the
needs-reviewer
gabor-mezei-arm
requested changes
Dec 7, 2020
paul-elliott-arm
force-pushed
the
fix_pem_write
branch
from
b3e9d2d to
2451aba
Compare
gilles-peskine-arm
requested review from
gilles-peskine-arm and
gabor-mezei-arm
gabor-mezei-arm
requested changes
Dec 7, 2020
paul-elliott-arm
force-pushed
the
fix_pem_write
branch
from
2451aba to
6b654c7
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
branch
from
6b654c7 to
557b8d6
Compare
gabor-mezei-arm
approved these changes
Dec 8, 2020
gilles-peskine-arm
approved these changes
Dec 8, 2020
gilles-peskine-arm
added
approved
|
The CI is being unstable, but between https://jenkins-mbedtls.oss.arm.com/blue/organizations/jenkins/mbed-tls-pr-merge/detail/PR-3898-merge/10 and https://jenkins-mbedtls.oss.arm.com/blue/organizations/jenkins/mbed-tls-pr-merge/detail/PR-3898-merge/11 all the jobs passed (for Windows-2013, which failed in both, all the sub-jobs passed in at least one of the runs). |
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 closes #3682
Status
READY
Requires Backporting
YES
2.16
2.7
(Bug did not affect the 2.7 branch, but wanted to add the tests)
Migrations
NO
Todos
Steps to test or reproduce
See #3682