From 6706c539023f9a2dec1aed94f6e18ae1e7877c84 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Sat, 1 Jun 2013 09:09:17 -0400 Subject: [PATCH 1/2] [libfko] HMAC comparison timing bug fix Ryman reported a timing attack bug in the HMAC comparison operation (#85) and suggested a fix derived from YaSSL: http://www.mail-archive.com/debian-bugs-rc@lists.debian.org/msg320402.html --- CREDITS | 5 +++++ lib/fko_hmac.c | 28 +++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/CREDITS b/CREDITS index ce0b4506..00f65291 100644 --- a/CREDITS +++ b/CREDITS @@ -128,3 +128,8 @@ Shawn Wilson Dan Lauber - Suggested a check for fwknopd to ensure that the jump rule on systems running iptables is not duplicated if it already exists. + +Ryman + - Reported a timing attack bug in the HMAC comparison operation (#85) and + suggested a fix derived from YaSSL: + http://www.mail-archive.com/debian-bugs-rc@lists.debian.org/msg320402.html diff --git a/lib/fko_hmac.c b/lib/fko_hmac.c index e3664cb8..59b8f836 100644 --- a/lib/fko_hmac.c +++ b/lib/fko_hmac.c @@ -34,6 +34,32 @@ #include "hmac.h" #include "base64.h" +/* Compare all bytes with constant run time regardless of + * input characteristics (i.e. don't return early if a difference + * is found before comparing all bytes). This code was adapted + * from YaSSL which is GPLv2 after a timing bug was reported by + * Ryman through github (#85) +*/ +static int +constant_runtime_compare(const char *a, const char *b, int len) +{ + int good = 0; + int bad = 0; + int i; + + for(i=0; i < len; i++) { + if (a[i] == b[i]) + good++; + else + bad++; + } + + if (good == len) + return 0; + else + return 0 - bad; +} + int fko_verify_hmac(fko_ctx_t ctx, const char * const hmac_key, const int hmac_key_len) { @@ -131,7 +157,7 @@ int fko_verify_hmac(fko_ctx_t ctx, if(res == FKO_SUCCESS) { - if(strncmp(hmac_digest_from_data, + if(constant_runtime_compare(hmac_digest_from_data, ctx->msg_hmac, hmac_b64_digest_len) != 0) { res = FKO_ERROR_INVALID_DATA; From 54872acfc34542d4ab800d4126a153854228cf11 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Sat, 1 Jun 2013 21:55:45 -0400 Subject: [PATCH 2/2] Convert strncmp() calls to constant_runtime_cmp() at various places This commit is a follow up to Ryman's report (#85) of a potential timing attack that could be leveraged against fwknop when strncmp() is used to compare HMAC digests. All strncmp() calls that do similar things have been replaced with a new constant_runtime_cmp() function that mitigates this problem. --- lib/cipher_funcs.c | 8 ++++---- lib/fko_decode.c | 2 +- lib/fko_hmac.c | 31 +++---------------------------- lib/fko_util.c | 27 +++++++++++++++++++++++++++ lib/fko_util.h | 1 + server/incoming_spa.c | 6 +++--- server/replay_cache.c | 3 ++- 7 files changed, 41 insertions(+), 37 deletions(-) diff --git a/lib/cipher_funcs.c b/lib/cipher_funcs.c index fed6216a..12dd94f2 100644 --- a/lib/cipher_funcs.c +++ b/lib/cipher_funcs.c @@ -320,8 +320,8 @@ add_salted_str(fko_ctx_t ctx) { char *tbuf; - if(strncmp(ctx->encrypted_msg, - B64_RIJNDAEL_SALT, B64_RIJNDAEL_SALT_STR_LEN)) + if(constant_runtime_cmp(ctx->encrypted_msg, + B64_RIJNDAEL_SALT, B64_RIJNDAEL_SALT_STR_LEN) != 0) { /* We need to realloc space for the salt. */ @@ -356,8 +356,8 @@ add_gpg_prefix(fko_ctx_t ctx) { char *tbuf; - if(strncmp(ctx->encrypted_msg, - B64_GPG_PREFIX, B64_GPG_PREFIX_STR_LEN)) + if(constant_runtime_cmp(ctx->encrypted_msg, + B64_GPG_PREFIX, B64_GPG_PREFIX_STR_LEN) != 0) { /* We need to realloc space for the prefix. */ diff --git a/lib/fko_decode.c b/lib/fko_decode.c index 5b1da332..ad2a4226 100644 --- a/lib/fko_decode.c +++ b/lib/fko_decode.c @@ -150,7 +150,7 @@ fko_decode_spa_data(fko_ctx_t ctx) /* We give up here if the computed digest does not match the * digest in the message data. */ - if(strncmp(ctx->digest, tbuf, t_size)) + if(constant_runtime_cmp(ctx->digest, tbuf, t_size) != 0) { free(tbuf); return(FKO_ERROR_DIGEST_VERIFICATION_FAILED); diff --git a/lib/fko_hmac.c b/lib/fko_hmac.c index 59b8f836..050d6ee0 100644 --- a/lib/fko_hmac.c +++ b/lib/fko_hmac.c @@ -34,33 +34,8 @@ #include "hmac.h" #include "base64.h" -/* Compare all bytes with constant run time regardless of - * input characteristics (i.e. don't return early if a difference - * is found before comparing all bytes). This code was adapted - * from YaSSL which is GPLv2 after a timing bug was reported by - * Ryman through github (#85) -*/ -static int -constant_runtime_compare(const char *a, const char *b, int len) -{ - int good = 0; - int bad = 0; - int i; - - for(i=0; i < len; i++) { - if (a[i] == b[i]) - good++; - else - bad++; - } - - if (good == len) - return 0; - else - return 0 - bad; -} - -int fko_verify_hmac(fko_ctx_t ctx, +int +fko_verify_hmac(fko_ctx_t ctx, const char * const hmac_key, const int hmac_key_len) { char *hmac_digest_from_data = NULL; @@ -157,7 +132,7 @@ int fko_verify_hmac(fko_ctx_t ctx, if(res == FKO_SUCCESS) { - if(constant_runtime_compare(hmac_digest_from_data, + if(constant_runtime_cmp(hmac_digest_from_data, ctx->msg_hmac, hmac_b64_digest_len) != 0) { res = FKO_ERROR_INVALID_DATA; diff --git a/lib/fko_util.c b/lib/fko_util.c index 0e54acff..de923657 100644 --- a/lib/fko_util.c +++ b/lib/fko_util.c @@ -62,6 +62,33 @@ static fko_enc_mode_str_t fko_enc_mode_strs[] = { "legacy", FKO_ENC_MODE_CBC_LEGACY_IV, FKO_ENC_MODE_SUPPORTED } }; +/* Compare all bytes with constant run time regardless of + * input characteristics (i.e. don't return early if a difference + * is found before comparing all bytes). This code was adapted + * from YaSSL which is GPLv2 after a timing bug was reported by + * Ryman through github (#85) +*/ +int +constant_runtime_cmp(const char *a, const char *b, int len) +{ + int good = 0; + int bad = 0; + int i; + + for(i=0; i < len; i++) { + if (a[i] == b[i]) + good++; + else + bad++; + } + + if (good == len) + return 0; + else + return 0 - bad; +} + + /* Validate encoded message length */ int diff --git a/lib/fko_util.h b/lib/fko_util.h index 7ea67fb8..483661fd 100644 --- a/lib/fko_util.h +++ b/lib/fko_util.h @@ -44,6 +44,7 @@ short digest_strtoint(const char *dt_str); short digest_inttostr(int digest, char* digest_str, size_t digest_size); short hmac_digest_strtoint(const char *dt_str); short hmac_digest_inttostr(int digest, char* digest_str, size_t digest_size); +int constant_runtime_cmp(const char *a, const char *b, int len); const char * enc_type_inttostr(const int type); const char * msg_type_inttostr(const int type); diff --git a/server/incoming_spa.c b/server/incoming_spa.c index 58b2cb06..dcad94b8 100644 --- a/server/incoming_spa.c +++ b/server/incoming_spa.c @@ -78,11 +78,11 @@ preprocess_spa_data(fko_srv_options_t *opts, const char *src_ip) * a prefix after the outer one is stripped off won't decrypt properly * anyway because libfko would not add a new one. */ - if(strncmp(ndx, B64_RIJNDAEL_SALT, B64_RIJNDAEL_SALT_STR_LEN) == 0) + if(constant_runtime_cmp(ndx, B64_RIJNDAEL_SALT, B64_RIJNDAEL_SALT_STR_LEN) == 0) return(SPA_MSG_BAD_DATA); if(pkt_data_len > MIN_GNUPG_MSG_SIZE - && strncmp(ndx, B64_GPG_PREFIX, B64_GPG_PREFIX_STR_LEN) == 0) + && constant_runtime_cmp(ndx, B64_GPG_PREFIX, B64_GPG_PREFIX_STR_LEN) == 0) return(SPA_MSG_BAD_DATA); /* Detect and parse out SPA data from an HTTP request. If the SPA data @@ -525,7 +525,7 @@ incoming_spa(fko_srv_options_t *opts) /* Add this SPA packet into the replay detection cache */ - if (! added_replay_digest) + if (added_replay_digest == 0) { res = add_replay(opts, raw_digest); if (res != SPA_MSG_SUCCESS) diff --git a/server/replay_cache.c b/server/replay_cache.c index 9dd4f822..10e7b2e6 100644 --- a/server/replay_cache.c +++ b/server/replay_cache.c @@ -490,7 +490,8 @@ is_replay_file_cache(fko_srv_options_t *opts, char *digest) digest_list_ptr != NULL; digest_list_ptr = digest_list_ptr->next) { - if (strncmp(digest_list_ptr->cache_info.digest, digest, digest_len) == 0) { + if (constant_runtime_cmp(digest_list_ptr->cache_info.digest, + digest, digest_len) == 0) { replay_warning(opts, &(digest_list_ptr->cache_info));