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.

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

Backport 2.16: Guard from undefined behaviour in case of an INT_MAX max_pathlen #3197

Merged

Conversation

piotr-now
Copy link
Contributor

When parsing a certificate with the basic constraints extension, the max_pathlen that was read from it was incremented regardless of its value. However, if the max_pathlen is equal to INT_MAX (which is highly unlikely), an undefined behaviour would occur. This commit adds a check to ensure that such value is not accepted as valid. Relevant tests for INT_MAX and INT_MAX-1 are also introduced.

Certificates added in this commit were generated using the test_suite_x509write, function test_x509_crt_check. Input data taken from the "Certificate write check Server1 SHA1" test case, so the generated files are like the "server1.crt", but with the "is_ca" field set to 1 and max_pathlen as described by the file name.

This PR addresses IOTSSL-2774.

@Patater
Copy link
Contributor

Patater commented Apr 17, 2020

Backport of #3192

Patater
Patater previously approved these changes Apr 17, 2020
Copy link
Contributor

@Patater Patater left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as representative backport, with minor style changes for consistency with the 2.16 branch.

When parsing a certificate with the basic constraints extension
the max_pathlen that was read from it was incremented regardless
of its value. However, if the max_pathlen is equal to INT_MAX (which
is highly unlikely), an undefined behaviour would occur.
This commit adds a check to ensure that such value is not accepted
as valid. Relevant tests for INT_MAX and INT_MAX-1 are also introduced.
Certificates added in this commit were generated using the
test_suite_x509write, function test_x509_crt_check. Input data taken
from the "Certificate write check Server1 SHA1" test case, so the generated
files are like the "server1.crt", but with the "is_ca" field set to 1 and
max_pathlen as described by the file name.

Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
Signed-off-by: Piotr Nowicki <piotr.nowicki@arm.com>
@piotr-now piotr-now force-pushed the max_pathlen_overflow_mbedtls-2.16 branch from 619bf3c to acf7f2c Compare April 17, 2020 09:29
@Patater Patater added component-x509 mbed TLS team needs-review Every commit must be reviewed by at least two team members, labels Apr 17, 2020
Copy link
Contributor

@dgreen-arm dgreen-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backport looks good to me

@Patater Patater removed the needs-review Every commit must be reviewed by at least two team members, label Apr 17, 2020
@Patater Patater merged commit da1d437 into Mbed-TLS:mbedtls-2.16 Apr 17, 2020
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Apr 28, 2020
…low fix

Backport of Mbed-TLS#3192

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants