Use newtypes to wrap basic types for more type safety about magic-wormhole.rs HOT 3 OPEN

I like it!

from magic-wormhole.rs.

I'm working through this process for all the String types that we use: I've gotten Side (both ours and theirs), Key, and Nameplates done so far.

I just started work on Code, and discovered a subtle and serious bug: the key::start_pake() function had swapped code and appid as they get passed into the SPAKE2 algorithm. This wouldn't have interoperated with the Python version, so we'd have caught it anyways, but it does interoperate with itself. And for SPAKE2, the consequence of reversing these two arguments is that the protocol becomes vulnerable to an online man-in-the-middle attack.

(a brief crypto digression so I have this written down somewhere)

On Alice's side, SPAKE2 (in symmetric mode, so M=N) accepts a secret password pw and application id idS, generates a random scalar x, uses a public group generator G and a public element M (for which nobody knows the discrete log relative to G). It computes Alice's msg1 as MsgA = x*G + pw*M and sends it to Bob. Bob computes MsgB = y*G + pw*M. Alice then computes ZA = x*(MsgB - pw*M) and generates a session key by hashing KA = H(MsgA, MsgB, ZA, idS, pw). Bob's session key is made with ZB = y*(MsgA - pw*M) and KB = H(MsgA, MsgB, ZB, idS, pw). If the passwords and application ids are the same, then KA==KB and they can communicate. In Magic-Wormhole (and many other protocols), the first thing each side does with their key is to send a hash of it as a Key Confirmation Message: KCMA = H("KCM", KA). The other side will receive this, compare it against a hash of their own key, and make sure it matches. This tells them that somebody else in the world computed the same key that they did. SPAKE2 then promises that the only other party who could compute this is the one who used the same pw in the same session (i.e. incorporating MsgB into their response).

However, if we swap idS and pw, then we get:

• MsgA = x*G + idS*M
• MsgB = y*G + idS*M
• ZA = x*(MsgB - idS*M)
• KA = H(MsgA, MsgB, ZA, pw, idS)

The pw*M blinding factors are now constant, independent of the password, and known to everyone (since the app-id is baked into the application). Mallory can sit between Alice and Bob and run his own protocol with each. He doesn't learn ZA, but he computes his own ZB which will be identical. Then when Alice sends her KCM, he hashes all possible values for the code/password until he finds one that produces a KB that matches Alice's KCM. Now he can communicate with Alice and she won't know the difference. Mallory can run this attack in parallel with both sides, or he can halve his work and wait until he learns the code from Alice before conducting a normal protocol interaction with Bob.

I'll fix this when I land the newtypes code that revealed it. Interestingly, the SPAKE2 API itself doesn't protect us here (it's defined as pub fn start_symmetric(password: &[u8], id_s: &[u8])). The magic-wormhole.rs start_pake() function is called with (code, appid) but accepts (appid, code), and then correctly calls SPAKE2::start_symmetric(code, appid).

So I'm thinking that SPAKE2.rs could benefit from a safer (i.e. different) set of types in its arguments. I wanted the id_a to be allowed to contain arbitrary bytes, not requiring it always be a String (so it could be a hash of something else). But maybe we should add Password and Identity newtypes into spake2.rs for safety.

from magic-wormhole.rs.

Ok, I think I've implemented all the easy ones. The next set to tackle will be different flavors of messages:

• the bytes (Vec<u8>) that come out of hex decoding on RxMessage and get fed as ciphertext into the libsodium::SecretBox decryption function
• the bytes of plaintext that come out of decryption, which are treated differently depending upon the "phase" value (for the application-destined numerical phases, they're raw bytes, but the "version" phase is treated as UTF-8-encoded JSON)

There are a couple of places that should hold JSON Values (the Welcome message, the application-supplied version data, which gets wrapped into a library-provided version object). We should 1: move these from HashMap<String,String> to a serde_json::Value, and 2: make different newtypes around those Values for the different uses. In particular, I've fixed bugs in the Python code where I got confused about whether a given object was the application-supplied version object, or the thing which wraps it and gets sent as the first encrypted message following key agreement. It'd be nice to have separate types to avoid that confusion.

from magic-wormhole.rs.