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

X.509 AttributeTypeAndValue parsing doesn't respect bounds #2437

Closed
hanno-arm opened this issue Feb 12, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@hanno-arm
Copy link
Contributor

commented Feb 12, 2019

Context: This is about the parsing of the X.509 structure

   AttributeTypeAndValue ::= SEQUENCE {
     type     AttributeType,
     value    AttributeValue }

in x509_get_attr_type_and_value() from library/x509.c:

mbedtls/library/x509.c

Lines 344 to 370 in f352f75

static int x509_get_attr_type_value( unsigned char **p,
const unsigned char *end,
mbedtls_x509_name *cur )
{
int ret;
size_t len;
mbedtls_x509_buf *oid;
mbedtls_x509_buf *val;
if( ( ret = mbedtls_asn1_get_tag( p, end, &len,
MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ) != 0 )
return( MBEDTLS_ERR_X509_INVALID_NAME + ret );
if( ( end - *p ) < 1 )
return( MBEDTLS_ERR_X509_INVALID_NAME +
MBEDTLS_ERR_ASN1_OUT_OF_DATA );
oid = &cur->oid;
oid->tag = **p;
if( ( ret = mbedtls_asn1_get_tag( p, end, &oid->len, MBEDTLS_ASN1_OID ) ) != 0 )
return( MBEDTLS_ERR_X509_INVALID_NAME + ret );
oid->p = *p;
*p += oid->len;
if( ( end - *p ) < 1 )

Issue: The function doesn't obey the len bound for the outer SEQUENCE. Instead, it only makes sure that the parsing doesn't read past the surrounding

   RelativeDistinguishedName ::=
     SET SIZE (1..MAX) OF AttributeTypeAndValue

the end of which is passed to the function as end. For example, if the length of the outer SEQUENCE is 0 but there's more space remaining in the current SET, the function won't fail. This situation is actually exercised by the test https://github.com/ARMmbed/mbedtls/blob/development/tests/suites/test_suite_x509parse.data#L1001 which wrongly expects MBEDTLS_ERR_X509_INVALID_NAME + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG, where it should be MBEDTLS_ERR_X509_INVALID_NAME + MBEDTLS_ERR_ASN1_OUT_OF_DATA.

This is not a security issue because the bounds of the surrounding SET are obeyed.

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Feb 12, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

ARM Internal Ref: IOTSSL-2780

@ciarmcom ciarmcom added the mirrored label Feb 12, 2019

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Feb 13, 2019

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Feb 13, 2019

@hanno-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

There is an analogous issue with the v3 extension parsing function x509_get_crt_ext():

mbedtls/library/x509_crt.c

Lines 694 to 714 in f352f75

static int x509_get_crt_ext( unsigned char **p,
const unsigned char *end,
mbedtls_x509_crt *crt )
{
int ret;
size_t len;
unsigned char *end_ext_data, *end_ext_octet;
if( ( ret = mbedtls_x509_get_ext( p, end, &crt->v3_ext, 3 ) ) != 0 )
{
if( ret == MBEDTLS_ERR_ASN1_UNEXPECTED_TAG )
return( 0 );
return( ret );
}
while( *p < end )
{
/*
* Extension ::= SEQUENCE {
* extnID OBJECT IDENTIFIER,

The call to mbedtls_x509_get_ext() reads the extensions header but doesn't update the end pointer, so that the subsequent parsing of extensions will treat everything until the end of the TBSCertificate as a potential extension. In fact, in the existing X.509 test "X509 Certificate ASN1 (SubjectAltName repeated)" the duplicated SubjectAltName extension is added outside of the Extensions block, but it still parsed; here we should fail with a MBEDTLS_ERR_ASN1_LENGTH_MISMATCH instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.