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.

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

Elliptic curve scalar multiplication with timing attack defenses #6009

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
7 participants
@bbbrumley
Copy link
Contributor

commented Apr 19, 2018

If this gets merged, there are several tiny follow ups that we can discuss. (Remove nonce padding throughout higher layers; deprecate ec2_mult.c or make it a no op; etc.)

Tagging @romen and @mattcaswell -- thanks!

Co-authored-by: Nicola Tuveri nic.tuv@gmail.com
Co-authored-by: Cesar Pereida Garcia cesar.pereidagarcia@tut.fi
Co-authored-by: Sohaib ul Hassan soh.19.hassan@gmail.com

Elliptic curve scalar multiplication with timing attack defenses
Co-authored-by: Nicola Tuveri <nic.tuv@gmail.com>
Co-authored-by: Cesar Pereida Garcia <cesar.pereidagarcia@tut.fi>
Co-authored-by: Sohaib ul Hassan <soh.19.hassan@gmail.com>
@mattcaswell
Copy link
Member

left a comment

Awesome work! Just a few minor comments.

Note that we'll need CLAs for all co-authors. Checking our records I see that we have one already for Nicola. We also have one for César but under a different email address (domain aalto.fi). I don't see one for Sohaib.

For César please can you either amend the commit message to list the email as per our records, or alternatively submit a new CLA with the new email address on it.

BN_CTX *new_ctx = NULL;

if (ctx == NULL)
if ((ctx = new_ctx = BN_CTX_secure_new()) == NULL)

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Apr 19, 2018

Member

