Skip to content

Commit

Permalink
Obey bounds of ASN.1 substructures
Browse files Browse the repository at this point in the history
When parsing a substructure of an ASN.1 structure, no field within
the substructure must exceed the bounds of the substructure.
Concretely, the `end` pointer passed to the ASN.1 parsing routines
must be updated to point to the end of the substructure while parsing
the latter.

This was previously not the case for the routines
- x509_get_attr_type_and_value(),
- mbedtls_x509_get_crt_ext(),
- mbedtls_x509_get_crl_ext().
These functions kept using the end of the parent structure as the
`end` pointer and would hence allow substructure fields to cross
the substructure boundary. This could lead to successful parsing
of ill-formed X.509 CRTs.

This commit fixes this.

Care has to be taken when adapting `mbedtls_x509_get_crt_ext()`
and `mbedtls_x509_get_crl_ext()`, as the underlying function
`mbedtls_x509_get_ext()` returns `0` if no extensions are present
but doesn't set the variable which holds the bounds of the Extensions
structure in case the latter is present. This commit addresses
this by returning early from `mbedtls_x509_get_crt_ext()` and
`mbedtls_x509_get_crl_ext()` if parsing has reached the end of
the input buffer.

The following X.509 parsing tests need to be adapted:
- "TBSCertificate, issuer two inner set datas"
  This test exercises the X.509 CRT parser with a Subject name
  which has two empty `AttributeTypeAndValue` structures.
  This is supposed to fail with `MBEDTLS_ERR_ASN1_OUT_OF_DATA`
  because the parser should attempt to parse the first structure
  and fail because of a lack of data. Previously, it failed to
  obey the (0-length) bounds of the first AttributeTypeAndValue
  structure and would try to interpret the beginning of the second
  AttributeTypeAndValue structure as the first field of the first
  AttributeTypeAndValue structure, returning an UNEXPECTED_TAG error.
- "TBSCertificate, issuer, no full following string"
  This test exercises the parser's behaviour on an AttributeTypeAndValue
  structure which contains more data than expected; it should therefore
  fail with MBEDTLS_ERR_ASN1_LENGTH_MISMATCH. Because of the missing bounds
  check, it previously failed with UNEXPECTED_TAG because it interpreted
  the remaining byte in the first AttributeTypeAndValue structure as the
  first byte in the second AttributeTypeAndValue structure.
- "SubjectAltName repeated"
  This test should exercise two SubjectAltNames extensions in succession,
  but a wrong length values makes the second SubjectAltNames extension appear
  outside of the Extensions structure. With the new bounds in place, this
  therefore fails with a LENGTH_MISMATCH error. This commit adapts the test
  data to put the 2nd SubjectAltNames extension inside the Extensions
  structure, too.
  • Loading branch information
Hanno Becker committed May 13, 2019
1 parent 261f33d commit 2499a45
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 3 deletions.
8 changes: 8 additions & 0 deletions library/x509.c
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,8 @@ static int x509_get_attr_type_value( unsigned char **p,
MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ) != 0 )
return( MBEDTLS_ERR_X509_INVALID_NAME + ret );

end = *p + len;

