[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #16794 [Core Tor/Tor]: All cryptography unit test coverage should be over 95%; all should have test vectors

#16794: All cryptography unit test coverage should be over 95%; all should have
test vectors
-------------------------------------------------+-------------------------
 Reporter:  nickm                                |          Owner:  nickm
     Type:  enhancement                          |         Status:
 Priority:  Medium                               |  merge_ready
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.9.x-final
 Keywords:  testing, 028-triage, tor-tests-      |        Version:
  coverage, tor-tests-unit, TorCoreTeam201605,   |     Resolution:
  review-group-1                                 |  Actual Points:
Parent ID:  #16791                               |         Points:  medium
 Reviewer:  isis                                 |        Sponsor:
                                                 |  SponsorS-can
-------------------------------------------------+-------------------------
Changes (by isis):

 * status:  needs_review => merge_ready


Comment:

 [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
 [https://trac.torproject.org/projects/tor/ticket/18956#comment:7 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? :)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16794#comment:28>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs