Re: Strong Crypto Support for GRUB2

[Robert Millan]

On Sun, Sep 02, 2007 at 07:25:26PM +0200, Simon Peter wrote:
> diff -u -r -x CVS -N grub2-orig/crypto/aes.c grub2/crypto/aes.c
> --- grub2-orig/crypto/aes.c   1970-01-01 01:00:00.000000000 +0100
> +++ grub2/crypto/aes.c        2007-09-02 00:45:00.000000000 +0200
> @@ -0,0 +1,1062 @@
> +/*
> + *  FIPS-197 compliant AES implementation
> + *
> + *  Copyright (C) 2006-2007  Christophe Devine
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License, version 2.1 as published by the Free Software Foundation.

According to http://www.gnu.org/licenses/gpl-faq.html#compat-matrix-footnote-7 ,
LGPL 2.1 is compatible with GPLv3 (much to my surprise).  Not sure if the
author being someone else will be a problem though (that's up to the GRUB
maintainers I guess...).

> +#ifndef GET_UINT32_BE
> +#define GET_UINT32_BE(n,b,i)                            \
> +{                                                       \
> +    (n) = ( (uint32) (b)[(i)    ] << 24 )        \
> +        | ( (uint32) (b)[(i) + 1] << 16 )        \
> +        | ( (uint32) (b)[(i) + 2] <<  8 )        \
> +        | ( (uint32) (b)[(i) + 3]       );       \
> +}

Doesn't follow GCS indentation style in a number of places.  I would suggest
using the indent(1) tool on it.

> +/*
> + * Forward S-box
> + */
> +static const uint8 FSb[256] =
> +{
> +    0x63, 0x7C, 0x77, 0x7B, 0xF2, 0x6B, 0x6F, 0xC5,
> +    0x30, 0x01, 0x67, 0x2B, 0xFE, 0xD7, 0xAB, 0x76,
> +    0xCA, 0x82, 0xC9, 0x7D, 0xFA, 0x59, 0x47, 0xF0,
> +    0xAD, 0xD4, 0xA2, 0xAF, 0x9C, 0xA4, 0x72, 0xC0,
> +    0xB7, 0xFD, 0x93, 0x26, 0x36, 0x3F, 0xF7, 0xCC,
> +    0x34, 0xA5, 0xE5, 0xF1, 0x71, 0xD8, 0x31, 0x15,
> +    0x04, 0xC7, 0x23, 0xC3, 0x18, 0x96, 0x05, 0x9A,
> +    0x07, 0x12, 0x80, 0xE2, 0xEB, 0x27, 0xB2, 0x75,
> +    0x09, 0x83, 0x2C, 0x1A, 0x1B, 0x6E, 0x5A, 0xA0,
> +    0x52, 0x3B, 0xD6, 0xB3, 0x29, 0xE3, 0x2F, 0x84,
> +    0x53, 0xD1, 0x00, 0xED, 0x20, 0xFC, 0xB1, 0x5B,
> +    0x6A, 0xCB, 0xBE, 0x39, 0x4A, 0x4C, 0x58, 0xCF,
> +    0xD0, 0xEF, 0xAA, 0xFB, 0x43, 0x4D, 0x33, 0x85,
> +    0x45, 0xF9, 0x02, 0x7F, 0x50, 0x3C, 0x9F, 0xA8,
> +    0x51, 0xA3, 0x40, 0x8F, 0x92, 0x9D, 0x38, 0xF5,
> +    0xBC, 0xB6, 0xDA, 0x21, 0x10, 0xFF, 0xF3, 0xD2,
> +    0xCD, 0x0C, 0x13, 0xEC, 0x5F, 0x97, 0x44, 0x17,
> +    0xC4, 0xA7, 0x7E, 0x3D, 0x64, 0x5D, 0x19, 0x73,
> +    0x60, 0x81, 0x4F, 0xDC, 0x22, 0x2A, 0x90, 0x88,
> +    0x46, 0xEE, 0xB8, 0x14, 0xDE, 0x5E, 0x0B, 0xDB,
> +    0xE0, 0x32, 0x3A, 0x0A, 0x49, 0x06, 0x24, 0x5C,
> +    0xC2, 0xD3, 0xAC, 0x62, 0x91, 0x95, 0xE4, 0x79,
> +    0xE7, 0xC8, 0x37, 0x6D, 0x8D, 0xD5, 0x4E, 0xA9,
> +    0x6C, 0x56, 0xF4, 0xEA, 0x65, 0x7A, 0xAE, 0x08,
> +    0xBA, 0x78, 0x25, 0x2E, 0x1C, 0xA6, 0xB4, 0xC6,
> +    0xE8, 0xDD, 0x74, 0x1F, 0x4B, 0xBD, 0x8B, 0x8A,
> +    0x70, 0x3E, 0xB5, 0x66, 0x48, 0x03, 0xF6, 0x0E,
> +    0x61, 0x35, 0x57, 0xB9, 0x86, 0xC1, 0x1D, 0x9E,
> +    0xE1, 0xF8, 0x98, 0x11, 0x69, 0xD9, 0x8E, 0x94,
> +    0x9B, 0x1E, 0x87, 0xE9, 0xCE, 0x55, 0x28, 0xDF,
> +    0x8C, 0xA1, 0x89, 0x0D, 0xBF, 0xE6, 0x42, 0x68,
> +    0x41, 0x99, 0x2D, 0x0F, 0xB0, 0x54, 0xBB, 0x16
> +};

