krb5 commit: Simplify null salt handling in string-to-key

Greg Hudson <[email protected]>
Newsgroups gmane.comp.encryption.kerberos.cvs
Message-ID <[email protected]>
https://github.com/krb5/krb5/commit/d9afa34fdc08798ccbdc6b8234122f0bdbb754d3
commit d9afa34fdc08798ccbdc6b8234122f0bdbb754d3
Author: Greg Hudson <[email protected]>
Date:   Mon Mar 27 15:40:08 2017 -0400

    Simplify null salt handling in string-to-key
    
    The per-enctype string_to_key implementations are inconsistent about
    whether a null salt is treated as empty or results in a null
    dereference.  Since the original DES string-to-key allowed a null
    salt, substitute an empty salt in krb5_c_string_to_key_with_params().
    Eliminate conditionals on accessing salt in the per-enctype
    implementations as they are no longer needed.  Based on a patch by
    Martin Kittel.

 src/lib/crypto/krb/s2k_des.c       |    4 ++--
 src/lib/crypto/krb/s2k_pbkdf2.c    |    4 ++--
 src/lib/crypto/krb/string_to_key.c |    7 ++++++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/lib/crypto/krb/s2k_des.c b/src/lib/crypto/krb/s2k_des.c
index 31a613b..d5c29be 100644
--- a/src/lib/crypto/krb/s2k_des.c
+++ b/src/lib/crypto/krb/s2k_des.c
@@ -509,7 +509,7 @@ des_s2k(const krb5_data *pw, const krb5_data *salt, unsigned char *key_out)
 #define FETCH4(VAR, IDX)        VAR = temp.ui[IDX/4]
 #define PUT4(VAR, IDX)          temp.ui[IDX/4] = VAR
 
-    copylen = pw->length + (salt ? salt->length : 0);
+    copylen = pw->length + salt->length;
     /* Don't need NUL termination, at this point we're treating it as
        a byte array, not a string.  */
     copy = malloc(copylen);
@@ -517,7 +517,7 @@ des_s2k(const krb5_data *pw, const krb5_data *salt, unsigned char *key_out)
         return ENOMEM;
     if (pw->length > 0)
         memcpy(copy, pw->data, pw->length);
-    if (salt != NULL && salt->length > 0)
+    if (salt->length > 0)
         memcpy(copy + pw->length, salt->data, salt->length);
 
     memset(&temp, 0, sizeof(temp));
diff --git a/src/lib/crypto/krb/s2k_pbkdf2.c b/src/lib/crypto/krb/s2k_pbkdf2.c
index ec5856c..1fea034 100644
--- a/src/lib/crypto/krb/s2k_pbkdf2.c
+++ b/src/lib/crypto/krb/s2k_pbkdf2.c
@@ -47,7 +47,7 @@ krb5int_dk_string_to_key(const struct krb5_keytypes *ktp,
     keybytes = ktp->enc->keybytes;
     keylength = ktp->enc->keylength;
 
-    concatlen = string->length + (salt ? salt->length : 0);
+    concatlen = string->length + salt->length;
 
     concat = k5alloc(concatlen, &ret);
     if (ret != 0)
@@ -63,7 +63,7 @@ krb5int_dk_string_to_key(const struct krb5_keytypes *ktp,
 
     if (string->length > 0)
         memcpy(concat, string->data, string->length);
-    if (salt != NULL && salt->length > 0)
+    if (salt->length > 0)
         memcpy(concat + string->length, salt->data, salt->length);
 
     krb5int_nfold(concatlen*8, concat, keybytes*8, foldstring);
diff --git a/src/lib/crypto/krb/string_to_key.c b/src/lib/crypto/krb/string_to_key.c
index b55ee75..352a8e8 100644
--- a/src/lib/crypto/krb/string_to_key.c
+++ b/src/lib/crypto/krb/string_to_key.c
@@ -43,6 +43,7 @@ krb5_c_string_to_key_with_params(krb5_context context, krb5_enctype enctype,
                                  const krb5_data *params, krb5_keyblock *key)
 {
     krb5_error_code ret;
+    krb5_data empty = empty_data();
     const struct krb5_keytypes *ktp;
     size_t keylength;
 
@@ -51,8 +52,12 @@ krb5_c_string_to_key_with_params(krb5_context context, krb5_enctype enctype,
         return KRB5_BAD_ENCTYPE;
     keylength = ktp->enc->keylength;
 
+    /* For compatibility with past behavior, treat a null salt as empty. */
+    if (salt == NULL)
+        salt = &empty;
+
     /* Fail gracefully if someone is using the old AFS string-to-key hack. */
-    if (salt != NULL && salt->length == SALT_TYPE_AFS_LENGTH)
+    if (salt->length == SALT_TYPE_AFS_LENGTH)
         return EINVAL;
 
     key->contents = malloc(keylength);
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.