Alter bounds check in ssl_load_session for slightly more safety

[guidovranken]

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]

ARM Internal Ref: IOTSSL-1046