From 7b6299b49b1663bbc7c50c74dfd7574763970994 Mon Sep 17 00:00:00 2001 From: Pengyu Lv Date: Tue, 7 Mar 2023 14:38:45 +0800 Subject: [PATCH 1/7] ssl_cache: Add an interface to remove cache entry by session id Signed-off-by: Pengyu Lv --- include/mbedtls/ssl_cache.h | 17 ++++++++++++ library/ssl_cache.c | 53 +++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/include/mbedtls/ssl_cache.h b/include/mbedtls/ssl_cache.h index 5cd1cd3f3..41ada540b 100644 --- a/include/mbedtls/ssl_cache.h +++ b/include/mbedtls/ssl_cache.h @@ -123,6 +123,23 @@ int mbedtls_ssl_cache_set(void *data, size_t session_id_len, const mbedtls_ssl_session *session); +/** + * \brief Remove the cache entry by the session ID + * (Thread-safe if MBEDTLS_THREADING_C is enabled) + * + * \param data The SSL cache context to use. + * \param session_id The pointer to the buffer holding the session ID + * associated to \p session. + * \param session_id_len The length of \p session_id in bytes. + * + * \return 0: The cache entry for session with provided ID + * is removed or does not exist. + * 1: Internal error. + */ +int mbedtls_ssl_cache_remove(void *data, + unsigned char const *session_id, + size_t session_id_len); + #if defined(MBEDTLS_HAVE_TIME) /** * \brief Set the cache timeout diff --git a/library/ssl_cache.c b/library/ssl_cache.c index 7c16e10e9..ff36b05ca 100644 --- a/library/ssl_cache.c +++ b/library/ssl_cache.c @@ -314,6 +314,59 @@ exit: return ret; } +int mbedtls_ssl_cache_remove(void *data, + unsigned char const *session_id, + size_t session_id_len) +{ + int ret = 1; + mbedtls_ssl_cache_context *cache = (mbedtls_ssl_cache_context *) data; + mbedtls_ssl_cache_entry *entry; + mbedtls_ssl_cache_entry *prev; + +#if defined(MBEDTLS_THREADING_C) + if (mbedtls_mutex_lock(&cache->mutex) != 0) { + return 1; + } +#endif + + ret = ssl_cache_find_entry(cache, session_id, session_id_len, &entry); + /* No valid entry found, exit with success */ + if (ret != 0) { + ret = 0; + goto exit; + } + + /* Now we remove the entry from the chain */ + if (entry == cache->chain) { + cache->chain = entry->next; + goto free; + } + for (prev = cache->chain; prev->next != NULL; prev = prev->next) { + if (prev->next == entry) { + prev->next = entry->next; + break; + } + } + +free: + if (entry->session != NULL) { + mbedtls_platform_zeroize(entry->session, entry->session_len); + mbedtls_free(entry->session); + } + mbedtls_platform_zeroize(entry, sizeof(mbedtls_ssl_cache_entry)); + mbedtls_free(entry); + ret = 0; + +exit: +#if defined(MBEDTLS_THREADING_C) + if (mbedtls_mutex_unlock(&cache->mutex) != 0) { + ret = 1; + } +#endif + + return ret; +} + #if defined(MBEDTLS_HAVE_TIME) void mbedtls_ssl_cache_set_timeout(mbedtls_ssl_cache_context *cache, int timeout) { From 753d02ffd41a228ef0d2f87325071dbb5ceeaf0c Mon Sep 17 00:00:00 2001 From: Pengyu Lv Date: Tue, 7 Mar 2023 14:51:09 +0800 Subject: [PATCH 2/7] ssl_server2: Add options to support cache removal Signed-off-by: Pengyu Lv --- programs/ssl/ssl_server2.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 2fa9a8133..e67ad8b07 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -127,6 +127,7 @@ int main(void) #define DFL_TICKET_AEAD MBEDTLS_CIPHER_AES_256_GCM #define DFL_CACHE_MAX -1 #define DFL_CACHE_TIMEOUT -1 +#define DFL_CACHE_REMOVE 0 #define DFL_SNI NULL #define DFL_ALPN_STRING NULL #define DFL_CURVES NULL @@ -326,9 +327,12 @@ int main(void) #else #define USAGE_CACHE_TIME "" #endif +#define USAGE_CACHE_REMOVE \ + " cache_remove=%%d default: 0 (disabled)\n" #else #define USAGE_CACHE "" #define USAGE_CACHE_TIME "" +#define USAGE_CACHE_REMOVE "" #endif /* MBEDTLS_SSL_CACHE_C */ #if defined(SNI_OPTION) @@ -549,6 +553,7 @@ int main(void) USAGE_NSS_KEYLOG_FILE \ USAGE_CACHE \ USAGE_CACHE_TIME \ + USAGE_CACHE_REMOVE \ USAGE_MAX_FRAG_LEN \ USAGE_ALPN \ USAGE_EMS \ @@ -667,6 +672,7 @@ struct options { #if defined(MBEDTLS_HAVE_TIME) int cache_timeout; /* expiration delay of session cache entries*/ #endif + int cache_remove; /* enable / disable cache removement */ char *sni; /* string describing sni information */ const char *curves; /* list of supported elliptic curves */ const char *sig_algs; /* supported TLS 1.3 signature algorithms */ @@ -1729,6 +1735,7 @@ usage: #if defined(MBEDTLS_HAVE_TIME) opt.cache_timeout = DFL_CACHE_TIMEOUT; #endif + opt.cache_remove = DFL_CACHE_REMOVE; opt.sni = DFL_SNI; opt.alpn_string = DFL_ALPN_STRING; opt.curves = DFL_CURVES; @@ -2142,7 +2149,12 @@ usage: } } #endif - else if (strcmp(p, "cookies") == 0) { + else if (strcmp(p, "cache_remove") == 0) { + opt.cache_remove = atoi(q); + if (opt.cache_remove < 0 || opt.cache_remove > 1) { + goto usage; + } + } else if (strcmp(p, "cookies") == 0) { opt.cookies = atoi(q); if (opt.cookies < -1 || opt.cookies > 1) { goto usage; @@ -4125,6 +4137,12 @@ close_notify: mbedtls_printf(" done\n"); +#if defined(MBEDTLS_SSL_CACHE_C) + if (opt.cache_remove > 0) { + mbedtls_ssl_cache_remove(&cache, ssl.session->id, ssl.session->id_len); + } +#endif + goto reset; /* From 62ed1aa3164f8592489f094a4668a10cac5a7630 Mon Sep 17 00:00:00 2001 From: Pengyu Lv Date: Tue, 7 Mar 2023 14:52:47 +0800 Subject: [PATCH 3/7] ssl-opt: Add test for cache entry removal Signed-off-by: Pengyu Lv --- tests/ssl-opt.sh | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index c176d0d62..0c87da3f3 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -4075,6 +4075,22 @@ run_test "Session resume using cache: cache_max=1" \ -s "a session has been resumed" \ -c "a session has been resumed" +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 +requires_config_enabled MBEDTLS_SSL_CACHE_C +run_test "Session resume using cache: cache removed" \ + "$P_SRV debug_level=3 tickets=0 cache_remove=1" \ + "$P_CLI debug_level=3 tickets=0 reconnect=1" \ + 0 \ + -C "client hello, adding session ticket extension" \ + -S "found session ticket extension" \ + -S "server hello, adding session ticket extension" \ + -C "found session_ticket extension" \ + -C "parse new session ticket" \ + -S "session successfully restored from cache" \ + -S "session successfully restored from ticket" \ + -S "a session has been resumed" \ + -C "a session has been resumed" + requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 requires_config_enabled MBEDTLS_SSL_CACHE_C run_test "Session resume using cache: timeout > delay" \ From f30488f5cde80eec4b50384935b42f9379d7825f Mon Sep 17 00:00:00 2001 From: Pengyu Lv Date: Wed, 15 Mar 2023 09:53:45 +0800 Subject: [PATCH 4/7] Move the usage string of cache_remove to USAGE_CACHE Signed-off-by: Pengyu Lv --- programs/ssl/ssl_server2.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index e67ad8b07..8277ddee1 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -320,19 +320,17 @@ int main(void) #if defined(MBEDTLS_SSL_CACHE_C) #define USAGE_CACHE \ - " cache_max=%%d default: cache default (50)\n" + " cache_max=%%d default: cache default (50)\n" \ + " cache_remove=%%d default: 0 (don't remove)\n" #if defined(MBEDTLS_HAVE_TIME) #define USAGE_CACHE_TIME \ " cache_timeout=%%d default: cache default (1d)\n" #else #define USAGE_CACHE_TIME "" #endif -#define USAGE_CACHE_REMOVE \ - " cache_remove=%%d default: 0 (disabled)\n" #else #define USAGE_CACHE "" #define USAGE_CACHE_TIME "" -#define USAGE_CACHE_REMOVE "" #endif /* MBEDTLS_SSL_CACHE_C */ #if defined(SNI_OPTION) @@ -553,7 +551,6 @@ int main(void) USAGE_NSS_KEYLOG_FILE \ USAGE_CACHE \ USAGE_CACHE_TIME \ - USAGE_CACHE_REMOVE \ USAGE_MAX_FRAG_LEN \ USAGE_ALPN \ USAGE_EMS \ From 744b507866bc8b31177d7dde81ddfdb378f60d1f Mon Sep 17 00:00:00 2001 From: Pengyu Lv Date: Wed, 15 Mar 2023 12:17:14 +0800 Subject: [PATCH 5/7] ssl_cache: use auxiliary function to zeroize cache entry This commit introduce a auxiliary function to zeroize the cache entry, especially the session structure. The function is called wherever we need to free the entry. Signed-off-by: Pengyu Lv --- library/ssl_cache.c | 43 ++++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/library/ssl_cache.c b/library/ssl_cache.c index ff36b05ca..d93508c6b 100644 --- a/library/ssl_cache.c +++ b/library/ssl_cache.c @@ -121,6 +121,23 @@ exit: return ret; } +/* zeroize a cache entry */ +static void ssl_cache_entry_zeroize(mbedtls_ssl_cache_entry *entry) +{ + if (entry == NULL) { + return; + } + + /* zeroize and free session structure */ + if (entry->session != NULL) { + mbedtls_platform_zeroize(entry->session, entry->session_len); + mbedtls_free(entry->session); + } + + /* zeroize the whole entry structure */ + mbedtls_platform_zeroize(entry, sizeof(mbedtls_ssl_cache_entry)); +} + MBEDTLS_CHECK_RETURN_CRITICAL static int ssl_cache_pick_writing_slot(mbedtls_ssl_cache_context *cache, unsigned char const *session_id, @@ -220,19 +237,19 @@ static int ssl_cache_pick_writing_slot(mbedtls_ssl_cache_context *cache, found: + /* If we're reusing an entry, free it first. */ + if (cur->session != NULL) { + /* `ssl_cache_entry_zeroize` would break the chain, + * so we reuse `old` to record `next` temporarily. */ + old = cur->next; + ssl_cache_entry_zeroize(cur); + cur->next = old; + } + #if defined(MBEDTLS_HAVE_TIME) cur->timestamp = t; #endif - /* If we're reusing an entry, free it first. */ - if (cur->session != NULL) { - mbedtls_free(cur->session); - cur->session = NULL; - cur->session_len = 0; - memset(cur->session_id, 0, sizeof(cur->session_id)); - cur->session_id_len = 0; - } - *dst = cur; return 0; } @@ -349,11 +366,7 @@ int mbedtls_ssl_cache_remove(void *data, } free: - if (entry->session != NULL) { - mbedtls_platform_zeroize(entry->session, entry->session_len); - mbedtls_free(entry->session); - } - mbedtls_platform_zeroize(entry, sizeof(mbedtls_ssl_cache_entry)); + ssl_cache_entry_zeroize(entry); mbedtls_free(entry); ret = 0; @@ -397,7 +410,7 @@ void mbedtls_ssl_cache_free(mbedtls_ssl_cache_context *cache) prv = cur; cur = cur->next; - mbedtls_free(prv->session); + ssl_cache_entry_zeroize(prv); mbedtls_free(prv); } From 0b9c012f21b341f7d14cc5aff4e1684ea7851721 Mon Sep 17 00:00:00 2001 From: Pengyu Lv Date: Wed, 15 Mar 2023 14:37:32 +0800 Subject: [PATCH 6/7] ssl_cache: return the error code for mutex failure Signed-off-by: Pengyu Lv --- include/mbedtls/ssl_cache.h | 2 +- library/ssl_cache.c | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/include/mbedtls/ssl_cache.h b/include/mbedtls/ssl_cache.h index 41ada540b..55dcf77c3 100644 --- a/include/mbedtls/ssl_cache.h +++ b/include/mbedtls/ssl_cache.h @@ -134,7 +134,7 @@ int mbedtls_ssl_cache_set(void *data, * * \return 0: The cache entry for session with provided ID * is removed or does not exist. - * 1: Internal error. + * Otherwise: fail. */ int mbedtls_ssl_cache_remove(void *data, unsigned char const *session_id, diff --git a/library/ssl_cache.c b/library/ssl_cache.c index d93508c6b..048c21d4f 100644 --- a/library/ssl_cache.c +++ b/library/ssl_cache.c @@ -92,8 +92,8 @@ int mbedtls_ssl_cache_get(void *data, mbedtls_ssl_cache_entry *entry; #if defined(MBEDTLS_THREADING_C) - if (mbedtls_mutex_lock(&cache->mutex) != 0) { - return 1; + if ((ret = mbedtls_mutex_lock(&cache->mutex)) != 0) { + return ret; } #endif @@ -114,7 +114,7 @@ int mbedtls_ssl_cache_get(void *data, exit: #if defined(MBEDTLS_THREADING_C) if (mbedtls_mutex_unlock(&cache->mutex) != 0) { - ret = 1; + ret = MBEDTLS_ERR_THREADING_MUTEX_ERROR; } #endif @@ -318,7 +318,7 @@ int mbedtls_ssl_cache_set(void *data, exit: #if defined(MBEDTLS_THREADING_C) if (mbedtls_mutex_unlock(&cache->mutex) != 0) { - ret = 1; + ret = MBEDTLS_ERR_THREADING_MUTEX_ERROR; } #endif @@ -341,8 +341,8 @@ int mbedtls_ssl_cache_remove(void *data, mbedtls_ssl_cache_entry *prev; #if defined(MBEDTLS_THREADING_C) - if (mbedtls_mutex_lock(&cache->mutex) != 0) { - return 1; + if ((ret = mbedtls_mutex_lock(&cache->mutex)) != 0) { + return ret; } #endif @@ -373,7 +373,7 @@ free: exit: #if defined(MBEDTLS_THREADING_C) if (mbedtls_mutex_unlock(&cache->mutex) != 0) { - ret = 1; + ret = MBEDTLS_ERR_THREADING_MUTEX_ERROR; } #endif From db47f2fbd4e12eb83e843b0146e466d209dae9c3 Mon Sep 17 00:00:00 2001 From: Pengyu Lv Date: Wed, 15 Mar 2023 15:01:36 +0800 Subject: [PATCH 7/7] Add changelog entry for new API Signed-off-by: Pengyu Lv --- ChangeLog.d/add-cache-remove-api.txt | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ChangeLog.d/add-cache-remove-api.txt diff --git a/ChangeLog.d/add-cache-remove-api.txt b/ChangeLog.d/add-cache-remove-api.txt new file mode 100644 index 000000000..950ff9730 --- /dev/null +++ b/ChangeLog.d/add-cache-remove-api.txt @@ -0,0 +1,5 @@ +Features + * Add new API mbedtls_ssl_cache_remove for cache entry removal by + its session id. +Security + * Zeroize SSL cache entries when they are freed.