Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#16794 closed enhancement (implemented)

All cryptography unit test coverage should be over 95%; all should have test vectors

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: testing, 028-triage, tor-tests-coverage, tor-tests-unit, TorCoreTeam201605, review-group-1
Cc: isis Actual Points:
Parent ID: #16791 Points: 3
Reviewer: isis Sponsor: SponsorS-can

Description

It's high, but it's not there yet:

x/crypto.c.gcov 204 738 78.34
x/crypto_curve25519.c.gcov 8 74 90.24
x/crypto_ed25519.c.gcov 17 95 84.82
x/crypto_format.c.gcov 5 95 95.00
x/crypto_pwbox.c.gcov 2 59 96.72
x/crypto_s2k.c.gcov 14 152 91.57
x/aes.c.gcov 0 17 100.00
TOTAL 250 1230 83.11

Child Tickets

TicketTypeStatusOwnerSummary
#18956defectclosednickmTrivial memory leak when reading truncated ed25519 key files

Change History (32)

comment:1 Changed 4 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

comment:2 Changed 4 years ago by nickm

Keywords: 028-triage added

comment:3 Changed 4 years ago by nickm

Keywords: SponsorS removed
Sponsor: SponsorS

Bulk-replace SponsorS keyword with SponsorS sponsor field in Tor component.

comment:4 Changed 4 years ago by nickm

Points: medium
Priority: normalmajor

comment:5 Changed 4 years ago by isis

Cc: isis added

comment:6 Changed 4 years ago by nickm

Severity: Normal

Update:

cc/crypto.c.gcov 166 800 82.82
cc/crypto_ed25519.c.gcov 12 124 91.18
cc/crypto_curve25519.c.gcov 3 85 96.59
cc/crypto_pwbox.c.gcov 1 62 98.41
cc/crypto_s2k.c.gcov 1 131 99.24
cc/aes.c.gcov 0 20 100.00
cc/crypto_format.c.gcov 0 91 100.00

Only two under-tested modules remain.

comment:7 Changed 4 years ago by nickm

Status: newneeds_review

I've got some code removal and new tests in feature16794_more.

comment:8 Changed 4 years ago by nickm

Owner: set to nickm
Status: needs_reviewassigned

Merged that.

comment:9 Changed 4 years ago by nickm

Priority: HighMedium

comment:10 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

These seem like features, or like other stuff unlikely to be possible this month. Bumping them to 0.2.9

comment:11 Changed 4 years ago by isabela

Sponsor: SponsorSSponsorS-can

comment:12 Changed 4 years ago by nickm

Owner: nickm deleted

comment:13 Changed 3 years ago by nickm

Owner: set to nickm

comment:14 Changed 3 years ago by nickm

by my latest, we're down to one under-covered file, but it's the big one:

cov/crypto.c.gcov 195 935 82.74
cov/crypto_curve25519.c.gcov 3 85 96.59
cov/crypto_ed25519.c.gcov 5 148 96.73
cov/crypto_format.c.gcov 0 92 100.00
cov/crypto_pwbox.c.gcov 1 63 98.44
cov/crypto_s2k.c.gcov 1 136 99.27
cov/aes.c.gcov 0 19 100.00

Also, looking at test_crypto, I see official-looking test vectors for:

  • SHA1
  • SHA256
  • SHA3-{256,512}
  • SHAKE256
  • Curve25519
  • SipHash

And I see test vectors that we generated with independent code for:

  • AES
  • Base16
  • Base32
  • Base64
  • KDF-TAP
  • HKDF-SHA256
  • Ed25519

But not for:

  • SHA512
  • RSA
  • DH

comment:15 Changed 3 years ago by nickm

I did some trivial exercises here this morning, in branch crypto_unit_tests. Reviewable but not finished.

comment:16 Changed 3 years ago by nickm

Keywords: tor-tests-coverage tor-tests-unit added

comment:17 Changed 3 years ago by nickm

Keywords: TorCoreTeam201605 added

Give myself a few items for May. I hope I can do even more than this, but let's be careful.

comment:18 Changed 3 years ago by nickm

Okay. In my branch (rebased as crypto_unit_tests_v2) we now have official test vectors for everything but RSA, DH, and Base*. Oh, and KDF-TAP, but KDF-TAP is ours.

On to the coverage.

comment:19 Changed 3 years ago by nickm

Also in my crypto_unit_test_v2 branch: 100% coverage on everything but crypto.c.

comment:20 Changed 3 years ago by nickm

It's up to 94.3% overall on crypto*.c. Getting closer and closer...

comment:21 Changed 3 years ago by nickm

Status: assignedneeds_review

Now I see 95.32%.

comment:22 Changed 3 years ago by nickm

(I would be okay if somebody reviewed just the src/common parts of this.)

comment:23 Changed 3 years ago by isis

Reviewer: isis

comment:24 Changed 3 years ago by nickm

Keywords: review-group-1 added

comment:25 Changed 3 years ago by isis

