#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)
Change History (16)
Changed 7 years ago by
Attachment: | enctest.py added |
---|
Changed 7 years ago by
Attachment: | padding.diff added |
---|
Changed 7 years ago by
Attachment: | pkcs7-prepare.patch added |
---|
Changed 7 years ago by
Attachment: | pkcs7-change-pad-char.patch added |
---|
splitting the patch into two: this changes the padding char
comment:1 follow-up: 2 Changed 7 years ago by
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 Changed 7 years ago by
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 7 years ago by
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 7 years ago by
Owner: | changed from Josh to alas |
---|
comment:5 Changed 7 years ago by
Owner: | changed from alas to Antoine Martin |
---|---|
Status: | new → assigned |
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 7 years ago by
Owner: | changed from Antoine Martin to Josh |
---|---|
Status: | assigned → new |
@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 theencryption-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 7 years ago by
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 7 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
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 7 years ago by
The password authentication was well documented already: wiki/Clients/HTML5.
comment:10 Changed 7 years ago by
comment:12 Changed 17 months ago by
this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/933
splitting the patch into two: this prepares the code but does not actually change the padding