Closed drbild closed 5 years ago
I really like this. Thank you for starting this off.
I've made a few suggestions, as commits that I've pushed to your branch.
@zanebeckwith Thanks for the fixes! I've squashed most of them into the prior commits.
I made two primary changes:
1) outlen
param to hash functions: keep as an input, to enable bounds checking when writing the hash output (since the hash functions take a plain char *
for out, not a struct xtt_crypto_hash...
).
This is more consistent with the prf
signature, too.
2) Removed "desired" from the docstring for outlen
arg in the prf
function. It is the size of the out
buffer (i.e., the max bytes to write). Of course, that is implicitly also the desired bytes.
@drbild Sorry for the delay, over the holiday break.
I pushed a commit just now addressing your first point above. This makes the hash function take a struct xtt_crypto_hmac
, and removes the outlen
parameter. All the uses of this hash function are in fact using an xtt_crypto_hmac
, so it makes sense for the underlying function to take that rather than a bare pointer. While this is different from the prf
function, it's consistent with the other crypto functions (e.g. aead and kx), and I think the prf
function should have a different signature (see my next point).
Regarding your second point, I actually think the prf
docstring should indicate that the outlen
parameter is not just the size of the buffer, but in fact sets the size of the output. To me, if I see that a parameter is for the size of an output buffer, I would think I could do something like:
unsigned char buf[1024];
prf(buf, sizeof(buf), msg ...)
In other words, I expect such a parameter to be for bounds-checking, but won't impact the actual logic.
This is just personal taste, and just for a comment, so I'm not passionate about it, but I did want to explain my thinking for including "desired" in that docstring.
I pushed a commit just now addressing your first point above. This makes the hash function take a
struct xtt_crypto_hmac
, and removes theoutlen
parameter.
Thanks! I'd thought the hash output was sometimes written directly into a message buffer, but apparently that's not the case. Using the struct is much better, since we can.
Regarding your second point, I actually think the
prf
docstring should indicate that theoutlen
parameter is not just the size of the buffer, but in fact sets the size of the output. In other words, I expect such a parameter to be for bounds-checking, but won't impact the actual logic.
I see. I think here it is for both specifying the output size and bound checking (the caller shouldn't request a size larger than the buffer).
I've updated the comment to indicate that the prf function will generate the requested amount of output, i.e., fill the provided buffer.
This is now good to merge in my view.
The
xtt
handshake context functions are fairly convoluted due to support for multiple suite specs. For example,This series contains two main refactoring to improve this: 1) It introduces a
suite_ops
struct to provide polymorphic access to the crypto primitives. This allows the mapping of suite spec to functions to be defined in one place, independent of the context.1) The repeated
union
definitions that allow static declaration of crypto data types at compile time (before the specific algorithm is known at runtime) are extracted to one file.There is significant amount of refactoring required before the
xtt
code is ready for external review or adding support to use the TPM for long-term keys. Rather than try to do it all at once (which will never happen), this PR takes the first step.Notice the reduced line count and complexity in
context.h
andcontext.c
. However, the changes tomessages.c
,crypto_utils.c
,message_utils.c
, key_derivation.c, and
signatures.c` are straightforward substitutions.