Can we combine this into a single "if", e.g. if (ctx == NULL && (ctx = new_ctx...

* scalar should be in the range [0,n) otherwise all constant time bets are off.
*
* NB: This says nothing about EC_POINT_add and EC_POINT_dbl,
* which of course are not constant time themselves.

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Apr 19, 2018

Member

How worried do we need to be about this?

This comment has been minimized.

Copy link
@bbbrumley

bbbrumley Apr 19, 2018

Author Contributor

The NB comment? shrug I'm honestly not aware of any attacks that exploit that. To give an example, the ec2_mult.c ladder has been pretty water tight since 2011; some minor branching issues that were fixed yet never exploited (CVE-2014-0076). Anyway, the underlying arithmetic is no way constant time, and no attacks yet.

Honest assessment: no matter what EC_POINT_add and EC_POINT_dbl do, it's an order of magnitude better than the leaks in ec_wNAF_mul. (That ofc use the same functions, BTW.)

Having just said that, SCA is funny -- plugging one leak sometimes "amplifies" others, since variable times start moving to constant. Maybe you'll attract some new young talented researchers to take a look, instead of the same code path for the last 9 years.

I can have my team do the same empirical analysis that you already saw, but on this new code path. But it will take time.

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Apr 19, 2018

Member

I can have my team do the same empirical analysis that you already saw, but on this new code path. But it will take time.

It would be nice-to-have, but lets not let it slow down getting this PR merged.

(b)->Z_is_one ^= (t); \
} while(0)

for (i = order_bits - 1; i >= 0; i--) {

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Apr 19, 2018

Member

Could you add some more commentary here about how this algorithm is working? I find the pbit logic quite confusing.

const EC_POINT *point, BN_CTX *ctx)
{
int i, order_bits, group_top, kbit, pbit, Z_is_one, ret;
ret = 0;

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Apr 19, 2018

Member

Don't mix declarations and code. Either put this after the declarations block, or just initialise ret in the declaration.

*/
return ec_mul_consttime(group, r, scalars[0], points[0], ctx);
}

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Apr 19, 2018

Member

This block needs moving down until after the declarations below (don't mix declarations and code). This and the other instance of this I already commented on is the cause of the Travis red cross against this PR.

This comment has been minimized.

Copy link
@romen

romen Apr 19, 2018

Member

@mattcaswell This and the other code style comments should now be fixed via
3112bb5

@mattcaswell

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

I note this will cherry-pick to 1.1.0 (but not 1.0.2). @openssl/omc should we backport this to 1.1.0? I'm thinking "yes".

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

Once it's approved, yes to backport.

@cpereida

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

@mattcaswell I submitted a new CLA with new emails to the address provided in the CLA

@bbbrumley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

@mattcaswell What can I pass to ./config to turn off NISTZ? So I can run more extensive tests.

@mattcaswell

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

Try no-asm

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

close/open to kick the CLA bot.

@richsalz richsalz closed this Apr 19, 2018

@richsalz richsalz reopened this Apr 19, 2018

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

We updated our records with your new CLA, thanks!

@mattcaswell

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

close/open to kick the CLA bot.

In this case that won't do anything because the CLA bot isn't complaining! The bot only checks the principle author. In this case we have co-authors. :-)

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

habit.

@sohhas

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

@mattcaswell I have submitted the CLA.

* 1       0         Swap
* 1       1         No Swap
*
* This is XOR. pbit tracks the previous bit of k.

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Apr 19, 2018

Member

Superb comment!

@mattcaswell
Copy link
Member

left a comment

Approved subject to the CLAs being sorted - @sohhas I haven't seen yours come through yet - but I'll keep an eye out for it.

Needs a second review now.

@richsalz
Copy link
Contributor

left a comment

I don't see the fixes (3112bb5) but will assume it's all set. Nice!

BN_CTX *new_ctx = NULL;
int ret = 0;

if (ctx == NULL && (ctx = new_ctx = BN_CTX_secure_new()) == NULL)

This comment has been minimized.

Copy link
@romen

romen Apr 19, 2018

Member

@mattcaswell can I ask your feedback on BN_CTX_secure_now() ?

I though it would make sense to default to it in case one is not passed, as the BNs allocated on it are going to contain material directly related to the secret scalar: that being said, I did a quick ack through the existing code, and it looks like no other implementation is using it currently.

Is there some downside or caveat I am unaware of? Should we use a regular BN_CTX_new() instead?

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Apr 19, 2018

Member

This seems ok to me. Probably it is not widely used because it is a very new API. Although I do wonder whether @richsalz has any view on this?

Your question does make me wonder though whether we should be calling BN_clear() on any of the intermediate temporary variables.

This comment has been minimized.

Copy link
@richsalz

richsalz Apr 19, 2018

Contributor

Next release I would like us to move to all-secure and all-constant-time. For now, I am fine with just this change.

This comment has been minimized.

Copy link
@romen

romen Apr 20, 2018

Member

Thanks @mattcaswell @richsalz for the comments!

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Apr 20, 2018

Member

So, @romen, do you think we need to call BN_clear() on the intermediate temporary vars?

This comment has been minimized.

Copy link
@romen

romen Apr 20, 2018

Member

I would say it would be overkill and unnecessary as I don't think the threat model is considering malicious access to memory locations of running functions, what we need to avoid is leaving secret values in the heap after we release control of that memory area.

We already preallocate words for those BN so that they don't need to be resized during computation, and shouldn't be automatically reduced during execution, so clearing them at the end when the CTX is released should be fine.

@bbbrumley do you agree?
@mattcaswell what are your thoughts on it?

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Apr 20, 2018

Member

Ah! Right I missed the fact that BN_CTX_free() calls BN_clear_free() on those vars.

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

Sohab's CLA is now on file.

@mattcaswell mattcaswell added the ready label Apr 19, 2018

@mattcaswell

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

I will merge this tomorrow barring any last minute comments from others.


ret = 1;

err:

This comment has been minimized.

Copy link
@dot-asm

dot-asm Apr 20, 2018

Contributor

Styling nit. Such labels are customarily intended with space.

This comment has been minimized.

Copy link
@romen

romen Apr 20, 2018

Member

Fixed in 985f307

* 0       0         No Swap
* 0       1         Swap
* 1       0         Swap
* 1       1         No Swap

This comment has been minimized.

Copy link
@dot-asm

dot-asm Apr 20, 2018

Contributor

Something fishy here with tabs formatting. I mean there are some utf-8 chars lurking here. Strangely enough there were reports that some compilers are allergic to that, yes, in comments...

This comment has been minimized.

Copy link
@romen

romen Apr 20, 2018

Member

Fixed in 985f307

* The way a->d is allocated etc.
* BN_FLG_MALLOCED, BN_FLG_STATIC_DATA, ...
*/
t = (a->flags ^ b->flags) & condition & BN_FLG_CONSTTIME;

This comment has been minimized.

Copy link
@dot-asm

dot-asm Apr 20, 2018

Contributor

I'd say that it would be worth explicitly say saying in commentary that goal it to swap given flag/bit field. I mean it reads "cannot just arbitrarily swap flags ...", and suggestion is "cannot swap all the flags, ..., only single one, BN_FLG_CONSTTIME."

On side note I also wonder if putting it as ((a->flags ^ b->flags) & BN_FLG_CONSTTIME) & condition would make code more readable. I mean if alternative grouping and explicit parenthesis would illuminate focus on specific flag better... [Side note means that no action is actually expected.]

This comment has been minimized.

Copy link
@bbbrumley

bbbrumley Apr 20, 2018

Author Contributor

Sure we can modify that comment. It started off as more of a call to core OpenSSL developers -- what flags should we actually be swapping?

Also, defect in BN_swap -- it does not swap BN_FLG_CONSTTIME. (Edit: in fact it throws it away in both.)

This comment has been minimized.

Copy link
@dot-asm

dot-asm Apr 20, 2018

Contributor

Ah! Yeah, that's real danger with looking at suggested changes alone. I mean you risk loosing context. As for flags. Idea behind BN_FLG_STATIC_DATA is actually to indicate that data may not be written to. Intention is actually to treat it as it's read-only data, and some (if not most) of it does reside in read-only segment. In other words observation of BN_FLG_STATIC_DATA in BN_consttime_swap should be treated as fatal condition. It would either cause SEGV or effectively cause data corruption. BN_FLG_MALLOCED refers to BN structure itself, and hence must be preserved. Remaining flags are BN_FLG_CONSTIME and BN_FLG_SECURE. Latter must be preserved, because it determines how x->d was allocated and hence how to free it. This leaves BN_FLG_CONSTTIME that one can do something about. To summarize it's sufficient to mask and swap BN_FLG_CONSTTIME alone, i.e. code is correct. Modulo fact that BN_FLG_STATIC_DATA should be treated as fatal...

This comment has been minimized.

Copy link
@dot-asm

dot-asm Apr 20, 2018

Contributor

Modulo fact that BN_FLG_STATIC_DATA should be treated as fatal...

Hmmm... BN_consttime_swap opens with bn_wcheck_size, which actually just a soft assert(*). So that if one of inputs is shorter than intended, it would also cause corruption. In other words this subroutine does make quite strong assumptions about inputs, and it's appropriate to say that absence of BN_FLG_STATIC_DATA is one of such assumptions.

(*) And it's double-conditional, i.e. soft asserts by themselves are conditional on -DNDEBUG, and bn_wcheck_size itself is conditional on BN_DEBUG. One can wonder if BN_consttime_swap deserves a little bit bigger overhaul that would remove this ambiguity, as well as assert for BN_FLG_STATIC_DATA [and remove that switch, it's really no point, loop would do just fine.]

