X509 BufferOverflowException in SSLEngineManager.write

[kisol-pw]

Encountered this one today:

• turbovnc 2.2.6 or current dev build
• openjdk version "11.0.11-ea"
• create a quick test CA with CA.pl in Debian sid
• remove passwort from the newkey.pem
• vncserver :21 -x509cert newcert.pem -x509key newkey2.pem -SecurityTypes x509none
• vncviewer -X509CA cacert.pem -SecurityTypes x509none localhost:21

java.nio.BufferOverflowException at java.base/java.nio.HeapByteBuffer.put(HeapByteBuffer.java:221) at com.turbovnc.network.SSLEngineManager.write(SSLEngineManager.java:185) at com.turbovnc.rdr.TLSOutStream.writeTLS(TLSOutStream.java:76) at com.turbovnc.rdr.TLSOutStream.flush(TLSOutStream.java:51) at com.turbovnc.rfb.CMsgWriterV3.endMsg(CMsgWriterV3.java:41) at com.turbovnc.rfb.CMsgWriterV3.writeClientInit(CMsgWriterV3.java:33) at com.turbovnc.rfb.CConnection.securityCompleted(CConnection.java:268) at com.turbovnc.rfb.CConnection.processSecurityResultMsg(CConnection.java:230) at com.turbovnc.rfb.CConnection.processMsg(CConnection.java:62) at com.turbovnc.vncviewer.VncViewer.run(VncViewer.java:851) at java.base/java.lang.Thread.run(Thread.java:829)

I threw in a System.out at the start of write: SSLWRITE 16676 src 0 length 1 cap 16704 rem 0 lim 16704

myAppData seems full before the first write of 1 byte. The only other occurence of myAppData is in doHandshake in the same file. There is a myAppData.compact() in there but I couldn't figure out what would happen once it reaches the Buffer limit, for some reason.

So I thought "maybe myAppData should be empty anyway after the handshake concludes" and added myAppData.clear(); at the very end of doHandshake.

Well, it connects now. No Idea if my conclusion is correct though, I'm kinda lost how that wrapping stuff is supposed to work.

The odd thing: I remember x509 working in 2019 at least, what broke it? Did the handshake procedure in SSLEngine expand? Or maybe keylength...

[dcommander]

I'll look into it. X.509 still works fine for me, so I'm not sure what's going on here.

[dcommander]

In the future, I would greatly appreciate it if you would provide details rather than glossing over important steps like "create a quick test CA with CA.pl in Debian sid". How specifically? Which arguments did you pass to that script? Taking a minute to provide those details often saves me an hour of work figuring them out. Thanks.

[dcommander]

This seems to be a product of the negotiated cipher suite. When connecting to a host that has OpenSSL v1.1, the first available cipher suite will be TLS_AES_256_GCM_SHA384, which will be used if the viewer supports it. Unfortunately, that cipher suite exhibits this issue. As a temporary workaround until I can figure out why, you can force-enable another cipher suite in the viewer by setting, for instance, JAVA_TOOL_OPTIONS='-Dturbovnc.ciphersuites=TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384' in the environment.

[kisol-pw]

Hi, thx for the workaround. That explains alot.

create a quick test CA with CA.pl in Debian sid

ya, it's literally just that... I figured the most important sub-information was "not messing with default debian sid openssl.cnf"

mkdir moo
cd moo
ln -s /usr/lib/ssl/misc/CA.pl
./CA.pl -newca
<enter password enter enter enter enter>
./CA.pl -newreq
<password enter enter enter enter localhost enter...>
./CA.pl -sign
<password enter y enter>
openssl rsa -in newkey.pem -out newkey2.key
<password enter>

== newcert.pem newkey2.key demoCA/cacert.pem

[dcommander]

@bphinz This same issue appears to have been fixed in TigerVNC with https://github.com/TigerVNC/tigervnc/commit/b40235af946e6854e7fcf1f8f7a00c165e71008e, but it's unclear why, since that commit claims that most of the changes to SSLEngineManager.java were cosmetic. Can you advise? The changes in question do not appear to be merely cosmetic to me.

[bphinz]

At the time I wasn't trying to fix a buffer overflow in the SSLEngine, I was fixing issues that primarily stemmed from trying to keep track of buffer pointers using byte arrays and ints by replacing them with ByteBuffers. Since SSLEngine needed to be re-plumbed to accommodate those changes, I tried to make it more robust. I'm sure that the relevant change is moving the buffer compact from the BUFFER_OVERFLOW case to the OK case of the switch statement (continuously freeing up space).

[dcommander]

@bphinz Unfortunately that is insufficient to fix the issue. I'm looking for a simple fix for this, not an overhaul, because the code is well-tested and has proven robust outside of this failing corner case, which is exposed only because OpenSSL v1.1 chooses a different default cipher.

Easiest way to repro it, on a Linux machine with OpenSSL v1.1:

cd ~
mkdir turbovnc_284
cd turbovnc_284
git clone https://gist.github.com/dcommander/fc608434735026dd8215 certs
cd certs
./gencert.cn
cd ..
git clone https://github.com/TurboVNC/turbovnc.git
cd turbovnc
mkdir build
cd build
cmake ..
make
bin/vncserver -x509cert ../../certs/server.crt -x509key ../../certs/server.key -securitytypes x509none
bin/vncviewer -securitytypes x509none -x509ca ../../certs/ca_server.crt `hostname`:1

[bphinz]

Agreed, I wasn't suggesting the complete overhaul, just that you mimic the changes inside the write() method. I'm assuming that's what you mean when you say it's insufficient?

[dcommander]

Yeah, I tried to mimic your write() method, and it still threw an overflow exception. Maybe I missed something, given that we're still using the old version of your code that doesn't take ByteBuffer arguments.

[bphinz]

There are similar changes in the doHandshake method that you probably need to make. I don't have access to a machine with GnuTLS or openssl that supports TLSv1.3 at the moment to test. I'll try later when I get home

[dcommander]

@bphinz Unfortunately I am still out of luck. Everything I've tried (including an attempt at porting your changes to SSLEngineManager without porting your changes to the underlying *Stream classes) either fixed the existing failures while causing new failures or caused the whole thing to lock up. I really wish I had a better understanding of the data flow in this class.

[dcommander]

Blerg. OK, I'm an idiot. I had forgotten that OpenSSL v1.1 doesn't support some of the ECDSA ciphers that OpenSSL v1.0 supports, so I was misinterpreting expected failures as unexpected failures. Upon retesting, I can confirm that the following one-line fix works around the issue:

--- a/java/com/turbovnc/network/SSLEngineManager.java
+++ b/java/com/turbovnc/network/SSLEngineManager.java
@@ -114,7 +114,6 @@ public class SSLEngineManager {
           // Check status
           switch (res.getStatus()) {
             case OK:
-              myAppData.compact();
               ((Buffer)myNetData).flip();
               os.writeBytes(myNetData.array(), 0, myNetData.remaining());
               os.flush();

I will push it shortly.