[security] send --code does not verify that a nameplate was provided and uses the entire code as the nameplate instead

[afontenot]

This is almost certainly not intended behavior, as it voids security since the nameplate is not private.

One of the more obvious impacts here is that users are vulnerable to a malicious mailbox server, which is why I'm disclosing this publicly - the only real fix for an affected user is to stop sending codes that don't contain a nameplate.

Unfortunately, this is more dangerous than I originally thought, because the wormhole mailbox protocol has a list feature that makes all nameplates public.

./wormhole-rs send -v --code "xxxxxx" Cargo.toml

In debug log:

DEBUG magic_wormhole::core::rendezvous] Sending Claim(xxxxxx)

In Wireshark (unmasked websocket text):

0000   7b 22 74 79 70 65 22 3a 22 63 6c 61 69 6d 22 2c   {"type":"claim",
0010   22 6e 61 6d 65 70 6c 61 74 65 22 3a 22 78 78 78   "nameplate":"xxx
0020   78 78 78 22 7d                                    xxx"}

Code:

pub fn nameplate(&self) -> Nameplate {
    Nameplate::new(self.0.splitn(2, '-').next().unwrap())
}

splitn doesn't fail when there is nothing to split, so the next() just takes the whole string. Unfortunately Code is instantiated as a tuple struct, not by calling new, so it doesn't get checked that way either. The code also doesn't fail when doing PAKE, unfortunately, because the protocol uses the entire code (including the nameplate) as the password, so the fact that the code is 100% nameplate and 0% password is no bother. This part seems like a bit of a footgun for implementers.

[piegamesde]

Oh wow, I somehow had completely ignored that the protocol uses the entire code including the Nameplate for PAKE. I'll have a look into this.

The thing is, that nameplates technically do not need to be numeric, so it is not easy to do validation. Maybe the best thing would be to simply assert something about the length of the password?

Also, note that in what you describe codes like foo-bar would parse "foo" as nameplate and "bar" as password, which still has some password but with only half the entropy.

[afontenot]

I don't see any issue with allowing string nameplates. (Actually, you can even set the code to the empty string, and have it claim an empty nameplate with an empty password. I transferred a file this way with no issues.)

Also, note that in what you describe codes like foo-bar would parse "foo" as nameplate and "bar" as password, which still has some password but with only half the entropy.

That's a footgun for users to be sure. The biggest issue is that it's not obvious to the user what part of the code is nameplate and what part of it is password. That's actually how I discovered this bug, I was trying to figure out how MWRS handled that internally.

I think the cleanest way to do this might be to break with the Python client and have separate --password and --nameplate options. If the user doesn't provide a nameplate, pick one automatically like normal, but continue to use their custom password. This would create a cleaner break between the two.

Of course this might require more refactoring, and I'm not sure if breaking backward compatibility is an issue here.

Edit: to clarify, I'm not suggesting breaking with the protocol and only using the password portion for PAKE. I'm suggesting changing the CLI so that the nameplate and password portions of the code are provided separately by the user.

[felinira]

This has somehow been falling through the cracks, and I am not looking forward to making a new semver-breaking release so soon after the last one.

Luckily this isn't difficult to solve, even with the current cli arguments. Generally this would "only" require some entropy validation in https://github.com/magic-wormhole/magic-wormhole.rs/blob/main/cli/src/main.rs#L295 by splitting the code once with - and then checking the entropy of the remaining string. To simplifly matters it would probably be enough to check whether the remaining code is at least 4 bytes long. This isn't enough to be really secure, but enough to ensure that we have at least something.

As a bonus, printing a warning whenever the code is less than, say, 10 bytes, is probably a good idea. And maybe amending the documentation to include some more information about how the code argument works, and about this check.

I'll get around to it before the next minor release in a couple weeks, but I am always glad about contributions :)

[afontenot]

It's worth thinking about which issues we want to fix here:

• It's possible to have no security by passing longtotallysecurephrase to --code, as long as it doesn't contain a dash. Someone right now could be repeatedly sending list to the mailbox server every second and attempting to download from any mailbox that doesn't use a number as its nameplate. This would be fixed by a solution as simple as str.contains("-").
• It's not possible to use a custom code if you want the wormhole client to pick an unused nameplate for you, which is extremely annoying. Wanting to use a custom code: really common. Wanting to use a custom nameplate (with no guarantee it will be available): probably very rare?? That's what my --nameplate and --password suggestion is supposed to solve.
• It's possible to have greatly reduced security by sending a code the user expects to be secure like --code claim-arrange, because the first word becomes the nameplate. If you're using the PGP wordlist to generate codes, that's a whole 8 bits of security.

A solution like "check that the code is at least four bytes long" doesn't solve this issue, because the PGP words are all >= 4 bytes but would still only have 8 bits of entropy. Given that the output of wormhole-rs is already very verbose, perhaps it would be a reasonable to explicitly print

nameplate (public): claim
passphrase (private): arrange

when a custom code is used. This wouldn't cause any issue with UX because we already explicitly print the whole wormhole "code" and give an example of usage "wormhole-rs receive claim-arrange".

[meejah]

I think nameplates do have to be numbers, no? https://github.com/magic-wormhole/magic-wormhole/blob/master/docs/server-protocol.rst#concepts

(At least, they currently are in the reference implementation, although I'm not entirely sure where that's enforced, or not)

[afontenot]

@meejah it's not enforced, in fact you can even make the nameplate the empty string and that works too.

[meejah]

The intent is clearly that they're numbers, so probably it should be enforced (both on clients and the server).

[felinira]

I have opened #257 and intend to merge this for 0.7.2

It's possible to have no security by passing longtotallysecurephrase to --code, as long as it doesn't contain a dash. Someone right now could be repeatedly sending list to the mailbox server every second and attempting to download from any mailbox that doesn't use a number as its nameplate. This would be fixed by a solution as simple as str.contains("-").

This will be a hard error in the next point release. This is too unsafe to ever be okay, and I'd rather have people's setups fall apart then.

It's not possible to use a custom code if you want the wormhole client to pick an unused nameplate for you, which is extremely annoying. Wanting to use a custom code: really common. Wanting to use a custom nameplate (with no guarantee it will be available): probably very rare?? That's what my --nameplate and --password suggestion is supposed to solve.

I'll add a flag to the CLI client to support this later. At the very least "--password" as an alternative to "--code" makes a lot of sense to have.

It's possible to have greatly reduced security by sending a code the user expects to be secure like --code claim-arrange, because the first word becomes the nameplate. If you're using the PGP wordlist to generate codes, that's a whole 8 bits of security.

#257 includes code to validate the entropy of the passed code words. This will be a hard error in the future, right now we only fail in interactive terminals for now. This should be almost all use cases.

This requires library users to enable the optional "entropy" feature until we can make this a hard error for all use cases in the next release.

The intent is clearly that they're numbers, so probably it should be enforced (both on clients and the server).

I have added number validation to nameplates. Right now it only prints a warning, I'll upgrade this to an error on the next breaking release.