Re: generic crypto - remarks

[Simon Josefsson]

Bruno Haible <address@hidden> writes:

> Hi Simon,
>
> Here are a few random remarks regarding the new code. It's a bit terse, but
> I hope you can decipher the meaning.

Hi!  Many thanks.  I have been pushing gc into gnulib rather rapidly
because we want to release GnuTLS 1.2.9 with that stuff quite soon,
then fork the release into a 1.3.x branch and declare 1.2.x a bug-fix
branch only.  I expect to continue review and fix things in gc over
quite some time, I know several things that are not perfect.  And I
hope others review all of the files too..

> 1) crc.h
>
> #if HAVE_INTTYPES_H
> # include <inttypes.h>
> #endif
> #if HAVE_STDINT_H
> # include <stdint.h>
> #endif
>
> Correct this to
>
> #include <stdint.h>

Fixed in CVS.

> 2) crc.c
>
> crc32_update. The 'crc' argument is unused. Looks like the line
>
>   return crc32_update_no_xor (0L ^ 0xffffffffL, buf, len) ^ 0xffffffffL;
>
> should be changed to
>
>   return crc32_update_no_xor (crc ^ 0xffffffffL, buf, len) ^ 0xffffffffL;

Fixed in CVS.

> 3) md5.h, md4.h
>
> I would change the type of 'buffer' in md4_ctx to uint32_t[32].
> That would take care of the alignment, and avoid casts in md5.c and md4.c.

I agree.

> But probably you don't want to do this, in order to avoid
> differences with glibc.

I'm not sure -- glibc didn't want my fixes anyway, so I think we are
officially forked.  However, there may be some point in minimizing
differences anyway, if at some point the glibc maintainers realize
that accepting the patch will be useful.  This costs us quite some
patience and result in unreadable code, so I'm not sure if it is a
good idea.

> Then at least please put the __attribute__ declaration in md4.h into
> comments. As written now, it causes a syntax error with non-gcc compilers.
> And defining __attribute__ to empty, like md5.h does, is bad because
> causes extra warnings with gcc-2.7.2. And you don't need it, because
> the rules for struct layout in C guarantee that a structure field is
> aligned to a multiple of the alignment of the previous field.

Are you saying that even if we don't change the type of buffer to
char, we don't need that alignment?  I would agree, but I'm not
certain.

> 4) md4.c
>
> In the line defining fillbuf, why two adjacent spaces?

GNU indent added them.  I have reverted manually.

> 5) md4.c
>
> The space after '&' in
>
>   *(uint32_t *) & ctx->buffer[bytes + pad] = SWAP (ctx->total[0] << 3);
>
> makes the '&' look like a binary operator and is therefore confusing.
> Better remove it.

Another GNU indent bug.  Fixed manually.

> 6) hmac-md5.c, hmac-sha1.c
>
> Comments inside the function would be appropriate. Something like this:

Fixed in CVS.


> 7) hmac-md5.h, hmac-sha1.h, hmac-md5.c, hmac-sha1.c
>
> You are ignoring the restriction in sha1.h, namely
>
>    IMPORTANT: On some systems it is required that RESBUF be correctly
>    aligned for a 32 bits value.
>
> The fix is to
>   - add this restriction also to hmac_md5 and hmac_sha1 in the .h files
>     (or, if this is unacceptable, use a temporary, aligned result on the
>     stack and add a memcpy from the temp result to the final result), and
>   - ensure that the optkeybuf and innerhash arrays on the stack are
>     correctly aligned. Currently you are speculating on undocumented
>     compiler properties. For forcing the alignment, you can use a union,
>     or simply change the declaration from
>         char optkeybuf[20];
>     to
>         uint32_t optkeybuf[20 / sizeof (uint32_t)];
>     and similarly for innerhash.

I'll have to think about this one.

> 8) rijndael-alg-fst.h
>
> The comment in the first line mentions a wrong filename.

Fixed in CVS.

> 9) rijndael-alg-fst.h
>
> Lack of comments describing the functions. In particular, it's not clear
> what the 4*(Nr + 1) in the first two functions mean - they don't take a
> Nr as argument.
>
> 10) rijndael-api-fst.h
>
> RIJNDAEL_MAX_KEY_SIZE: What does "ASCII char" mean here? Does it mean
> that the key has only 7*64 bits, i.e. 56 bytes? Or does it mean that the
> bit 7 of each byte must be zero?
>
> 11) rijndael-api-fst.h
>
> rijndaelMakeKey: The comment says that the KEYMATERIAL is of size KEYLEN.
> This means KEYLEN elements of type 'char'. But it's wrong: the implementation
> uses KEYLEN / 4 elements of type 'char'.
>
> 12) rijndael-api-fst.h
>
> For rijndaelBlockEncrypt and rijndaelBlockDecrypt it is not clear whether
> INPUTLEN must be a multiple of 8, or what happens if it is not. In the
> latter case, it is not clear whether the remaining INPUTLEN%8 bits are
> taken from the first or from the last byte, and whether they are taken
> from the upper or from the lower bits.
>
> 13) rijndael-api-fst.h
>
> The INPUT array must be aligned to alignof(uint32_t), when I look in the
> implementation (case RIJNDAEL_MODE_CBC). This should be mentioned in the .h
> file.
>
> 14) rijndael-api-fst.c
>
> In all 4 functions, "char block[16];" does not guarantee the necessary
> alignment. See above, remark #7. Likewise for gc_pbkdf2_sha1.c and maybe
> other places.
>
> 15) rijndael-api-fst.c
>
> Why the code
>     if (input == NULL)
>       return 0;
> ? Passing a NULL data pointer together with a positive data length is
> always a programming error and should result in a core dump.

