use PKCS#7 padding for AES-CBC encryption

[Josh]

It seems that zero-byte padding causes issues with some crypto libraries, namely every JavaScript? library available that does AES-CBC mode. Some of the data is decrypted correctly and then truncated or corrupted.

It would be better to implement PKCS7 padding where the value of the pad byte is the number of bytes being added (​https://en.wikipedia.org/wiki/Padding_%28cryptography%29#PKCS7).

The script enctest.py tests the current Xpra padding and PKCS7. If you copy the details into here ​https://jswebcrypto.azurewebsites.net/demo.html#/aes for example, every implementation gets the Xpra padded data wrong.

Attached is also a quick and dirty patch to implement this padding. It seems that it won't break compatibility with older version since the padding is the same length and stripped off anyway, but I have not tested this yet.

[Antoine Martin]

splitting the patch into two: this prepares the code but does not actually change the padding

[Antoine Martin]

splitting the patch into two: this changes the padding char

[Antoine Martin]

There was at least one typo in the padding patch: data = data[:-padding] refers to a non-existent variable.

I've split it into two:

• the prepare part makes sense on its own, makes the code cleaner
• the second one only changes the padding byte

Does that still work for you? (not had time to test yet)
(btw, to be pedantic: at the moment it's not "zero byte padding", it's "space byte padding" 0x20... which is non standard)

[Josh]

I must have read zero byte padding on another ticket and got it stuck in my head.

The second patch doesn't apply clean for some reason - the first hunk is rejected - but the changes work as expected (thanks for spotting that typo).

Should I commit these?

[Antoine Martin]

Should I commit these?

Please do, then re-assign to 'afarr' for testing.
We should verify that encryption still works when mixing older client and newer servers and vice versa.

[Josh]

Okay, cool. These are merged in r10182 and r10183 respectively.

[Antoine Martin]

This breaks backwards compatibility, we cannot release 0.16.0 without:

• adding a new capability to the hello packet to specify the padding so we still default to the "older" space padding, see also #951
• backporting the changes?

(taking the ticket back)

[Antoine Martin]

@joshiggins: please read the commit message in r10351, does that make sense to you?

This preserves backwards compatibility with previous versions of the client and servers by making them specify what they can / want to use as padding.

You should be able to specify the new key: "cipher.padding.options" = ["PKCS#7"] to force newer servers to select this padding mode.

The only corner case that we cannot handle this way is the initial packet, which is currently unencrypted (see #951): for this one, you would need to run the server with XPRA_CRYPTO_INITIAL_PADDING="PKCS#7" XPRA_ENCRYPT_FIRST_PACKET="1" xpra start ....

If that's OK with you, please re-assign to afarr for testing.
This will need quite thorough testing:

• html client of course
• mixing 0.14, 0.15 and trunk clients and servers
• different encryption options: using the legacy password-file option, but also the encryption-keyfile option
• verifying that we use the preferred padding when possible, and the legacy one when not, ie:

  $ xpra info | grep cipher
  client.connection.input.cipher=AES
  client.connection.input.cipher.padding=PKCS#7
  client.connection.output.cipher=AES
  client.connection.output.cipher.padding=PKCS#7
  

As for backporting most of this, I can't see that happen..

[Josh]

@antoine: this looks good.

I added the new key to the HTML5 client in r10357 and seems to be working as expected.

[alas]

Trying to test... I can't seem to guess/hack the syntax to connect via a browser to a server session set up with a password-file and encryption.

What's the secret handshake?

Launched server with a variation on my usual:

[[email protected] ~]$ XPRA_CRYPTO_INITIAL_PADDING="PKCS#7" XPRA_ENCRYPT_FIRST_PACKET="1"  dbus-launch xpra start :13 --no-daemon --bind-tcp=0.0.0.0:2200 --start-new-commands=yes --start-child=xterm --start-child=xterm --html=on --password-file=key --encryption=AES

Then tried to connect by going to the server vm address (http://10.0.32.136:2200) much as I would connect a typical client... which brings me to a sign in window that has no provision for entering a password, or specifying encryption/no-encryption.

After a few wacky tries, I figured I could maybe pass the password file as a cookie:

http://10.0.32.136:2200/connect.html?password-file=/Users/Schadenfreude/Desktop/xpra-catalog/xpra-ant-16-10828/Xpra.app/Contents/MacOS/key&encryption=AES

I get nowhere with that attempt, and see several connection timeout/lost messages on server side.

At the risk of exposing my extremely secure password, I went ahead and tried to pass it through in plain text as a cookie:

http://10.0.32.136:2200/connect.html?password=bob&encryption=AES

Same errors & failures.

When I try to enter the server address and port number into the xpra connection page (hoping for a second page to enter password and specify encryption) I fail to connect and fall back to the address http://10.0.32.136:2200/connect.html?disconnect=No%20password%20specified%20for%20authentication%20challenge

...

Well, checked with aradtech, and realized that I needed to use the new (to me) --encryption-keyfile=stuff parameter as well as my old friend --password-file=otherstuff parameter.

So... launching the server with another variation on my usual:

[[email protected] ~]$ XPRA_CRYPTO_INITIAL_PADDING="PKCS#7" XPRA_ENCRYPT_FIRST_PACKET="1"  dbus-launch xpra start :13 --no-daemon --bind-tcp=0.0.0.0:2200 --start-new-commands=yes --start-child=xterm --start-child=xterm --html=on --password-file=password --encryption-keyfile=key --encryption=AES

And then pointing my browser at the url: http://10.0.32.136:2200/?encryption=AES&password=bob&key=bob&encoding=h264&/ I was able to connect as expected... and the server output indicates that the session is indeed using AES.

I'll leave the trail of my failures here in case anyone else wants some tips on what not to try. Everyone should also be sure to append that final & to the address, or else the server seems to grab the final / of the url and interpret it as part of the specified encryption, failing with a message that there is no common encoding to use (2015-10-14 15:35:22,487 cannot find compatible encoding between client (h264/) and server (rgb, h264, vp9, vp8, png, png/L, png/P, jpeg, webp)).

It does feel awkward to pass my password and key in plain text as a cookie in the address bar... but that seems to be expected behavior (?).

That said... I'll go ahead and close this.

[Antoine Martin]

The password authentication was well documented already: wiki/Clients/HTML5.

[Josh]

The trailing / problem should be fixed in r10847.

It does feel awkward to pass my password and key in plain text as a cookie in the address bar... but that seems to be expected behavior (?).

see #1005

[Antoine Martin]

Note: XPRA_ENCRYPT_FIRST_PACKET is a bad idea, see #951