Dynamically allocate requested CSR write buffer size #3464
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
gilles-peskine-arm
added
component-x509
needs: changelog
needs-review
bors bot
pushed a commit
to openenclave/openenclave
that referenced
this pull request
Jun 29, 2020
3192: Update upstream mbedTLS patches backported to v2.16 r=mingweishih a=CodeMonkeyLeet - Add 0001-Avoid-stack-allocation-of-large-memory-buffers.patch from upstream [mbedTLS PR #3464](Mbed-TLS/mbedtls#3464). - Replace 0001-Patch-x509_write_crt.c-for-attestedTLS.patch with patch from upstream mbedTLS 0001-Backport-2632-code-changes-to-2.16.patch. This contains commits 6ad3fd1, 4063ad2 and 67d4259 from [mbedTLS PR #2632](Mbed-TLS/mbedtls#2632). - Apply the upstream changes to 3rdparty/mbedtls. Fixes #3170 Signed-off-by: Simon Leet <[email protected]> 3197: Remove the import of logging from tls_client.edl r=radhikaj a=mingweishih The PR removes the import of logging.edl in the sample that was missed by #3151. Signed-off-by: Ming-Wei Shih <[email protected]> Co-authored-by: Simon Leet <[email protected]> Co-authored-by: Ming-Wei Shih <[email protected]>
bors bot
pushed a commit
to openenclave/openenclave
that referenced
this pull request
Jun 29, 2020
3192: Update upstream mbedTLS patches backported to v2.16 r=mingweishih a=CodeMonkeyLeet - Add 0001-Avoid-stack-allocation-of-large-memory-buffers.patch from upstream [mbedTLS PR #3464](Mbed-TLS/mbedtls#3464). - Replace 0001-Patch-x509_write_crt.c-for-attestedTLS.patch with patch from upstream mbedTLS 0001-Backport-2632-code-changes-to-2.16.patch. This contains commits 6ad3fd1, 4063ad2 and 67d4259 from [mbedTLS PR #2632](Mbed-TLS/mbedtls#2632). - Apply the upstream changes to 3rdparty/mbedtls. Fixes #3170 Signed-off-by: Simon Leet <[email protected]> Co-authored-by: Simon Leet <[email protected]>
gilles-peskine-arm
previously approved these changes
Jun 30, 2020
ChangeLog
Outdated
| Changes | ||
| * Reduce the stack consumption of mbedtls_x509write_csr_der() which | ||
| previously could lead to stack overflow on constrained devices. | ||
| Contributed by Doru Gucea. |
There was a problem hiding this comment.
Feel free to add “and Simon Leet”. Reworking a pull request is a contribution that warrants acknowledgement.
There was a problem hiding this comment.
Thanks for the quick turnaround on the review, I've updated the Changelog as suggested. Is there any additional follow-up needed from me? I don't have read permissions to the CI failures, but let me know if I need to be involved.
There was a problem hiding this comment.
Thanks, it's entirely on us now. The next step is for another team member to review. Mbed OS CI is currently failing for unrelated reasons, so we'll ignore it if it isn't repaired by the time this PR is ready to merge.
CodeMonkeyLeet
force-pushed
the
csr_heap_alloc
branch
from
9a36649 to
2d11f82
Compare
gilles-peskine-arm
approved these changes
Jul 1, 2020
gilles-peskine-arm
requested changes
Jul 3, 2020
Using a stack-buffer with a size > 2K could easily produce a stack overflow for an embedded device which has a limited stack size. This commit dynamically allocates the large CSR buffer. This commit avoids using a temporary buffer for storing the OIDs. A single buffer is used: a) OIDs are written backwards starting with the end of the buffer; b) OIDs are memmove'd to the beginning of the buffer; c) signature over this OIDs is computed and written backwards from the end of the buffer; d) the two memory regions are compacted. Signed-off-by: Doru Gucea <[email protected]>
Address remaining PR comments for Mbed-TLS#2118 - Add ChangeLog.d/x509write_csr_heap_alloc.txt. - Fix parameter alignment per Gille's recommendation. - Update comments to more explicitly describe the manipulation of buf. - Replace use of `MBEDTLS_MPI_MAX_SIZE` as `sig` buffer size for call to `x509write_csr_der_internal()` with more intuitive `MBEDTLS_PK_SIGNATURE_MAX_SIZE`. - Update `mbedtls_x509write_csr_der()` to return `MBEDTLS_ERR_X509_ALLOC_FAILED` on mbedtls_calloc error. Signed-off-by: Simon Leet <[email protected]>
CodeMonkeyLeet
force-pushed
the
csr_heap_alloc
branch
from
2d11f82 to
40ca54a
Compare
gilles-peskine-arm
approved these changes
Jul 8, 2020
mpg
approved these changes
Jul 10, 2020
mpg
added
needs-backports
|
I'm merging this without waiting for potential backports as it's ready, and since it's marked as change in the ChangeLog we normally wouldn't backport it (we normally only backport bug fixes and some testing improvements). However you mention that a 2.16 backport would be helpful to you. The change doesn't seem very invasive and can reasonably be argued to be a bug fix (the same issues with cert writing were labelled "bug"), so while I can't speak for other gatekeepers, I'd be willing to accept a backport to 2.16 if you're willing to submit one. |
This was referenced Jul 10, 2020
CodeMonkeyLeet
pushed a commit
to CodeMonkeyLeet/mbedtls
that referenced
this pull request
Jul 18, 2020
Signed-off-by: Simon Leet <[email protected]>
gilles-peskine-arm
added a commit
that referenced
this pull request
Aug 12, 2020
Backport 2.16: PR #3464 Dynamically allocate requested CSR write buffer size
bors bot
pushed a commit
to openenclave/openenclave
that referenced
this pull request
Aug 20, 2020
3398: Update to mbedTLS 2.16.7 r=CodeMonkeyLeet a=CodeMonkeyLeet mbedTLS 2.16.7 no longer releases through the https://tls.mbed.org/download-archive we previously depended on, so this PR also takes the opportunity to convert the mbedTLS cloned sources into a git submodule dependency instead. The submodule dependency points at the new openenclave-mbedtls fork, which currently has the following patches applied: - [Backport ](openenclave/openenclave-mbedtls@b3500d5) of [mbedTLS PR #2632](Mbed-TLS/mbedtls#2632) - [Backport](openenclave/openenclave-mbedtls@9d4e4a7) of [mbedTLS PR #3464](Mbed-TLS/mbedtls#3464) - [Patch being upstreamed](openenclave/openenclave-mbedtls@98a7b77) as [mbedTLS PR #3564](Mbed-TLS/mbedtls#3564) Co-authored-by: Simon Leet <[email protected]>
Closed
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
x509write_csr_der_internal()uses a fixed 2KB stack buffer for writing the CSR. This is problematic because:This PR revives #2118, which appears to be abandoned.
x509write_csr_der_internal()to dynamically allocate the requested CSR buffer size, which resolves both issues.Status
READY
Requires Backporting
NO
Migrations
NO
Additional comments
In reference to issue #2631, we were waiting on change #2118 in addition to the committed #2632. While nothing necessitates this change being backported, we would like to have both of these PRs backported to 2.16 LTS if possible, so that we don't have to carry the separate patching ourselves.
Todos
Steps to test or reproduce
Per the original PR, the changes are self-contained and exercised through existing
makeandmake check.