Makes sense -- but perhaps justify the arc4random with a comment,
explaining what is being done, so that people don't need to look
in the commitlog?
Ted Unangst wrote:
> In the event that a program uses invalid parameters, I think we should
> overwrite the key with random data. Otherwise, there's a chance the program
> will continue with a zero key. It may even appear to work, encrypting and
> decrypting data, but with a weak key. Random data means it fails closed, and
> should also make it easier to detect such errors since it no longer
> interoperates.
>
>
> Index: bcrypt_pbkdf.c
> ===
> RCS file: /home/cvs/src/lib/libutil/bcrypt_pbkdf.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 bcrypt_pbkdf.c
> --- bcrypt_pbkdf.c12 Jan 2015 03:20:04 - 1.13
> +++ bcrypt_pbkdf.c15 Oct 2019 19:14:12 -
> @@ -110,10 +110,10 @@ bcrypt_pbkdf(const char *pass, size_t pa
>
> /* nothing crazy */
> if (rounds < 1)
> - return -1;
> + goto bad;
> if (passlen == 0 || saltlen == 0 || keylen == 0 ||
> keylen > sizeof(out) * sizeof(out))
> - return -1;
> + goto bad;
> stride = (keylen + sizeof(out) - 1) / sizeof(out);
> amt = (keylen + stride - 1) / stride;
>
> @@ -166,4 +166,8 @@ bcrypt_pbkdf(const char *pass, size_t pa
> explicit_bzero(out, sizeof(out));
>
> return 0;
> +
> +bad:
> + arc4random_buf(key, keylen);
> + return -1;
> }
> Index: pkcs5_pbkdf2.c
> ===
> RCS file: /home/cvs/src/lib/libutil/pkcs5_pbkdf2.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 pkcs5_pbkdf2.c
> --- pkcs5_pbkdf2.c18 Apr 2017 04:06:21 - 1.10
> +++ pkcs5_pbkdf2.c15 Oct 2019 19:17:08 -
> @@ -84,11 +84,11 @@ pkcs5_pbkdf2(const char *pass, size_t pa
> size_t r;
>
> if (rounds < 1 || key_len == 0)
> - return -1;
> + goto bad;
> if (salt_len == 0 || salt_len > SIZE_MAX - 4)
> - return -1;
> + goto bad;
> if ((asalt = malloc(salt_len + 4)) == NULL)
> - return -1;
> + goto bad;
>
> memcpy(asalt, salt, salt_len);
>
> @@ -118,4 +118,8 @@ pkcs5_pbkdf2(const char *pass, size_t pa
> explicit_bzero(obuf, sizeof(obuf));
>
> return 0;
> +
> +bad:
> + arc4random_buf(key, key_len);
> + return -1;
> }
>
