Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code fixes from audit #105

Merged
merged 3 commits into from
Nov 26, 2019
Merged

Code fixes from audit #105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
684aa26
Use sizeof(level_t) instead of sizeof(int) in apdu_reset
matthewbauer Nov 12, 2019
675f4e8
Use explicit_bzero in place of memset
matthewbauer Nov 12, 2019
74e9ed2
Guard against buffer underflow in b58enc
matthewbauer Nov 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/apdu_baking.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ static bool reset_ok(void);
size_t handle_apdu_reset(__attribute__((unused)) uint8_t instruction) {
uint8_t *dataBuffer = G_io_apdu_buffer + OFFSET_CDATA;
uint32_t dataLength = G_io_apdu_buffer[OFFSET_LC];
if (dataLength != sizeof(int)) {
if (dataLength != sizeof(level_t)) {
THROW(EXC_WRONG_LENGTH_FOR_INS);
}
level_t const lvl = READ_UNALIGNED_BIG_ENDIAN(level_t, dataBuffer);
Expand Down
2 changes: 1 addition & 1 deletion src/base58.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ bool b58enc(/* out */ char *b58, /* in/out */ size_t *b58sz, const void *data, s

for (i = zcount, high = size - 1; i < binsz; ++i, high = j)
{
for (carry = bin[i], j = size - 1; (j > high) || carry; --j)
for (carry = bin[i], j = size - 1; ((int)j >= 0) && ((j > high) || carry); --j)
{
carry += 256 * buf[j];
buf[j] = carry % 58;
Expand Down
28 changes: 17 additions & 11 deletions src/keys.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,23 @@ key_pair_t *generate_key_pair_return_global(
priv->private_key_data, NULL);
}

cx_ecfp_init_private_key(cx_curve, priv->private_key_data, sizeof(priv->private_key_data), &priv->res.private_key);
cx_ecfp_generate_pair(cx_curve, &priv->res.public_key, &priv->res.private_key, 1);

if (cx_curve == CX_CURVE_Ed25519) {
cx_edward_compress_point(
CX_CURVE_Ed25519,
priv->res.public_key.W,
priv->res.public_key.W_len);
priv->res.public_key.W_len = 33;
BEGIN_TRY {
TRY {
cx_ecfp_init_private_key(cx_curve, priv->private_key_data, sizeof(priv->private_key_data), &priv->res.private_key);
cx_ecfp_generate_pair(cx_curve, &priv->res.public_key, &priv->res.private_key, 1);

if (cx_curve == CX_CURVE_Ed25519) {
cx_edward_compress_point(CX_CURVE_Ed25519,
priv->res.public_key.W,
priv->res.public_key.W_len);
priv->res.public_key.W_len = 33;
}
} FINALLY {
explicit_bzero(priv->private_key_data, sizeof(priv->private_key_data));
}
}
memset(priv->private_key_data, 0, sizeof(priv->private_key_data));
END_TRY;

return &priv->res;
}

Expand All @@ -85,7 +91,7 @@ cx_ecfp_public_key_t const *generate_public_key_return_global(
) {
check_null(bip32_path);
key_pair_t *const pair = generate_key_pair_return_global(curve, bip32_path);
memset(&pair->private_key, 0, sizeof(pair->private_key));
explicit_bzero(&pair->private_key, sizeof(pair->private_key));
return &pair->public_key;
}

Expand Down
2 changes: 1 addition & 1 deletion src/keys.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ static inline void generate_key_pair(
check_null(out);
key_pair_t *const result = generate_key_pair_return_global(derivation_type, bip32_path);
memcpy(out, result, sizeof(*out));
memset(result, 0, sizeof(*result));
explicit_bzero(result, sizeof(*result));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of other places that we use memset(..., 0, ...). Should we replace all of them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might not hurt to do that! Here I am just rewriting the ones that have some private key behind it. If the data's not sensitive, it's probably cheaper to just use memset, but maybe not something we should worry about.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've merged this as-is; I'll get a ticket in the backlog to investigate whether to replace the other uses of memset(..., 0, ...) with explicit_bzero.

}

// Non-reentrant
Expand Down