This (and similar binary blobs) seems like a problem.  What do these numbers
mean?  If the spec defines a simple algorithm to generate them, a comment
with its implementation would be nice to have (e.g. like in md5sum.c from
coreutils).

> +GRUB_MOD_INIT(crypto)
> +{
> +  (void)mod;                 /* To stop warning. */
> +  grub_crypto_cipher_register(&grub_cipher_none);
> +  grub_crypto_cipher_register(&grub_hash_none);
> +}

Which warning was that?

> diff -u -r -x CVS -N grub2-orig/crypto/rmd160.c grub2/crypto/rmd160.c
> --- grub2-orig/crypto/rmd160.c        1970-01-01 01:00:00.000000000 +0100
> +++ grub2/crypto/rmd160.c     2007-09-02 00:52:57.000000000 +0200
> @@ -0,0 +1,430 @@
> +/*
> + * 2007-09-01: Modified for GNU GRUB by Simon Peter <address@hidden>.
> + */
> +/********************************************************************\
> + *
> + *      FILE:     rmd160.c
> + *
> + *      CONTENTS: A sample C-implementation of the RIPEMD-160
> + *                hash-function.
> + *      TARGET:   any computer with an ANSI C compiler
> + *
> + *      AUTHOR:   Antoon Bosselaers, ESAT-COSIC
> + *      DATE:     1 March 1996
> + *      VERSION:  1.0
> + *
> + *      Copyright (c) Katholieke Universiteit Leuven
> + *      1996, All Rights Reserved
> + *
> + *  Conditions for use of the RIPEMD-160 Software
> + *
> + *  The RIPEMD-160 software is freely available for use under the terms and
> + *  conditions described hereunder, which shall be deemed to be accepted by
> + *  any user of the software and applicable on any use of the software:
> + * 
> + *  1. K.U.Leuven Department of Electrical Engineering-ESAT/COSIC shall for
> + *     all purposes be considered the owner of the RIPEMD-160 software and of
> + *     all copyright, trade secret, patent or other intellectual property
> + *     rights therein.
> + *  2. The RIPEMD-160 software is provided on an "as is" basis without
> + *     warranty of any sort, express or implied. K.U.Leuven makes no
> + *     representation that the use of the software will not infringe any
> + *     patent or proprietary right of third parties. User will indemnify
> + *     K.U.Leuven and hold K.U.Leuven harmless from any claims or liabilities
> + *     which may arise as a result of its use of the software. In no
> + *     circumstances K.U.Leuven R&D will be held liable for any deficiency,
> + *     fault or other mishappening with regard to the use or performance of
> + *     the software.
> + *  3. User agrees to give due credit to K.U.Leuven in scientific 
> publications 
> + *     or communications in relation with the use of the RIPEMD-160 software 
> + *     as follows: RIPEMD-160 software written by Antoon Bosselaers, 
> + *     available at http://www.esat.kuleuven.be/~cosicart/ps/AB-9601/.
> + *
> +\********************************************************************/

3) doesn't look GPL-compatible.  As for 1), note the author is claiming
ownership of any patents that might be covered by this code.  GPL compatibility
aside, I'm not sure what the consequences of accepting the license would be
(could it lead to someone acknowledging K.U.Leuven as the owner of their
own patents?), but it looks dangerous.

> +  /* Choke on alrady existing devices */

Typo ;-)

> +static void
> +grub_crypto_close (grub_disk_t disk)
> +{
> +  crypto_private_t private = (crypto_private_t)disk->data;
> +/*   char buf[48]; */
> +
> +/*   grub_disk_read(disk, 0, 0, 48, buf); */
> +/*   grub_printf("disk id: %-48s\n", buf); */
> +
> +  grub_disk_close(private->srcdisk);
> +  grub_free(private);
> +}

You left some temporary debugging stuff here.  But instead of removing it, may
I suggest using grub_dprintf ?  (see http://grub.enbug.org/HowToDebug)

> +enum grub_cipher_type
> +  {
> +    GRUB_CIPHER_TYPE_NONE = 0,
> +    GRUB_CIPHER_TYPE_CIPHER = 1,
> +    GRUB_CIPHER_TYPE_HASH = 2
> +  };

Wasn't the point of using enum to avoid hardcoding these numbers? :-)

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)