PKCS #7 requires at least one byte of padding

[totaam]

Until now, we didn't care because we already knew the size of the padding: the packets have a size and we just round to the nearest block size... This was not a problem with python-cryptography or with the javascript forge libraries. But this is now causing issues with native AES in Javascript...

[totaam]

Details in rfc2315 section 10.3 - see part 2.

[totaam]

The new toggle works and is backwards compatible, for now. This will need to be backported and at some point, warn users if the peer doesn't support the new always-pad option. Then later, we can enable always-pad by default.

Is it worth considering using an unused bit from the header? (would this cause even more backwards compatibility issues?)

[totaam]

As per wikipedia: PBKDF2, let's keep up with the times and raise the default number of iterations: 441c3dab708ac16e456f69003f6b3a81a4094a84 and the maximum number of iterations: cb92bf6ebc9cc0b1fd355f3aa814213542451ed1 so we can raise the default again in the future, and also remove legacy padding: 27b0b22601f43bdd84451389aa110845cfb65e54. Note: we still want an upper bound to prevent CPU DoS.

The documentation should probably be updated too: https://github.com/Xpra-org/xpra/blob/master/docs/Network/AES.md

[totaam]

Another problem is that we used 256 bits of padding for AES-CBC... and the correct padding is only 128 bits: https://github.com/Xpra-org/xpra/blame/b398fe3ff3072dd5a81a41d67d46a7a1a8faf609/xpra/net/crypto.py#L259-L264 This explains why it sometimes worked for me and sometimes not! (the hello packet size varies as it includes some timestamps) We'll see if this needs to be gated using a flag for backwards compatibility. This is required for interoperability with the html5 client: https://github.com/Xpra-org/xpra-html5/issues/314

[totaam]

Recording these errors for my own sanity.

After spending way too much time on the html5 AES code, I found out that it is in fact correct. Using the same (temporarily hard-coded) IV, key and salt from the unit test data (wrongly named DEFAULT in the code): https://github.com/Xpra-org/xpra/blob/89cbc11ca27afcffd962b55d2d3dd140d57c7ae6/xpra/net/crypto.py#L26-L27 Both in the python server and html5 client, and then dumping the raw and encrypted packet data to the log using:

index 02a808910f..0294cedf5f 100644
--- a/xpra/net/protocol/socket_handler.py
+++ b/xpra/net/protocol/socket_handler.py
@@ -456,6 +456,7 @@ class SocketProtocol:
                     raise RuntimeError(f"expected encrypted size to be {actual_size}, but got {len(data)}")
                 cryptolog("sending %6s bytes %s encrypted with %2s bytes of padding",
                           payload_size, self.cipher_out_name, padding_size)
+                cryptolog.warn("encrypted(%s)=%s", hexstr(padded), hexstr(data))
             if proto_flags & FLAGS_NOHEADER:
                 assert not self.cipher_out
                 # for plain/text packets (ie: gibberish response)

With a server running as:

XPRA_CRYPTO_ALWAYS_PAD=1 xpra start --start=xterm \
    --bind-tcp=0.0.0.0:10000,encryption=AES-CBC,keyfile=./aes.txt-d crypto \
    --minimal --html=on --websocket-upgrade=yes

I've added the test data to the html5 crypto test page: https://github.com/Xpra-org/xpra-html5/commit/9bf096b9fe640109445e2271e7d4dd01eff60654

The same test data was also added to the python unit tests: ed9d15fd494b7e512ffd33c01979ccb83df2010d And they're failing with the same encrypted result! (and in both cases, the encrypted values can be decrypted back to the correct original message)

So my guess is that there is a difference between what the unit tests do and what the real server does.. Probably something to do with the iv, key or salt being bytes or strings and decoding to different values. Somehow. Except that the very first packet does match! Why only this one?

Oh, and the python client does decrypt the packets without errors - so whatever is going wrong, the python code handles it fine - just not in the unit tests!?

[totaam]

What was needed: CipherContext.finalize This will break older versions, but is the correct implementation.. (and works with the html5 client - at least for sending!)

[totaam]

Danger, dragons.

Adding the calls to finalize() may make the html5 client work, but this is definitely the wrong thing to do as this makes us initialize the CBC cipher with the same IV multiple times - and that's bad, very bad.

[totaam]

This ticket is turning into a more general AES review ticket, so should we worry about rekeying? Cryptographic Wear-Out for Symmetric Encryption: AES-CBC Safety Limits: Maximum data safely encrypted under a single key with a random nonce: 2^{47} bytes (approximately 141 TiB) TLDR: no. At least for CBC.

[totaam]

The original issue has been fixed, closing in favour of #4375