generic crypto - remarks

[Bruno Haible]

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.

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>

because
- On old platforms with neither <inttypes.h> nor <stdint.h>, these
  includes fail to define uint32_t.
- On platforms where <inttypes.h> defines the types but <stdint.h>
  does not, our stdint.h replacement takes care to #include the relevant
  headers.

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;

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.
But probably you don't want to do this, in order to avoid differences
with glibc.

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.

4) md4.c

In the line defining fillbuf, why two adjacent spaces?

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.

6) hmac-md5.c, hmac-sha1.c

Comments inside the function would be appropriate. Something like this:


  /* Reduce the key's size, so that it becomes <= 64 bytes large.  */

  if (keylen > 64)
    {
      struct md5_ctx keyhash;

      md5_init_ctx (&keyhash);
      md5_process_bytes (key, keylen, &keyhash);
      md5_finish_ctx (&keyhash, optkeybuf);

      key = optkeybuf;
      keylen = 16;
    }

  /* Compute INNERHASH from KEY and IN.  */

  md5_init_ctx (&inner);

  memset (block, IPAD, sizeof (block));
  memxor (block, key, keylen);

  md5_process_block (block, 64, &inner);
  md5_process_bytes (in, inlen, &inner);

  md5_finish_ctx (&inner, innerhash);

  /* Compute result from KEY and INNERHASH.  */

  md5_init_ctx (&outer);

  ...

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.

8) rijndael-alg-fst.h

The comment in the first line mentions a wrong filename.

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.

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>])

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.

18) gc-gnulib.c

Why do you write <gc.h> here but "gc.h" in the other files?

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...


Bruno