This comment has been minimized.

Copy link
@bbbrumley

bbbrumley Apr 20, 2018

Author Contributor

+1 we noticed the asserts and no return values. But it feels like a separate PR

@mattcaswell

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

Ping @dot-asm. Are you ok with this now?

goto err;

if ((group->order == NULL) || (group->field == NULL))
goto err;

This comment has been minimized.

Copy link
@dot-asm

dot-asm Apr 23, 2018

Contributor

Every time I see such construct I struggle with question "why". Can there be an EC_GROUP instance without field and order? Or is NULL possible only as result of memory corruption? Trouble is that if so, then wouldn't it be as appropriate to proceed without check and crash? Thing is that masking would take you further from root cause and only complicate trouble-shooting. But even trouble-shooting aside, if there is corruption you are genuinely interested to take as little as possible security-critical decision, ideally none, or in other words crash as soon as possible.

As for possibility of EC_GROUP without field and order. Even in extreme case when EC_GROUP is instantiated from user-supplied ASN.1 structure it, EC_GROUP_new_from_ecparameters, refuses to create EC_GROUP without field and order (and of course bunch of other fields).

To summarize, suggestion is to remove this condition. As well as check for group->generator below. As for other goto err-s. Question is whether or not it would be appropriate to set some ECerrs... I mean when application fails what would be user's chances to figure out the cause... Yes, most of callees set their own errors, but question is if they would be too generic to trouble-shoot...

levitte pushed a commit that referenced this pull request Apr 23, 2018

Elliptic curve scalar multiplication with timing attack defenses
Co-authored-by: Nicola Tuveri <nic.tuv@gmail.com>
Co-authored-by: Cesar Pereida Garcia <cesar.pereidagarcia@tut.fi>
Co-authored-by: Sohaib ul Hassan <soh.19.hassan@gmail.com>

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6009)

