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

Alter bounds check in ssl_load_session for slightly more safety #659

Closed
guidovranken opened this Issue Oct 17, 2016 · 1 comment

Comments

Projects
None yet
4 participants
@guidovranken
Copy link

guidovranken commented Oct 17, 2016

In ssl_ticket.c ssl_load_session():

    cert_len = ( p[0] << 16 ) | ( p[1] << 8 ) | p[2];
    p += 3;

    if( cert_len == 0 )
    {
        session->peer_cert = NULL;
    }
    else
    {
        int ret;

        if( p + cert_len > end )
            return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA );

I would change this last check into

        if( end < p || cert_len > (size_t)(end - p) )
            return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA );

I don't think that under normal circumstances cert_len can ever exceed the buffer size, because it was always generated by the server itself in ssl_save_session, and is always verified to be legitimate in mbedtls_ssl_ticket_parse. But in the unlikely event that an attacker can break the ticket decryption/authentication, the check p + cert_len > end doesn't work if p happens to be a very high virtual address and cert_len happens to be very large.

So say that p is address 0xFFFF0000, and end is 0xFFFFFF00, and cert_len is 0x00FFFFFF, then the comparison becomes

if ( 0xFFFF0000 + 0x00FFFFFF > 0xFFFFFF00 )

which on 32 bit is

if ( 0xFEFFFF > 0xFFFFFF00 )

which is false, so it isn't treated as an error, despite cert_len exceeding the buffer size.

If the attacker can also control the certificate that was embedded in the ticket, then they might be able to craft that certificate in such a way that mbedtls_x509_crt_parse_der writes N bytes of their choosing beyond buffer bounds (instead of occupying the whole cert_len amount of space that probably leads to a crash).

I hope this makes sense. The rationale is that breaking ticket encryption and authentication should only lead to session takeover, and not to a server compromise.

@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Oct 18, 2016

ARM Internal Ref: IOTSSL-1046

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Oct 24, 2018

Fix unsafe bounds checks in ssl_load_session()
This commit replaces multiple bounds checks of the form

     `if( ptr + offset > end )`

by

     `if( offset > end - ptr )`.

The former is unsafe as the pointer arithmetic `ptr + offset` can
overflow in case of a large value of `offset` paired with a value
of `ptr` close to the (virtual) address boundary.

The latter bounds check, in turn, is always safe if `offset` is a
signed integral value, even if `ptr` happens to be larger than
`end` (which should never happen, but it's better to be prepared
just in case).

Concretely, ssl_load_session() contains the bounds check

     `if( p + cert_len > end )`

where `cert_len` is a 24-bit value of type `size_t`. This
check is accordingly replaced by  `if( (int) cert_len > end - p )`;
the explicit cast `(int) cert_len` is safe because `int` is assumed
to be 32-bit wide and paddingless, hence capable of holding 24-bit
values.

Fixes ARMmbed#659 reported by Guido Vranken.

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Oct 24, 2018

Fix unsafe bounds checks in ssl_load_session()
This commit replaces multiple bounds checks of the form

     `if( ptr + offset > end )`

by

     `if( offset > end - ptr )`.

The former is unsafe as the pointer arithmetic `ptr + offset` can
overflow in case of a large value of `offset` paired with a value
of `ptr` close to the (virtual) address boundary.

The latter bounds check, in turn, is always safe if `offset` is a
signed integral value, even if `ptr` happens to be larger than
`end` (which should never happen, but it's better to be prepared
just in case).

Concretely, ssl_load_session() contains the bounds check

     `if( p + cert_len > end )`

where `cert_len` is a 24-bit value of type `size_t`. This
check is accordingly replaced by  `if( (int) cert_len > end - p )`;
the explicit cast `(int) cert_len` is safe because `int` is assumed
to be 32-bit wide and paddingless, hence capable of holding 24-bit
values.

Fixes ARMmbed#659 reported by Guido Vranken.

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Oct 24, 2018

Fix unsafe bounds checks in ssl_load_session()
This commit replaces multiple bounds checks of the form

     `if( ptr + offset > end )`

by

     `if( offset > end - ptr )`.

The former is unsafe as the pointer arithmetic `ptr + offset` can
overflow in case of a large value of `offset` paired with a value
of `ptr` close to the (virtual) address boundary.

The latter bounds check, in turn, is always safe if `offset` is a
signed integral value, even if `ptr` happens to be larger than
`end` (which should never happen, but it's better to be prepared
just in case).

Concretely, ssl_load_session() contains the bounds check

     `if( p + cert_len > end )`

where `cert_len` is a 24-bit value of type `size_t`. This
check is accordingly replaced by  `if( (int) cert_len > end - p )`;
the explicit cast `(int) cert_len` is safe because `int` is assumed
to be 32-bit wide and paddingless, hence capable of holding 24-bit
values.

Fixes ARMmbed#659 reported by Guido Vranken.

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Oct 24, 2018

Fix unsafe bounds checks in ssl_load_session()
This commit replaces multiple bounds checks of the form

     `if( ptr + offset > end )`

by

     `if( offset > end - ptr )`.

The former is unsafe as the pointer arithmetic `ptr + offset` can
overflow in case of a large value of `offset` paired with a value
of `ptr` close to the (virtual) address boundary.

The latter bounds check, in turn, is always safe if `offset` is a
signed integral value, even if `ptr` happens to be larger than
`end` (which should never happen, but it's better to be prepared
just in case).

Concretely, ssl_load_session() contains the bounds check

     `if( p + cert_len > end )`

where `cert_len` is a 24-bit value of type `size_t`. This
check is accordingly replaced by  `if( (int) cert_len > end - p )`;
the explicit cast `(int) cert_len` is safe because `int` is assumed
to be 32-bit wide and paddingless, hence capable of holding 24-bit
values.

Fixes ARMmbed#659 reported by Guido Vranken.

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Oct 25, 2018

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Oct 25, 2018

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Oct 25, 2018

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Oct 25, 2018

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Oct 26, 2018

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Oct 26, 2018

@Patater Patater closed this in #2131 Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment