salsa20: support insecure 16-byte keys

[micolous]

Fixes #432.

This adds a KeySize parameter to SalsaCore (like RC4), and sets it to U32 by default, for compatibility with existing code.

I had a look at implementing 10 byte keys, but couldn't find test vectors for it, so I've left it out.

I haven't added support for XSalsa with other key sizes; that would need a similar change.

I tested this with aarch64-apple-darwin and x86_64-apple-darwin (via Rosetta 2, which seems to support SSE2).

ECRYPT test vectors were acquired via https://github.com/oxarbitrage/salsa20-ecrypt-vectors-converter, with a little jq to convert their JSON files into blobby's text format:

def convert(base):
  def stream:
    recurse(if . >= base then ./base|floor else empty end) | . % base ;
  [stream] | reverse
  | if   base <  10 then map(tostring) | join("")
    elif base <= 36 then map(if . < 10 then 48 + . else . + 55 end) | implode
    else error("base too large")
    end;

def pad:
  ("0" * (8 - length)) + .;

map_values(.[]) | map(
  [
    [.key1, .iv, (.stream1index | convert(16) | pad), .stream1expected],
    [.key1, .iv, (.stream2index | convert(16) | pad), .stream2expected],
    [.key1, .iv, (.stream3index | convert(16) | pad), .stream3expected],
    [.key1, .iv, (.stream4index | convert(16) | pad), .stream4expected]
  ]
) | flatten | .[]

[micolous]

Of course it fails immediately in CI. I'll have a look. 😆

[micolous]

Fixed CI.

[tarcieri]

This looks OK to me, but @micolous looks like there's a merge conflict?

[newpavlov]

I don't think we need to make the backends generic over key size since only state initialization is different.

[micolous]

This looks OK to me, but @micolous looks like there's a merge conflict?

Resolved, looks like this was caused by (later) PRs #436 and #446.

I don't think we need to make the backends generic over key size since only state initialization is different.

I don't think there's any way around this with RustCrypto's traits, without providing a different version of KeyIvInit::new() which takes in a 16-byte key, similar to SalsaCore::from_raw_state() (which doesn't affect key size). That would be different to how other ciphers implement these traits, even if in salsa20's case they're doing the exact same thing under the hood.

If we're implementing KeyIvInit:

• KeyIvInit::new uses <Self as KeySizeUser>::KeySize to determine the key length.
• KeySizeUser::KeySize must be constrained by something; currently it's constrained statically by the impl, but the only other candidate I see is for it to be constrained by a type parameter.
• Because Backend has a &'a mut SalsaCore field, all of the field's type parameters need to be replicated.

I had a poke around RustCrypto cipher implementations for those that take multiple key sizes. They seem to either take a type parameter (eg: camellia, rc4 and rc5 have a PhantomData) or use macros to build multiple distinct struct definitions with a size provided as a macro parameter (eg: aes).

Adding an extra struct definition for 16-byte keys seems worse, because there'd be a bunch of duplicated code (even if we hide that behind macros).

By comparison, adding a type parameter only affects the KeyIvInit impl, and there is no duplicated code. Providing a default for the type parameter means we shouldn't break anyone else.

If someone wants to add support for 10 byte keys in the future, then they would just provide another KeyIvInit impl for SalsaCore<..., U10>.

[newpavlov]

We have 3 options:

1. Overwrite the KeyIvInit::new_from_slices method to add support of 16 byte keys (e.g. see the blowfish crate).
2. Replace the R: Rounds with V: Variant, where Variant would keep information about number of rounds, key size, and whether it's an X-variant or not.
3. Explicitly use &mut [u32; STATE_WORDS] and PhantomData<R> in backends instead of &mut SalsaCore.

Personally, I prefer the last option.