[liberationtech] Trsst Encryption (was: About Telegram)

[Steve Weis]

Hi Michael. Some comments inline...

On Wed, Mar 19, 2014 at 9:01 AM, Michael Powers <michael at mpowers.net> wrote:
> For a private message, we generate a random 256-bit key and encrypt with
> AES.  Then for each recipient, we use a hash of the shared ECDH secret and
> the message-id to encrypt the key and append it to the message.  We're xor
> ciphering because the inputs are random-ish keys and we need it to be simple
> and fast.  All public keys are static, and all public and private messages
> are viewable by anyone.

I don't understand what this is trying to accomplish. It generates:
(inputKey || Hash(inputKey)) ^ Hash(sharedKey || entryId))

What is that trying to do? It looks like you made up your own attempt
at an authenticated cipher mode. That's generally a bad idea.

I don't buy the argument that you need to make up your own
construction for speed. As is, in this Java code, I think the
KeyAgreement and multiple MessageDigest object instantiations will
dominate the cost.

I'd use AES-CBC and HMAC if you're trying to authenticate and encrypt.
Keep it simple and well-understood.

Some specific comments & questions:
1. You don't do anything to check that sharedKey is a fixed-length or
use an unambiguous delimiter between entryId:
https://github.com/TrsstProject/trsst/blob/master/src/main/java/com/trsst/Crypto.java#L72
2. What cipher mode is being used here? It's not using ECB by default is it?
https://github.com/TrsstProject/trsst/blob/master/src/main/java/com/trsst/Crypto.java#L176
3. Where is the authentication?
https://github.com/TrsstProject/trsst/blob/master/src/main/java/com/trsst/Crypto.java#L180
4. This is padding with zero bytes. Is it expecting fixed-length messages?
https://github.com/TrsstProject/trsst/blob/master/src/main/java/com/trsst/Crypto.java#L177
5. This might be shady since it doesn't do any input validation on the token:
https://github.com/TrsstProject/trsst/blob/master/src/main/java/com/trsst/Crypto.java#L246
6. Vulnerable to timing attack:
https://github.com/TrsstProject/trsst/blob/master/src/main/java/com/trsst/Crypto.java#L263
7. Vulnerable to timing attack:
https://github.com/TrsstProject/trsst/blob/master/src/main/java/com/trsst/Crypto.java#L331
8. Why did you decide to use BouncyCastle?
9. You're throwing the wrong SecurityException. Probably want to catch
GeneralSecurityException and not Exception:
https://github.com/TrsstProject/trsst/blob/master/src/main/java/com/trsst/Crypto.java#L31

I only spent a few minutes looking at this. It needs a lot of work.