protect against mitm attacks

Description

Originally recorded in #197 (only mildly related):
*1) Wait until a new client tries to connect and intercept
*2) Connect your own client to the server, obtain "challenge" from server
*3) Feed "challenge" to the original client, wait for "challenge_response"
*4) Feed "challenge_response" to the server and allow your own client to interact with the server
*5) Drop the original client

In the meantime, this is seriously mitigated by r1981 which encrypts all traffic except for the initial hello packet. A mitm attacker can still intercept and forward all traffic between the client and server, but since the AES key is derived from the secret, the attacker has no way of decrypting the traffic, and no way of modifying it either.

meh - I don't really like ssl, the whole certificate business is complicated, it would take some effort to do this right *and* to document how to set things up properly.

I like ​SPEKE: "Unlike unauthenticated Diffie-Hellman, SPEKE prevents man in the middle attack by the incorporation of the password. An attacker who is able to read and modify all messages between Alice and Bob cannot learn the shared key K and cannot make more than one guess for the password in each interaction with a party that knows it."

It's simple and effective.

We're not far off that, modifying our code to accommodate it shouldn't be too hard.

Great read thanks. We're safe from padding oracle, and none of the data is under user control (although as discussed previously, the key's salt is), I chose zero-byte-padding for simplicity and also because we don't need any of the features of PKCS and friends.

The good thing is that we don't have to do anything with the transport at all, unlike EAP, SPEKE relies on just one random integer sent across from both ends.
(it really is beautifully simple)

Shame that there is nothing in python for doing ​AES CCM Mode.
Some more useful pointers:

Note: still need to copy the elgamal code to use a "SafePrime" and not a "StrongPrime", and maybe use password stretching? (not sure it would make any difference here as the hash value is still deterministic from the input and that is all we use it for)

I just recently started looking at Xpra, and like many aspects of it. But the current attempts to implement cryptography worry me. I'd like to repeat a concern that lindi has expressed: that implementing your own secure transport layer is difficult; even experts make mistakes and it's very easy to end up with something with security problems.

Unfortunately, some of what I've seen so far in the code doesn't inspire confidence. Among the issues thus far:

"since the AES key is derived from the secret, the attacker has no way
of decrypting the traffic, and no way of modifying it either": not
true as the attacker can still change data (without decrypting it) and
replay old traffic. You seem to have realized this (you're talking
about authenticated encryption now), but I haven't seen what design
you intend to use. Probably from Python, the easiest approach is to
add a MAC to each of the packets (be sure to encrypt then MAC, not
some other order), and the MAC should also include a counter to
protect against replay attacks.

In the current client_base.py code, using UUIDs to generate salt and IVs
isn't a good idea. UUIDs have some fixed content (they aren't
completely random), and by using hexadecimal strings you're cutting the
number of random bits in half. The Cyrpto.Random module can already
give you cryptographically-strong random data, so you should be using
that instead.

The padding used in protocol.py is I think non-standard. You could
instead use CTR mode for the cipher, which doesn't require any padding
at all (and is secure as long as you are very careful to never repeat an
IV).

If you do choose to use something like SPEKE (which I'm not familiar
with, though at first glance it seems reasonable), I hope you're
planning to improve the code snippet you posted:

No need to randomly generate a prime number each time to define the
group. Diffie-Hellman key exchange works just fine with a well-known
shared prime number. There are some standard ones you could use, or
"openssl dhparam" can generate a safe value, and then you can
hard-code it in the Xpra source. I'd probably go larger than 768
bits.

