Add support for small curves #223

tomato42 · 2020-12-08T09:31:35Z

finish fixing #197, fix #224
fix #24

because we deduplicate some fairly well-tested code, the overall percentage coverage of ecdsa.keys falls, making straight-up change fail instrumental testing; add some test coverage for it to counteract it

also adds some static test coverage for is_prime() to stop the line coverage fluctuating based on hypothesis choices

coveralls · 2020-12-08T09:55:59Z

Coverage decreased (-2.07%) to 96.516% when pulling 4bd1d1c on tomato42:small-curves into bbe3679 on warner:master.

tomato42 · 2020-12-08T10:09:16Z

CI run: https://travis-ci.org/github/warner/python-ecdsa/builds/748309556

the public key recovery needs to truncate large hashes just like the signature verification and creation, so reuse the code that does that

tomato42 · 2020-12-08T10:24:41Z

CI run: https://travis-ci.org/github/warner/python-ecdsa/builds/748311825

tomato42 · 2020-12-08T20:03:56Z

CI run: https://travis-ci.org/github/warner/python-ecdsa/builds/748412193

tomato42 · 2020-12-08T22:23:36Z

CI run: https://travis-ci.org/github/warner/python-ecdsa/builds/748433990

tomato42 · 2020-12-09T09:08:57Z

CI run: https://travis-ci.org/github/warner/python-ecdsa/builds/748513675

Jakuje · 2020-12-09T13:16:16Z

src/ecdsa/ecdsa.py

+_Gy = int(remove_whitespace("CF5AC839 5BAFEB13 C02DA292 DDED7A83"), 16)
+_r = int(remove_whitespace("FFFFFFFE 00000000 75A30D1B 9038A115"), 16)
+_h = 1
+curve_128r1 = ellipticcurve.CurveFp(_p, -3, _b, _h)


I do not see the -3 argument in neither of specifications I was able to find.

because it's -3 % p or _p - 3 in this code

Jakuje · 2020-12-09T13:17:51Z

src/ecdsa/test_pyecdsa.py

+    def test_to_openssl_secp128r1(self):
+        self.do_test_to_openssl(SECP128r1)
+
+    @pytest.mark.slow


nit: This change is probably not strictly related to the content of this commit. Same above.
It is actually removed in the following commit, which makes is a good candidate for removal already here.

ah, yeah, it's a leftover from when I cherry-picked it from the mutation testing PR, will fix

Jakuje · 2020-12-09T13:22:58Z

src/ecdsa/ecdsa.py

+# secp160r1
+_p = int(remove_whitespace("FFFFFFFF FFFFFFFF FFFFFFFF FFFFFFFF 7FFFFFFF"), 16)
+# S = 1053CDE4 2C14D696 E6768756 1517533B F3F83345
+# _a = -3


Same here -- the specifications mention a=0xffffffffffffffffffffffffffffffff7ffffffc. If there is a trivial reason in the ECC theory that I miss, it might make sense to add at least a comment as all the sources come with the above.

actually, the -3 curves are special: https://hyperelliptic.org/EFD/g1p/auto-shortw-jacobian-3.html we just don't use this property in the implementation (at least not now)

Jakuje · 2020-12-09T15:18:37Z

.travis.yml

      - python: 2.7
        env: INSTRUMENTAL=yes
+        dist: bionic
+        sudo: true


This does not do anything in current travis (see the "View config" -> "Config validation" in the travis build)

yes, "sudo: true" is supposed to not do anything, but it breaks when i don't use it... so I keep it

weird. Can you be more specific what breaks? I think I removed all these from OpenSC and all works fine.

I wasn't getting the distribution versions and thus python versions I expected from configuration, that being said I tried it some time ago...
Something to try when travis build queue isn't measured in hours...

the secret multiplier is limited by the order of the base point, that also informs the size of elements for the signature (as they are calculated modulo order), but the public point is a point, so its elements are modulo prime from the curve. The same thing applies to the shared secret: it's just one coordinate of the point, so it's modulo p of the curve, not modulo order of generator. for all curves up till now the size of order and size of the prime was the same so it worked fine, but secp160r1 is different, so it showed the bugs so fix this bug and add secp160r1 as the test coverage for it

if we don't ignore _compat for the new checkout, we will get a big decrease in code coverage

many tests (in particular ecdh) require new OpenSSL, so run it on a distro with new OpenSSL

make it easier to test instrumental coverage

looks like there is some test coverage variability from hypothesis so do few static examples

tomato42 · 2020-12-09T18:14:39Z

CI run: https://travis-ci.org/github/tlsfuzzer/python-ecdsa/builds/748606904

Jakuje

The CI looks broken, but otherwise the code looks good.

Jakuje · 2020-12-09T20:49:09Z

.travis.yml

      - python: 2.7
        env: INSTRUMENTAL=yes
+        dist: bionic
+        sudo: true


weird. Can you be more specific what breaks? I think I removed all these from OpenSC and all works fine.

tomato42 added feature functionality to be implemented bug unintended behaviour in ecdsa code labels Dec 8, 2020

tomato42 added this to the v0.17.0 milestone Dec 8, 2020

tomato42 self-assigned this Dec 8, 2020

tomato42 changed the title ~~Small curves~~ Add support for small curves Dec 8, 2020

tomato42 force-pushed the small-curves branch from 46bdd1e to 3704153 Compare December 8, 2020 10:07

tomato42 added 3 commits December 8, 2020 11:19

fix from_public_key_recovery() and sign() for large hashes

e0d1c05

the public key recovery needs to truncate large hashes just like the signature verification and creation, so reuse the code that does that

add secp112r1

36e2e92

add secp112r2

de82152

tomato42 force-pushed the small-curves branch from 3704153 to a81c7e3 Compare December 8, 2020 10:23

tomato42 force-pushed the small-curves branch from a81c7e3 to bb8254a Compare December 8, 2020 20:02

tomato42 mentioned this pull request Dec 8, 2020

Add mutation testing to CI #156

Merged

5 tasks

Jakuje reviewed Dec 9, 2020

View reviewed changes

tomato42 added 8 commits December 9, 2020 19:09

add secp128r1

e74f8e6

ignore _compat module both for old and new checkout

57b3f7b

if we don't ignore _compat for the new checkout, we will get a big decrease in code coverage

run instrumental with new OpenSSL

2879582

many tests (in particular ecdh) require new OpenSSL, so run it on a distro with new OpenSSL

add instrumental to tox config

3bda7d7

make it easier to test instrumental coverage

add some test coverage to ecdsa.keys

95aa83f

is_prime() test coverage

bae6ddc

looks like there is some test coverage variability from hypothesis so do few static examples

ignore environment-caused lack of branch coverage for inverse_mod()

4bd1d1c

tomato42 force-pushed the small-curves branch from 6767973 to 4bd1d1c Compare December 9, 2020 18:14

Jakuje approved these changes Dec 9, 2020

View reviewed changes

tomato42 merged commit 0e464f2 into tlsfuzzer:master Dec 10, 2020

tomato42 deleted the small-curves branch December 10, 2020 00:40

Add support for small curves #223

Add support for small curves #223

Uh oh!

Conversation

tomato42 commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomato42 commented Dec 8, 2020

Uh oh!

tomato42 commented Dec 8, 2020

Uh oh!

tomato42 commented Dec 8, 2020

Uh oh!

tomato42 commented Dec 8, 2020

Uh oh!

tomato42 commented Dec 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomato42 commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jakuje left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tomato42 commented Dec 8, 2020 •

edited

Loading

coveralls commented Dec 8, 2020 •

edited

Loading

tomato42 commented Dec 9, 2020 •

edited

Loading