When we say "coverage should be over 95%", do we mean line coverage, function coverage, or branch coverage? I currently see line coverage for src/common/crypto.c at 93.3% (but function coverage is over 96%).

Review:

  • d1f2af57 LGTM.
  • 405b6375 LGTM.
  • 44a32481 I checked that the vectors match those in RFC7748. LGTM.
  • 54697fa4 I checked that the vectors match those in NIST SP800-38a §F.5 for AES-128 CTR. LGTM.
  • 5845c228 Again, I manually checked that the vectors match those in draft-irtf-cfrg-eddsa-05. LGTM. I'm not super familiar with IRTF drafts and their statuses (specifically what might change in them), but that draft is currently in "Waiting for Document Shepherd" state. How much should we worry that, through the shepherding process, the vectors might change? Should we be keeping a close eye on this?

[review to be continued…]

comment:26 in reply to:  25 Changed 3 years ago by nickm

Replying to isis:

When we say "coverage should be over 95%", do we mean line coverage, function coverage, or branch coverage? I currently see line coverage for src/common/crypto.c at 93.3% (but function coverage is over 96%).

I've been looking at overall line coverage of src/common/crypto*.c . Different compilers will instrument a bit differently, of course.

Review:

  • d1f2af57 LGTM.
  • 405b6375 LGTM.
  • 44a32481 I checked that the vectors match those in RFC7748. LGTM.
  • 54697fa4 I checked that the vectors match those in NIST SP800-38a §F.5 for AES-128 CTR. LGTM.
  • 5845c228 Again, I manually checked that the vectors match those in draft-irtf-cfrg-eddsa-05. LGTM. I'm not super familiar with IRTF drafts and their statuses (specifically what might change in them), but that draft is currently in "Waiting for Document Shepherd" state. How much should we worry that, through the shepherding process, the vectors might change? Should we be keeping a close eye on this?

We should probably keep an eye on it, but I would be surprised if they decided to break backward compatibility with all the existing Ed25519 implementations.

comment:27 Changed 3 years ago by ahf

While working on Talla, I had a ton of troubles getting the code for the x25519-to-ed25519 algorithm to work because I was using libsodium, which is incompatible with Tor's ed25519 implementation. I ended up implementing a modified version of the Ed25519 code (from DJB's Python reference implementation) in "pure" Erlang.

During this process I generated a few test vectors for my (hopefully) correct, but slow code, to ensure that when I was done porting Tor's ed25519-ref10 implementation to an Erlang native function that I would be able to verify that it was giving the same result.

The test vectors can be found here: https://lab.baconsvin.org/talla/onion/blob/develop/src/onion_ed25519.erl#L272-302

comment:28 Changed 3 years ago by isis

Status: needs_reviewmerge_ready

[continuing…]

  • 371cdf48 The vectors do not match RFC5869, in particular the OKM from the test in section A.3 doesn't match. In test_crypto_hkdf_sha256_testvecs at some point there is char *mem_op_hex_tmp = NULL; and then it doesn't look like it gets used until in the done: stanza there is tor_free(mem_op_hex_tmp);.
  • ebfc73cb Aha! Now the vectors from the previous commit match. Nobody panic. LGTM.
  • cc20a1b3 s/scrypto/scrypt/ perhaps? Otherwise LGTM.
  • d49be31b LGTM.
  • 96389ab8 LGTM.
  • bdb5443a LGTM.
  • a50a4de3 See my comment on #18956.
  • 94923f68 LGTM.
  • 46801954 LGTM.
  • 1f52cc25 LGTM.
  • f4ec948b Should dh1_dup be tor_free()ed at the end of test_crypto_dh() in src/test/test_crypto.c?
  • 7d3d92ee LGTM.
  • b0a3d00d Ah, great, this fixes the memleak from f4ec948b. Both LGTM now. (Also you got a commit hash with d00d in it, pretty cool.)
  • 3ffcd571 "At long last, unit tests for degenerate DH public keys. Apparently, we detect and reject them correctly. Aren't you glad?" Yes, and also LGTM.
  • 0efe717b LGTM.
  • 8866580e LGTM.
  • 2469579b LGTM.

Overall, fix the changes file from #18956 mentioned above, and then it's ready for merge.

Unrelated, where does EXPAND("AN ALARMING ITEM TO FIND ON YOUR CREDIT-RATING STATEMENT"); come from? :)

comment:29 Changed 3 years ago by isis

Status: merge_readyneeds_revision

comment:30 Changed 3 years ago by nickm

Unrelated, where does EXPAND("AN ALARMING ITEM TO FIND ON YOUR CREDIT-RATING STATEMENT"); come from? :)

It's from John Brunner's The Shockwave Rider, which I recommend to everybody who likes SF and likes hacker stuff and hasn't read it. Warning: you probably know at least half a dozen people who think they are the protagonist of this book.

I'll fix the changes issue and merge; thanks!

comment:31 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_revisionclosed

Thanks again for the review; merged into master.

comment:32 Changed 3 years ago by isabela

Points: medium3
Note: See TracTickets for help on using tickets.