[PATCH] Limit and document Blowfish key lengths to 8-576 bits
Jussi Kivilinna
jussi.kivilinna at iki.fi
Thu Apr 18 17:38:48 CEST 2019
Hello,
On 18.4.2019 0.50, Peter Wu wrote:
> Hi Jussi,
>
> Just some notes below on the tests, documentation looks good to me.
> Additionally, indentation in this file is a bit of a mess with mixed
> tabs and spaces.
>
> Should gcry_cipher_get_algo_keylen be modified as well to return "the
> maximum supported key length"? Hopefully it does not break stuff that
> assumed this to be fixed.
I think it's better not to change the return value for
gcry_cipher_get_algo_keylen as existing users might depend it to stay
fixed to 128bits.
>
> On Wed, Apr 17, 2019 at 11:16:17PM +0300, Jussi Kivilinna wrote:
>> * cipher/blowfish.c (BLOWFISH_KEY_MIN_BITS)
>> (BLOWFISH_KEY_MAX_BITS): New.
>> (do_bf_setkey): Check input key length to MIN_BITS and MAX_BITS.
>> * doc/gcrypt.texi: Update supported Blowfish key lengths.
>> * tests/basic.c (check_ecb_cipher): New, with Blowfish test vectors
>> for different key lengths.
>> (check_cipher_modes): Call 'check_ecb_cipher'.
>> --
>>
>> As noted by Peter Wu, Blowfish cipher implementation already supports key
>> lengths 8 to 576 bits [1]. This change updates documentation to reflect
>> that and adds new test vectors to check handling of different key lengths.
>>
>> [1] https://lists.gnupg.org/pipermail/gcrypt-devel/2019-April/004680.html
>>
>> Signed-off-by: Jussi Kivilinna <jussi.kivilinna at iki.fi>
>> ---
>> 0 files changed
>>
>> diff --git a/cipher/blowfish.c b/cipher/blowfish.c
>> index ea6e64a7b..a1d81d310 100644
>> --- a/cipher/blowfish.c
>> +++ b/cipher/blowfish.c
>> @@ -41,6 +41,8 @@
>> #include "cipher-selftest.h"
>>
>> #define BLOWFISH_BLOCKSIZE 8
>> +#define BLOWFISH_KEY_MIN_BITS 8
>> +#define BLOWFISH_KEY_MAX_BITS 576
>>
>>
>> /* USE_AMD64_ASM indicates whether to use AMD64 assembly code. */
>> @@ -1018,6 +1020,10 @@ do_bf_setkey (BLOWFISH_context *c, const byte *key, unsigned keylen)
>> if( selftest_failed )
>> return GPG_ERR_SELFTEST_FAILED;
>>
>> + if (keylen < BLOWFISH_KEY_MIN_BITS / 8 ||
>> + keylen > BLOWFISH_KEY_MAX_BITS / 8)
>> + return GPG_ERR_INV_KEYLEN;
>> +
>> memset(hset, 0, sizeof(hset));
>>
>> for(i=0; i < 16+2; i++ )
>> diff --git a/doc/gcrypt.texi b/doc/gcrypt.texi
>> index 8b765ba80..d7bfa4c27 100644
>> --- a/doc/gcrypt.texi
>> +++ b/doc/gcrypt.texi
>> @@ -1538,7 +1538,7 @@ This is the IDEA algorithm.
>> @cindex Triple-DES
>> @cindex DES-EDE
>> @cindex Digital Encryption Standard
>> -Triple-DES with 3 Keys as EDE. The key size of this algorithm is 168 but
>> +Triple-DES with 3 Keys as EDE. The key size of this algorithm is 168 bits but
>> you have to pass 192 bits because the most significant bits of each byte
>> are ignored.
>>
>> @@ -1548,8 +1548,8 @@ CAST128-5 block cipher algorithm. The key size is 128 bits.
>>
>> @item GCRY_CIPHER_BLOWFISH
>> @cindex Blowfish
>> -The blowfish algorithm. The current implementation allows only for a key
>> -size of 128 bits.
>> +The blowfish algorithm. The supported key sizes are 8 to 576 bits in
>> +8 bit increments.
>>
>> @item GCRY_CIPHER_SAFER_SK128
>> Reserved and not currently implemented.
>> diff --git a/tests/basic.c b/tests/basic.c
>> index 3d6e8fc1e..792b7737b 100644
>> --- a/tests/basic.c
>> +++ b/tests/basic.c
>> @@ -446,6 +446,239 @@ check_aes128_cbc_cts_cipher (void)
>> fprintf (stderr, " Completed AES128 CBC CTS checks.\n");
>> }
>>
>> +static void
>> +check_ecb_cipher (void)
>> +{
>> + /* ECB cipher check. Mainly for testing underlying block cipher. */
>> + static const struct tv
>> + {
>> + int algo;
>> + const char *key;
>> + struct
>> + {
>> + const char *plaintext;
>> + int keylen;
>> + int inlen;
>> + const char *out;
>> + } data[MAX_DATA_LEN];
>> + } tv[] =
>> + {
>> + /* Test vectors from OpenSSL for key lengths of 8 to 200 bits */
>> + { GCRY_CIPHER_BLOWFISH,
>> + "\xf0\xe1\xd2\xc3\xb4\xa5\x96\x87\x78\x69\x5a\x4b\x3c\x2d\x1e\x0f"
>> + "\x00\x11\x22\x33\x44\x55\x66\x77\x88",
>> + { { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 1,
>> + 8,
>> + "\xf9\xad\x59\x7c\x49\xdb\x00\x5e" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 2,
>> + 8,
>> + "\xe9\x1d\x21\xc1\xd9\x61\xa6\xd6" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 3,
>> + 8,
>> + "\xe9\xc2\xb7\x0a\x1b\xc6\x5c\xf3" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 4,
>> + 8,
>> + "\xbe\x1e\x63\x94\x08\x64\x0f\x05" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 5,
>> + 8,
>> + "\xb3\x9e\x44\x48\x1b\xdb\x1e\x6e" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 6,
>> + 8,
>> + "\x94\x57\xaa\x83\xb1\x92\x8c\x0d" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 7,
>> + 8,
>> + "\x8b\xb7\x70\x32\xf9\x60\x62\x9d" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 8,
>> + 8,
>> + "\xe8\x7a\x24\x4e\x2c\xc8\x5e\x82" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 9,
>> + 8,
>> + "\x15\x75\x0e\x7a\x4f\x4e\xc5\x77" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 10,
>> + 8,
>> + "\x12\x2b\xa7\x0b\x3a\xb6\x4a\xe0" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 11,
>> + 8,
>> + "\x3a\x83\x3c\x9a\xff\xc5\x37\xf6" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 12,
>> + 8,
>> + "\x94\x09\xda\x87\xa9\x0f\x6b\xf2" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 13,
>> + 8,
>> + "\x88\x4f\x80\x62\x50\x60\xb8\xb4" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 14,
>> + 8,
>> + "\x1f\x85\x03\x1c\x19\xe1\x19\x68" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 15,
>> + 8,
>> + "\x79\xd9\x37\x3a\x71\x4c\xa3\x4f" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 16,
>> + 8,
>> + "\x93\x14\x28\x87\xee\x3b\xe1\x5c" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 17,
>> + 8,
>> + "\x03\x42\x9e\x83\x8c\xe2\xd1\x4b" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 18,
>> + 8,
>> + "\xa4\x29\x9e\x27\x46\x9f\xf6\x7b" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 19,
>> + 8,
>> + "\xaf\xd5\xae\xd1\xc1\xbc\x96\xa8" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 20,
>> + 8,
>> + "\x10\x85\x1c\x0e\x38\x58\xda\x9f" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 21,
>> + 8,
>> + "\xe6\xf5\x1e\xd7\x9b\x9d\xb2\x1f" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 22,
>> + 8,
>> + "\x64\xa6\xe1\x4a\xfd\x36\xb4\x6f" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 23,
>> + 8,
>> + "\x80\xc7\xd7\xd4\x5a\x54\x79\xad" },
>> + { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 24,
>> + 8,
>> + "\x05\x04\x4b\x62\xfa\x52\xd0\x80" }
>> + }
>> + },
>> + /* Test vector from Linux kernel for key length of 448 bits */
>> + { GCRY_CIPHER_BLOWFISH,
>> + "\xf0\xe1\xd2\xc3\xb4\xa5\x96\x87\x78\x69\x5a\x4b\x3c\x2d\x1e\x0f"
>> + "\x00\x11\x22\x33\x44\x55\x66\x77\x04\x68\x91\x04\xc2\xfd\x3b\x2f"
>> + "\x58\x40\x23\x64\x1a\xba\x61\x76\x1f\x1f\x1f\x1f\x0e\x0e\x0e\x0e"
>> + "\xff\xff\xff\xff\xff\xff\xff\xff",
>> + { { "\xfe\xdc\xba\x98\x76\x54\x32\x10",
>> + 56,
>> + 8,
>> + "\xc0\x45\x04\x01\x2e\x4e\x1f\x53" } }
>> + },
>> + };
>> + gcry_cipher_hd_t hde, hdd;
>> + unsigned char out[MAX_DATA_LEN];
>> + int i, j, keylen, algo;
>> + gcry_error_t err = 0;
>> +
>> + if (verbose)
>> + fprintf (stderr, " Starting ECB checks.\n");
>> +
>> + for (i = 0; i < sizeof (tv) / sizeof (tv[0]); i++)
>> + {
>> + algo = tv[i].algo;
>> +
>> + if (gcry_cipher_test_algo (algo) && in_fips_mode)
>> + {
>> + if (verbose)
>> + fprintf (stderr, " algorithm %d not available in fips mode\n",
>> + algo);
>> + continue;
>> + }
>> +
>> + if (verbose)
>> + fprintf (stderr, " checking ECB mode for %s [%i]\n",
>> + gcry_cipher_algo_name (algo),
>> + algo);
>> + err = gcry_cipher_open (&hde, algo, GCRY_CIPHER_MODE_ECB, 0);
>> + if (!err)
>> + err = gcry_cipher_open (&hdd, algo, GCRY_CIPHER_MODE_ECB, 0);
>> + if (err)
>> + {
>> + fail ("ecb-algo:%d-tv:%d, gcry_cipher_open failed: %s\n", algo, i,
>> + gpg_strerror (err));
>
> You do close the cipher handle below in the error case. For consistency,
> should you do it here (and below) as well?
Yes, handles should be closed here too.
>
>> + return;
>> + }
>> +
>> + for (j = 0; tv[i].data[j].inlen; j++)
>
> The arrays are not terminated with an empty element, this probably trips
> over a buffer overflow error if you run it with AddressSanitizer.
Need to add terminating last entry.
>
>> + {
>> + keylen = tv[i].data[j].keylen;
>
>> + if (!keylen)
>> + {
>> + keylen = gcry_cipher_get_algo_keylen(algo);
>> + if (!keylen)
>> + {
>> + fail ("ecb-algo:%d-tv:%d-data:%d, gcry_cipher_get_algo_keylen failed\n",
>> + algo, i, j);
>> + return;
>> + }
>> + }
>
> This check is dead code, the key length is always specified here.
For now all test vectors specify key length, but if new vectors are
add they could use default key length (and also test that
gcry_cipher_get_algo_keylen returns expected value).
-Jussi
More information about the Gcrypt-devel
mailing list