Opened 5 years ago

Closed 5 years ago

#12981 closed defect (implemented)

Add backends for encrypted storage, scrypt

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay, scrypt, nickm-patch
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We need an encrypted storage format for private keys that is better than openssl's armor, once we start storing ed25519 private keys (optionally encrypted).

We should also use a better passphrase-based-key-derivation function than we have now. scrypt isn't my favorite, but until the PHC is done, it's probably a good choice.

Once those are in, we can use scrypt in place of our current openpgp RFC2440 password-to-key function.

Child Tickets

Change History (13)

comment:1 Changed 5 years ago by nickm

I've got this mostly done in a branch "libscrypt" in my public repository. It needs one or two more tests for failing cases, but atm I think it's ready for review.

It optionally uses libscrypt: if you have it, you get scrypt; if not, you don't. Let me know if you need me to port libscrypt to your weird platform or compiler.

comment:2 Changed 5 years ago by nickm

Status: newneeds_review

comment:3 Changed 5 years ago by nickm

Just added test vectors for scrypt and pbkdf2

comment:4 Changed 5 years ago by nickm

Keywords: nickm-patch added

Apply a nickm-patch keyword to tickets in needs_review in 0.2.6 where I wrote the patch, so I know which ones I can('t) review myself.

comment:5 Changed 5 years ago 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.

comment:6 Changed 5 years ago by yawning

Status: needs_reviewneeds_revision

comment:7 Changed 5 years ago by nickm

Status: needs_revisionneeds_review

Okay, I've made the suggested changes. Better now?

comment:8 Changed 5 years ago by yawning

The changes look good to me. I assume tests for various pwbox failures are the "one or two more tests for failing cases" and will follow shortly.

comment:9 Changed 5 years ago by nickm

There's now also a "libscrypt_trunnel" branch that does the right thing with trunnel. (Make sure you get the latest version before reviewing; I had to force-update it a couple of times)

comment:10 Changed 5 years ago by nickm

That branch now also has tests for the failing cases

comment:11 in reply to:  9 Changed 5 years ago by yawning

Replying to nickm:

There's now also a "libscrypt_trunnel" branch that does the right thing with trunnel. (Make sure you get the latest version before reviewing; I had to force-update it a couple of times)

Looks good, is there a reason why you do enc->header_len = spec_len; instead of calling pwbox_encoded_set_header_len()? Apart from that, I don't have any other objections.

(NB: I didn't look at the autogenerated code too closely.)

comment:12 Changed 5 years ago by nickm

Is there a reason why you do enc->header_len = spec_len; instead of calling pwbox_encoded_set_header_len()?

Mainly because I don't personally like using accesor functions when C syntax is simpler. I'm mainly using them in this code to access complex/dynamic data structures.

comment:13 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Thanks for the review; merging!

Note: See TracTickets for help on using tickets.