libcrypt/md5.c: a few statics can be auto variables; strlen("const") is silly
Denys Vlasenko
vda.linux at googlemail.com
Thu Jun 12 07:08:15 UTC 2008
On Wednesday 11 June 2008 17:32, Will Newton wrote:
> >> This looks like it should be safe. Although it might be worth finding
> >> the original author and asking why it is done this way, following the
> >> Debian openssl incident. ;-)
> >
> > openssl incident? Did I miss some fun? :)
>
> This is a fairly good summary:
>
> http://research.swtch.com/2008/05/lessons-from-debianopenssl-fiasco.html
>
> The general lesson being: be very careful when modifying crypto code.
I'd place the blame on the author. He wrote unreadable code.
Unfortunately, it's far too common.
> btw I tested the strlen change and this code:
>
> #include <string.h>
>
> static const unsigned char __md5__magic[] = "$1$";
>
> int foo(void)
> {
> return strlen(__md5__magic);
> }
>
> compiles to this:
>
> Disassembly of section .text:
>
> 00000000 <foo>:
> 0: 55 push %ebp
> 1: 89 e5 mov %esp,%ebp
> 3: b8 03 00 00 00 mov $0x3,%eax
> 8: 5d pop %ebp
> 9: c3 ret
Don't place too much faith in gcc, that would be a mistake.
Piling up a bit more code around, and it will fail to optimize
it completely.
This is the patch wich ONLY replaces strlen("const") with
sizeof("const")-1. Let's check assembly, do we see strlen
in original version?
--- md5_old.c Thu Jun 12 08:58:54 2008
+++ md5_new.c Thu Jun 12 09:01:27 2008
@@ -95,8 +95,11 @@
static void __md5_Transform __P((u_int32_t [4], const unsigned char [64]));
-static const unsigned char __md5__magic[] = "$1$"; /* This string is magic for this algorithm. Having
- it this way, we can get better later on */
+#define MD5_MAGIC_STR "$1$"
+#define MD5_MAGIC_LEN (sizeof(MD5_MAGIC_STR) - 1)
+static const unsigned char __md5__magic[] = MD5_MAGIC_STR;
+/* This string is magic for this algorithm. Having
+ * it this way, we can get better later on */
static const unsigned char __md5_itoa64[] = /* 0 ... 63 => ascii - 64 */
"./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
@@ -534,7 +537,7 @@
static char passwd[120], *p;
unsigned char final[17]; /* final[16] exists only to aid in looping */
- int sl,pl,i,__md5__magic_len,pw_len;
+ int sl,pl,i,pw_len;
struct MD5Context ctx,ctx1;
unsigned long l;
@@ -542,10 +545,8 @@
sp = salt;
/* If it starts with the magic string, then skip that */
- __md5__magic_len = strlen(__md5__magic);
- if(!strncmp(sp,__md5__magic,__md5__magic_len))
- sp += __md5__magic_len;
-
+ if(!strncmp(sp,__md5__magic,MD5_MAGIC_LEN))
+ sp += MD5_MAGIC_LEN;
/* It stops at the first '$', max 8 chars */
for(ep=sp;*ep && *ep != '$' && ep < (sp+8);ep++)
continue;
@@ -560,7 +561,7 @@
__md5_Update(&ctx,pw,pw_len);
/* Then our magic string */
- __md5_Update(&ctx,__md5__magic,__md5__magic_len);
+ __md5_Update(&ctx,__md5__magic,MD5_MAGIC_LEN);
/* Then the raw salt */
__md5_Update(&ctx,sp,sl);
Whoa! strlen is there!
--- md5_old.s Thu Jun 12 08:59:17 2008
+++ md5_new.s Thu Jun 12 09:01:49 2008
@@ -259,21 +259,17 @@
pushl %edi
pushl %esi
pushl %ebx
- subl $232, %esp
- movl 256(%esp), %ebx
+ subl $224, %esp
+ movl 248(%esp), %ebx
movl %ebx, sp.2176
+ pushl $3
pushl $__md5__magic
- call strlen <=================
- addl $12, %esp
- movl %eax, %esi
- pushl %eax
- pushl $__md5__magic
pushl %ebx
call strncmp
addl $16, %esp
testl %eax, %eax
jne .L28
- leal (%ebx,%esi), %eax
+ leal 3(%ebx), %eax
movl %eax, sp.2176
.L28:
movl sp.2176, %eax
@@ -311,7 +307,7 @@
movl 240(%esp), %edx
movl %ebx, %eax
call __md5_Update
- movl %esi, %ecx
+ movl $3, %ecx
movl $__md5__magic, %edx
movl %ebx, %eax
call __md5_Update
# i486-linux-uclibc-gcc -v 2>&1 | grep 'gcc version'
gcc version 4.3.1 (GCC)
--
vda
More information about the uClibc
mailing list