[Dovecot] weakness in hash salt generation
Dovecot has routines for password hashing; two of these, crypt_generate
and md5_generate, both use sizeof(salt_chars) when reducing a random
string to salt.
I became suspicious when I noted that all salts generated are composed
only of "./01".
Unfortunately salt_char is declared static const char * rather than
static const char [], and so sizeof(salt_chars) is the size of a pointer.
Recommend:
diff -u -r1.8 password-scheme.c
--- password-scheme.c 30 May 2004 03:57:15 -0000 1.8
+++ password-scheme.c 23 Jul 2004 17:05:36 -0000
@@ -15,7 +15,7 @@
# include
On 23.7.2004, at 20:11, Joshua Goodall wrote:
Dovecot has routines for password hashing; two of these, crypt_generate and md5_generate, both use sizeof(salt_chars) when reducing a random string to salt.
So it seems. But how did you notice it? I don't think those functions are ever called by Dovecot itself? They're there just in case some day they would be useful..
On Fri, Jul 23, 2004 at 09:06:05PM +0300, Timo Sirainen wrote:
So it seems. But how did you notice it? I don't think those functions are ever called by Dovecot itself? They're there just in case some day they would be useful..
They're useful now. Reason attached, a first draft of dovecotpw.c. Only tested on FreeBSD 5-CURRENT.
Regards Joshua.
-- Joshua Goodall "as modern as tomorrow afternoon" joshua@roughtrade.net - FW109
On Sat, Jul 24, 2004 at 11:49:12AM +1000, Joshua Goodall wrote:
On Fri, Jul 23, 2004 at 09:06:05PM +0300, Timo Sirainen wrote:
So it seems. But how did you notice it? I don't think those functions are ever called by Dovecot itself? They're there just in case some day they would be useful..
They're useful now. Reason attached, a first draft of dovecotpw.c. Only tested on FreeBSD 5-CURRENT.
I fleshed this out a bit. OK, a lot, because this was done to improve interoperability with OpenLDAP. The attached diff:
- Provides two new schemes, {SMD5} and {SSHA} (salted strong password hashes, both available in OpenLDAP);
- Supports the {MD5} that OpenLDAP uses (which is actually more like the unsalted {PLAIN-MD5} only base64-encoded;
- Provides a BSD-licensed (non-advertising clause) SHA implementation so you don't have to rely on openssl for {SHA};
- Adds a password generation tool that hooks directly into Dovecot's password scheme code:
usage: dovecotpw [-l] [-p plaintext] [-s scheme] [-u user] [-V] -l List known password schemes -p plaintext New password -s scheme Password scheme -u user Username (if scheme uses it) -V Internally verify the hash
I originally wrote this for production of HMAC-MD5 contexts to support CRAM-MD5, and just generalised it.
Diff against cvs HEAD attached. Tested on FreeBSD 5-CURRENT and Debian GNU/Linux (unstable), both only on i386. You'll need to rerun automake & autoconf after patching.
- Joshua
On Sun, 2004-07-25 at 15:40, Joshua Goodall wrote:
I fleshed this out a bit. OK, a lot, because this was done to improve interoperability with OpenLDAP. The attached diff:
Committed with a few minor changes. And btw:
+#define STRWIPE(s) do { \
- char *c; \
- for (c = s; *c != '\0'; c++) \
+} while (0)*c = '\0'; \
safe_memset() exists pretty much for this reason. Compilers may sooner or later optimize out this kind of code because it seems useless to it. Microsoft's compilers already do in some cases.
Of course there could be safe_strwipe() to avoid extra strlen() call but it's a bit unnecessary optimization IMHO :)
Hi.
Timo Sirainen wrote:
On Sun, 2004-07-25 at 15:40, Joshua Goodall wrote:
I fleshed this out a bit. OK, a lot, because this was done to improve interoperability with OpenLDAP. The attached diff:
Committed with a few minor changes. And btw:
+#define STRWIPE(s) do { \
- char *c; \
- for (c = s; *c != '\0'; c++) \
*c = '\0'; \safe_memset() exists pretty much for this reason. Compilers may sooner or later optimize out this kind of code because it seems useless to it. Microsoft's compilers already do in some cases.
That's called bzero() on *nix (POSIX.1).
Lets now pray that MS compilers won't optimize out security-critical parts of code. Ha ha. Not funny.
--
./lxnt
On 27.7.2004, at 09:27, Alexander Sabourenkov wrote:
safe_memset() exists pretty much for this reason. Compilers may sooner or later optimize out this kind of code because it seems useless to it. Microsoft's compilers already do in some cases.
That's called bzero() on *nix (POSIX.1).
Nope:
CONFORMING TO 4.3BSD. This function is deprecated -- use memset in new programs.
Also I just tested how gcc 3.3.3 works. In a function like:
void test(void) { char arr[4];
scanf("%3s", arr);
printf("your secure password: %s\n", arr);
bzero(arr, sizeof(arr));
memset(arr, 0, sizeof(arr));}
Both bzero() and memset() are optimized away if optimizations are turned on.
Timo Sirainen wrote:
On 27.7.2004, at 09:27, Alexander Sabourenkov wrote:
safe_memset() exists pretty much for this reason. Compilers may sooner or later optimize out this kind of code because it seems useless to it. Microsoft's compilers already do in some cases.
That's called bzero() on *nix (POSIX.1).
Nope:
CONFORMING TO 4.3BSD. This function is deprecated -- use memset in new programs.
May be so in glibc, but (freebsd 5.2.1):
HISTORY
A bzero() function appeared in 4.3BSD. Its prototype existed previously
in
and nothing on deprecation. Though I made a mistake of mentioning POSIX.1 here, seems like it was moved out of string.h to make string.h posix-compliant.
Also I just tested how gcc 3.3.3 works. In a function like:
void test(void) { char arr[4];
scanf("%3s", arr); printf("your secure password: %s\n", arr); bzero(arr, sizeof(arr)); memset(arr, 0, sizeof(arr));}
Both bzero() and memset() are optimized away if optimizations are turned on.
Note I haven' said it should not ever be optimized away.
Anyway I really prefer the way strings are handled in exim MTA, complete with pool memory allocation. Second preferable is APR, taking subversion as an example. That's much cleaner than the (more) traditional methods, IMHO.
--
./lxnt
- On 2004.07.27, in 4105FE76.1000508@lxnt.info,
- "Alexander Sabourenkov" screwdriver@lxnt.info wrote:
HISTORY A bzero() function appeared in 4.3BSD. Its prototype existed previously in
before it was moved to for IEEE Std 1003.1-2001 (``POSIX.1'') compliance.
bzero(), bcopy(), and bcmp() date from early days of C/Unix standardization, when everyone had one. These were the BSD byte range operations, and memset(), memcpy(), and memcmp() were the non-BSD ones. bzero() maps precisely to memset(), and bcmp() to memcmp(), provided you swap appropriate arguments. These days, since mem*() are in the standard library and b*() are not, it's generally wiser/more portable to use them. bcopy() is slightly different from memcpy(), but if you have no need of this difference, it's likewise better to use memcpy(). b*() are not necessarily supported on a given platform; mem*() are.
The difference between bcopy() and memcpy() is narrow, and occurs only when the destination address plus the range is greater than the source address -- i.e., the copy operation will overlap its own source. In this case, bcopy() is guaranteed to use at least two copy operations, thus preserving the original data. Memcpy() can use only one copy operation, overwriting the original data. (Or: bcopy() is focussed on accuracy, and memcpy() may be optimized for execution speed.) This distinction tends to bring the other b*() and mem*() operations with it, as they're all parts of families.
That's why you'll find it deprecated by some, and praised by others.
-- -D. dgc@uchicago.edu NSIT::ENSS
participants (4)
-
Alexander Sabourenkov
-
David Champion
-
Joshua Goodall
-
Timo Sirainen