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.
splitting the patch into two: this prepares the code but does not actually change the padding
splitting the patch into two: this changes the padding char
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)
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?
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.
Okay, cool. These are merged in r10182 and r10183 respectively.
This breaks backwards compatibility, we cannot release 0.16.0 without:
(taking the ticket back)
@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:
password-file
option, but also the encryption-keyfile
option
$ 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..
@antoine: this looks good.
I added the new key to the HTML5 client in r10357 and seems to be working as expected.
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.
The password authentication was well documented already: wiki/Clients/HTML5.
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
Note: XPRA_ENCRYPT_FIRST_PACKET
is a bad idea, see #951
this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/933