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

X.509: Fix bug in SAN parsing and enhance negative testing #2839

Closed
wants to merge 10 commits into from
Closed

X.509: Fix bug in SAN parsing and enhance negative testing #2839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
82a1fe5
X.509: Remove CRT policy parsing test 'bool len missing'
Sep 12, 2019
7b8330a
X.509: Move negative tests for CertificatePolicy parsing
Sep 12, 2019
c15ff98
X.509: Adapt negative parsing test for no data in CrtPolicy ext
Sep 12, 2019
6dfa665
X.509: Add numerous negative parsing tests for CertificatePolicy ext
Sep 12, 2019
37f750a
Fix X.509 SAN parsing
Sep 13, 2019
305bc31
X.509: Improve negative testing for SubjectAltName parsing
Sep 13, 2019
1bfd979
Adapt ChangeLog
Sep 13, 2019
4962822
X.509: Improve negative testing for SubjectAltName parsing
Sep 13, 2019
63cf8aa
X.509: Remove red'n bounds checks and zeroiz'n in OtherName parsing
Sep 13, 2019
f21de55
X.509: Add length consistency checks to x509_get_other_name()
Sep 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
mbed TLS ChangeLog (Sorted per branch, date)

= mbed TLS x.xx.x branch released xxxx-xx-xx

Bugfix
* Fix parsing of X.509 SubjectAlternativeName extension. Previously,
malformed alternative name components were not caught during initial
certificate parsing, but only on subsequent calls to
mbedtls_x509_parse_subject_alt_name(). Fixes #2838.

= mbed TLS 2.19.0 branch released 2019-09-06

Security
Expand Down
48 changes: 24 additions & 24 deletions library/x509_crt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,8 +627,6 @@ static int x509_get_subject_alt_name( unsigned char **p,
{
int ret;
size_t len, tag_len;
mbedtls_asn1_buf *buf;
unsigned char tag;
mbedtls_asn1_sequence *cur = subject_alt_name;

/* Get main sequence tag */
Expand All @@ -643,18 +641,23 @@ static int x509_get_subject_alt_name( unsigned char **p,
while( *p < end )
{
mbedtls_x509_subject_alternative_name dummy_san_buf;
mbedtls_x509_buf tmp_san_buf;
memset( &dummy_san_buf, 0, sizeof( dummy_san_buf ) );

if( ( end - *p ) < 1 )
return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS +
MBEDTLS_ERR_ASN1_OUT_OF_DATA );

tag = **p;
tmp_san_buf.tag = **p;
(*p)++;

if( ( ret = mbedtls_asn1_get_len( p, end, &tag_len ) ) != 0 )
return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS + ret );

if( ( tag & MBEDTLS_ASN1_TAG_CLASS_MASK ) !=
tmp_san_buf.p = *p;
tmp_san_buf.len = tag_len;

if( ( tmp_san_buf.tag & MBEDTLS_ASN1_TAG_CLASS_MASK ) !=
MBEDTLS_ASN1_CONTEXT_SPECIFIC )
{
return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS +
Expand All @@ -664,7 +667,7 @@ static int x509_get_subject_alt_name( unsigned char **p,
/*
* Check that the SAN are structured correct.
*/
ret = mbedtls_x509_parse_subject_alt_name( &(cur->buf), &dummy_san_buf );
ret = mbedtls_x509_parse_subject_alt_name( &tmp_san_buf, &dummy_san_buf );
/*
* In case the extension is malformed, return an error,
* and clear the allocated sequences.
Expand Down Expand Up @@ -700,11 +703,8 @@ static int x509_get_subject_alt_name( unsigned char **p,
cur = cur->next;
}

buf = &(cur->buf);
buf->tag = tag;
buf->p = *p;
buf->len = tag_len;
*p += buf->len;
cur->buf = tmp_san_buf;
Copy link
Contributor

Choose a reason for hiding this comment

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

Although technically this works, since it is assigning by value tmp_buf to cur->buf, I don't like that there is an assignment of a structure defined in the stack, without a proper memory copying.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's deliberate - why do you prefer a memcpy()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are counting for the struct to be copied by value, it wouldn't do a deep-copy(neither does memcpy actually), and this is not future proof.
In addition, in a matter of style, I always prefer using to copy data from non primitive types.

*p += tmp_san_buf.len;
}

/* Set final sequence entry's next pointer to NULL */
Expand Down Expand Up @@ -1685,34 +1685,36 @@ static int x509_get_other_name( const mbedtls_x509_buf *subject_alt_name,
return( MBEDTLS_ERR_X509_FEATURE_UNAVAILABLE );
}

if( p + len >= end )
{
mbedtls_platform_zeroize( other_name, sizeof( *other_name ) );
return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS +
MBEDTLS_ERR_ASN1_LENGTH_MISMATCH );
}
p += len;
if( ( ret = mbedtls_asn1_get_tag( &p, end, &len,
MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_CONTEXT_SPECIFIC ) ) != 0 )
{
return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS + ret );
}

if( end != p + len )
{
return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS +
MBEDTLS_ERR_ASN1_LENGTH_MISMATCH );
}

if( ( ret = mbedtls_asn1_get_tag( &p, end, &len,
MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ) != 0 )
return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS + ret );

if( end != p + len )
{
return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS +
MBEDTLS_ERR_ASN1_LENGTH_MISMATCH );
}

if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, MBEDTLS_ASN1_OID ) ) != 0 )
return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS + ret );

other_name->value.hardware_module_name.oid.tag = MBEDTLS_ASN1_OID;
other_name->value.hardware_module_name.oid.p = p;
other_name->value.hardware_module_name.oid.len = len;

if( p + len >= end )
{
mbedtls_platform_zeroize( other_name, sizeof( *other_name ) );
return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS +
MBEDTLS_ERR_ASN1_LENGTH_MISMATCH );
}
p += len;
if( ( ret = mbedtls_asn1_get_tag( &p, end, &len,
MBEDTLS_ASN1_OCTET_STRING ) ) != 0 )
Expand All @@ -1724,8 +1726,6 @@ static int x509_get_other_name( const mbedtls_x509_buf *subject_alt_name,
p += len;
if( p != end )
{
mbedtls_platform_zeroize( other_name,
sizeof( *other_name ) );
return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS +
MBEDTLS_ERR_ASN1_LENGTH_MISMATCH );
}
Expand Down
Loading