[crypto/ec] for ECC parameters with NULL or zero cofactor, compute it (1.1.0) #9795

bbbrumley · 2019-09-06T17:26:06Z

Backport of #9727 to 1.1.0. The test there isn't at all compatible with 1.1.0. So to test this, I did the following:

hack test

Ignored the passed cofactor, always computed it, then added an assert:

diff --git a/crypto/ec/ec_lib.c b/crypto/ec/ec_lib.c
index eaf44ccef9..af55de37d1 100644
--- a/crypto/ec/ec_lib.c
+++ b/crypto/ec/ec_lib.c
@@ -366,7 +366,7 @@ int EC_GROUP_set_generator(EC_GROUP *group, const EC_POINT *generator,
         return 0;
 
     /* Either take the provided positive cofactor, or try to compute it */
-    if (cofactor != NULL && !BN_is_zero(cofactor)) {
+    if (0 && cofactor != NULL && !BN_is_zero(cofactor)) {
         if (!BN_copy(group->cofactor, cofactor))
             return 0;
     } else if (!ec_guess_cofactor(group)) {
@@ -374,6 +374,9 @@ int EC_GROUP_set_generator(EC_GROUP *group, const EC_POINT *generator,
         return 0;
     }
 
+#include <assert.h>
+    assert(BN_cmp(group->cofactor, cofactor) == 0);
+
     /*
      * Some groups have an order with
      * factors of two, which makes the Montgomery setup fail.

Then make test went swell.

EVP test

I added two explicit parameter PEMs with no cofactor to the EVP tests. The two key pairs are from RFC 6979 for P-256 and P-384. Unfortunately my team's tooling only currently supports NIST curves over prime fields.

gdb

I checked the codepath in gdb to make sure it's taking the proper early exit to the ladder.

The cofactor argument to EC_GROUP_set_generator is optional, and SCA mitigations for ECC currently use it. So the library currently falls back to very old SCA-vulnerable code if the cofactor is not present. This PR allows EC_GROUP_set_generator to compute the cofactor for all curves of cryptographic interest. Steering scalar multiplication to more SCA-robust code. This issue affects persisted private keys in explicit parameter form, where the (optional) cofactor field is zero or absent. It also affects curves not built-in to the library, but constructed programatically with explicit parameters, then calling EC_GROUP_set_generator with a nonsensical value (NULL, zero). The very old scalar multiplication code is known to be vulnerable to local uarch attacks, outside of the OpenSSL threat model. New results suggest the code path is also vulnerable to traditional wall clock timing attacks. CVE-2019-1547

romen

Approved, assumed the CHANGES entry is moved at the top if its section.

CHANGES

…compute it

mattcaswell

Approved assuming the CHANGES nit is fixed.

CHANGES

include/openssl/ec.h

…compute it Add CVE to CHANGES entry

…pute it The numeric error code for `EC_R_UNKNOWN_COFACTOR` should be `164` to maintain ABI compatibility between 1.1.0 and 1.1.1

romen · 2019-09-07T09:39:34Z

I know @bbbrumley is busy until Sunday night, and I am authorized to apply changes to his PR for this level of triviality, so in the interest of expediting the review process in view of the upcoming release day, I took the liberty of pushing the required changes.

romen · 2019-09-07T09:41:39Z

I have also personally confirmed the manual "hack testing" done by @bbbrumley

I will now proceed to merge

The cofactor argument to EC_GROUP_set_generator is optional, and SCA mitigations for ECC currently use it. So the library currently falls back to very old SCA-vulnerable code if the cofactor is not present. This PR allows EC_GROUP_set_generator to compute the cofactor for all curves of cryptographic interest. Steering scalar multiplication to more SCA-robust code. This issue affects persisted private keys in explicit parameter form, where the (optional) cofactor field is zero or absent. It also affects curves not built-in to the library, but constructed programatically with explicit parameters, then calling EC_GROUP_set_generator with a nonsensical value (NULL, zero). The very old scalar multiplication code is known to be vulnerable to local uarch attacks, outside of the OpenSSL threat model. New results suggest the code path is also vulnerable to traditional wall clock timing attacks. CVE-2019-1547 Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #9795)

Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #9795)

romen · 2019-09-07T09:51:29Z

Merged to 1.1.0-stable with:

c31be97 [test/recipes/30-test_evp_data] computing ECC cofactors: regression test
7c1709c [crypto/ec] for ECC parameters with NULL or zero cofactor, compute it

richsalz · 2019-09-09T19:23:30Z

Why does the CHANGES addition have to go at the top? Is chronology within a release important? After all, many more CHANGES entries are going to appear before the official release; does it matter if an entry is the 32nd or the 45th?

mspncp · 2019-09-09T21:29:52Z

Why does the CHANGES addition have to go at the top?

I don't understand what disturbs you; isn't it common practice to add new CHANGES entries at the top?

richsalz · 2019-09-09T22:38:39Z

Every time I rebase, I have to fix a merge conflict to put my entry back on the top because there's a new top entry and the context-diff gets confused. If I just let it slide down, then future CHANGES rebasing will not cause a problem.

mspncp · 2019-09-09T23:22:07Z

Oh, now I understand your point. Nice trick. Still I prefer that new entries appear at the top in the order in which the pull requests were merged.

Also, your nice trick has a little flaw: letting your entry sink down when rebasing only guarantuees a conflict-free experience if nobody else uses the same strategy. Because otherwise new entries on master will also appear at lower places and conflict with your entry.

So an improved strategy would be: while rebasing a pull request on master, let the entry sink down, but when the pull request gets finally merged, the entry should be moved back to the top. Unfortunately, only committers could do this last step.

mspncp · 2019-09-09T23:25:07Z

(I wrote 'master', but of course the same reasoning applies to the stable branches)

richsalz · 2019-09-10T02:20:23Z

So your saying it only works for me? That's fine :)

mspncp · 2019-09-10T04:20:53Z

Sorry, no. It works only until you tell other people. Which you just did. 😆

bbbrumley added 3 commits September 6, 2019 19:34

[test/recipes/30-test_evp_data] computing ECC cofactors: regression test

990ab2e

CHANGES entry: for ECC parameters with NULL or zero cofactor, compute it

d7ea8b7

romen added the 1.1.0 label Sep 7, 2019

romen approved these changes Sep 7, 2019

View reviewed changes

CHANGES Outdated Show resolved Hide resolved

romen self-assigned this Sep 7, 2019

romen requested a review from mattcaswell September 7, 2019 01:15

romen added the approval: review pending This pull request needs review by a committer label Sep 7, 2019

fixup! CHANGES entry: for ECC parameters with NULL or zero cofactor, …

2f80845

…compute it

mattcaswell approved these changes Sep 7, 2019

View reviewed changes

CHANGES Show resolved Hide resolved

mattcaswell requested changes Sep 7, 2019

View reviewed changes

include/openssl/ec.h Outdated Show resolved Hide resolved

romen mentioned this pull request Sep 7, 2019

[crypto/ec] for ECC parameters with NULL or zero cofactor, compute it #9727

Closed

romen added 2 commits September 7, 2019 12:29

fixup! CHANGES entry: for ECC parameters with NULL or zero cofactor, …

dbbdea6

…compute it Add CVE to CHANGES entry

fixup! [crypto/ec] for ECC parameters with NULL or zero cofactor, com…

9b29942

…pute it The numeric error code for `EC_R_UNKNOWN_COFACTOR` should be `164` to maintain ABI compatibility between 1.1.0 and 1.1.1

mattcaswell approved these changes Sep 7, 2019

View reviewed changes

mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 7, 2019

levitte pushed a commit that referenced this pull request Sep 7, 2019

[test/recipes/30-test_evp_data] computing ECC cofactors: regression test

c31be97

Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #9795)

romen closed this Sep 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[crypto/ec] for ECC parameters with NULL or zero cofactor, compute it (1.1.0) #9795

[crypto/ec] for ECC parameters with NULL or zero cofactor, compute it (1.1.0) #9795

[crypto/ec] for ECC parameters with NULL or zero cofactor, compute it (1.1.0) #9795

[crypto/ec] for ECC parameters with NULL or zero cofactor, compute it (1.1.0) #9795

Conversation

hack test

EVP test

gdb

Choose a reason for hiding this comment

Choose a reason for hiding this comment