Closed bk138 closed 1 year ago
server terminates afterwards 💣 ...
but also, in another run:
works OK ✔️
https://github.com/LibVNC/libvncserver/issues/493 is related...
I also found this bug of UltraVNC when implementing MSLogonII for noVNC and TigerVNC. It seems if the username/password is long enough or is not null terminalted, the UltraVNC server will crash. If this happens, the encryption algorithm is likely to be incorrect so the server gets the wrong decrypteed plaintext (which may be long or not null terminated).
@pdlan were you able to pinpoint the exact cause so we can find some kinda fix/workaround? Is it:
I just tested it. When the username length is > 179 the server crashes. The password length doesn't matter.
I had the problem with "usual" usernames as well. What about the 0-termination?
The case without null termination should be similar to long usernames because the length can be as long as 256 (if there isn't a zero by accident). If your username is very short (e.g. crashes even if the length is 8), I guess it's mostly likely to be because you don't handle DES-CBC correctly, i.e., all blocks but the first one are corrupted.
The case without null termination should be similar to long usernames because the length can be as long as 256 (if there isn't a zero by accident). If your username is very short (e.g. crashes even if the length is 8), I guess it's mostly likely to be because you don't handle DES-CBC correctly, i.e., all blocks but the first one are corrupted.
Yeah true.
So, I ran UltraVNC under Visual Studio debugger and found that it is throwing an exception during DH key exchange. The exception is not handled, leading to this crash.
Exception source: https://github.com/ultravnc/UltraVNC/blob/1586ec05577d7b82eb19cac5746cf02d6823df5b/rfb/dh.cpp#L136 Called from: https://github.com/ultravnc/UltraVNC/blob/1586ec05577d7b82eb19cac5746cf02d6823df5b/winvnc/winvnc/vncclient.cpp#L1718
After looking around a bit, it seems that UltraVNC limits DH key to 31bits, and when LibVNCClient sends 64bit public key, it throws an exception.
BTW, AVNC with wolfSSL has same issue, so it doesn't seem to related to OpenSSL specifically.
I can't explain why it works in some cases for you guys. For me, It always crashes, even for 4-5 character username.
UltraVNC version: 1.4.0.6 from GitHub
Oh, it seems that you guys might find a different bug from what I found. The long username bug occurs even with UltraVNC client + server.
BTW I don't think the length limit is a issue because the UltraVNC server always sends a prime < maxNum, so no matter how big the client's private key is, the public key sent by it should always be < prime < maxNum as long as the implementation is correct.
The issue seems to be incorrect handling of buffers used for holding DH keys crypto_openssl.c.
dh_generate_keypair()
& dh_compute_shared_key()
are given 8-byte buffers to fill with DH keys. But if a key is smaller, e.g. here it is only 4-byte long, it will be stored in starting bytes of the buffer. Later, when that buffer is treated as full 8-byte key, it results in incorrect calculations.
Now, LibVNCClient already uses BN_bn2binpad
which automatically pads the buffer with zeros, but it is not available in older OpenSSL/LibreSSL version. LibVNCClient will fall back to BN_bn2bin
which doesn't add zeros. This is probably why SDLvncviewer is working, as it might have been compiled with OpenSSL 1.1.1.
If I manually pad the buffer with this quick patch, I can connect successfully.
If I manually pad the buffer with this quick patch, I can connect successfully.
Great work @gujjwal00! (as always) Can you whip up a PR for libvncclient?
Sure 👍
If you'd like to put out an incentive for fixing this bug, you can do so at https://issuehunt.io/r/bk138/multivnc?tab=idle
Is your bug report about the Desktop Multivnc or the Mobile MultiVNC?
Which MultiVNC version are you using?
2.0.9
Which server are you connecting to?
Describe the bug
Crashes UltraVNC server that has MSLogonII enabled
To Reproduce
Expected Behavior
server does not crash ;-)
Screenshots
For the Desktop Version (please complete the following information):
For the Mobile Version (please complete the following information):
Additional context