[PATCH] Limit and document Blowfish key lengths to 8-576 bits
Peter Wu
peter at lekensteyn.nl
Wed Apr 17 23:50:36 CEST 2019
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.
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?
> + 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.
> + {
> + 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.
> +
> + err = gcry_cipher_setkey (hde, tv[i].key, keylen);
> + if (!err)
> + err = gcry_cipher_setkey (hdd, tv[i].key, keylen);
> + if (err)
> + {
> + fail ("ecb-algo:%d-tv:%d-data:%d, gcry_cipher_setkey failed: %s\n",
> + algo, i, j, gpg_strerror (err));
> + gcry_cipher_close (hde);
> + gcry_cipher_close (hdd);
> + return;
> + }
> +
> + err = gcry_cipher_encrypt (hde, out, MAX_DATA_LEN,
> + tv[i].data[j].plaintext,
> + tv[i].data[j].inlen);
> + if (err)
> + {
> + fail ("ecb-algo:%d-tv:%d-data:%d, gcry_cipher_encrypt failed: %s\n",
> + algo, i, j, gpg_strerror (err));
> + gcry_cipher_close (hde);
> + gcry_cipher_close (hdd);
> + return;
> + }
> +
> + if (memcmp (tv[i].data[j].out, out, tv[i].data[j].inlen))
> + {
> + fail ("ecb-algo:%d-tv:%d-data:%d, encrypt mismatch entry\n",
> + algo, i, j);
> + }
> +
> + err = gcry_cipher_decrypt (hdd, out, tv[i].data[j].inlen, NULL, 0);
> + if (err)
> + {
> + fail ("ecb-algo:%d-tv:%d-data:%d, gcry_cipher_decrypt failed: %s\n",
> + algo, i, j, gpg_strerror (err));
> + gcry_cipher_close (hde);
> + gcry_cipher_close (hdd);
> + return;
> + }
> +
> + if (memcmp (tv[i].data[j].plaintext, out, tv[i].data[j].inlen))
> + {
> + fail ("ecb-algo:%d-tv:%d-data:%d, decrypt mismatch entry\n",
> + algo, i, j);
> + }
> + }
> +
> + gcry_cipher_close (hde);
> + gcry_cipher_close (hdd);
> + }
> + if (verbose)
> + fprintf (stderr, " Completed ECB checks.\n");
> +}
> +
> static void
> check_ctr_cipher (void)
> {
> @@ -7916,6 +8149,7 @@ check_cipher_modes(void)
> if (verbose)
> fprintf (stderr, "Starting Cipher Mode checks.\n");
>
> + check_ecb_cipher ();
> check_aes128_cbc_cts_cipher ();
> check_cbc_mac_cipher ();
> check_ctr_cipher ();
>
>
More information about the Gcrypt-devel
mailing list