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

ssl_ticket_update_keys removes keys newer than 1 second #1968

Closed
hanno-becker opened this issue Aug 20, 2018 · 1 comment · Fixed by #1970
Closed

ssl_ticket_update_keys removes keys newer than 1 second #1968

hanno-becker opened this issue Aug 20, 2018 · 1 comment · Fixed by #1970

Comments

@hanno-becker
Copy link

Context: ssl_ticket_update_keys() checks the lifetime of keys and generates fresh ones on expiration:

static int ssl_ticket_update_keys( mbedtls_ssl_ticket_context *ctx )
{
#if !defined(MBEDTLS_HAVE_TIME)
    ((void) ctx);
#else
    if( ctx->ticket_lifetime != 0 )
    {
        uint32_t current_time = (uint32_t) mbedtls_time( NULL );
        uint32_t key_time = ctx->keys[ctx->active].generation_time;

        if( current_time > key_time &&
            current_time - key_time < ctx->ticket_lifetime )
        {
            return( 0 );
        }

        ctx->active = 1 - ctx->active;

        return( ssl_ticket_gen_key( ctx, ctx->active ) );
    }
    else
#endif /* MBEDTLS_HAVE_TIME */
        return( 0 );
}

Issue: In the check

        if( current_time > key_time &&
            current_time - key_time < ctx->ticket_lifetime )
        {
            return( 0 );
        }

a key is considered expired if ssl_ticket_update_keys() is called within a second after key generation. Instead, the check should read as follows:

        if( current_time >= key_time &&
            current_time - key_time < ctx->ticket_lifetime )
        {
            return( 0 );
        }

Impact: Ticket keys might be invalidated too early, leading to rejection of all attempts to resume sessions through tickets that were protected through them.

This does currently not affect the relevant tests in ssl-opt.sh because when the server parses the second ClientHello and calls ssl_ticket_update_keys(), it only invalidates one of the two keys stored internally, keeping the one that was used to encrypt the session ticket.

However, if session resumption was multiple times through the same ticket within less than a second, the key would be invalidated, and session resumption would fail. This can be witnessed by using reconnect=2 in ssl_client2, and making sure that mbedtls_ssl_get_session() (saving the session to resume subsequently) is only called after the first handshake. Alternatively, when testing ticket-based session resumption in DTLS -- where the ClientHello sent for session resumption doesn't contain a valid ticket -- the failure can be witnessed, because the client sends two ClientHellos on session resumption, one without and one with a cookie.

@hanno-becker hanno-becker changed the title ssl_ticket_update_keys removes tickets newer than 1 second ssl_ticket_update_keys removes keys newer than 1 second Aug 20, 2018
@ciarmcom
Copy link

ARM Internal Ref: IOTSSL-2482

hanno-becker pushed a commit to hanno-becker/mbedtls that referenced this issue Aug 21, 2018
hanno-becker pushed a commit to hanno-becker/mbedtls that referenced this issue Aug 22, 2018
hanno-becker pushed a commit to hanno-becker/mbedtls that referenced this issue Aug 22, 2018
hanno-becker pushed a commit to hanno-becker/mbedtls that referenced this issue Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants