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

Jump to bottom

[crypto/ec] blind coordinates in ec_wNAF_mul for robustness #11439

Closed
wants to merge 1 commit into from
+36 −20
Closed

[crypto/ec] blind coordinates in ec_wNAF_mul for robustness #11439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions crypto/ec/ec_mult.c
View file
Open in desktop
Expand Up @@ -746,6 +746,20 @@ int ec_wNAF_mul(const EC_GROUP *group, EC_POINT *r, const BIGNUM *scalar,
if (r_is_at_infinity) {
if (!EC_POINT_copy(r, val_sub[i][digit >> 1]))
goto err;

/*-
* Apply coordinate blinding for EC_POINT.
*
* The underlying EC_METHOD can optionally implement this function:
* ec_point_blind_coordinates() returns 0 in case of errors or 1 on
* success or if coordinate blinding is not implemented for this
* group.
*/
if (!ec_point_blind_coordinates(group, r, ctx)) {
Copy link
Member

Choose a reason for hiding this comment

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

I hope public keys do not need the blinding?
I remember I fixed a bug, where signature validation was failing because of failed
random number generator

Copy link
Member

Choose a reason for hiding this comment

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

see 3051bf2

Copy link
Member

Choose a reason for hiding this comment

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

So, what I mean is, multiplication by group order never secret,
so at least those do not need blinding.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, unless I am mistaken this will break my fix above, since the case where
the exponent is the group order will not use the montgomery ladder, but
will instead be blinded (and fail if the random source is not available)
please do at least add a similar precaution, that the exponent is not the group
order, it need not be a sophisticated compare, a simple pointer compare is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, what I mean is, multiplication by group order never secret,
so at least those do not need blinding.

Bug attacks aren't necessarily about secrets ;) They are about finding clever inputs that trigger bugs and do interesting things. Yes, one of these interesting things is recovering secrets. But others could be

  1. Fooling signature verification, i.e. creating a forgery using the bug
  2. Fooling key validation, like we did in our first real world bug attack (independent from us stealing keys), CVE-2011-4354
  3. Other stuff I'm not clever enough to think of.

I wonder if there is a better solution here than disabling coordinate randomization? Does switching from BN_priv_rand_range_ex to something less hangy help? These blinding values are not long term keys -- so while ideally they are generated similarly, in practice the requirements are much less strict.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least we already have a cool name for this function.

Yep, RAND_psuedo_bytes 😁

Copy link
Member

Choose a reason for hiding this comment

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

or RAND_paranoid_bytes 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think paranoid is a good choice for a function that mightn't be random.
OSSL_blinding_bytes avoiding the RAND connection.

Copy link
Member

Choose a reason for hiding this comment

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

I just mean, we are a bit paranoid, to assume a bug-attack and a random-generator
failure at the same time. But I think that is about the security level you need in
an atomic power plant.

Copy link
Member

Choose a reason for hiding this comment

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

And I hope I did not infect you :), oops.

ECerr(EC_F_EC_WNAF_MUL, EC_R_POINT_COORDINATES_BLIND_FAILURE);
goto err;
}

r_is_at_infinity = 0;
} else {
if (!EC_POINT_add
Expand Down
42 changes: 22 additions & 20 deletions crypto/ec/ecp_smpl.c
View file
Open in desktop
Expand Up @@ -1442,36 +1442,38 @@ int ec_GFp_simple_blind_coordinates(const EC_GROUP *group, EC_POINT *p,
temp = BN_CTX_get(ctx);
if (temp == NULL) {
ECerr(EC_F_EC_GFP_SIMPLE_BLIND_COORDINATES, ERR_R_MALLOC_FAILURE);
goto err;
goto end;
}

/* make sure lambda is not zero */
/*-
* Make sure lambda is not zero.
* If the RNG fails, we cannot blind but nevertheless want
* code to continue smoothly and not clobber the error stack.
*/
do {
if (!BN_priv_rand_range_ex(lambda, group->field, ctx)) {
ECerr(EC_F_EC_GFP_SIMPLE_BLIND_COORDINATES, ERR_R_BN_LIB);
goto err;
ERR_set_mark();
ret = BN_priv_rand_range_ex(lambda, group->field, ctx);
ERR_pop_to_mark();
if (ret == 0) {
ret = 1;
goto end;
}
} while (BN_is_zero(lambda));

/* if field_encode defined convert between representations */
if (group->meth->field_encode != NULL
&& !group->meth->field_encode(group, lambda, lambda, ctx))
goto err;
if (!group->meth->field_mul(group, p->Z, p->Z, lambda, ctx))
goto err;
if (!group->meth->field_sqr(group, temp, lambda, ctx))
goto err;
if (!group->meth->field_mul(group, p->X, p->X, temp, ctx))
goto err;
if (!group->meth->field_mul(group, temp, temp, lambda, ctx))
goto err;
if (!group->meth->field_mul(group, p->Y, p->Y, temp, ctx))
goto err;
p->Z_is_one = 0;
if ((group->meth->field_encode != NULL
&& !group->meth->field_encode(group, lambda, lambda, ctx))
|| !group->meth->field_mul(group, p->Z, p->Z, lambda, ctx)
bernd-edlinger marked this conversation as resolved.
Show resolved Hide resolved
|| !group->meth->field_sqr(group, temp, lambda, ctx)
|| !group->meth->field_mul(group, p->X, p->X, temp, ctx)
|| !group->meth->field_mul(group, temp, temp, lambda, ctx)
|| !group->meth->field_mul(group, p->Y, p->Y, temp, ctx))
goto end;

p->Z_is_one = 0;
ret = 1;

err:
end:
BN_CTX_end(ctx);
return ret;
}
Expand Down