diff --git a/libxcrypt-4.3.1-add_test_gensalt_extradata.patch b/libxcrypt-4.3.1-add_test_gensalt_extradata.patch new file mode 100644 index 0000000..3d72ca8 --- /dev/null +++ b/libxcrypt-4.3.1-add_test_gensalt_extradata.patch @@ -0,0 +1,378 @@ +From ea4d762cab0649dcbdac2de19dcc49dcc89c009d Mon Sep 17 00:00:00 2001 +From: Zack Weinberg +Date: Sat, 10 Nov 2018 20:28:38 -0500 +Subject: [PATCH] New test: crypt_gensalt should look only at the prefix. +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +For quite some time there’s been a chunk of code at the end of +test-gensalt.c labeled + + /* FIXME: This test is a little too specific. It used to be in + test-bcrypt.c and I'm not sure what it's meant to be testing. */ + +I finally figured out what the point of this code is, which means I +can generalize it from $2a$-variant bcrypt to all hashing methods. +It’s testing that, when you supply a full setting string (or even a +full hashed passphrase) as the “prefix” argument to crypt_gensalt, +crypt_gensalt does not use any information from that string other than +the method-selection prefix. (For instance, if you give it "$2a$05" +but a cost of 12, you should get back a string beginning "$2a$12".) + +I wasn’t expecting the generalized test to detect any bugs, because in +libxcrypt, individual hashing methods’ gensalt procedures don’t get to +see the “prefix” string at all. But it did! Turns out the salt +generation for NTHASH fails to NUL-terminate its output properly and +we never noticed till now (mostly because nobody uses that thing, I +hope). So that’s fixed and there’s extra code in the new test to make +sure we notice if the same mistake ever pops up again in the future. +--- + Makefile.am | 2 + + crypt-nthash.c | 2 +- + test-badsetting.c | 3 +- + test-gensalt-extradata.c | 242 +++++++++++++++++++++++++++++++++++++++ + test-gensalt.c | 32 ------ + 5 files changed, 246 insertions(+), 35 deletions(-) + create mode 100644 test-gensalt-extradata.c + +diff --git a/Makefile.am b/Makefile.am +index fa41938..0eecf0f 100644 +--- a/Makefile.am ++++ b/Makefile.am +@@ -180,6 +180,7 @@ check_PROGRAMS = \ + test-crypt-pbkdf1-sha1 test-crypt-scrypt test-crypt-sha256 \ + test-crypt-sha512 test-crypt-sunmd5 test-crypt-yescrypt \ + test-byteorder test-badsalt test-badsetting test-gensalt \ ++ test-gensalt-extradata \ + test-crypt-badargs test-short-outbuf test-compile-strong-alias \ + test-getrandom-interface test-getrandom-fallbacks + +@@ -225,6 +226,7 @@ test_crypt_yescrypt_LDADD = crypt-common.lo libcrypt.la + test_badsalt_LDADD = crypt-common.lo libcrypt.la + test_badsetting_LDADD = crypt-common.lo libcrypt.la + test_gensalt_LDADD = crypt-common.lo libcrypt.la ++test_gensalt_extradata_LDADD = crypt-common.lo libcrypt.la + test_checksalt_LDADD = crypt-common.lo libcrypt.la + test_des_obsolete_LDADD = crypt-common.lo libcrypt.la + test_des_obsolete_r_LDADD = crypt-common.lo libcrypt.la +diff --git a/crypt-nthash.c b/crypt-nthash.c +index 78fdd63..5ae7842 100644 +--- a/crypt-nthash.c ++++ b/crypt-nthash.c +@@ -144,7 +144,7 @@ gensalt_nt_rn (unsigned long count, + sprintf (&(hashstr[i * 2]), "%02x", hashbuf[i]); + + memcpy (output, salt, 15); +- memcpy (output + 15, hashstr, 14); ++ memcpy (output + 15, hashstr, 14+1); + } + + #endif +diff --git a/test-badsetting.c b/test-badsetting.c +index 8a96cd9..20b3d3e 100644 +--- a/test-badsetting.c ++++ b/test-badsetting.c +@@ -8,11 +8,10 @@ + details. */ + + #include "crypt-port.h" +-#include ++ + #include + #include + #include +-#include + + /* Supply 64 bytes of "random" data to each gensalt call, for + determinism. */ +diff --git a/test-gensalt-extradata.c b/test-gensalt-extradata.c +new file mode 100644 +index 0000000..27b87eb +--- /dev/null ++++ b/test-gensalt-extradata.c +@@ -0,0 +1,242 @@ ++/* Test that the prefix argument to crypt_gensalt affects only the ++ choice of hashing method, not any of the parameters or the salt. ++ ++ Written by Zack Weinberg in 2018. ++ To the extent possible under law, Zack Weinberg has waived all ++ copyright and related or neighboring rights to this work. ++ ++ See https://creativecommons.org/publicdomain/zero/1.0/ for further ++ details. */ ++ ++#include "crypt-port.h" ++ ++#include ++#include ++ ++/* Random bytes used when calling crypt_gensalt; for determinism, these ++ are fixed from run to run. */ ++#define N_RBYTES 64ul ++ ++static const char rbytes1[] = ++ "90idUkI2+mu2E/tMTViD418j2sPdEYq9LYq0yRW7RYhr4RqQ+oVzIIEcfJBqpf/D"; ++ ++static const char rbytes2[] = ++ "sEwXQxrjBTEADauxCpvOQqq7iU9oq6uJ+Iux/fbhtLRj1MWgBFyo/t+nh/nzm0Kn"; ++ ++static_assert(sizeof rbytes1 == N_RBYTES + 1, "rbytes1 is wrong length"); ++static_assert(sizeof rbytes2 == N_RBYTES + 1, "rbytes2 is wrong length"); ++ ++struct testcase ++{ ++ const char prefix[8]; ++ unsigned long count1; ++ unsigned long count2; ++}; ++ ++/* This list should include one entry for each potentially-supported ++ hash prefix. If the hash method has tunable cost, set count1 and ++ count2 to two different nonzero values, within the supported cost ++ range. Neither value should equal the default cost. If the hash ++ method does not have tunable cost, set count1 and count2 to zero. */ ++static const struct testcase testcases[] = ++{ ++#if INCLUDE_descrypt || INCLUDE_bigcrypt ++ { "", 0, 0 }, ++#endif ++#if INCLUDE_bsdicrypt ++ { "_", 7019, 1120211 }, ++#endif ++#if INCLUDE_nt ++ { "$3$", 0, 0 }, ++#endif ++#if INCLUDE_md5crypt ++ { "$1$", 0, 0 }, ++#endif ++#if INCLUDE_sunmd5 ++ { "$md5", 7019, 1120211 }, ++#endif ++#if INCLUDE_sha1crypt ++ { "$sha1", 7019, 1120211 }, ++#endif ++#if INCLUDE_sha256crypt ++ { "$5$", 7019, 1120211 }, ++#endif ++#if INCLUDE_sha512crypt ++ { "$6$", 7019, 1120211 }, ++#endif ++#if INCLUDE_bcrypt ++ { "$2b$", 7, 11 }, ++#endif ++#if INCLUDE_bcrypt_y ++ { "$2y$", 7, 11 }, ++#endif ++#if INCLUDE_bcrypt_a ++ { "$2a$", 7, 11 }, ++#endif ++#if INCLUDE_bcrypt_x ++ { "$2x$", 7, 11 }, ++#endif ++#if INCLUDE_scrypt ++ { "$7$", 7, 11, }, ++#endif ++#if INCLUDE_yescrypt ++ { "$y$", 7, 11, }, ++#endif ++#if INCLUDE_gost_yescrypt ++ { "$gy$", 7, 11, }, ++#endif ++}; ++ ++static int ++do_crypt_gensalt(const char *prefix, ++ const char rbytes[MIN_SIZE(N_RBYTES)], ++ unsigned long count, ++ char outbuf[MIN_SIZE(CRYPT_GENSALT_OUTPUT_SIZE)]) ++{ ++ /* Detect failure to NUL-terminate the output properly. */ ++ static int ncalls = 0; ++ memset(outbuf, '!' + (ncalls % ('~' - '!' + 1)), ++ CRYPT_GENSALT_OUTPUT_SIZE - 1); ++ outbuf[CRYPT_GENSALT_OUTPUT_SIZE - 1] = 0; ++ ncalls++; ++ ++ char *rv = crypt_gensalt_rn(prefix, count, rbytes, N_RBYTES, ++ outbuf, CRYPT_GENSALT_OUTPUT_SIZE); ++ if (rv == 0) ++ { ++ printf("ERROR: gensalt(%s, %lu, %c%c..., %lu, outbuf, %lu) = 0/%s\n", ++ prefix, count, rbytes[0], rbytes[1], ++ N_RBYTES, (unsigned long)CRYPT_GENSALT_OUTPUT_SIZE, ++ strerror(errno)); ++ outbuf[0] = '*'; ++ memset (outbuf+1, 0, CRYPT_GENSALT_OUTPUT_SIZE-1); ++ return 1; ++ } ++ else if (rv[0] == '*') ++ { ++ printf("ERROR: gensalt(%s, %lu, %c%c..., %lu, outbuf, %lu) = %s/%s\n", ++ prefix, count, rbytes[0], rbytes[1], ++ N_RBYTES, (unsigned long)CRYPT_GENSALT_OUTPUT_SIZE, ++ outbuf, strerror(errno)); ++ outbuf[0] = '*'; ++ memset (outbuf+1, 0, CRYPT_GENSALT_OUTPUT_SIZE-1); ++ return 1; ++ } ++ else ++ return 0; ++} ++ ++static int ++do_check_equal(const char *stst, const char *sref, ++ const char *prefix, const char rbytes[N_RBYTES], ++ unsigned long count, const char *setting) ++{ ++ if (!strcmp(stst, sref)) ++ return 0; ++ ++ printf("FAIL: expected %s\n" ++ " got %s\n" ++ " from %s, %lu, %c%c...\n" ++ " and %s\n", ++ sref, stst, prefix, count, rbytes[0], rbytes[1], setting); ++ return 1; ++} ++ ++int ++main(void) ++{ ++ int status = 0; ++ char sref[6][CRYPT_GENSALT_OUTPUT_SIZE]; ++ char stst[CRYPT_GENSALT_OUTPUT_SIZE]; ++ ++ for (size_t i = 0; i < ARRAY_SIZE(testcases); i++) ++ { ++ const char *prefix = testcases[i].prefix; ++ unsigned long count1 = testcases[i].count1; ++ unsigned long count2 = testcases[i].count2; ++ int ncases; ++ ++ memset(sref, 0, sizeof sref); ++ ++ /* If count1 and count2 are both nonzero, then they should also ++ be unequal, and we have six reference cases: ++ (0, count1, count2) x (rbytes1, rbytes2). ++ If count1 and count2 are both zero, then we only have two ++ reference cases: 0 x (rbytes1, rbytes2) (this happens when the ++ hash method doesn't have tunable cost). ++ It is incorrect for only one of count1 and count2 to be zero, ++ or for them to be equal but nonzero. */ ++ if (count1 == 0 && count2 == 0) ++ { ++ ncases = 2; ++ status |= do_crypt_gensalt(prefix, rbytes1, 0, sref[0]); ++ status |= do_crypt_gensalt(prefix, rbytes2, 0, sref[1]); ++ } ++ else if (count1 != 0 && count2 != 0 && count1 != count2) ++ { ++ ncases = 6; ++ status |= do_crypt_gensalt(prefix, rbytes1, 0, sref[0]); ++ status |= do_crypt_gensalt(prefix, rbytes2, 0, sref[1]); ++ status |= do_crypt_gensalt(prefix, rbytes1, count1, sref[2]); ++ status |= do_crypt_gensalt(prefix, rbytes2, count1, sref[3]); ++ status |= do_crypt_gensalt(prefix, rbytes1, count2, sref[4]); ++ status |= do_crypt_gensalt(prefix, rbytes2, count2, sref[5]); ++ } ++ else ++ { ++ printf ("ERROR: %zu/%s: inappropriate count1=%lu count2=%lu\n", ++ i, prefix, count1, count2); ++ status = 1; ++ continue; ++ } ++ ++ /* At this point, sref[0..ncases] are filled with setting ++ strings corresponding to different combinations of salt and ++ cost. If we reuse those strings as prefixes for crypt_gensalt, ++ none of the additional information should affect the output. */ ++ for (int j = 0; j < ncases; j++) ++ { ++ if (sref[j][0] == '*') ++ continue; /* initial crypt_gensalt call failed */ ++ if (count1 == 0 && count2 == 0) ++ { ++ status |= do_crypt_gensalt(sref[j], rbytes1, 0, stst); ++ status |= do_check_equal(stst, sref[0], ++ prefix, rbytes1, 0, sref[j]); ++ ++ status |= do_crypt_gensalt(sref[j], rbytes2, 0, stst); ++ status |= do_check_equal(stst, sref[1], ++ prefix, rbytes2, 0, sref[j]); ++ } ++ else ++ { ++ status |= do_crypt_gensalt(sref[j], rbytes1, 0, stst); ++ status |= do_check_equal(stst, sref[0], ++ prefix, rbytes1, 0, sref[j]); ++ ++ status |= do_crypt_gensalt(sref[j], rbytes2, 0, stst); ++ status |= do_check_equal(stst, sref[1], ++ prefix, rbytes2, 0, sref[j]); ++ ++ status |= do_crypt_gensalt(sref[j], rbytes1, count1, stst); ++ status |= do_check_equal(stst, sref[2], ++ prefix, rbytes1, count1, sref[j]); ++ ++ status |= do_crypt_gensalt(sref[j], rbytes2, count1, stst); ++ status |= do_check_equal(stst, sref[3], ++ prefix, rbytes2, count1, sref[j]); ++ ++ status |= do_crypt_gensalt(sref[j], rbytes1, count2, stst); ++ status |= do_check_equal(stst, sref[4], ++ prefix, rbytes1, count2, sref[j]); ++ ++ status |= do_crypt_gensalt(sref[j], rbytes2, count2, stst); ++ status |= do_check_equal(stst, sref[5], ++ prefix, rbytes2, count2, sref[j]); ++ } ++ } ++ ++ } ++ ++ return status; ++} +diff --git a/test-gensalt.c b/test-gensalt.c +index 2099cd2..0ef3809 100644 +--- a/test-gensalt.c ++++ b/test-gensalt.c +@@ -556,37 +556,5 @@ main (void) + free (setting); + } + #endif +- +-#if INCLUDE_bcrypt_a +- /* FIXME: This test is a little too specific. It used to be in +- test-bcrypt.c and I'm not sure what it's meant to be testing. */ +- { +- char *setting1, *setting2; +- const char *which = "$2a$05$CCCCCCCCCCCCCCCCCCCCC.E5YPO9kmyuRGyh0XouQYb4YMJKvyOeW"; +- setting1 = crypt_gensalt (which, 12, "CCCCCCCCCCCCCCCCCCCCC", 21); +- if (!setting1 || strncmp (setting1, "$2a$12$", 7)) +- { +- printf ("FAILED (crypt_gensalt: wrong prefix) s1=%s\n", setting1); +- status = 1; +- } +- +- setting2 = crypt_gensalt_ra (setting1, 12, "CCCCCCCCCCCCCCCCCCCCC", 21); +- if (strcmp (setting1, setting2)) +- { +- printf ("FAILED (crypt_gensalt_ra/1: s1=%s s2=%s)\n", setting1, setting2); +- status = 1; +- } +- +- setting1 = crypt_gensalt_ra (setting2, 12, "DCCCCCCCCCCCCCCCCCCCC", 21); +- if (!strcmp (setting1, setting2)) +- { +- printf ("FAILED (crypt_gensalt_ra/2: s1=%s s2=%s)\n", setting1, setting2); +- status = 1; +- } +- +- free (setting1); +- free (setting2); +- } +-#endif + return status; + } diff --git a/libxcrypt-4.3.1-nthash_output_buffer_gensalt.patch b/libxcrypt-4.3.1-nthash_output_buffer_gensalt.patch new file mode 100644 index 0000000..931175e --- /dev/null +++ b/libxcrypt-4.3.1-nthash_output_buffer_gensalt.patch @@ -0,0 +1,40 @@ +From 0a58300cdea4733afad31df8a4ff283f3b42caca Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Bj=C3=B6rn=20Esser?= +Date: Sun, 11 Nov 2018 12:25:50 +0100 +Subject: [PATCH] nthash: The output buffer for gensalt must be at least 30 + bytes long. + +The size of the buffer provided by 'o_size' must be at least 30 bytes +long to fit the terminating null byte. +Also use 'XCRYPT_STRCPY_OR_ABORT' over plain 'memcpy', since it is +the preferred method to copy strings. +--- + crypt-nthash.c | 7 ++++--- + 1 file changed, 4 insertions(+), 3 deletions(-) + +diff --git a/crypt-nthash.c b/crypt-nthash.c +index 5ae7842..bb7c1ff 100644 +--- a/crypt-nthash.c ++++ b/crypt-nthash.c +@@ -119,7 +119,7 @@ gensalt_nt_rn (unsigned long count, + At least 1 byte of RBYTES is needed + to calculate the MD4 hash used in the + fake salt. */ +- if ((o_size < 29) || (nrbytes < 1)) ++ if ((o_size < 30) || (nrbytes < 1)) + { + errno = ERANGE; + return; +@@ -142,9 +142,10 @@ gensalt_nt_rn (unsigned long count, + + for (i = 0; i < 7; i++) + sprintf (&(hashstr[i * 2]), "%02x", hashbuf[i]); ++ hashstr[14] = '\0'; + +- memcpy (output, salt, 15); +- memcpy (output + 15, hashstr, 14+1); ++ XCRYPT_STRCPY_OR_ABORT (output, o_size, salt); ++ XCRYPT_STRCPY_OR_ABORT (output + 15, o_size - 15, hashstr); + } + + #endif diff --git a/libxcrypt.spec b/libxcrypt.spec index 998fd8f..6905857 100644 --- a/libxcrypt.spec +++ b/libxcrypt.spec @@ -90,7 +90,7 @@ fi \ Name: libxcrypt Version: 4.3.1 -Release: 1%{?dist} +Release: 2%{?dist} Summary: Extended crypt library for DES, MD5, Blowfish and others # For explicit license breakdown, see the @@ -99,6 +99,10 @@ License: LGPLv2+ and BSD and Public Domain URL: https://github.com/besser82/%{name} Source0: %{url}/archive/v%{version}/%{name}-%{version}.tar.gz +# Backported patches from upstream. +Patch0001: %{url}/commit/ea4d762cab0649dcbdac2de19dcc49dcc89c009d.patch#/%{name}-4.3.1-add_test_gensalt_extradata.patch +Patch0002: %{url}/commit/0a58300cdea4733afad31df8a4ff283f3b42caca.patch#/%{name}-4.3.1-nthash_output_buffer_gensalt.patch + BuildRequires: fipscheck BuildRequires: libtool %if %{with memcheck} @@ -340,6 +344,10 @@ done %changelog +* Sun Nov 11 2018 Björn Esser - 4.3.1-2 +- Backport two patches from upstream fixing the gensalt function for + NT to properly terminate its returned output + * Sat Nov 10 2018 Björn Esser - 4.3.1-1 - New upstream release