Zoltan Fridrich <zfrid...@redhat.com> writes:
> sorry for bothering but I would like to ask you to review the updated
> balloon hashing patch. I have fixed all of the issues you mentioned and I
> would like to continue working on it until it gets approved for Nettle.
> Could you please take a look? Attaching the updated patch again, just in
> case.
Thanks for your patience. A new round of comments below.
> diff --color -ruNp a/balloon.c b/balloon.c
> --- a/balloon.c 1970-01-01 01:00:00.000000000 +0100
> +++ b/balloon.c 2022-08-19 11:11:11.591188729 +0200
> @@ -0,0 +1,127 @@
> +/* balloon.c - Balloon password-hashing algorithm
> +
Would be good with a comment providing a link to authoritative
specification. Here, and/or in the header file. (When design is
finished, it also needs a section in the reference manual).
> +static inline void
> +hash(const struct nettle_hash *alg,
> + uint64_t cnt,
> + size_t a_len, const uint8_t *a,
> + size_t b_len, const uint8_t *b,
> + uint8_t *result)
Is there a good reason to declare this and other utility function
inline? May be better to leave that to the compiler, depending on number
of call sites, size of generated code, etc.
> +{
> + uint8_t ctx[NETTLE_MAX_HASH_CONTEXT_SIZE];
I'm afraid this won't guarantee required alignment for the context
struct. If you have to allocate it, you need to use the
TMP_DECLARE_ALIGN / TMP_ALLOC_ALIGN macros.
That said, maybe you can avoid allocating it at all, and leave that to
the caller (more on that below).
> +static inline void
> +hash_ints(const struct nettle_hash *alg,
> + uint64_t i, uint64_t j, uint64_t k, uint8_t *result)
> +{
> + uint8_t ctx[NETTLE_MAX_HASH_CONTEXT_SIZE];
> + uint8_t tmp[sizeof(uint64_t)];
> +
> + alg->init(ctx);
> + LE_WRITE_UINT64(tmp, i);
> + alg->update(ctx, sizeof(tmp), tmp);
> + LE_WRITE_UINT64(tmp, j);
> + alg->update(ctx, sizeof(tmp), tmp);
> + LE_WRITE_UINT64(tmp, k);
> + alg->update(ctx, sizeof(tmp), tmp);
> + alg->digest(ctx, alg->digest_size, result);
> +}
It will reduce overhead a bit if you process all three numbers with a
single update call,
uint8_t tmp[24];
LE_WRITE_UINT64(tmp, i);
LE_WRITE_UINT64(tmp + 8, j);
LE_WRITE_UINT64(tmp + 16, k);
...update...
> +static inline unsigned
> +block_to_int(size_t length, const uint8_t *block, unsigned mod)
I think this function deserves a comment.
> +{
> + unsigned r = 0;
> +
> + for (int i = length - 1; i >= 0; --i) {
Minor nit, but in nettle, loops like this are usually written with an
unsigned loop variable, according to this pattern:
unsigned i;
for (i = length; i-- > 0; )
Also, code should stick to c89, so the loop variable must be declared
outside of the for statement.
> + r <<= 8;
What's the valid range for mod? Assuming unsigned is 32 bits, this could
overflow if mod is larger than 24 bits. Would be good to back up
limitations with asserts.
> + r += (block[i] & 0xFF);
The & 0xFF looks redundant, since block[i] is uint8_t.
> + r %= mod;
Division is a pretty expensive operation. If this loop is in any way
performance critical, there are ways to speed up this arithmetic.
> + }
> + return r;
> +}
> +
> +void
> +balloon(size_t passwd_len, const uint8_t *passwd,
> + size_t salt_len, const uint8_t *salt,
> + unsigned s_cost, unsigned t_cost,
> + const struct nettle_hash *alg,
> + uint8_t *buf, uint8_t *result)
> +{
For consistency, can you see if you can make the interface/function
signature closer to the pbkdf2 function (see pbkdf2.h)? It takes a
pointer to a mac context, already initialized by the caller (in this
case, it would instead be a hash context), function pointers for update
and digest, and the digest size.
Note that the digest function will always reset the context to its
initial state, and if I read the code correctly, you need only a single
context, if you reuse it.
What are reasonable values for the s_cost and t_cost? Will an unsigned
(usually 32 bits, but might be as small as 16 on some compilers for
embedded systems) always be large enough? Alternatives include uint32_t,
uint64_t (likely much overkill? But in principle, I guess one could run
ballon with a buffer size of a few dozen GB?) and size_t.
> +size_t
> +balloon_buf_size(const struct nettle_hash *alg, unsigned s_cost)
> +{
> + return s_cost * alg->digest_size;
> +}
I think one of the operands to the multiplication operator should be
cast to size_t, to avoid overflow when size_t is larger than an
unsigned.
I'd like to think a bit more on the name of this function. Maybe
ballon_scratch_size, or (to follow a somewhat silly convention used by
some gmp and nettle functions), balloon_itch.
> +void
> +test_main(void)
> +{
> + test_balloon(8, "hunter42", 11, "examplesalt", 1024, 3, &nettle_sha256,
> +
> SHEX("716043dff777b44aa7b88dcbab12c078abecfac9d289c5b5195967aa63440dfb"));
> + test_balloon(0, "", 4, "salt", 3, 3, &nettle_sha256,
> +
> SHEX("5f02f8206f9cd212485c6bdf85527b698956701ad0852106f94b94ee94577378"));
> + test_balloon(8, "password", 0, "", 3, 3, &nettle_sha256,
> +
> SHEX("20aa99d7fe3f4df4bd98c655c5480ec98b143107a331fd491deda885c4d6a6cc"));
> + test_balloon(1, "", 1, "", 3, 3, &nettle_sha256,
> +
> SHEX("4fc7e302ffa29ae0eac31166cee7a552d1d71135f4e0da66486fb68a749b73a4"));
> + test_balloon(8, "password", 4, "salt", 1, 1, &nettle_sha256,
> +
> SHEX("eefda4a8a75b461fa389c1dcfaf3e9dfacbc26f81f22e6f280d15cc18c417545"));
> +}
It would be good to document the source of these test vectors.
Regards,
/Niels
--
Niels Möller. PGP key CB4962D070D77D7FCB8BA36271D8F1FF368C6677.
Internet email is subject to wholesale government surveillance.
_______________________________________________
nettle-bugs mailing list -- nettle-bugs@lists.lysator.liu.se
To unsubscribe send an email to nettle-bugs-le...@lists.lysator.liu.se
