From e879f3a0c11cf1e39b525bc424a2213b48437e4f Mon Sep 17 00:00:00 2001 From: Griffin Smith Date: Tue, 8 Mar 2016 13:36:38 -0500 Subject: [PATCH] Update to crypt-blowfish 1.3 for MRI Manually merge the update to crypt-blowfish 1.3 (code from https://github.com/rg3/bcrypt) into the MRI extension, adding (and testing) support for the $2b$ prefix. --- ext/mri/crypt_blowfish.c | 356 +++++++++++++++++++++++++------------ ext/mri/wrapper.c | 21 ++- spec/bcrypt/engine_spec.rb | 3 +- 3 files changed, 257 insertions(+), 123 deletions(-) diff --git a/ext/mri/crypt_blowfish.c b/ext/mri/crypt_blowfish.c index cf96c17..a0b88f2 100644 --- a/ext/mri/crypt_blowfish.c +++ b/ext/mri/crypt_blowfish.c @@ -3,24 +3,32 @@ * and crypt(3) interfaces added, but optimizations specific to password * cracking removed. * - * Written by Solar Designer in 1998-2002 and - * placed in the public domain. Quick self-test added in 2011 and also - * placed in the public domain. + * Written by Solar Designer in 1998-2014. + * No copyright is claimed, and the software is hereby placed in the public + * domain. In case this attempt to disclaim copyright and place the software + * in the public domain is deemed null and void, then the software is + * Copyright (c) 1998-2014 Solar Designer and it is hereby released to the + * general public under the following terms: * - * There's absolutely no warranty. + * Redistribution and use in source and binary forms, with or without + * modification, are permitted. + * + * There's ABSOLUTELY NO WARRANTY, express or implied. * * It is my intent that you should be able to use this on your system, - * as a part of a software package, or anywhere else to improve security, + * as part of a software package, or anywhere else to improve security, * ensure compatibility, or for any other purpose. I would appreciate * it if you give credit where it is due and keep your modifications in * the public domain as well, but I don't require that in order to let * you place this code and any modifications you make under a license * of your choice. * - * This implementation is compatible with OpenBSD bcrypt.c (version 2a) - * by Niels Provos , and uses some of his - * ideas. The password hashing algorithm was designed by David Mazieres - * . + * This implementation is fully compatible with OpenBSD's bcrypt.c for prefix + * "$2b$", originally by Niels Provos , and it uses + * some of his ideas. The password hashing algorithm was designed by David + * Mazieres . For information on the level of + * compatibility for bcrypt hash prefixes other than "$2b$", please refer to + * the comments in BF_set_key() below and to the included crypt(3) man page. * * There's a paper on the algorithm that explains its design decisions: * @@ -390,7 +398,7 @@ static int BF_decode(BF_word *dst, __CONST char *src, int size) { unsigned char *dptr = (unsigned char *)dst; unsigned char *end = dptr + size; - unsigned char *sptr = (unsigned char *)src; + __CONST unsigned char *sptr = (__CONST unsigned char *)src; unsigned int tmp, c1, c2, c3, c4; do { @@ -412,8 +420,8 @@ static int BF_decode(BF_word *dst, __CONST char *src, int size) static void BF_encode(char *dst, __CONST BF_word *src, int size) { - unsigned char *sptr = (unsigned char *)src; - unsigned char *end = sptr + size; + __CONST unsigned char *sptr = (__CONST unsigned char *)src; + __CONST unsigned char *end = sptr + size; unsigned char *dptr = (unsigned char *)dst; unsigned int c1, c2; @@ -540,33 +548,118 @@ static void BF_swap(BF_word *x, int count) } while (ptr < &data.ctx.S[3][0xFF]); static void BF_set_key(__CONST char *key, BF_key expanded, BF_key initial, - int sign_extension_bug) + unsigned char flags) { __CONST char *ptr = key; - int i, j; - BF_word tmp; + unsigned int bug, i, j; + BF_word safety, sign, diff, tmp[2]; + +/* + * There was a sign extension bug in older revisions of this function. While + * we would have liked to simply fix the bug and move on, we have to provide + * a backwards compatibility feature (essentially the bug) for some systems and + * a safety measure for some others. The latter is needed because for certain + * multiple inputs to the buggy algorithm there exist easily found inputs to + * the correct algorithm that produce the same hash. Thus, we optionally + * deviate from the correct algorithm just enough to avoid such collisions. + * While the bug itself affected the majority of passwords containing + * characters with the 8th bit set (although only a percentage of those in a + * collision-producing way), the anti-collision safety measure affects + * only a subset of passwords containing the '\xff' character (not even all of + * those passwords, just some of them). This character is not found in valid + * UTF-8 sequences and is rarely used in popular 8-bit character encodings. + * Thus, the safety measure is unlikely to cause much annoyance, and is a + * reasonable tradeoff to use when authenticating against existing hashes that + * are not reliably known to have been computed with the correct algorithm. + * + * We use an approach that tries to minimize side-channel leaks of password + * information - that is, we mostly use fixed-cost bitwise operations instead + * of branches or table lookups. (One conditional branch based on password + * length remains. It is not part of the bug aftermath, though, and is + * difficult and possibly unreasonable to avoid given the use of C strings by + * the caller, which results in similar timing leaks anyway.) + * + * For actual implementation, we set an array index in the variable "bug" + * (0 means no bug, 1 means sign extension bug emulation) and a flag in the + * variable "safety" (bit 16 is set when the safety measure is requested). + * Valid combinations of settings are: + * + * Prefix "$2a$": bug = 0, safety = 0x10000 + * Prefix "$2b$": bug = 0, safety = 0 + * Prefix "$2x$": bug = 1, safety = 0 + * Prefix "$2y$": bug = 0, safety = 0 + */ + bug = (unsigned int)flags & 1; + safety = ((BF_word)flags & 2) << 15; + + sign = diff = 0; for (i = 0; i < BF_N + 2; i++) { - tmp = 0; + tmp[0] = tmp[1] = 0; for (j = 0; j < 4; j++) { - tmp <<= 8; - if (sign_extension_bug) - tmp |= (BF_word_signed)(signed char)*ptr; + tmp[0] <<= 8; + tmp[0] |= (unsigned char)*ptr; /* correct */ + tmp[1] <<= 8; + tmp[1] |= (BF_word_signed)(signed char)*ptr; /* bug */ +/* + * Sign extension in the first char has no effect - nothing to overwrite yet, + * and those extra 24 bits will be fully shifted out of the 32-bit word. For + * chars 2, 3, 4 in each four-char block, we set bit 7 of "sign" if sign + * extension in tmp[1] occurs. Once this flag is set, it remains set. + */ + if (j) + sign |= tmp[1] & 0x80; + if (!*ptr) + ptr = key; else - tmp |= (unsigned char)*ptr; - - if (!*ptr) ptr = key; else ptr++; + ptr++; } + diff |= tmp[0] ^ tmp[1]; /* Non-zero on any differences */ - expanded[i] = tmp; - initial[i] = BF_init_state.P[i] ^ tmp; + expanded[i] = tmp[bug]; + initial[i] = BF_init_state.P[i] ^ tmp[bug]; } + +/* + * At this point, "diff" is zero iff the correct and buggy algorithms produced + * exactly the same result. If so and if "sign" is non-zero, which indicates + * that there was a non-benign sign extension, this means that we have a + * collision between the correctly computed hash for this password and a set of + * passwords that could be supplied to the buggy algorithm. Our safety measure + * is meant to protect from such many-buggy to one-correct collisions, by + * deviating from the correct algorithm in such cases. Let's check for this. + */ + diff |= diff >> 16; /* still zero iff exact match */ + diff &= 0xffff; /* ditto */ + diff += 0xffff; /* bit 16 set iff "diff" was non-zero (on non-match) */ + sign <<= 9; /* move the non-benign sign extension flag to bit 16 */ + sign &= ~diff & safety; /* action needed? */ + +/* + * If we have determined that we need to deviate from the correct algorithm, + * flip bit 16 in initial expanded key. (The choice of 16 is arbitrary, but + * let's stick to it now. It came out of the approach we used above, and it's + * not any worse than any other choice we could make.) + * + * It is crucial that we don't do the same to the expanded key used in the main + * Eksblowfish loop. By doing it to only one of these two, we deviate from a + * state that could be directly specified by a password to the buggy algorithm + * (and to the fully correct one as well, but that's a side-effect). + */ + initial[0] ^= sign; } +static __CONST unsigned char flags_by_subtype[26] = + {2, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 4, 0}; + static char *BF_crypt(__CONST char *key, __CONST char *setting, char *output, int size, BF_word min) { +#if BF_ASM + extern void _BF_body_r(BF_ctx *ctx); +#endif struct { BF_ctx ctx; BF_key expanded_key; @@ -588,7 +681,8 @@ static char *BF_crypt(__CONST char *key, __CONST char *setting, if (setting[0] != '$' || setting[1] != '2' || - (setting[2] != 'a' && setting[2] != 'x') || + setting[2] < 'a' || setting[2] > 'z' || + !flags_by_subtype[(unsigned int)(unsigned char)setting[2] - 'a'] || setting[3] != '$' || setting[4] < '0' || setting[4] > '3' || setting[5] < '0' || setting[5] > '9' || @@ -600,13 +694,13 @@ static char *BF_crypt(__CONST char *key, __CONST char *setting, count = (BF_word)1 << ((setting[4] - '0') * 10 + (setting[5] - '0')); if (count < min || BF_decode(data.binary.salt, &setting[7], 16)) { - clean(data.binary.salt, sizeof(data.binary.salt)); __set_errno(EINVAL); return NULL; } BF_swap(data.binary.salt, 4); - BF_set_key(key, data.expanded_key, data.ctx.P, setting[2] == 'x'); + BF_set_key(key, data.expanded_key, data.ctx.P, + flags_by_subtype[(unsigned int)(unsigned char)setting[2] - 'a']); memcpy(data.ctx.S, BF_init_state.S, sizeof(data.ctx.S)); @@ -636,51 +730,33 @@ static char *BF_crypt(__CONST char *key, __CONST char *setting, } while (ptr < &data.ctx.S[3][0xFF]); do { - data.ctx.P[0] ^= data.expanded_key[0]; - data.ctx.P[1] ^= data.expanded_key[1]; - data.ctx.P[2] ^= data.expanded_key[2]; - data.ctx.P[3] ^= data.expanded_key[3]; - data.ctx.P[4] ^= data.expanded_key[4]; - data.ctx.P[5] ^= data.expanded_key[5]; - data.ctx.P[6] ^= data.expanded_key[6]; - data.ctx.P[7] ^= data.expanded_key[7]; - data.ctx.P[8] ^= data.expanded_key[8]; - data.ctx.P[9] ^= data.expanded_key[9]; - data.ctx.P[10] ^= data.expanded_key[10]; - data.ctx.P[11] ^= data.expanded_key[11]; - data.ctx.P[12] ^= data.expanded_key[12]; - data.ctx.P[13] ^= data.expanded_key[13]; - data.ctx.P[14] ^= data.expanded_key[14]; - data.ctx.P[15] ^= data.expanded_key[15]; - data.ctx.P[16] ^= data.expanded_key[16]; - data.ctx.P[17] ^= data.expanded_key[17]; - - BF_body(); - - tmp1 = data.binary.salt[0]; - tmp2 = data.binary.salt[1]; - tmp3 = data.binary.salt[2]; - tmp4 = data.binary.salt[3]; - data.ctx.P[0] ^= tmp1; - data.ctx.P[1] ^= tmp2; - data.ctx.P[2] ^= tmp3; - data.ctx.P[3] ^= tmp4; - data.ctx.P[4] ^= tmp1; - data.ctx.P[5] ^= tmp2; - data.ctx.P[6] ^= tmp3; - data.ctx.P[7] ^= tmp4; - data.ctx.P[8] ^= tmp1; - data.ctx.P[9] ^= tmp2; - data.ctx.P[10] ^= tmp3; - data.ctx.P[11] ^= tmp4; - data.ctx.P[12] ^= tmp1; - data.ctx.P[13] ^= tmp2; - data.ctx.P[14] ^= tmp3; - data.ctx.P[15] ^= tmp4; - data.ctx.P[16] ^= tmp1; - data.ctx.P[17] ^= tmp2; - - BF_body(); + int done; + + for (i = 0; i < BF_N + 2; i += 2) { + data.ctx.P[i] ^= data.expanded_key[i]; + data.ctx.P[i + 1] ^= data.expanded_key[i + 1]; + } + + done = 0; + do { + BF_body(); + if (done) + break; + done = 1; + + tmp1 = data.binary.salt[0]; + tmp2 = data.binary.salt[1]; + tmp3 = data.binary.salt[2]; + tmp4 = data.binary.salt[3]; + for (i = 0; i < BF_N; i += 4) { + data.ctx.P[i] ^= tmp1; + data.ctx.P[i + 1] ^= tmp2; + data.ctx.P[i + 2] ^= tmp3; + data.ctx.P[i + 3] ^= tmp4; + } + data.ctx.P[16] ^= tmp1; + data.ctx.P[17] ^= tmp2; + } while (1); } while (--count); for (i = 0; i < 6; i += 2) { @@ -706,64 +782,116 @@ static char *BF_crypt(__CONST char *key, __CONST char *setting, BF_encode(&output[7 + 22], data.binary.output, 23); output[7 + 22 + 31] = '\0'; -#ifndef BF_SELF_TEST -/* Overwrite the most obvious sensitive data we have on the stack. Note - * that this does not guarantee there's no sensitive data left on the - * stack and/or in registers; I'm not aware of portable code that does. */ - clean(&data, sizeof(data)); -#endif - return output; } +int _crypt_output_magic(__CONST char *setting, char *output, int size) +{ + if (size < 3) + return -1; + + output[0] = '*'; + output[1] = '0'; + output[2] = '\0'; + + if (setting[0] == '*' && setting[1] == '0') + output[1] = '1'; + + return 0; +} + +/* + * Please preserve the runtime self-test. It serves two purposes at once: + * + * 1. We really can't afford the risk of producing incompatible hashes e.g. + * when there's something like gcc bug 26587 again, whereas an application or + * library integrating this code might not also integrate our external tests or + * it might not run them after every build. Even if it does, the miscompile + * might only occur on the production build, but not on a testing build (such + * as because of different optimization settings). It is painful to recover + * from incorrectly-computed hashes - merely fixing whatever broke is not + * enough. Thus, a proactive measure like this self-test is needed. + * + * 2. We don't want to leave sensitive data from our actual password hash + * computation on the stack or in registers. Previous revisions of the code + * would do explicit cleanups, but simply running the self-test after hash + * computation is more reliable. + * + * The performance cost of this quick self-test is around 0.6% at the "$2a$08" + * setting. + */ char *_crypt_blowfish_rn(__CONST char *key, __CONST char *setting, char *output, int size) { -#ifdef BF_SELF_TEST __CONST char *test_key = "8b \xd0\xc1\xd2\xcf\xcc\xd8"; - __CONST char *test_2a = - "$2a$00$abcdefghijklmnopqrstuui1D709vfamulimlGcq0qq3UvuUasvEa" - "\0" - "canary"; - __CONST char *test_2x = - "$2x$00$abcdefghijklmnopqrstuuVUrPmXD6q/nVSSp7pNDhCR9071IfIRe" - "\0" - "canary"; - __CONST char *test_hash, *p; - int ok; - char buf[7 + 22 + 31 + 1 + 6 + 1]; - - output = BF_crypt(key, setting, output, size, 16); - -/* Do a quick self-test. This also happens to overwrite BF_crypt()'s data. */ - test_hash = (setting[2] == 'x') ? test_2x : test_2a; - memcpy(buf, test_hash, sizeof(buf)); - memset(buf, -1, sizeof(buf) - (6 + 1)); /* keep "canary" only */ - p = BF_crypt(test_key, test_hash, buf, sizeof(buf) - 6, 1); - - ok = (p == buf && !memcmp(p, test_hash, sizeof(buf))); - -/* This could reveal what hash type we were using last. Unfortunately, we - * can't reliably clean the test_hash pointer. */ - clean(&buf, sizeof(buf)); + __CONST char *test_setting = "$2a$00$abcdefghijklmnopqrstuu"; + static __CONST char * __CONST test_hashes[2] = + {"i1D709vfamulimlGcq0qq3UvuUasvEa\0\x55", /* 'a', 'b', 'y' */ + "VUrPmXD6q/nVSSp7pNDhCR9071IfIRe\0\x55"}; /* 'x' */ + __CONST char *test_hash = test_hashes[0]; + char *retval; + __CONST char *p; + int save_errno, ok; + struct { + char s[7 + 22 + 1]; + char o[7 + 22 + 31 + 1 + 1 + 1]; + } buf; + +/* Hash the supplied password */ + _crypt_output_magic(setting, output, size); + retval = BF_crypt(key, setting, output, size, 16); + save_errno = errno; + +/* + * Do a quick self-test. It is important that we make both calls to BF_crypt() + * from the same scope such that they likely use the same stack locations, + * which makes the second call overwrite the first call's sensitive data on the + * stack and makes it more likely that any alignment related issues would be + * detected by the self-test. + */ + memcpy(buf.s, test_setting, sizeof(buf.s)); + if (retval) { + unsigned int flags = flags_by_subtype[ + (unsigned int)(unsigned char)setting[2] - 'a']; + test_hash = test_hashes[flags & 1]; + buf.s[2] = setting[2]; + } + memset(buf.o, 0x55, sizeof(buf.o)); + buf.o[sizeof(buf.o) - 1] = 0; + p = BF_crypt(test_key, buf.s, buf.o, sizeof(buf.o) - (1 + 1), 1); + ok = (p == buf.o && + !memcmp(p, buf.s, 7 + 22) && + !memcmp(p + (7 + 22), test_hash, 31 + 1 + 1 + 1)); + + { + const char *k = "\xff\xa3" "34" "\xff\xff\xff\xa3" "345"; + BF_key ae, ai, ye, yi; + BF_set_key(k, ae, ai, 2); /* $2a$ */ + BF_set_key(k, ye, yi, 4); /* $2y$ */ + ai[0] ^= 0x10000; /* undo the safety (for comparison) */ + ok = ok && ai[0] == 0xdb9c59bc && ye[17] == 0x33343500 && + !memcmp(ae, ye, sizeof(ae)) && + !memcmp(ai, yi, sizeof(ai)); + } + + __set_errno(save_errno); if (ok) - return output; + return retval; /* Should not happen */ + _crypt_output_magic(setting, output, size); __set_errno(EINVAL); /* pretend we don't support this hash type */ return NULL; -#else -#warning Self-test is disabled, please enable - return BF_crypt(key, setting, output, size, 16); -#endif } -char *_crypt_gensalt_blowfish_rn(unsigned long count, +char *_crypt_gensalt_blowfish_rn(__CONST char *prefix, unsigned long count, __CONST char *input, int size, char *output, int output_size) { if (size < 16 || output_size < 7 + 22 + 1 || - (count && (count < 4 || count > 31))) { + (count && (count < 4 || count > 31)) || + prefix[0] != '$' || prefix[1] != '2' || + (prefix[2] != 'a' && prefix[2] != 'b' && prefix[2] != 'y')) { if (output_size > 0) output[0] = '\0'; __set_errno((output_size < 7 + 22 + 1) ? ERANGE : EINVAL); return NULL; @@ -773,13 +901,13 @@ char *_crypt_gensalt_blowfish_rn(unsigned long count, output[0] = '$'; output[1] = '2'; - output[2] = 'a'; + output[2] = prefix[2]; output[3] = '$'; output[4] = '0' + count / 10; output[5] = '0' + count % 10; output[6] = '$'; - BF_encode(&output[7], (BF_word *)input, 16); + BF_encode(&output[7], (__CONST BF_word *)input, 16); output[7 + 22] = '\0'; return output; diff --git a/ext/mri/wrapper.c b/ext/mri/wrapper.c index fe832db..192b79b 100644 --- a/ext/mri/wrapper.c +++ b/ext/mri/wrapper.c @@ -38,17 +38,21 @@ #endif #include "ow-crypt.h" +extern int _crypt_output_magic(__CONST char *setting, char *output, int size); extern char *_crypt_blowfish_rn(__CONST char *key, __CONST char *setting, char *output, int size); -extern char *_crypt_gensalt_blowfish_rn(unsigned long count, +extern char *_crypt_gensalt_blowfish_rn(__CONST char *prefix, + unsigned long count, __CONST char *input, int size, char *output, int output_size); extern unsigned char _crypt_itoa64[]; -extern char *_crypt_gensalt_traditional_rn(unsigned long count, +extern char *_crypt_gensalt_traditional_rn(__CONST char *prefix, + unsigned long count, __CONST char *input, int size, char *output, int output_size); -extern char *_crypt_gensalt_extended_rn(unsigned long count, +extern char *_crypt_gensalt_extended_rn(__CONST char *prefix, + unsigned long count, __CONST char *input, int size, char *output, int output_size); -extern char *_crypt_gensalt_md5_rn(unsigned long count, +extern char *_crypt_gensalt_md5_rn(__CONST char *prefix, unsigned long count, __CONST char *input, int size, char *output, int output_size); #if defined(__GLIBC__) && defined(_LIBC) @@ -200,8 +204,8 @@ char *crypt_r(__CONST char *key, __CONST char *setting, void *data) char *__crypt_gensalt_rn(__CONST char *prefix, unsigned long count, __CONST char *input, int size, char *output, int output_size) { - char *(*use)(unsigned long count, - __CONST char *input, int size, char *output, int output_size); + char *(*use)(__CONST char *_prefix, unsigned long _count, + __CONST char *_input, int _size, char *_output, int _output_size); /* This may be supported on some platforms in the future */ if (!input) { @@ -209,7 +213,8 @@ char *__crypt_gensalt_rn(__CONST char *prefix, unsigned long count, return NULL; } - if (!strncmp(prefix, "$2a$", 4)) + if (!strncmp(prefix, "$2a$", 4) || !strncmp(prefix, "$2b$", 4) || + !strncmp(prefix, "$2y$", 4)) use = _crypt_gensalt_blowfish_rn; else if (!strncmp(prefix, "$1$", 3)) @@ -228,7 +233,7 @@ char *__crypt_gensalt_rn(__CONST char *prefix, unsigned long count, return NULL; } - return use(count, input, size, output, output_size); + return use(prefix, count, input, size, output, output_size); } char *__crypt_gensalt_ra(__CONST char *prefix, unsigned long count, diff --git a/spec/bcrypt/engine_spec.rb b/spec/bcrypt/engine_spec.rb index 11ec907..4327c16 100644 --- a/spec/bcrypt/engine_spec.rb +++ b/spec/bcrypt/engine_spec.rb @@ -73,7 +73,8 @@ class MyInvalidSecret ["U*U*", "$2a$05$CCCCCCCCCCCCCCCCCCCCC.", "$2a$05$CCCCCCCCCCCCCCCCCCCCC.VGOzA784oUp/Z0DY336zx7pLYAy0lwK"], ["U*U*U", "$2a$05$XXXXXXXXXXXXXXXXXXXXXO", "$2a$05$XXXXXXXXXXXXXXXXXXXXXOAcXxm9kjPGEMsLznoKqmqw7tc8WCx4a"], ["", "$2a$05$CCCCCCCCCCCCCCCCCCCCC.", "$2a$05$CCCCCCCCCCCCCCCCCCCCC.7uG0VCzI2bS7j6ymqJi9CdcdxiRTWNy"], - ["0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789", "$2a$05$abcdefghijklmnopqrstuu", "$2a$05$abcdefghijklmnopqrstuu5s2v8.iXieOjg/.AySBTTZIIVFJeBui"] + ["0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789", "$2a$05$abcdefghijklmnopqrstuu", "$2a$05$abcdefghijklmnopqrstuu5s2v8.iXieOjg/.AySBTTZIIVFJeBui"], + ['password', '$2b$12$8WjpIlI8u0lM0BQ0SS9B3O', '$2b$12$8WjpIlI8u0lM0BQ0SS9B3OX.HJs/YuS.ombb6vr2lYodYYv/ZdpVe'] ] for secret, salt, test_vector in test_vectors expect(BCrypt::Engine.hash_secret(secret, salt)).to eql(test_vector)