(cherry picked from commit 40e48e5)

levitte pushed a commit that referenced this pull request Apr 23, 2018

Address code style comments
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6009)

(cherry picked from commit 36bed23)

levitte pushed a commit that referenced this pull request Apr 23, 2018

ladder description: why it works
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6009)

(cherry picked from commit a067a87)

levitte pushed a commit that referenced this pull request Apr 23, 2018

Pass through
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6009)

(cherry picked from commit f467537)

levitte pushed a commit that referenced this pull request Apr 23, 2018

Move up check for EC_R_INCOMPATIBLE_OBJECTS and for the point at infi…
…nity case

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6009)

(cherry picked from commit 736b31e)

levitte pushed a commit that referenced this pull request Apr 23, 2018

Remove superfluous NULL checks. Add Andy's BN_FLG comment.
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6009)

(cherry picked from commit 39df515)
@mattcaswell

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

Pushed to master and 1.1.0. Thanks all!

levitte pushed a commit that referenced this pull request Apr 23, 2018

Elliptic curve scalar multiplication with timing attack defenses
Co-authored-by: Nicola Tuveri <nic.tuv@gmail.com>
Co-authored-by: Cesar Pereida Garcia <cesar.pereidagarcia@tut.fi>
Co-authored-by: Sohaib ul Hassan <soh.19.hassan@gmail.com>

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6009)

levitte pushed a commit that referenced this pull request Apr 23, 2018

Address code style comments
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6009)

levitte pushed a commit that referenced this pull request Apr 23, 2018

ladder description: why it works
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6009)

levitte pushed a commit that referenced this pull request Apr 23, 2018

Pass through
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6009)

levitte pushed a commit that referenced this pull request Apr 23, 2018

Move up check for EC_R_INCOMPATIBLE_OBJECTS and for the point at infi…
…nity case

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6009)

levitte pushed a commit that referenced this pull request Apr 23, 2018

Remove superfluous NULL checks. Add Andy's BN_FLG comment.
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6009)

@bbbrumley bbbrumley referenced this pull request Apr 24, 2018

Closed

ECC cleanup #6070

romen added a commit to romen/openssl that referenced this pull request Jul 10, 2018

EC point multiplication: add `ladder` scaffold
for specialized Montgomery ladder implementations

PR openssl#6009 and openssl#6070 replaced the default EC point multiplication path for
prime and binary curves with a unified Montgomery ladder implementation
with various timing attack defenses (for the common paths when a secret
scalar is feed to the point multiplication).
The newly introduced default implementation directly used
EC_POINT_add/dbl in the main loop.

The scaffolding introduced by this commit allows EC_METHODs to define a
specialized `ladder_step` function to improve performances by taking
advantage of efficient formulas for differential addition-and-doubling
and different coordinate systems.

- `ladder_pre` is executed before the main loop of the ladder: by
  default it copies the input point P into S, and doubles it into R.
  Specialized implementations could, e.g., use this hook to transition
  to different coordinate systems before copying and doubling;
- `ladder_step` is the core of the Montgomery ladder loop: by default it
  computes `S := R+S; R := 2R;`, but specific implementations could,
  e.g., implement a more efficient formula for differential
  addition-and-doubling;
- `ladder_post` is executed after the Montgomery ladder loop: by default
  it's a noop, but specialized implementations could, e.g., use this
  hook to transition back from the coordinate system used for optimizing
  the differential addition-and-doubling or recover the y coordinate of
  the result point.

Co-authored-by: Sohaib ul Hassan <soh.19.hassan@gmail.com>
Co-authored-by: Billy Brumley <bbrumley@gmail.com>

@romen romen referenced this pull request Jul 10, 2018

Closed

EC2m LD ladder #6690

7 of 7 tasks complete

romen added a commit to romen/openssl that referenced this pull request Jul 12, 2018

EC point multiplication: add `ladder` scaffold
for specialized Montgomery ladder implementations

PR openssl#6009 and openssl#6070 replaced the default EC point multiplication path for
prime and binary curves with a unified Montgomery ladder implementation
with various timing attack defenses (for the common paths when a secret
scalar is feed to the point multiplication).
The newly introduced default implementation directly used
EC_POINT_add/dbl in the main loop.

The scaffolding introduced by this commit allows EC_METHODs to define a
specialized `ladder_step` function to improve performances by taking
advantage of efficient formulas for differential addition-and-doubling
and different coordinate systems.

