xpra icon
Bug tracker and wiki

Opened 20 months ago

Closed 18 months ago

Last modified 17 months ago

#933 closed defect (fixed)

use PKCS#7 padding for AES-CBC encryption

Reported by: Josh Owned by: alas
Priority: major Milestone: 0.16
Component: network Version: 0.15.x
Keywords: Cc:

Description

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.

Attachments (4)

enctest.py (726 bytes) - added by Josh 20 months ago.
padding.diff (5.3 KB) - added by Josh 20 months ago.
pkcs7-prepare.patch (4.5 KB) - added by Antoine Martin 20 months ago.
splitting the patch into two: this prepares the code but does not actually change the padding
pkcs7-change-pad-char.patch (2.2 KB) - added by Antoine Martin 20 months ago.
splitting the patch into two: this changes the padding char

Download all attachments as: .zip

Change History (15)

Changed 20 months ago by Josh

Attachment: enctest.py added

Changed 20 months ago by Josh

Attachment: padding.diff added

Changed 20 months ago by Antoine Martin

Attachment: pkcs7-prepare.patch added

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

Changed 20 months ago by Antoine Martin

Attachment: pkcs7-change-pad-char.patch added

splitting the patch into two: this changes the padding char

comment:1 Changed 20 months ago by Antoine Martin

Owner: changed from Antoine Martin to Josh

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)

comment:2 in reply to:  1 Changed 20 months ago by 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?

comment:3 Changed 20 months ago by 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.

comment:4 Changed 20 months ago by Josh

Owner: changed from Josh to alas

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

comment:5 Changed 19 months ago by Antoine Martin

Owner: changed from alas to Antoine Martin
Status: newassigned

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)

comment:6 Changed 19 months ago by Antoine Martin

Owner: changed from Antoine Martin to Josh
Status: assignednew

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

comment:7 Changed 19 months ago by Josh

Owner: changed from Josh to alas

@antoine: this looks good.

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

comment:8 Changed 18 months ago by alas

Resolution: fixed
Status: newclosed

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.

comment:9 Changed 18 months ago by Antoine Martin

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

comment:10 Changed 18 months ago by 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

comment:11 Changed 17 months ago by Antoine Martin

Note: XPRA_ENCRYPT_FIRST_PACKET is a bad idea, see #951

Note: See TracTickets for help on using tickets.