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