Merge branch 'hmac_timing_bug_fix'

Fixes #85

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

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.
         */

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);

lib/fko_hmac.c

@@ -34,7 +34,8 @@
 #include "hmac.h"
 #include "base64.h"
 
-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;
@@ -131,7 +132,7 @@ int fko_verify_hmac(fko_ctx_t ctx,
 
         if(res == FKO_SUCCESS)
         {
-            if(strncmp(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;

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

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);

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)

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));