- `ladder_pre` is executed before the main loop of the ladder: by
  default it copies the input point P into S, and doubles it into R.
  Specialized implementations could, e.g., use this hook to transition
  to different coordinate systems before copying and doubling;
- `ladder_step` is the core of the Montgomery ladder loop: by default it
  computes `S := R+S; R := 2R;`, but specific implementations could,
  e.g., implement a more efficient formula for differential
  addition-and-doubling;
- `ladder_post` is executed after the Montgomery ladder loop: by default
  it's a noop, but specialized implementations could, e.g., use this
  hook to transition back from the coordinate system used for optimizing
  the differential addition-and-doubling or recover the y coordinate of
  the result point.

Co-authored-by: Sohaib ul Hassan <soh.19.hassan@gmail.com>
Co-authored-by: Billy Brumley <bbrumley@gmail.com>

romen added a commit to romen/openssl that referenced this pull request Jul 13, 2018

EC point multiplication: add `ladder` scaffold
for specialized Montgomery ladder implementations

PR openssl#6009 and openssl#6070 replaced the default EC point multiplication path for
prime and binary curves with a unified Montgomery ladder implementation
with various timing attack defenses (for the common paths when a secret
scalar is feed to the point multiplication).
The newly introduced default implementation directly used
EC_POINT_add/dbl in the main loop.

The scaffolding introduced by this commit allows EC_METHODs to define a
specialized `ladder_step` function to improve performances by taking
advantage of efficient formulas for differential addition-and-doubling
and different coordinate systems.

- `ladder_pre` is executed before the main loop of the ladder: by
  default it copies the input point P into S, and doubles it into R.
  Specialized implementations could, e.g., use this hook to transition
  to different coordinate systems before copying and doubling;
- `ladder_step` is the core of the Montgomery ladder loop: by default it
  computes `S := R+S; R := 2R;`, but specific implementations could,
  e.g., implement a more efficient formula for differential
  addition-and-doubling;
- `ladder_post` is executed after the Montgomery ladder loop: by default
  it's a noop, but specialized implementations could, e.g., use this
  hook to transition back from the coordinate system used for optimizing
  the differential addition-and-doubling or recover the y coordinate of
  the result point.

Co-authored-by: Sohaib ul Hassan <soh.19.hassan@gmail.com>
Co-authored-by: Billy Brumley <bbrumley@gmail.com>

romen added a commit to romen/openssl that referenced this pull request Jul 13, 2018

EC point multiplication: add `ladder` scaffold
for specialized Montgomery ladder implementations

PR openssl#6009 and openssl#6070 replaced the default EC point multiplication path for
prime and binary curves with a unified Montgomery ladder implementation
with various timing attack defenses (for the common paths when a secret
scalar is feed to the point multiplication).
The newly introduced default implementation directly used
EC_POINT_add/dbl in the main loop.

The scaffolding introduced by this commit allows EC_METHODs to define a
specialized `ladder_step` function to improve performances by taking
advantage of efficient formulas for differential addition-and-doubling
and different coordinate systems.

- `ladder_pre` is executed before the main loop of the ladder: by
  default it copies the input point P into S, and doubles it into R.
  Specialized implementations could, e.g., use this hook to transition
  to different coordinate systems before copying and doubling;
- `ladder_step` is the core of the Montgomery ladder loop: by default it
  computes `S := R+S; R := 2R;`, but specific implementations could,
  e.g., implement a more efficient formula for differential
  addition-and-doubling;
- `ladder_post` is executed after the Montgomery ladder loop: by default
  it's a noop, but specialized implementations could, e.g., use this
  hook to transition back from the coordinate system used for optimizing
  the differential addition-and-doubling or recover the y coordinate of
  the result point.

This commit also renames `ec_mul_consttime` to `ec_scalar_mul_ladder`,
as it better corresponds to what this function does: nothing can be
truly said about the constant-timeness of the overall execution of this
function, given that the underlying operations are not necessarily
constant-time themselves.
What this implementation ensures is that the same fixed sequence of
operations is executed for each scalar multiplication (for a given
EC_GROUP), with no dependency on the value of the input scalar.

Co-authored-by: Sohaib ul Hassan <soh.19.hassan@gmail.com>
Co-authored-by: Billy Brumley <bbrumley@gmail.com>

