diff options
author | tb <> | 2020-09-01 17:25:17 +0000 |
---|---|---|
committer | tb <> | 2020-09-01 17:25:17 +0000 |
commit | a528105b51540c5edf59368d36bf78b914cd2899 (patch) | |
tree | 923a83ec6bfc6873fcf70ec3684be8700290615e /src/lib/libssl/ssl_sess.c | |
parent | 1e6510105e17f4686509b6cef5e4a607664dd5c0 (diff) | |
download | openbsd-a528105b51540c5edf59368d36bf78b914cd2899.tar.gz openbsd-a528105b51540c5edf59368d36bf78b914cd2899.tar.bz2 openbsd-a528105b51540c5edf59368d36bf78b914cd2899.zip |
Split session retrieval out of ssl_get_prev_session()
In case the session ticket was empty or missing, an attempt is made to
retrieve the session from the internal cache or via a callback. This
code can easily be flattened a bit and factored into two functions. I
decided to wrap those into a third function to make the call from the
switch easier on the eye.
I could have kept the try_session_cache flag, but it now seems rather
pointless and awkwardly named anyway, so I took its negation and named
it ticket_decrypted.
To top things off, a little bit of polish in the exit path.
ok beck inoguchi jsing (with the usual healthy dose of nits)
Diffstat (limited to 'src/lib/libssl/ssl_sess.c')
-rw-r--r-- | src/lib/libssl/ssl_sess.c | 170 |
1 files changed, 92 insertions, 78 deletions
diff --git a/src/lib/libssl/ssl_sess.c b/src/lib/libssl/ssl_sess.c index 460c5d85f1..327164da25 100644 --- a/src/lib/libssl/ssl_sess.c +++ b/src/lib/libssl/ssl_sess.c | |||
@@ -1,4 +1,4 @@ | |||
1 | /* $OpenBSD: ssl_sess.c,v 1.92 2020/09/01 12:40:53 tb Exp $ */ | 1 | /* $OpenBSD: ssl_sess.c,v 1.93 2020/09/01 17:25:17 tb Exp $ */ |
2 | /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) | 2 | /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) |
3 | * All rights reserved. | 3 | * All rights reserved. |
4 | * | 4 | * |
@@ -413,6 +413,84 @@ ssl_get_new_session(SSL *s, int session) | |||
413 | return (1); | 413 | return (1); |
414 | } | 414 | } |
415 | 415 | ||
416 | static SSL_SESSION * | ||
417 | ssl_session_from_cache(SSL *s, CBS *session_id) | ||
418 | { | ||
419 | SSL_SESSION *sess; | ||
420 | SSL_SESSION data; | ||
421 | |||
422 | if ((s->session_ctx->internal->session_cache_mode & | ||
423 | SSL_SESS_CACHE_NO_INTERNAL_LOOKUP)) | ||
424 | return NULL; | ||
425 | |||
426 | data.ssl_version = s->version; | ||
427 | data.session_id_length = CBS_len(session_id); | ||
428 | memcpy(data.session_id, CBS_data(session_id), CBS_len(session_id)); | ||
429 | |||
430 | CRYPTO_r_lock(CRYPTO_LOCK_SSL_CTX); | ||
431 | sess = lh_SSL_SESSION_retrieve(s->session_ctx->internal->sessions, &data); | ||
432 | if (sess != NULL) | ||
433 | CRYPTO_add(&sess->references, 1, CRYPTO_LOCK_SSL_SESSION); | ||
434 | CRYPTO_r_unlock(CRYPTO_LOCK_SSL_CTX); | ||
435 | |||
436 | if (sess == NULL) | ||
437 | s->session_ctx->internal->stats.sess_miss++; | ||
438 | |||
439 | return sess; | ||
440 | } | ||
441 | |||
442 | static SSL_SESSION * | ||
443 | ssl_session_from_callback(SSL *s, CBS *session_id) | ||
444 | { | ||
445 | SSL_SESSION *sess; | ||
446 | int copy; | ||
447 | |||
448 | if (s->session_ctx->internal->get_session_cb == NULL) | ||
449 | return NULL; | ||
450 | |||
451 | copy = 1; | ||
452 | if ((sess = s->session_ctx->internal->get_session_cb(s, | ||
453 | CBS_data(session_id), CBS_len(session_id), ©)) == NULL) | ||
454 | return NULL; | ||
455 | |||
456 | s->session_ctx->internal->stats.sess_cb_hit++; | ||
457 | |||
458 | /* | ||
459 | * The copy handler may have set copy == 0 to indicate that the session | ||
460 | * structures are shared between threads and that it handles the | ||
461 | * reference count itself. If it didn't set copy to zero, we must | ||
462 | * increment the reference count. | ||
463 | */ | ||
464 | if (copy) | ||
465 | CRYPTO_add(&sess->references, 1, CRYPTO_LOCK_SSL_SESSION); | ||
466 | |||
467 | /* Add the externally cached session to the internal cache as well. */ | ||
468 | if (!(s->session_ctx->internal->session_cache_mode & | ||
469 | SSL_SESS_CACHE_NO_INTERNAL_STORE)) { | ||
470 | /* | ||
471 | * The following should not return 1, | ||
472 | * otherwise, things are very strange. | ||
473 | */ | ||
474 | SSL_CTX_add_session(s->session_ctx, sess); | ||
475 | } | ||
476 | |||
477 | return sess; | ||
478 | } | ||
479 | |||
480 | static SSL_SESSION * | ||
481 | ssl_session_by_id(SSL *s, CBS *session_id) | ||
482 | { | ||
483 | SSL_SESSION *sess; | ||
484 | |||
485 | if (CBS_len(session_id) == 0) | ||
486 | return NULL; | ||
487 | |||
488 | if ((sess = ssl_session_from_cache(s, session_id)) == NULL) | ||
489 | sess = ssl_session_from_callback(s, session_id); | ||
490 | |||
491 | return sess; | ||
492 | } | ||
493 | |||
416 | /* | 494 | /* |
417 | * ssl_get_prev attempts to find an SSL_SESSION to be used to resume this | 495 | * ssl_get_prev attempts to find an SSL_SESSION to be used to resume this |
418 | * connection. It is only called by servers. | 496 | * connection. It is only called by servers. |
@@ -439,16 +517,13 @@ ssl_get_prev_session(SSL *s, CBS *session_id, CBS *ext_block, int *alert) | |||
439 | SSL_SESSION *sess = NULL; | 517 | SSL_SESSION *sess = NULL; |
440 | size_t session_id_len; | 518 | size_t session_id_len; |
441 | int alert_desc = SSL_AD_INTERNAL_ERROR, fatal = 0; | 519 | int alert_desc = SSL_AD_INTERNAL_ERROR, fatal = 0; |
442 | int try_session_cache = 1; | 520 | int ticket_decrypted = 0; |
443 | 521 | ||
444 | /* This is used only by servers. */ | 522 | /* This is used only by servers. */ |
445 | 523 | ||
446 | if (CBS_len(session_id) > SSL_MAX_SSL_SESSION_ID_LENGTH) | 524 | if (CBS_len(session_id) > SSL_MAX_SSL_SESSION_ID_LENGTH) |
447 | goto err; | 525 | goto err; |
448 | 526 | ||
449 | if (CBS_len(session_id) == 0) | ||
450 | try_session_cache = 0; | ||
451 | |||
452 | /* Sets s->internal->tlsext_ticket_expected. */ | 527 | /* Sets s->internal->tlsext_ticket_expected. */ |
453 | switch (tls1_process_ticket(s, ext_block, &alert_desc, &sess)) { | 528 | switch (tls1_process_ticket(s, ext_block, &alert_desc, &sess)) { |
454 | case TLS1_TICKET_FATAL_ERROR: | 529 | case TLS1_TICKET_FATAL_ERROR: |
@@ -456,12 +531,13 @@ ssl_get_prev_session(SSL *s, CBS *session_id, CBS *ext_block, int *alert) | |||
456 | goto err; | 531 | goto err; |
457 | case TLS1_TICKET_NONE: | 532 | case TLS1_TICKET_NONE: |
458 | case TLS1_TICKET_EMPTY: | 533 | case TLS1_TICKET_EMPTY: |
459 | break; /* Ok to carry on processing session id. */ | 534 | if ((sess = ssl_session_by_id(s, session_id)) == NULL) |
535 | goto err; | ||
536 | break; | ||
460 | case TLS1_TICKET_NOT_DECRYPTED: | 537 | case TLS1_TICKET_NOT_DECRYPTED: |
461 | try_session_cache = 0; | ||
462 | goto err; | 538 | goto err; |
463 | case TLS1_TICKET_DECRYPTED: | 539 | case TLS1_TICKET_DECRYPTED: |
464 | try_session_cache = 0; | 540 | ticket_decrypted = 1; |
465 | 541 | ||
466 | /* | 542 | /* |
467 | * The session ID is used by some clients to detect that the | 543 | * The session ID is used by some clients to detect that the |
@@ -480,66 +556,6 @@ ssl_get_prev_session(SSL *s, CBS *session_id, CBS *ext_block, int *alert) | |||
480 | goto err; | 556 | goto err; |
481 | } | 557 | } |
482 | 558 | ||
483 | if (try_session_cache && sess == NULL && | ||
484 | !(s->session_ctx->internal->session_cache_mode & | ||
485 | SSL_SESS_CACHE_NO_INTERNAL_LOOKUP)) { | ||
486 | SSL_SESSION data; | ||
487 | |||
488 | data.ssl_version = s->version; | ||
489 | data.session_id_length = CBS_len(session_id); | ||
490 | memcpy(data.session_id, CBS_data(session_id), | ||
491 | CBS_len(session_id)); | ||
492 | |||
493 | CRYPTO_r_lock(CRYPTO_LOCK_SSL_CTX); | ||
494 | sess = lh_SSL_SESSION_retrieve(s->session_ctx->internal->sessions, &data); | ||
495 | if (sess != NULL) { | ||
496 | /* Don't allow other threads to steal it. */ | ||
497 | CRYPTO_add(&sess->references, 1, | ||
498 | CRYPTO_LOCK_SSL_SESSION); | ||
499 | } | ||
500 | CRYPTO_r_unlock(CRYPTO_LOCK_SSL_CTX); | ||
501 | |||
502 | if (sess == NULL) | ||
503 | s->session_ctx->internal->stats.sess_miss++; | ||
504 | } | ||
505 | |||
506 | if (try_session_cache && sess == NULL && | ||
507 | s->session_ctx->internal->get_session_cb != NULL) { | ||
508 | int copy = 1; | ||
509 | |||
510 | if ((sess = s->session_ctx->internal->get_session_cb(s, | ||
511 | CBS_data(session_id), CBS_len(session_id), ©))) { | ||
512 | s->session_ctx->internal->stats.sess_cb_hit++; | ||
513 | |||
514 | /* | ||
515 | * Increment reference count now if the session | ||
516 | * callback asks us to do so (note that if the session | ||
517 | * structures returned by the callback are shared | ||
518 | * between threads, it must handle the reference count | ||
519 | * itself [i.e. copy == 0], or things won't be | ||
520 | * thread-safe). | ||
521 | */ | ||
522 | if (copy) | ||
523 | CRYPTO_add(&sess->references, 1, | ||
524 | CRYPTO_LOCK_SSL_SESSION); | ||
525 | |||
526 | /* | ||
527 | * Add the externally cached session to the internal | ||
528 | * cache as well if and only if we are supposed to. | ||
529 | */ | ||
530 | if (!(s->session_ctx->internal->session_cache_mode & | ||
531 | SSL_SESS_CACHE_NO_INTERNAL_STORE)) | ||
532 | /* | ||
533 | * The following should not return 1, | ||
534 | * otherwise, things are very strange. | ||
535 | */ | ||
536 | SSL_CTX_add_session(s->session_ctx, sess); | ||
537 | } | ||
538 | } | ||
539 | |||
540 | if (sess == NULL) | ||
541 | goto err; | ||
542 | |||
543 | /* Now sess is non-NULL and we own one of its reference counts. */ | 559 | /* Now sess is non-NULL and we own one of its reference counts. */ |
544 | 560 | ||
545 | if (sess->sid_ctx_length != s->sid_ctx_length || | 561 | if (sess->sid_ctx_length != s->sid_ctx_length || |
@@ -576,7 +592,7 @@ ssl_get_prev_session(SSL *s, CBS *session_id, CBS *ext_block, int *alert) | |||
576 | if (sess->timeout < (time(NULL) - sess->time)) { | 592 | if (sess->timeout < (time(NULL) - sess->time)) { |
577 | /* timeout */ | 593 | /* timeout */ |
578 | s->session_ctx->internal->stats.sess_timeout++; | 594 | s->session_ctx->internal->stats.sess_timeout++; |
579 | if (try_session_cache) { | 595 | if (!ticket_decrypted) { |
580 | /* session was from the cache, so remove it */ | 596 | /* session was from the cache, so remove it */ |
581 | SSL_CTX_remove_session(s->session_ctx, sess); | 597 | SSL_CTX_remove_session(s->session_ctx, sess); |
582 | } | 598 | } |
@@ -591,15 +607,13 @@ ssl_get_prev_session(SSL *s, CBS *session_id, CBS *ext_block, int *alert) | |||
591 | return 1; | 607 | return 1; |
592 | 608 | ||
593 | err: | 609 | err: |
594 | if (sess != NULL) { | 610 | SSL_SESSION_free(sess); |
595 | SSL_SESSION_free(sess); | 611 | if (ticket_decrypted) { |
596 | if (!try_session_cache) { | 612 | /* |
597 | /* | 613 | * The session was from a ticket. Issue a ticket for the new |
598 | * The session was from a ticket, so we should | 614 | * session. |
599 | * issue a ticket for the new session. | 615 | */ |
600 | */ | 616 | s->internal->tlsext_ticket_expected = 1; |
601 | s->internal->tlsext_ticket_expected = 1; | ||
602 | } | ||
603 | } | 617 | } |
604 | if (fatal) { | 618 | if (fatal) { |
605 | *alert = alert_desc; | 619 | *alert = alert_desc; |