As I'm not sure this AES implementation is the best one (I'm having
some problems with the CBC mode, the "setkey" API is a bit cumbersome,
and there is a confusing mix of lengths in units of
bits/blocks/bytes), I'll wait with fixing these until I have made sure
the implementation is correct.

> 16) gc.m4
>
> The test AC_CHECK_HEADER(gcrypt.h) is misplaced. It can happen - for
> example, when a user has installed libgcrypt but not libgcrypt-devel
> [assuming the typical Debian split of packages] - that libgcrypt.so is
> found but the header file is not. In this case you cannot use the
> library. The autoconf macro should undo the effects of
> AC_LIB_HAVE_LINKFLAGS([gcrypt])
> in this case. AC_LIB_HAVE_LINKFLAGS has an argument for this:
>
>   AC_LIB_HAVE_LINKFLAGS([gcrypt], [], [#include <gcrypt.h>])

Fixed in CVS.

> 17) gc.m4
>
> I don't understand what happens when someone specifies
> --disable-random-device. Then NAME_OF_RANDOM_DEVICE will be set to "no",
> the AC_CHECK_FILE will check for a file named "no" and bail out.

Hm.  How can this be solved?  Is AC_ARG_ENABLE the proper method to
set non-boolean parameters at all?  If it is, it should probably just
a "no" value, --disable-random-device[=foo] doesn't have any
semantics.


> 18) gc-gnulib.c
>
> Why do you write <gc.h> here but "gc.h" in the other files?

Fixed in CVS.

> 19) modules/gc
>
> The Makefile.am rule should use $(LTLIBGCRYPT) if libtool is use
> to link the library. I think this is best realized through an automake
> conditional
>
> if GL_COND_LIBTOOL
> lib_LIBADD += $(LTLIBGCRYPT)
> else
> lib_LIBADD += $(LIBGCRYPT)
> endif
>
> whose value should be given by gnulib-tool...

I applied that, and installed the patch below to gnulib-tool.

Thanks!

Index: gnulib-tool
===================================================================
RCS file: /cvsroot/gnulib/gnulib/gnulib-tool,v
retrieving revision 1.87
diff -u -p -r1.87 gnulib-tool
--- gnulib-tool 17 Oct 2005 15:44:51 -0000      1.87
+++ gnulib-tool 21 Oct 2005 13:13:45 -0000
@@ -1226,6 +1226,11 @@ func_import ()
     echo "# \"Check for header files, types and library functions\"."
     echo "AC_DEFUN([${macro_prefix}_INIT],"
     echo "["
+    if test -z "$libtool"; then
+      echo "AM_CONDITIONAL(GL_COND_LIBTOOL, false)"
+    else
+      echo "AM_CONDITIONAL(GL_COND_LIBTOOL, true)"
+    fi
     for module in $modules; do
       func_verify_module
       if test -n "$module"; then
@@ -1426,6 +1431,11 @@ func_create_testdir ()
        echo "gl_USE_SYSTEM_EXTENSIONS"
        echo
      fi
+     if test -z "$libtool"; then
+       echo "AM_CONDITIONAL(GL_COND_LIBTOOL, false)"
+     else
+       echo "AM_CONDITIONAL(GL_COND_LIBTOOL, true)"
+     fi
      # We don't have explicit ordering constraints between the various
      # autoconf snippets. It's cleanest to put those of the library before
      # those of the tests.
@@ -1484,6 +1494,11 @@ func_create_testdir ()
    if grep gl_USE_SYSTEM_EXTENSIONS "$testdir"/m4/*.m4 > /dev/null; then
      echo "gl_USE_SYSTEM_EXTENSIONS"
      echo
+   fi
+   if test -z "$libtool"; then
+     echo "AM_CONDITIONAL(GL_COND_LIBTOOL, false)"
+   else
+     echo "AM_CONDITIONAL(GL_COND_LIBTOOL, true)"
    fi
    for module in $modules; do
      func_verify_nontests_module