romen added a commit to romen/openssl that referenced this pull request Jul 13, 2018

EC point multiplication: add `ladder` scaffold
for specialized Montgomery ladder implementations

PR openssl#6009 and openssl#6070 replaced the default EC point multiplication path for
prime and binary curves with a unified Montgomery ladder implementation
with various timing attack defenses (for the common paths when a secret
scalar is feed to the point multiplication).
The newly introduced default implementation directly used
EC_POINT_add/dbl in the main loop.

The scaffolding introduced by this commit allows EC_METHODs to define a
specialized `ladder_step` function to improve performances by taking
advantage of efficient formulas for differential addition-and-doubling
and different coordinate systems.

- `ladder_pre` is executed before the main loop of the ladder: by
  default it copies the input point P into S, and doubles it into R.
  Specialized implementations could, e.g., use this hook to transition
  to different coordinate systems before copying and doubling;
- `ladder_step` is the core of the Montgomery ladder loop: by default it
  computes `S := R+S; R := 2R;`, but specific implementations could,
  e.g., implement a more efficient formula for differential
  addition-and-doubling;
- `ladder_post` is executed after the Montgomery ladder loop: by default
  it's a noop, but specialized implementations could, e.g., use this
  hook to transition back from the coordinate system used for optimizing
  the differential addition-and-doubling or recover the y coordinate of
  the result point.

This commit also renames `ec_mul_consttime` to `ec_scalar_mul_ladder`,
as it better corresponds to what this function does: nothing can be
truly said about the constant-timeness of the overall execution of this
function, given that the underlying operations are not necessarily
constant-time themselves.
What this implementation ensures is that the same fixed sequence of
operations is executed for each scalar multiplication (for a given
EC_GROUP), with no dependency on the value of the input scalar.

Co-authored-by: Sohaib ul Hassan <soh.19.hassan@gmail.com>
Co-authored-by: Billy Brumley <bbrumley@gmail.com>

levitte pushed a commit that referenced this pull request Jul 16, 2018

EC point multiplication: add `ladder` scaffold
for specialized Montgomery ladder implementations

PR #6009 and #6070 replaced the default EC point multiplication path for
prime and binary curves with a unified Montgomery ladder implementation
with various timing attack defenses (for the common paths when a secret
scalar is feed to the point multiplication).
The newly introduced default implementation directly used
EC_POINT_add/dbl in the main loop.

The scaffolding introduced by this commit allows EC_METHODs to define a
specialized `ladder_step` function to improve performances by taking
advantage of efficient formulas for differential addition-and-doubling
and different coordinate systems.

- `ladder_pre` is executed before the main loop of the ladder: by
  default it copies the input point P into S, and doubles it into R.
  Specialized implementations could, e.g., use this hook to transition
  to different coordinate systems before copying and doubling;
- `ladder_step` is the core of the Montgomery ladder loop: by default it
  computes `S := R+S; R := 2R;`, but specific implementations could,
  e.g., implement a more efficient formula for differential
  addition-and-doubling;
- `ladder_post` is executed after the Montgomery ladder loop: by default
  it's a noop, but specialized implementations could, e.g., use this
  hook to transition back from the coordinate system used for optimizing
  the differential addition-and-doubling or recover the y coordinate of
  the result point.

This commit also renames `ec_mul_consttime` to `ec_scalar_mul_ladder`,
as it better corresponds to what this function does: nothing can be
truly said about the constant-timeness of the overall execution of this
function, given that the underlying operations are not necessarily
constant-time themselves.
What this implementation ensures is that the same fixed sequence of
operations is executed for each scalar multiplication (for a given
EC_GROUP), with no dependency on the value of the input scalar.

Co-authored-by: Sohaib ul Hassan <soh.19.hassan@gmail.com>
Co-authored-by: Billy Brumley <bbrumley@gmail.com>

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #6690)

@romen romen referenced this pull request Jul 24, 2018

Closed

EC GFp ladder #6772

1 of 1 task complete

bbbrumley added a commit to bbbrumley/openssl that referenced this pull request Nov 11, 2018

CVE-2018-5407 fix for 1.1.0h
Co-authored-by: Nicola Tuveri <nic.tuv@gmail.com>
Co-authored-by: Cesar Pereida Garcia <cesar.pereidagarcia@tut.fi>
Co-authored-by: Sohaib ul Hassan <soh.19.hassan@gmail.com>

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#6009)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.