if( ( end - *p ) < 1 )
return( MBEDTLS_ERR_X509_INVALID_NAME +
MBEDTLS_ERR_ASN1_OUT_OF_DATA );
Expand Down Expand Up @@ -394,6 +396,12 @@ static int x509_get_attr_type_value( unsigned char **p,
val->p = *p;
*p += val->len;

if( *p != end )
{
return( MBEDTLS_ERR_X509_INVALID_NAME +
MBEDTLS_ERR_ASN1_LENGTH_MISMATCH );
}

cur->next = NULL;

return( 0 );
Expand Down
5 changes: 5 additions & 0 deletions library/x509_crl.c
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,9 @@ static int x509_get_crl_ext( unsigned char **p,
{
int ret;

if( *p == end )
return( 0 );

/*
* crlExtensions [0] EXPLICIT Extensions OPTIONAL
* -- if present, version MUST be v2
Expand All @@ -115,6 +118,8 @@ static int x509_get_crl_ext( unsigned char **p,
return( ret );
}

end = ext->p + ext->len;

while( *p < end )
{
/*
Expand Down
4 changes: 4 additions & 0 deletions library/x509_crt.c
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,9 @@ static int x509_get_crt_ext( unsigned char **p,
size_t len;
unsigned char *end_ext_data, *end_ext_octet;

if( *p == end )
return( 0 );

if( ( ret = mbedtls_x509_get_ext( p, end, &crt->v3_ext, 3 ) ) != 0 )
{
if( ret == MBEDTLS_ERR_ASN1_UNEXPECTED_TAG )
Expand All @@ -716,6 +719,7 @@ static int x509_get_crt_ext( unsigned char **p,
return( ret );
}

end = crt->v3_ext.p + crt->v3_ext.len;
while( *p < end )
{
/*
Expand Down
6 changes: 3 additions & 3 deletions tests/suites/test_suite_x509parse.data
Original file line number Diff line number Diff line change
Expand Up @@ -1016,7 +1016,7 @@ x509parse_crt:"30223020a0030201028204deadbeef300d06092a864886f70d01010b050030043

X509 Certificate ASN1 (TBSCertificate, issuer two inner set datas)
depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C
x509parse_crt:"30243022a0030201028204deadbeef300d06092a864886f70d01010b05003006310430003000":"":MBEDTLS_ERR_X509_INVALID_NAME + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG
x509parse_crt:"30243022a0030201028204deadbeef300d06092a864886f70d01010b05003006310430003000":"":MBEDTLS_ERR_X509_INVALID_NAME + MBEDTLS_ERR_ASN1_OUT_OF_DATA

X509 Certificate ASN1 (TBSCertificate, issuer no oid data)
depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C
Expand All @@ -1032,7 +1032,7 @@ x509parse_crt:"30253023a0030201028204deadbeef300d06092a864886f70d01010b050030073

X509 Certificate ASN1 (TBSCertificate, issuer, no full following string)
depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C
x509parse_crt:"302b3029a0030201028204deadbeef300d06092a864886f70d01010b0500300d310b3009060013045465737400":"":MBEDTLS_ERR_X509_INVALID_NAME+MBEDTLS_ERR_ASN1_UNEXPECTED_TAG
x509parse_crt:"302b3029a0030201028204deadbeef300d06092a864886f70d01010b0500300d310b3009060013045465737400":"":MBEDTLS_ERR_X509_INVALID_NAME+MBEDTLS_ERR_ASN1_LENGTH_MISMATCH

X509 Certificate ASN1 (TBSCertificate, valid issuer, no validity)
depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C
Expand Down Expand Up @@ -1164,7 +1164,7 @@ x509parse_crt:"3081de3081dba003020102020900ebdbcd14105e1839300906072a8648ce3d040

X509 Certificate ASN1 (SubjectAltName repeated)
depends_on:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA1_C
x509parse_crt:"3081fd3081faa003020102020900a8b31ff37d09a37f300906072a8648ce3d0401300f310d300b0603550403130454657374301e170d3134313131313231333731365a170d3234313130383231333731365a300f310d300b06035504031304546573743059301306072a8648ce3d020106082a8648ce3d0301070342000437cc56d976091e5a723ec7592dff206eee7cf9069174d0ad14b5f768225962924ee500d82311ffea2fd2345d5d16bd8a88c26b770d55cd8a2a0efa01c8b4edffa321301f301d0603551d11041630148208666f6f2e7465737482086261722e74657374301d0603551d11041630148208666f6f2e7465737482086261722e74657374":"":MBEDTLS_ERR_X509_INVALID_EXTENSIONS
x509parse_crt:"3081fd3081faa003020102020900a8b31ff37d09a37f300906072a8648ce3d0401300f310d300b0603550403130454657374301e170d3134313131313231333731365a170d3234313130383231333731365a300f310d300b06035504031304546573743059301306072a8648ce3d020106082a8648ce3d0301070342000437cc56d976091e5a723ec7592dff206eee7cf9069174d0ad14b5f768225962924ee500d82311ffea2fd2345d5d16bd8a88c26b770d55cd8a2a0efa01c8b4edffa340303e301d0603551d11041630148208666f6f2e7465737482086261722e74657374301d0603551d11041630148208666f6f2e7465737482086261722e74657374":"":MBEDTLS_ERR_X509_INVALID_EXTENSIONS

X509 Certificate ASN1 (ExtKeyUsage repeated)
depends_on:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA1_C
Expand Down

0 comments on commit 2499a45

Please sign in to comment.