From 41614c6435ede4c75363753da8974b708c3925a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Esser?= Date: Sun, 28 Jun 2020 10:10:23 +0200 Subject: [PATCH] Salt string compatibility with generic implementations. All other existing implementations of the md5crypt, sha256crypt, and sha512crypt hashing methods allow any ASCII character to be present in the salt string. We changed our implementation of these hash methods to be compliant with the general behaviour, except for the colon ':' character, which serves a special purpose in the Unix shadow file. Also adapt test/badsalt to expect this difference in behaviour from the other hashing methods available in libxcrypt. Fixes #105 --- lib/crypt-md5.c | 12 ++++++++++-- lib/crypt-sha256.c | 12 ++++++++++-- lib/crypt-sha512.c | 13 ++++++++++--- test/badsalt.c | 18 +++++++++++++++++- 4 files changed, 47 insertions(+), 8 deletions(-) diff --git a/lib/crypt-md5.c b/lib/crypt-md5.c index b656de9d..074ead16 100644 --- a/lib/crypt-md5.c +++ b/lib/crypt-md5.c @@ -2,6 +2,7 @@ Compatible with the behavior of MD5 crypt introduced in FreeBSD 2.0. Copyright (C) 1996-2017 Free Software Foundation, Inc. + Modified by Björn Esser in 2020. This library is free software; you can redistribute it and/or modify it under the terms of the GNU Lesser General Public License @@ -73,6 +74,7 @@ crypt_md5crypt_rn (const char *phrase, size_t phr_size, uint8_t *result = buf->result; char *cp = (char *)output; const char *salt = setting; + const char *badsalt; size_t salt_size; size_t cnt; @@ -83,12 +85,18 @@ crypt_md5crypt_rn (const char *phrase, size_t phr_size, /* Skip salt prefix. */ salt += sizeof (md5_salt_prefix) - 1; - salt_size = strspn (salt, b64t); - if (salt[salt_size] && salt[salt_size] != '$') + /* Find the next '$', or string-terminator if not present. */ + salt_size = strcspn (salt, "$"); + + /* Check if there is no seperator for columns ':' in the salt. */ + badsalt = strpbrk (setting, ":"); + if (badsalt && badsalt < salt + salt_size) { errno = EINVAL; return; } + + /* Ensure we do not use more salt than SALT_LEN_MAX. */ if (salt_size > SALT_LEN_MAX) salt_size = SALT_LEN_MAX; diff --git a/lib/crypt-sha256.c b/lib/crypt-sha256.c index 5a55de1c..f2b0d03e 100644 --- a/lib/crypt-sha256.c +++ b/lib/crypt-sha256.c @@ -3,6 +3,7 @@ * Written by Ulrich Drepper in 2007 [1]. * Modified by Zack Weinberg in 2017, 2018. * Composed by Björn Esser in 2018. + * Modified by Björn Esser in 2020. * To the extent possible under law, the named authors have waived all * copyright and related or neighboring rights to this work. * @@ -100,6 +101,7 @@ crypt_sha256crypt_rn (const char *phrase, size_t phr_size, uint8_t *s_bytes = buf->s_bytes; char *cp = (char *)output; const char *salt = setting; + const char *badsalt; size_t salt_size; size_t cnt; @@ -140,12 +142,18 @@ crypt_sha256crypt_rn (const char *phrase, size_t phr_size, rounds_custom = true; } - salt_size = strspn (salt, b64t); - if (salt[salt_size] && salt[salt_size] != '$') + /* Find the next '$', or string-terminator if not present. */ + salt_size = strcspn (salt, "$"); + + /* Check if there is no seperator for columns ':' in the salt. */ + badsalt = strpbrk (setting, ":"); + if (badsalt && badsalt < salt + salt_size) { errno = EINVAL; return; } + + /* Ensure we do not use more salt than SALT_LEN_MAX. */ if (salt_size > SALT_LEN_MAX) salt_size = SALT_LEN_MAX; diff --git a/lib/crypt-sha512.c b/lib/crypt-sha512.c index a1f91386..da55ecc4 100644 --- a/lib/crypt-sha512.c +++ b/lib/crypt-sha512.c @@ -3,6 +3,7 @@ * Written by Ulrich Drepper in 2007 [1]. * Modified by Zack Weinberg in 2017, 2018. * Composed by Björn Esser in 2018. + * Modified by Björn Esser in 2020. * To the extent possible under law, the named authors have waived all * copyright and related or neighboring rights to this work. * @@ -101,6 +102,7 @@ crypt_sha512crypt_rn (const char *phrase, size_t phr_size, uint8_t *s_bytes = buf->s_bytes; char *cp = (char *)output; const char *salt = setting; + const char *badsalt; size_t salt_size; size_t cnt; @@ -141,15 +143,20 @@ crypt_sha512crypt_rn (const char *phrase, size_t phr_size, rounds_custom = true; } - salt_size = strspn (salt, b64t); - if (salt[salt_size] && salt[salt_size] != '$') + /* Find the next '$', or string-terminator if not present. */ + salt_size = strcspn (salt, "$"); + + /* Check if there is no seperator for columns ':' in the salt. */ + badsalt = strpbrk (setting, ":"); + if (badsalt && badsalt < salt + salt_size) { errno = EINVAL; return; } + + /* Ensure we do not use more salt than SALT_LEN_MAX. */ if (salt_size > SALT_LEN_MAX) salt_size = SALT_LEN_MAX; - phr_size = strlen (phrase); /* Compute alternate SHA512 sum with input PHRASE, SALT, and PHRASE. The final result will be added to the first context. */ diff --git a/test/badsalt.c b/test/badsalt.c index 887ce4ab..28a82d9a 100644 --- a/test/badsalt.c +++ b/test/badsalt.c @@ -252,7 +252,8 @@ check_results (const char *label, const char *fn, { if (retval[0] == '*' || strncmp (retval, setting, - (setting[l_setting - 1] == '*') ? l_setting - 1 : l_setting)) + (setting[l_setting - 1] == '*' || + setting[l_setting - 1] == ':') ? l_setting - 1 : l_setting)) { printf ("FAIL: %s/%s/%s: expected success, got non-matching %s\n", label, setting, fn, retval); @@ -433,6 +434,21 @@ test_one_case (const struct testcase *t, p = page + pagesize - (t->plen + 2); memcpy (p, goodhash, t->plen); p[t->plen] = '*'; + + const char *md5_label = "MD5 (FreeBSD)"; + const char *sha256_plain_label = "SHA-256 (plain)"; + const char *sha512_plain_label = "SHA-512 (plain)"; + const char *sha256_rounds_label = "SHA-256 (rounds)"; + const char *sha512_rounds_label = "SHA-512 (rounds)"; + + /* The asterisk character is a valid salt character for these hashes. */ + if (!strncmp (t->label, md5_label, strlen (md5_label)) || + !strncmp (t->label, sha256_plain_label, strlen (sha256_plain_label)) || + !strncmp (t->label, sha512_plain_label, strlen (sha512_plain_label)) || + !strncmp (t->label, sha256_rounds_label, strlen (sha256_rounds_label)) || + !strncmp (t->label, sha512_rounds_label, strlen (sha512_rounds_label))) + p[t->plen] = ':'; + p[t->plen+1] = '\0'; if (!test_one_setting (t->label, p, cd, true)) return false;