Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CSR verification allows invalid ASN.1 #22068

Closed
tcarmelveilleux opened this issue Aug 22, 2022 · 0 comments · Fixed by #22069
Closed

CSR verification allows invalid ASN.1 #22068

tcarmelveilleux opened this issue Aug 22, 2022 · 0 comments · Fixed by #22069
Assignees
@tcarmelveilleux
Labels
crypto security V1.0

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

SDK's CSR verification (VerifyCertificateSigningRequest) allows 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.

It was found that one CryptoPAL backend (CHIPCryptoPALPsaEfr32.cpp) did not
resize its output buffer, leading to a valid CSR followed by zero bytes. This failed
verification in at least one integration of the SDK using state of the art crypto libraries.

Proposed Solution

  • Add checks in VerifyCertificateSigningRequest in SDK to catch bad CSRs.
@tcarmelveilleux tcarmelveilleux self-assigned this Aug 22, 2022
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Aug 22, 2022
@tcarmelveilleux
Add missing validity checks to CSR verification
5ce2840 
- 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
@tcarmelveilleux tcarmelveilleux mentioned this issue Aug 22, 2022
Merged
tcarmelveilleux added a commit that referenced this issue Aug 23, 2022
@tcarmelveilleux
Add missing validity checks to CSR verification (#22069)
48f87f3 
* 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
github-actions bot pushed a commit that referenced this issue Aug 23, 2022
@tcarmelveilleux @web-flow
Add missing validity checks to CSR verification (#22069)
6e57432 
* 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 woody-apple mentioned this issue Aug 23, 2022
Merged
woody-apple added a commit that referenced this issue Aug 24, 2022
@woody-apple @tcarmelveilleux @andy31415
Add missing validity checks to CSR verification (#22069) (#22092)
1ef4eef 
* 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 issue Sep 16, 2022
@tcarmelveilleux @isiu-apple
Add missing validity checks to CSR verification (project-chip#22069)
411b970 
* 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
Assignees

@tcarmelveilleux tcarmelveilleux

Labels
crypto security V1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add missing validity checks to CSR verification tcarmelveilleux/connectedhomeip
1 participant
@tcarmelveilleux