From d9afa34fdc08798ccbdc6b8234122f0bdbb754d3 Mon Sep 17 00:00:00 2001 From: Greg Hudson Date: Mon, 27 Mar 2017 15:40:08 -0400 Subject: [PATCH] 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 31a613bebc..d5c29befcb 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 ec5856c2be..1fea03408c 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 b55ee75d2f..352a8e8dcc 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 = ∅ + /* 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); -- 2.47.2