Add missing validity checks to CSR verification #22069
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
- SDK's CSR verification (VerifyCertificateSigningRequest) allowed trailing garbage past the end of the buffer if the primary SEQUENCE element is OK and checks-out. This is looser enforcement than some crypto libraries which expect a CSR to be 100% valid ASN.1 DER and have no unnecessary bytes or otherwise unparsable bytes. Fixes project-chip#22068 This PR: - Adds validity checks for size and basic format that catches the problem. - Adds unit tests that use externally generated CSRs to validate the `VerifyCertificateSigningRequest` logic, rather than only relying on round-trips with generation. Testing done: - Added new unit tests. Existing unit tests pass - Tested under OpenSSL, BoringSSL and mbedTLS
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
gjc13,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
lazarkov,
LuDuda and
mlepage-google
pullapprove
bot
requested review from
rgoliver,
robszewczyk,
saurabhst,
selissia,
tecimovic,
tehampson,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21,
yufengwangca and
yunhanw-google
|
PR #22069: Size comparison from a8d12af to 5ce2840 Increases (5 builds for linux, psoc6)
Decreases (3 builds for bl602, telink)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
tehampson
approved these changes
Aug 22, 2022
emargolis
approved these changes
Aug 22, 2022
|
/rebase |
jmartinez-silabs
approved these changes
Aug 22, 2022
|
PR #22069: Size comparison from f6929bb to 45444f7 Increases (11 builds for cc13x2_26x2, esp32, linux, psoc6, telink)
Decreases (2 builds for cc13x2_26x2, nrfconnect)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
github-actions bot
pushed a commit
that referenced
this pull request
Aug 23, 2022
* Add missing validity checks to CSR verification - SDK's CSR verification (VerifyCertificateSigningRequest) allowed trailing garbage past the end of the buffer if the primary SEQUENCE element is OK and checks-out. This is looser enforcement than some crypto libraries which expect a CSR to be 100% valid ASN.1 DER and have no unnecessary bytes or otherwise unparsable bytes. Fixes #22068 This PR: - Adds validity checks for size and basic format that catches the problem. - Adds unit tests that use externally generated CSRs to validate the `VerifyCertificateSigningRequest` logic, rather than only relying on round-trips with generation. Testing done: - Added new unit tests. Existing unit tests pass - Tested under OpenSSL, BoringSSL and mbedTLS * Fix docs typo
woody-apple
added a commit
that referenced
this pull request
Aug 24, 2022
* Add missing validity checks to CSR verification - SDK's CSR verification (VerifyCertificateSigningRequest) allowed trailing garbage past the end of the buffer if the primary SEQUENCE element is OK and checks-out. This is looser enforcement than some crypto libraries which expect a CSR to be 100% valid ASN.1 DER and have no unnecessary bytes or otherwise unparsable bytes. Fixes #22068 This PR: - Adds validity checks for size and basic format that catches the problem. - Adds unit tests that use externally generated CSRs to validate the `VerifyCertificateSigningRequest` logic, rather than only relying on round-trips with generation. Testing done: - Added new unit tests. Existing unit tests pass - Tested under OpenSSL, BoringSSL and mbedTLS * Fix docs typo Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> Co-authored-by: Andrei Litvin <[email protected]>
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
* Add missing validity checks to CSR verification - SDK's CSR verification (VerifyCertificateSigningRequest) allowed trailing garbage past the end of the buffer if the primary SEQUENCE element is OK and checks-out. This is looser enforcement than some crypto libraries which expect a CSR to be 100% valid ASN.1 DER and have no unnecessary bytes or otherwise unparsable bytes. Fixes project-chip#22068 This PR: - Adds validity checks for size and basic format that catches the problem. - Adds unit tests that use externally generated CSRs to validate the `VerifyCertificateSigningRequest` logic, rather than only relying on round-trips with generation. Testing done: - Added new unit tests. Existing unit tests pass - Tested under OpenSSL, BoringSSL and mbedTLS * Fix docs typo
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.
Problem
garbage past the end of the buffer if the primary SEQUENCE element is OK
and checks-out. This is looser enforcement than some crypto libraries
which expect a CSR to be 100% valid ASN.1 DER and have no unnecessary bytes
or otherwise unparsable bytes.
Fixes #22068
Change overview
the problem.
the
VerifyCertificateSigningRequestlogic, rather than onlyrelying on round-trips with generation.
Testing