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

Re: [tor-bugs] #12981 [Tor]: Add backends for encrypted storage, scrypt

#12981: Add backends for encrypted storage, scrypt
------------------------+--------------------------------------------
     Reporter:  nickm   |      Owner:
         Type:  defect  |     Status:  needs_review
     Priority:  normal  |  Milestone:  Tor: 0.2.6.x-final
    Component:  Tor     |    Version:
   Resolution:          |   Keywords:  tor-relay, scrypt, nickm-patch
Actual Points:          |  Parent ID:
       Points:          |
------------------------+--------------------------------------------

Comment (by yawning):

 Review:
  * `9b2d8c4e20a57ce849395a2135ac4e720bf99c42` - Rename secret_to_key to
 secret_to_key_rfc2440
    * Mechanical rename, looks good to me.
  * `e72a5b3c070451e7762b1d22553cf077c50eb123` - Move secret-to-key
 functionality into a separate module
    * Trivial code move, looks good to me.
  * `a7cad1069c39a694ee6153f8fc848ea056cb6ffe` - More generic passphrase
 hashing code, including scrypt support
    * `common/crypto_s2k.c`
      * `secret_to_key_get_type()`
        * If there ever is a case where only one of
 `secret_to_key_spec_len()`/`secret_to_key_key_len(type)` fails,
 `S2K_BAD_LEN` will be returned instead of `S2K_BAD_ALGORITHM`.  Fairly
 nitpicky, since it's obvious that it never happens.
      * `make_specifier()`
        * Remove `(void)flags;`
        * Return `S2K_BAD_ALGORITHM` instead of `tor_assert(0);` in the
 default case?  (Invariant so an assert is also ok)
      * `secret_to_key_derivekey()`
        * `return S2K_NO_SCRYPT;` should be `return S2K_NO_SCRYPT_SUPPORT;`
      * `secret_to_key_check()`
        * Probably should assert that `spec_len` and `key_len` are > 0.
    * `common/crypto_s2k.h`
       * `TOR_CRYPTO_H_INCLUDED` -> `CRYPTO_S2K_H_` (Adjust to taste, but
 needs renaming)
  * `8c02faf5034a3bb14459d2c41ba18be76dc5d0fe` - Rudimentary-but-sufficient
 passphrase-encrypted box code.
    * Instead of using a 0 filled IV, randomly generate one and include it
 in the header?  (Obligatory bikesheding/tinfoil-hattery).
    * `crypto_pwbox()`
      * memwipe `keys`?
    * `crypto_unpwbox()`
      * memwipe `keys`?
    * Maybe test cryptoboxes with flags != 0?
  * `18d810fdbe8593000dc25240d5911027ebe3b506` - fixup! More generic
 passphrase hashing code, including scrypt support
    * No functional changes, looks good to me.
  * `0ffe93ba2096b8a9a2d153619b3cc40ab24cc364` - Tweak and expose
 secret_to_key_compute_key for
 testing/`81dc2271c8d94d3bd58aca0ce4e751391bb31e75` - fixup! Tweak and
 expose secret_to_key_compute_key for testing
    * Ok with the fixup commit.
  * `bf57e5d0bf9dab6b61f437da9be00440f8f7e911` - Test vectors for scrypt
 from draft-josefsson-scrypt-kdf-00
    * Looks good to me.
  * `06f3346fdd36f190e878c35d6a0f8298d12a01be` - Test vectors for PBKDF2
 from RFC6070
    * Looks good to me.
  * `6999fa4a12ca38a98144339a4642bb1c7489750c` - Use preferred key-
 expansion means for pbkdf2, scrypt.
    * Looks good to me.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12981#comment:5>
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