Xpra: Ticket #933: use PKCS#7 padding for AES-CBC encryption

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.



Thu, 30 Jul 2015 00:30:09 GMT - Josh: attachment set


Thu, 30 Jul 2015 00:30:34 GMT - Josh: attachment set


Thu, 30 Jul 2015 01:28:15 GMT - Antoine Martin: attachment set

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


Thu, 30 Jul 2015 01:29:17 GMT - Antoine Martin: attachment set

splitting the patch into two: this changes the padding char


Thu, 30 Jul 2015 01:33:12 GMT - Antoine Martin: owner changed

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:

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)


Thu, 30 Jul 2015 21:43:30 GMT - 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?


Fri, 31 Jul 2015 05:28:32 GMT - 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.


Fri, 31 Jul 2015 21:08:48 GMT - Josh: owner changed

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


Wed, 19 Aug 2015 05:37:09 GMT - Antoine Martin: owner, status changed

This breaks backwards compatibility, we cannot release 0.16.0 without:

(taking the ticket back)


Wed, 19 Aug 2015 09:49:29 GMT - Antoine Martin: owner, status changed

@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:

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


Wed, 19 Aug 2015 17:23:40 GMT - Josh: owner changed

@antoine: this looks good.

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


Wed, 14 Oct 2015 22:48:10 GMT - alas: status changed; resolution set

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:

[tlaloc@jimador ~]$ 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:

[tlaloc@jimador ~]$ 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.


Thu, 15 Oct 2015 05:43:01 GMT - Antoine Martin:

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


Thu, 15 Oct 2015 12:37:56 GMT - 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


Mon, 09 Nov 2015 17:18:40 GMT - Antoine Martin:

Note: XPRA_ENCRYPT_FIRST_PACKET is a bad idea, see #951


Sat, 23 Jan 2021 05:10:10 GMT - migration script:

this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/933