Picking a random value between 210 and 215 is a serious problem
(it's easy for an attacker to exhaustively check all those values).
You want to pick random values from the entire allowable range.

The implementation is very inefficient, computing ga and only
reducing modulo p at the end. Take a look at the Python pow()
builtin. (But even this shouldn't be used carelessly for serious
cryptography, since it doesn't take care to avoid timing or other
side-channel attacks. It might be safe here, but I'd have to think to
be more certain.)

I'll need to think over the initial handshake in more detail; not too much point in commenting on the current design now as it looks likely to change in the near future.

I may see if I can implement something better and if so send a patch, but if you make further changes yourself I'd at least advise waiting to release a version with an encrypted connection until the design can be reviewed in more detail.

Please bear in mind that we will not release *anything* until SPEKE key exchange (or something like it) is implemented.
Nonetheless, let me try to reply to your queries:

(...) not true as the attacker can still change data (without decrypting it) and replay old traffic

If you change data the packet becomes invalid and the connection is dropped. You only get one attempt. Or am I missing something?
(and we are talking about SPEKE-like key exchange setup, right?)

(...) but I haven't seen what design you intend to use

I don't have pretty diagrams ;)
But the general idea is to hook something based on the PoC code into the handshake code.

(...) and the MAC should also include a counter to protect against replay attacks

With CBC mode, I fail to see how this provides any benefits: the replayed packets would be invalid and the connection dropped. And since we are trying to prevent both eavesdropping and password stealing, if you can modify traffic you can already get the connection to drop. Nothing gained here?

(...) using UUIDs to generate salt and IVs isn't a good idea

Generally no, but these values are public anyway, so although we should try make them stronger, I don't think that there is an inherently big risk here: salts don't matter much as long as they are there. (but I will change this to Crypto.Random when I get around to this ticket, hex strings were just easier to pass around multi-language code - thanks for reminding me)

The padding used in protocol.py is I think non-standard. You could instead use CTR mode for the cipher, which doesn't require any padding at all (and is secure as long as you are very careful to never repeat an IV)

I'm pretty sure null padding is standard, I will check. CTR mode is harder to get right, and is IMHO not worth the effort - although it would save an average of 16 bytes per packet (security vs space).

I hope you're planning to improve the code snippet you posted

It took me about 10 minutes to write as a PoC, so definitely...

No need to randomly generate a prime number each time (...)

Thanks, I might do that, although I really don't like "hardcoding" any kind of value. I'll have to think about that (comments welcome) as there are pros and cons: hardcoding makes the code easier, but should any crypto vulnerability rely on pre-calculating tables with known primes this could also make it more vulnerable. On the other hand, letting the server choose the prime number could also expose the clients to more attacks. Not an easy one.

Picking a random value between 210 and 215 is a serious problem (it's easy for an attacker to exhaustively check all those values). You want to pick random values from the entire allowable range.

Again, this was a 10min PoC sample code, but I'm not sure about this one:

the range discarded is very small comparatively (1/25) so there is almost nothing lost by excluding small values and small values make the calculations far less costly for an attacker (not that there are any known attacks - but still),

using very large values becomes onerous (noticeably), and apparently for no benefit

Ideas?

The implementation is very inefficient (...)

Again, PoC code generally is.

(...) Python pow() builtin. (But even this shouldn't be used carelessly for serious cryptography, since it doesn't take care to avoid timing or other side-channel attacks. It might be safe here, but I'd have to think to be more certain)

Yes, that's one of the beautiful things about SPEKE that I liked when I first saw it: A and B are public, so the time it takes to calculate power() does not really matter when it comes to timing attacks. (although as mentioned above, I would be weary of using really low cheap/easy values still)

I'll need to think over the initial handshake in more detail; not too much point in commenting on the current design now as it looks likely to change in the near future

Yes, I chose SPEKE because it fits very well with the handshake we currently have. I don't think many things will need to change there. (and even less if the prime number is fixed)

I may see if I can implement something better and if so send a patch

Please do - I am currently on holiday

but if you make further changes yourself I'd at least advise waiting to release a version with an encrypted connection until the design can be reviewed in more detail

We generally stick to a monthly release cycle (more or less), and this is just the beginning of the new cycle. It would not be unusual to ship code that is disabled until properly reviewed and tested (and sometimes dropped completely).

I just attached a patch to this ticket (xpra.patch), with a prototype for a new key exchange and encryption protocol for Xpra. This isn't quite ready to be checked in, but has most of the basics there.

I have some problem with the initial handshake still, I think, because in testing the connection authenticates but then the client disconnects with the error message "xpra: Fatal IO error 2 (No such file or directory) on X server :0." I'm still debugging.

Some code still needs a bit more cleanup.

I'd like to add authentication for the initial packets in the exchange: basically, compute a hash of all data sent before the connection switches over to being encrypted, then communicate that hash when the connection is secure. That will prevent a man-in-the-middle from tampering with the initial handshake (without the tampering being evident).

I'd like to revisit the key exchange. Right now it's a simple Diffie-Hellman key exchange with a little bit of authentication, but this definitely leaks some information about the password and isn't based on an existing protocol. Using something based on EAP-EKE, SPEKE, or some existing protocol would be better. (This should be easy to do within the framework, just hasn't been done yet.)

I like the separation between crypto code and network, will keep that no matter what

I'm not sure about the dedicated packet type for key-exchange (which should block the hello until complete as per TODO item) - might be easier to overload the hello packet as per the current password code (it might make it easier to fail gracefully with a meaningful error message) - actually I'm not sure, maybe this is ok too, I will have to try both to see

it would be nice to split mac from the encryption step, or at least be able to choose the mac algo - no biggie, could be done later too

encrypt then/and/before mac: while encrypt-then-mac is theoretically better, it is also somewhat harder to get right. meh, I don't think it is worth the hassle of trying to support anything else, is it? in any case:

the whole key exchange is there for doing things like DH, which is insecure... whereas authenticated encryption, like SPEKE and others like ​GCM are secure and do not need any key exchange step at all. What bothers me is the level of complexity in adding a key exchange which does not add value. I quote:

The need for Authenticated Encryption emerged from observation that securely compositing a confidentiality mode with an authentication mode could be error prone and difficult.[1][2] This was confirmed by a number of practical attacks has been introduced into production protocols and applications by incorrect implementation, or lack of, authentication (including SSL/TLS).[3]
In summary: I'm just really not keen on key exchange since it can be avoided.
I do understand that some algos may be patented in the USA, but GCM is not, so maybe that's a good way forward?

I have also been meaning for quite some time to go back and work on my patches a bit more; apologies that I have been busy and haven't gotten to that.

A few other comments:

I'm not sure about the dedicated packet type for key-exchange (which should block the hello until complete as per TODO item) - might be easier to overload the hello packet as per the current password code (it might make it easier to fail gracefully with a meaningful error message) - actually I'm not sure, maybe this is ok too, I will have to try both to see

I'll have to go back to look at exactly why I set this up this way. One thing I was trying to support was key exchange protocols that might take multiple round trips

it would be nice to split mac from the encryption step, or at least be able to choose the mac algo - no biggie, could be done later too

I have no problem with letting the MAC be chosen separately from the encryption algorithm.

encrypt then/and/before mac: while encrypt-then-mac is theoretically better, it is also somewhat harder to get right. meh, I don't think it is worth the hassle of trying to support anything else, is it? in any case:

I don't feel like encrypt-then-mac is much more difficult, and we should only support one variant so I'd go with that.

In summary: I'm just really not keen on key exchange since it can be avoided.

You need some method to establish the key used to encrypt and MAC. It's best if this is a session key (used for a single connection only) since this avoids some potential attacks (that could occur if the same key was used in multiple sessions). The session key could be derived from some master key (based on a shared password) and per-session nonce values. The advantage of a DH-like key exchange is that it provides forward secrecy (if the master key/password is revealed, that wouldn't allow an attacker who had recorded network traffic to decrypt the past traffic). DH is not necessary if you give up forward secrecy.

Whatever is done, I would again strongly recommend using a session-specific key for encryption, once the handshaking is all done.

I do understand that some algos may be patented in the USA, but GCM is not, so maybe that's a good way forward?

AES-GCM is essentially AES-CTR for encryption combined with a MAC that can be optimized very well given appropriate hardware support. But, since I don't think we have a native AES-GCM implementation in Python, I would still suggest separating the encrypt and MAC steps that have Python support. My personal vote would be for something like in the current patch: AES-CTR encryption combined HMAC-SHA-256. There should be no patent problems with any of the above.

Clarification: re-reading comment:17, I'm not against DH per se, or at least not variants like ECDH... just plain DH. And as long as the mechanism is "DH-like" (like you said) then I have not problem with it.

Even though I still think that the "forward secrecy" is not a very serious issue for our use case: getting access to the session passwords plus the full network capture is a tall order - reaching this goal and being able to see the session being replayed would not be the most interesting thing for the attacker IMO.

FYI: 0.10 is getting nearer, so this looks like getting delayed again..

I really really want to get something done about this ticket for this milestone, so raising the priority.
I think we want to try something incremental, probably starting with a better separation of the network code proper and the encryption layer and the consumers. (a bit like what was done in the patch attached to this ticket)
The number one thing to implement would have been MAC... except GCM makes this redundant.
OTOH the multiple round-trips for establishing session keys may be getting in the way of things like #426 (though making it all part of the "hello" packet might help)

FYI: the recent changes in trunk make the protocol class much easier to work with: the compression and encoding aspects have been moved to separate classes. The same should be done with the crypto bits, I have only moved the bare minimum so far.