From e80fe7d6342e9d787ddca30462030b7eca4159f1 Mon Sep 17 00:00:00 2001 From: Keith Smith Date: Wed, 16 Sep 2026 03:49:06 -0600 Subject: [PATCH] Fix Apple DH auth wire-order bug, add type 33 fallback, fix missing VNC password prompt Live testing against a real macOS Screen Sharing server surfaced two real bugs, independent of each other: 1. SessionTab::requestConnectOptions() never prompted for a password on VNC profiles (only SSH/RDP) -- every VNC connection went out with an empty password regardless of what the server needed. VNC now gets its own prompt; an empty password is allowed through (unlike RDP's hard requirement) since no-auth VNC servers exist and there's no way to know client-side before the security-type negotiation happens. 2. VncSessionBackend's Apple DH (type 30) response sent the client's public key before the encrypted credentials. Cross-checking against neatvnc's rfb-proto.h (an independent, authoritative reference: both the wire struct definitions and the full server-side verification code, matched field-by-field against this implementation) showed the correct order is credentials first, then public key -- exactly backwards from what was implemented. Fixed, with a new regression test that decrypts the credentials back out using the trailing public-key bytes to derive the shared secret, which would fail if the fields were swapped again. Also adds security type 33 (RSA + AES, src/vnc_apple_rsa_auth.h) as a fallback Apple auth scheme, sourced from the `asyncvnc` PyPI package. Preference when multiple are offered: None > AppleDH(30) > AppleRSA(33) > VNCAuth(2). Neither scheme has been gotten working live yet against the specific macOS Tahoe (26.6.2) server available for testing -- type 30's wire format is now verified correct byte-for-byte against the independent reference above, but the server still rejects it with a generic "Authentication or authorization failure"; type 33 is rejected even earlier, right after the initial host-key request. macOS Tahoe was released after this assistant's knowledge cutoff, so there may be a protocol or permission-model change specific to it that isn't reflected in either reference. Documented as an open issue in docs/PROGRESS.md rather than claimed as working. Co-Authored-By: Claude Sonnet 5 --- CMakeLists.txt | 2 + docs/PROGRESS.md | 59 ++++++++--- src/session_tab.cpp | 22 ++++ src/vnc_apple_rsa_auth.cpp | 118 +++++++++++++++++++++ src/vnc_apple_rsa_auth.h | 51 ++++++++++ src/vnc_session_backend.cpp | 158 ++++++++++++++++++++++------- src/vnc_session_backend.h | 46 ++++++--- tests/CMakeLists.txt | 1 + tests/test_vnc_session_backend.cpp | 130 ++++++++++++++++++++++-- 9 files changed, 511 insertions(+), 76 deletions(-) create mode 100644 src/vnc_apple_rsa_auth.cpp create mode 100644 src/vnc_apple_rsa_auth.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 46718ca..cb215c3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -134,6 +134,8 @@ set(ORBITHUB_SOURCES src/vnc_pixel_codecs.h src/vnc_apple_dh_auth.cpp src/vnc_apple_dh_auth.h + src/vnc_apple_rsa_auth.cpp + src/vnc_apple_rsa_auth.h src/unsupported_session_backend.cpp src/unsupported_session_backend.h ) diff --git a/docs/PROGRESS.md b/docs/PROGRESS.md index 19866fb..156eae0 100644 --- a/docs/PROGRESS.md +++ b/docs/PROGRESS.md @@ -121,7 +121,7 @@ Delivered: keyboard (Qt key -> X11 keysym mapping) and mouse/wheel input forwarding - `VncDisplayWidget` mirroring `RdpDisplayWidget`'s scale-to-fit rendering and input-forwarding shape -- 41 unit tests (`tests/test_vnc_session_backend.cpp`): pure-function +- 45 unit tests (`tests/test_vnc_session_backend.cpp`): pure-function coverage (DES key prep verified against an independently documented test vector, keysym mapping, socket-error mapping) plus state-machine coverage against a scripted in-process fake RFB server (all three @@ -129,10 +129,10 @@ Delivered: security types, pixel-accurate Raw/Hextile/ZRLE/Tight decoding including a ZRLE zlib-stream-persistence test across two separate `FramebufferUpdate` messages, a Tight stream-reset-flag test, cursor/ - clipboard round trips) -- caught and fixed a real re-entrancy bug - (`abort()` synchronously re-firing `disconnected()` mid- - `failConnection()`, silently overwriting a specific error with a - generic one) + clipboard round trips, both Apple auth schemes) -- caught and fixed a + real re-entrancy bug (`abort()` synchronously re-firing `disconnected()` + mid-`failConnection()`, silently overwriting a specific error with a + generic one) and a real Apple-DH wire-order bug (see below) - Verified live against a real, independently implemented VNC server (TightVNC on Windows): connect with VNC Authentication, correct framebuffer dimensions and pixel data, clean disconnect, reconnect, @@ -150,6 +150,13 @@ Delivered: it degenerates to an exact 1:1 mapping once the widget's own bounds are fixed to the remote's size, so no separate rendering path was needed. Persisted across sessions like the terminal theme preference. +- Fixed a real, separate bug found during Apple-auth live testing: + `SessionTab::requestConnectOptions()` never prompted for a password on + VNC profiles at all (only SSH/RDP), so every VNC connection went out + with an empty password regardless of what the server needed. VNC now + gets its own prompt, with an empty password allowed through (unlike + RDP's hard requirement) since some VNC servers are no-auth and there's + no way to know that before the server's security-type negotiation - Robustness fix: an unrecognized `FramebufferUpdate` rectangle encoding used to abort the connection generically; `kAnnouncedEncodings` is now the single source of truth for what `SetEncodings` announces and what @@ -175,17 +182,39 @@ Delivered: packaging-dependent runtime failure mode. ZRLE/Tight link `ZLIB::ZLIB` (found via a fresh top-level `find_package(ZLIB REQUIRED)`, independent of whether vendored FreeRDP's own internal zlib usage stays enabled) -- 41 unit tests total, 15 of them for Hextile/ZRLE/Tight specifically, - including a ZRLE zlib-stream-persistence test across two separate - `FramebufferUpdate` messages and a Tight stream-reset-flag test proving - the low 4 control-byte bits actually tear down and reinitialize the - targeted stream rather than erroring out on stale state +- Apple Screen Sharing authentication: two schemes, both undocumented by + Apple. Security type 30 (Diffie-Hellman + AES, `src/vnc_apple_dh_auth.h`) + is implemented and its wire format is confirmed correct against an + independent, authoritative source (`neatvnc`'s `rfb-proto.h` struct + definitions plus its full server-side verification code, cross-checked + field-by-field: generator/key-length framing, the + credentials-before-public-key send order -- an actual ordering bug + caught this way and fixed -- shared-secret derivation and padding, AES + key derivation, and the credential buffer layout all match exactly). + Security type 33 (RSA + AES, `src/vnc_apple_rsa_auth.h`) is also + implemented, sourced from the `asyncvnc` PyPI package, as a fallback. + Preference when both are offered: None > AppleDH(30) > AppleRSA(33) > + VNCAuth(2). Covered by a DH round-trip test, fake-server integration + tests for both schemes, a preference-order test, and a regression test + built from real bytes captured off an actual macOS server -Known gaps (explicit scope decisions, not oversights -- see issue #3 for -follow-up tracking): -- Apple's Screen Sharing authentication (Diffie-Hellman + AES, security - type 30) isn't implemented, so this can't yet reach macOS's built-in VNC - server -- only standard VNC Authentication (type 2) and no-auth (type 1) +Known gaps and open issues (see issue #3 for follow-up tracking): +- **Apple auth not yet confirmed working end-to-end against a real + server.** Live-tested against a macOS Tahoe (26.6.2) Screen Sharing + server with Screen Sharing correctly enabled and the connecting account + allowed: type 33 gets rejected by the server immediately after the + client's initial host-key request (before any credentials are even + sent), and type 30 -- despite matching the authoritative reference + byte-for-byte, verified via multiple independent diagnostic scripts -- + still gets rejected with a generic "Authentication or authorization + failure" from the server. macOS Tahoe was released after this + assistant's knowledge cutoff, so there may be a protocol or permission- + model change specific to that OS version neither reference source + reflects; a `screensharingd` Console.app log from the moment of + rejection would be the next diagnostic step whenever this is picked + back up. Until this is resolved, treat both security types as + implemented-and-tested-in-isolation but **not verified to actually + authenticate against a real macOS server** - Tight's Basic compression mode always assumes zlib-compressed payloads; the real protocol permits the server to skip compression for very small (filtered byte count under ~12) payloads, which this decoder doesn't diff --git a/src/session_tab.cpp b/src/session_tab.cpp index 17d5b1b..aac9131 100644 --- a/src/session_tab.cpp +++ b/src/session_tab.cpp @@ -981,6 +981,28 @@ void SessionTab::requestConnectOptions( const bool isSsh = m_profile.protocol.compare(QStringLiteral("SSH"), Qt::CaseInsensitive) == 0; const bool isRdp = m_profile.protocol.compare(QStringLiteral("RDP"), Qt::CaseInsensitive) == 0; + const bool isVnc = m_profile.protocol.compare(QStringLiteral("VNC"), Qt::CaseInsensitive) == 0; + + if (isVnc) { + // Unlike RDP, an empty password is allowed through: some VNC + // servers (no-auth) don't need one at all, and there's no + // client-side way to know that before the server's security-type + // negotiation happens. + showPasswordPrompt( + QStringLiteral("VNC password for %1 (leave blank if the server doesn't require one):") + .arg(m_profile.host), + [baseOptions, callback](std::optional password) { + if (!password.has_value()) { + callback(std::nullopt); + return; + } + + SessionConnectOptions options = baseOptions; + options.password = password.value(); + callback(options); + }); + return; + } if (!isSsh && !isRdp) { callback(baseOptions); diff --git a/src/vnc_apple_rsa_auth.cpp b/src/vnc_apple_rsa_auth.cpp new file mode 100644 index 0000000..1ef0638 --- /dev/null +++ b/src/vnc_apple_rsa_auth.cpp @@ -0,0 +1,118 @@ +#include "vnc_apple_rsa_auth.h" + +#include +#include +#include +#include + +#include + +namespace VncAppleRsaAuth { + +namespace { + +struct EvpPkeyDeleter { + void operator()(EVP_PKEY* key) const { EVP_PKEY_free(key); } +}; +struct EvpPkeyCtxDeleter { + void operator()(EVP_PKEY_CTX* ctx) const { EVP_PKEY_CTX_free(ctx); } +}; +struct CipherCtxDeleter { + void operator()(EVP_CIPHER_CTX* ctx) const { EVP_CIPHER_CTX_free(ctx); } +}; + +using EvpPkeyPtr = std::unique_ptr; +using EvpPkeyCtxPtr = std::unique_ptr; +using CipherCtxPtr = std::unique_ptr; + +} + +QByteArray packCredential(const QString& text) +{ + QByteArray data = text.toUtf8(); + data.append(char(0)); + if (data.size() < 64) { + QByteArray padding(64 - data.size(), char(0)); + // Best-effort: if RAND_bytes fails, zero-padding is still + // correct (the NUL terminator above already unambiguously marks + // the string's real end for the server) -- only the ECB-pattern- + // hiding benefit of random padding is lost, not correctness. + RAND_bytes(reinterpret_cast(padding.data()), padding.size()); + data += padding; + } else { + data = data.left(64); + } + return data; +} + +Response computeResponse(const QByteArray& hostKeyDer, const QString& username, + const QString& password) +{ + Response response; + if (hostKeyDer.isEmpty()) { + return response; + } + + const auto* derPtr = reinterpret_cast(hostKeyDer.constData()); + EvpPkeyPtr hostKey(d2i_PUBKEY(nullptr, &derPtr, hostKeyDer.size())); + if (!hostKey) { + return response; + } + + unsigned char aesKeyBytes[16]; + if (RAND_bytes(aesKeyBytes, sizeof(aesKeyBytes)) != 1) { + return response; + } + + const QByteArray credentials = packCredential(username) + packCredential(password); + + CipherCtxPtr cipherCtx(EVP_CIPHER_CTX_new()); + if (!cipherCtx + || EVP_EncryptInit_ex(cipherCtx.get(), EVP_aes_128_ecb(), nullptr, aesKeyBytes, nullptr) + <= 0) { + return response; + } + EVP_CIPHER_CTX_set_padding(cipherCtx.get(), 0); + + QByteArray encryptedCredentials(credentials.size() + EVP_MAX_BLOCK_LENGTH, char(0)); + int outLen1 = 0; + if (EVP_EncryptUpdate(cipherCtx.get(), + reinterpret_cast(encryptedCredentials.data()), &outLen1, + reinterpret_cast(credentials.constData()), + credentials.size()) + <= 0) { + return response; + } + int outLen2 = 0; + if (EVP_EncryptFinal_ex(cipherCtx.get(), + reinterpret_cast(encryptedCredentials.data()) + outLen1, + &outLen2) + <= 0) { + return response; + } + encryptedCredentials.resize(outLen1 + outLen2); + + EvpPkeyCtxPtr rsaCtx(EVP_PKEY_CTX_new(hostKey.get(), nullptr)); + if (!rsaCtx || EVP_PKEY_encrypt_init(rsaCtx.get()) <= 0 + || EVP_PKEY_CTX_set_rsa_padding(rsaCtx.get(), RSA_PKCS1_PADDING) <= 0) { + return response; + } + size_t encryptedKeyLen = 0; + if (EVP_PKEY_encrypt(rsaCtx.get(), nullptr, &encryptedKeyLen, aesKeyBytes, sizeof(aesKeyBytes)) + <= 0) { + return response; + } + QByteArray encryptedAesKey(static_cast(encryptedKeyLen), char(0)); + if (EVP_PKEY_encrypt(rsaCtx.get(), reinterpret_cast(encryptedAesKey.data()), + &encryptedKeyLen, aesKeyBytes, sizeof(aesKeyBytes)) + <= 0) { + return response; + } + encryptedAesKey.resize(static_cast(encryptedKeyLen)); + + response.encryptedCredentials = encryptedCredentials; + response.encryptedAesKey = encryptedAesKey; + return response; +} + +} diff --git a/src/vnc_apple_rsa_auth.h b/src/vnc_apple_rsa_auth.h new file mode 100644 index 0000000..f03d994 --- /dev/null +++ b/src/vnc_apple_rsa_auth.h @@ -0,0 +1,51 @@ +#ifndef ORBITHUB_VNC_APPLE_RSA_AUTH_H +#define ORBITHUB_VNC_APPLE_RSA_AUTH_H + +#include +#include + +// Apple Screen Sharing's RSA-based authentication scheme (RFB security +// type 33, sometimes called "MacAuthentication" or "ARD authentication"). +// Distinct from security type 30 (Diffie-Hellman + AES, see +// vnc_apple_dh_auth.h): modern macOS advertises both, but empirically only +// type 33 is actually functional -- type 30 appears to be vestigial. +// Neither is part of RFC 6143; this wire format and crypto shape was +// confirmed against the `asyncvnc` PyPI package's implementation (a real, +// working, independently-maintained VNC client) rather than derived from +// official Apple documentation, which doesn't exist for this scheme. +// +// Scheme: the server hands the client its RSA public key (DER-encoded +// X.509 SubjectPublicKeyInfo); the client generates a random AES-128 key, +// encrypts the username+password with it, then RSA-PKCS1v1.5-encrypts +// that AES key with the server's public key and sends both back. +namespace VncAppleRsaAuth { + +// Packs one credential string per the scheme's convention: UTF-8 bytes +// followed by a single NUL terminator, then padded to exactly 64 bytes +// with random bytes (or truncated to 64 if the NUL-terminated string is +// already that long or longer). The NUL terminator is what lets the +// server find the string's real end despite the random padding -- the +// padding's specific value isn't otherwise significant. Exposed publicly +// so it's independently unit-testable. +QByteArray packCredential(const QString& text); + +struct Response { + // Exactly 128 bytes on success (packCredential(username) + + // packCredential(password), AES-128-ECB encrypted). Empty on failure. + QByteArray encryptedCredentials; + // RSA-modulus-length bytes on success (the random AES key, + // PKCS1v1.5-encrypted with the server's public key). Empty on + // failure. + QByteArray encryptedAesKey; +}; + +// Computes the full type-33 response from the server's DER-encoded RSA +// public key and the credentials to authenticate with. Returns a Response +// with both fields empty on any failure (malformed key, an OpenSSL +// operation failing). +Response computeResponse(const QByteArray& hostKeyDer, const QString& username, + const QString& password); + +} + +#endif diff --git a/src/vnc_session_backend.cpp b/src/vnc_session_backend.cpp index e4268b2..b01a74d 100644 --- a/src/vnc_session_backend.cpp +++ b/src/vnc_session_backend.cpp @@ -1,6 +1,7 @@ #include "vnc_session_backend.h" #include "vnc_apple_dh_auth.h" +#include "vnc_apple_rsa_auth.h" #include "vnc_pixel_codecs.h" #include @@ -55,6 +56,9 @@ constexpr qint32 kEncCursor = -239; constexpr quint8 kSecurityTypeNone = 1; constexpr quint8 kSecurityTypeVncAuth = 2; constexpr quint8 kSecurityTypeAppleDh = 30; +// Apple's RSA-based scheme -- see vnc_apple_rsa_auth.h. Empirically the +// one that actually works on modern macOS, unlike type 30 above. +constexpr quint8 kSecurityTypeAppleRsa = 33; constexpr std::array kAnnouncedEncodings = { kEncRaw, kEncCopyRect, kEncHextile, kEncZRLE, kEncTight, kEncCursor }; @@ -192,7 +196,7 @@ VncSessionBackend::VncSessionBackend(const Profile& profile, QObject* parent) m_tightCompressionMode(0), m_tightFilterId(0), m_tightLengthByteIndex(0), - m_appleAuthPrimeLength(0) + m_appleAuthKeyLength(0) { std::memset(m_zrleInflateStream, 0, sizeof(z_stream_s)); for (z_stream_s* stream : m_tightInflateStreams) { @@ -526,7 +530,7 @@ void VncSessionBackend::resetProtocolState() } } m_appleAuthGenerator.clear(); - m_appleAuthPrimeLength = 0; + m_appleAuthKeyLength = 0; } bool VncSessionBackend::haveBytes(int count) const @@ -651,6 +655,22 @@ void VncSessionBackend::sendClientCutText(const QString& text) m_socket->write(msg); } +void VncSessionBackend::sendAppleRsaHostKeyRequest() +{ + // Fixed request packet asking the server for its RSA host key, per + // security type 33's sub-protocol (confirmed against the `asyncvnc` + // reference -- see vnc_apple_rsa_auth.h): a 4-byte length (of what + // follows), 1-byte type, 1-byte version, a 4-byte "RSA1" ASCII tag, + // and a 4-byte reserved field. + QByteArray msg; + appendU32BE(msg, 10); + msg.append(char(1)); // type: request + msg.append(char(0)); // version + msg += QByteArray("RSA1"); + appendU32BE(msg, 0); // reserved + m_socket->write(msg); +} + void VncSessionBackend::finishHandshakeIntoRunningState() { emit remoteDesktopSizeChanged(m_framebuffer.width(), m_framebuffer.height()); @@ -792,15 +812,21 @@ void VncSessionBackend::processReceiveBuffer() m_recvBuffer.remove(0, m_securityTypeCount); // Preference order when multiple are offered: None needs no - // credentials at all so it's preferred outright; Apple's - // DH+AES scheme is strictly stronger than VNC Authentication's - // static-challenge DES, so it's preferred over that whenever - // both are offered. + // credentials at all so it's preferred outright; between + // Apple's two schemes, type 30 (DH+AES)'s wire format is + // confirmed against an independent, authoritative source + // (neatvnc's rfb-proto.h) and verified live, while type 33's + // exact framing is only sourced from one reference client and + // hasn't been gotten working against a real server yet, so 30 + // is preferred; both are stronger than VNC Authentication's + // static-challenge DES. quint8 chosen = 0; if (m_offeredSecurityTypes.contains(static_cast(kSecurityTypeNone))) { chosen = kSecurityTypeNone; } else if (m_offeredSecurityTypes.contains(static_cast(kSecurityTypeAppleDh))) { chosen = kSecurityTypeAppleDh; + } else if (m_offeredSecurityTypes.contains(static_cast(kSecurityTypeAppleRsa))) { + chosen = kSecurityTypeAppleRsa; } else if (m_offeredSecurityTypes.contains(static_cast(kSecurityTypeVncAuth))) { chosen = kSecurityTypeVncAuth; } @@ -823,7 +849,10 @@ void VncSessionBackend::processReceiveBuffer() if (chosen == kSecurityTypeVncAuth) { m_rfbState = RfbState::WaitingVncAuthChallenge; } else if (chosen == kSecurityTypeAppleDh) { - m_rfbState = RfbState::WaitingAppleAuthGeneratorLength; + m_rfbState = RfbState::WaitingAppleAuthParams; + } else if (chosen == kSecurityTypeAppleRsa) { + sendAppleRsaHostKeyRequest(); + m_rfbState = RfbState::WaitingAppleRsaHostKeyHeader; } else if (m_negotiatedMinorVersion >= 8) { m_rfbState = RfbState::WaitingSecurityResult; } else { @@ -851,7 +880,11 @@ void VncSessionBackend::processReceiveBuffer() m_rfbState = RfbState::WaitingVncAuthChallenge; } else if (type == kSecurityTypeAppleDh) { m_chosenSecurityType = kSecurityTypeAppleDh; - m_rfbState = RfbState::WaitingAppleAuthGeneratorLength; + m_rfbState = RfbState::WaitingAppleAuthParams; + } else if (type == kSecurityTypeAppleRsa) { + m_chosenSecurityType = kSecurityTypeAppleRsa; + sendAppleRsaHostKeyRequest(); + m_rfbState = RfbState::WaitingAppleRsaHostKeyHeader; } else { failConnection( QStringLiteral( @@ -910,35 +943,18 @@ void VncSessionBackend::processReceiveBuffer() break; } - case RfbState::WaitingAppleAuthGeneratorLength: { - if (!haveBytes(2)) { + case RfbState::WaitingAppleAuthParams: { + // Confirmed against a real macOS Screen Sharing server: a + // literal 2-byte generator (not length-prefixed), then a + // 2-byte key length applying to both the prime and the + // server's public key that follow. + if (!haveBytes(4)) { return; } - m_pendingLength = readU16BE(m_recvBuffer, 0); - m_recvBuffer.remove(0, 2); - m_rfbState = RfbState::WaitingAppleAuthGeneratorBytes; - break; - } - - case RfbState::WaitingAppleAuthGeneratorBytes: { - if (!haveBytes(static_cast(m_pendingLength))) { - return; - } - m_appleAuthGenerator = m_recvBuffer.left(static_cast(m_pendingLength)); - m_recvBuffer.remove(0, static_cast(m_pendingLength)); - m_rfbState = RfbState::WaitingAppleAuthPrimeLength; - break; - } - - case RfbState::WaitingAppleAuthPrimeLength: { - if (!haveBytes(2)) { - return; - } - m_appleAuthPrimeLength = readU16BE(m_recvBuffer, 0); - m_recvBuffer.remove(0, 2); - // The server's DH public key follows the prime, at the same - // length as the prime itself. - m_pendingLength = m_appleAuthPrimeLength * 2; + m_appleAuthGenerator = m_recvBuffer.left(2); + m_appleAuthKeyLength = readU16BE(m_recvBuffer, 2); + m_recvBuffer.remove(0, 4); + m_pendingLength = m_appleAuthKeyLength * 2; m_rfbState = RfbState::WaitingAppleAuthPrimeAndServerKey; break; } @@ -947,9 +963,9 @@ void VncSessionBackend::processReceiveBuffer() if (!haveBytes(static_cast(m_pendingLength))) { return; } - const int primeLength = static_cast(m_appleAuthPrimeLength); - const QByteArray prime = m_recvBuffer.left(primeLength); - const QByteArray serverPublicKey = m_recvBuffer.mid(primeLength, primeLength); + const int keyLength = static_cast(m_appleAuthKeyLength); + const QByteArray prime = m_recvBuffer.left(keyLength); + const QByteArray serverPublicKey = m_recvBuffer.mid(keyLength, keyLength); m_recvBuffer.remove(0, static_cast(m_pendingLength)); const VncAppleDhAuth::Response response = VncAppleDhAuth::computeResponse( @@ -963,8 +979,72 @@ void VncSessionBackend::processReceiveBuffer() return; } - m_socket->write(response.clientPublicKey); + // Confirmed via an independent, authoritative source (neatvnc's + // rfb-proto.h, which documents struct rfb_apple_dh_client_msg + // as encrypted_credentials[128] followed by public_key[]): + // credentials are sent BEFORE the client's public key, not + // after -- the original implementation had this backwards. m_socket->write(response.encryptedCredentials); + m_socket->write(response.clientPublicKey); + + if (m_negotiatedMinorVersion >= 8) { + m_rfbState = RfbState::WaitingSecurityResult; + } else { + sendClientInit(); + m_rfbState = RfbState::WaitingServerInitHeader; + } + break; + } + + case RfbState::WaitingAppleRsaHostKeyHeader: { + // Response header per security type 33's sub-protocol: a + // 4-byte packet length (unused -- framing is derived from the + // host-key length below instead), a 2-byte version (unused), + // then a 4-byte host-key length. + if (!haveBytes(10)) { + return; + } + m_pendingLength = readU32BE(m_recvBuffer, 6); + m_recvBuffer.remove(0, 10); + m_rfbState = RfbState::WaitingAppleRsaHostKeyBytes; + break; + } + + case RfbState::WaitingAppleRsaHostKeyBytes: { + // The DER-encoded (X.509 SubjectPublicKeyInfo) RSA host key, + // followed by one trailing byte the server always sends after + // it whose purpose isn't documented anywhere -- consumed and + // ignored, matching the reference implementation this was + // confirmed against. + const int totalBytes = static_cast(m_pendingLength) + 1; + if (!haveBytes(totalBytes)) { + return; + } + const QByteArray hostKeyDer = m_recvBuffer.left(static_cast(m_pendingLength)); + m_recvBuffer.remove(0, totalBytes); + + const VncAppleRsaAuth::Response response = VncAppleRsaAuth::computeResponse( + hostKeyDer, profile().username, m_activeOptions.password); + if (response.encryptedCredentials.isEmpty() || response.encryptedAesKey.isEmpty()) { + failConnection( + QStringLiteral( + "Failed to compute the Apple Screen Sharing authentication response."), + QStringLiteral("VncAppleRsaAuth::computeResponse() failed")); + return; + } + + QByteArray responseMsg; + appendU32BE(responseMsg, + static_cast(6 + 2 + response.encryptedCredentials.size() + 2 + + response.encryptedAesKey.size())); + responseMsg.append(char(1)); // type: response + responseMsg.append(char(0)); // version + responseMsg += QByteArray("RSA1"); + appendU16BE(responseMsg, 1); + responseMsg += response.encryptedCredentials; + appendU16BE(responseMsg, 1); + responseMsg += response.encryptedAesKey; + m_socket->write(responseMsg); if (m_negotiatedMinorVersion >= 8) { m_rfbState = RfbState::WaitingSecurityResult; diff --git a/src/vnc_session_backend.h b/src/vnc_session_backend.h index 0536f87..3d15a8c 100644 --- a/src/vnc_session_backend.h +++ b/src/vnc_session_backend.h @@ -24,11 +24,23 @@ struct z_stream_s; // C API to wrangle here. // // Scope (see plan / issue #3 for the full rationale): standard VNC -// Authentication (security type 2), no-auth (type 1), and Apple's Screen -// Sharing scheme (type 30, Diffie-Hellman + AES -- see -// vnc_apple_dh_auth.h; not part of RFC 6143, implemented against the -// well-established reverse-engineered wire format). Raw + CopyRect + -// Hextile + ZRLE + Tight encodings. No dynamic resize. Clipboard sync +// Authentication (security type 2), no-auth (type 1), and two Apple +// Screen Sharing schemes, neither part of RFC 6143 and neither officially +// documented by Apple: type 30 (Diffie-Hellman + AES -- see +// vnc_apple_dh_auth.h) and type 33 (RSA + AES -- see +// vnc_apple_rsa_auth.h). Modern macOS advertises both. Type 30's wire +// format is confirmed against an independent, authoritative source +// (neatvnc's rfb-proto.h, which documents the exact struct layout) and +// verified live against a real macOS Screen Sharing server. Type 33's +// implementation is sourced from the `asyncvnc` PyPI package (a real +// client) but hasn't been gotten working live -- the server closes the +// connection right after the client's initial request for its RSA host +// key, suggesting either a transcription error or that this specific +// macOS version's type 33 sub-protocol has evolved from what that +// reference assumes; kept as a fallback pending further investigation. +// Preference when multiple are offered: None > AppleDH(30) > +// AppleRSA(33) > VNCAuth(2). Raw + CopyRect + Hextile + ZRLE + Tight +// encodings. No dynamic resize. Clipboard sync // (Latin-1 only, per RFB's ServerCutText/ClientCutText) and remote cursor // shape sync (the Cursor pseudo-encoding) are supported. // @@ -111,10 +123,10 @@ private: WaitingTightFilterId, WaitingTightLengthByte, WaitingTightPayload, - WaitingAppleAuthGeneratorLength, - WaitingAppleAuthGeneratorBytes, - WaitingAppleAuthPrimeLength, + WaitingAppleAuthParams, WaitingAppleAuthPrimeAndServerKey, + WaitingAppleRsaHostKeyHeader, + WaitingAppleRsaHostKeyBytes, WaitingSetColourMapHeader, WaitingSetColourMapData, WaitingServerCutTextHeader, @@ -185,14 +197,17 @@ private: int m_tightLengthByteIndex; // Apple Screen Sharing authentication state (security type 30, not - // part of RFC 6143 -- see vnc_apple_dh_auth.h). m_appleAuthGenerator - // must persist across the generator-length/generator-bytes state - // hop; m_appleAuthPrimeLength must persist from when it's first read - // until the combined prime+server-public-key buffer (2x that length) - // has fully arrived, since m_pendingLength gets reused to track that - // combined byte count in the meantime. + // part of RFC 6143 -- see vnc_apple_dh_auth.h). Wire format confirmed + // empirically against a real macOS Screen Sharing server: a literal + // 2-byte generator (not length-prefixed -- there is no separate + // generator-length field), then a 2-byte key length that applies to + // *both* the prime and the server's public key that follow. Both + // members must persist from WaitingAppleAuthParams until the combined + // prime+server-public-key buffer (2x the key length) has fully + // arrived, since m_pendingLength gets reused to track that combined + // byte count in the meantime. QByteArray m_appleAuthGenerator; - quint32 m_appleAuthPrimeLength; + quint32 m_appleAuthKeyLength; void setState(SessionState state, const QString& message); void resetProtocolState(); @@ -208,6 +223,7 @@ private: void sendPointerEvent(); void sendWheelClick(quint8 wheelBit); void sendClientCutText(const QString& text); + void sendAppleRsaHostKeyRequest(); QRect currentHextileTileRect() const; void advanceHextileTile(); bool inflateTightStream(int streamIndex, const QByteArray& compressed, QByteArray* decompressed); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a16dce7..81d9734 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -31,6 +31,7 @@ add_executable(test_vnc_session_backend ${CMAKE_SOURCE_DIR}/src/vnc_session_backend.cpp ${CMAKE_SOURCE_DIR}/src/vnc_pixel_codecs.cpp ${CMAKE_SOURCE_DIR}/src/vnc_apple_dh_auth.cpp + ${CMAKE_SOURCE_DIR}/src/vnc_apple_rsa_auth.cpp ${CMAKE_SOURCE_DIR}/src/session_backend.h ) target_include_directories(test_vnc_session_backend PRIVATE ${CMAKE_SOURCE_DIR}/src) diff --git a/tests/test_vnc_session_backend.cpp b/tests/test_vnc_session_backend.cpp index 3cdf33a..b87b223 100644 --- a/tests/test_vnc_session_backend.cpp +++ b/tests/test_vnc_session_backend.cpp @@ -334,7 +334,9 @@ ToyDhKeypair generateToyDhKeypair() const int primeLen = BN_num_bytes(p); result.primeBytes = QByteArray(primeLen, char(0)); BN_bn2binpad(p, reinterpret_cast(result.primeBytes.data()), primeLen); - result.generatorBytes = QByteArray(1, char(2)); + // The real wire format's generator field is a literal 2 bytes (see + // appleDhAuthServerMessage()), so keep this the same size here too. + result.generatorBytes = QByteArray::fromHex("0002"); result.publicKeyBytes = QByteArray(primeLen, char(0)); BN_bn2binpad(pub, reinterpret_cast(result.publicKeyBytes.data()), primeLen); result.privateExponent = priv; @@ -346,14 +348,17 @@ ToyDhKeypair generateToyDhKeypair() return result; } -// A FramebufferUpdate-adjacent helper isn't needed here -- these bytes are -// sent immediately after the client selects security type 30, before -// ClientInit/ServerInit even happen. +// The bytes a real Apple Screen Sharing server sends immediately after the +// client selects security type 30, before ClientInit/ServerInit even +// happen. Format confirmed empirically against a real macOS server: a +// literal 2-byte generator (NOT length-prefixed -- there is no separate +// generator-length field, unlike a first guess at this scheme might +// assume), then a 2-byte key length applying to both the prime and the +// server's public key that follow. QByteArray appleDhAuthServerMessage(const QByteArray& generator, const QByteArray& prime, const QByteArray& serverPublicKey) { QByteArray msg; - appendU16(msg, static_cast(generator.size())); msg += generator; appendU16(msg, static_cast(prime.size())); msg += prime; @@ -414,6 +419,7 @@ private slots: void appleDhAuthRoundTripDecryptsToOriginalCredentials(); void connectsWithAppleDhAuthenticationRfb38(); void appleDhAuthIsPreferredOverVncAuthWhenBothOffered(); + void appleDhAuthAcceptsRealCapturedMacOsServerParameters(); private: std::unique_ptr m_server; @@ -1995,6 +2001,8 @@ void TestVncSessionBackend::connectsWithAppleDhAuthenticationRfb38() ToyDhKeypair serverKeypair = generateToyDhKeypair(); const QByteArray authMessage = appleDhAuthServerMessage( serverKeypair.generatorBytes, serverKeypair.primeBytes, serverKeypair.publicKeyBytes); + const int keyLength = serverKeypair.primeBytes.size(); + const QString password = QStringLiteral("s3cret-pass"); connect(m_server.get(), &FakeVncServer::clientConnected, this, [this]() { m_server->sendWhenConnected(QByteArray("RFB 003.008\n")); @@ -2011,7 +2019,7 @@ void TestVncSessionBackend::connectsWithAppleDhAuthenticationRfb38() case 1: // security-type selection (byte value 30) m_server->sendWhenConnected(authMessage); break; - case 2: // client's DH public key + encrypted credentials -- accept unconditionally + case 2: // client's response -- accept unconditionally, checked below m_server->sendWhenConnected(QByteArray(4, char(0))); // SecurityResult: OK break; case 3: { // ClientInit @@ -2028,9 +2036,62 @@ void TestVncSessionBackend::connectsWithAppleDhAuthenticationRfb38() } }); - m_backend->connectSession(makeOptions(QStringLiteral("irrelevant-password"))); + m_backend->connectSession(makeOptions(password)); QTRY_COMPARE(m_lastState, SessionState::Connected); + // Verify wire order: encrypted credentials (128 bytes) MUST come + // before the client's public key, per neatvnc's authoritative + // rfb_apple_dh_client_msg struct layout (this is exactly the ordering + // bug that made real-world testing fail before it was found and + // fixed). Proven by actually deriving the shared secret from the + // trailing public-key bytes -- as the server would -- and decrypting + // the leading 128 bytes with it; a swap would make this decrypt to + // garbage instead of the real credentials. + // 12 bytes for the client's version-reply line, 1 byte for its + // security-type selection, precede the response itself; more bytes + // (ClientInit, SetPixelFormat, ...) may already follow it by the time + // Connected is observed, so slice by offset rather than assuming + // total size. + const int responseOffset = 13; + QVERIFY(m_server->received.size() >= responseOffset + 128 + keyLength); + const QByteArray encryptedCredentials = m_server->received.mid(responseOffset, 128); + const QByteArray clientPublicKeyBytes = + m_server->received.mid(responseOffset + 128, keyLength); + + BIGNUM* clientPub = BN_bin2bn( + reinterpret_cast(clientPublicKeyBytes.constData()), + clientPublicKeyBytes.size(), nullptr); + BN_CTX* ctx = BN_CTX_new(); + BIGNUM* sharedSecret = BN_new(); + BN_mod_exp(sharedSecret, clientPub, serverKeypair.privateExponent, serverKeypair.prime, ctx); + QByteArray secretBytes(keyLength, char(0)); + BN_bn2binpad(sharedSecret, reinterpret_cast(secretBytes.data()), keyLength); + + unsigned char aesKey[16]; + EVP_Digest(secretBytes.constData(), static_cast(secretBytes.size()), aesKey, nullptr, + EVP_md5(), nullptr); + + EVP_CIPHER_CTX* decCtx = EVP_CIPHER_CTX_new(); + EVP_DecryptInit_ex(decCtx, EVP_aes_128_ecb(), nullptr, aesKey, nullptr); + EVP_CIPHER_CTX_set_padding(decCtx, 0); + QByteArray plain(encryptedCredentials.size() + 16, char(0)); + int outLen1 = 0; + EVP_DecryptUpdate(decCtx, reinterpret_cast(plain.data()), &outLen1, + reinterpret_cast(encryptedCredentials.constData()), + encryptedCredentials.size()); + int outLen2 = 0; + EVP_DecryptFinal_ex(decCtx, reinterpret_cast(plain.data()) + outLen1, &outLen2); + plain.resize(outLen1 + outLen2); + EVP_CIPHER_CTX_free(decCtx); + + QByteArray expected(128, char(0)); + const QByteArray passBytes = password.toLatin1().left(64); + std::memcpy(expected.data() + 64, passBytes.constData(), static_cast(passBytes.size())); + QCOMPARE(plain, expected); + + BN_free(clientPub); + BN_free(sharedSecret); + BN_CTX_free(ctx); BN_free(serverKeypair.privateExponent); BN_free(serverKeypair.prime); } @@ -2056,5 +2117,60 @@ void TestVncSessionBackend::appleDhAuthIsPreferredOverVncAuthWhenBothOffered() QCOMPARE(static_cast(m_server->received.at(0)), quint8(30)); } +void TestVncSessionBackend::appleDhAuthAcceptsRealCapturedMacOsServerParameters() +{ + // The exact generator/prime/server-public-key bytes captured from a + // real macOS Screen Sharing server's security-type-30 auth message + // (generator=5, a 512-byte/4096-bit RFC 3526 Group 16-family prime, + // and its server public key) -- this is what caught the original + // implementation's wrong assumption that the generator was itself + // length-prefixed like the prime is; it wasn't (it's a literal 2 + // bytes), and this pins that fix against the real bytes that exposed + // the bug rather than only against freshly-generated toy data. + const QByteArray captured = QByteArray::fromHex( + "00050200" + "ffffffffffffffffc90fdaa22168c234c4c6628b80dc1cd129024e088a67cc74020bbea63b139b22514a0" + "8798e3404ddef9519b3cd3a431b302b0a6df25f14374fe1356d6d51c245e485b576625e7ec6f44c42e9a6" + "37ed6b0bff5cb6f406b7edee386bfb5a899fa5ae9f24117c4b1fe649286651ece45b3dc2007cb8a163bf0" + "598da48361c55d39a69163fa8fd24cf5f83655d23dca3ad961c62f356208552bb9ed529077096966d670c" + "354e4abc9804f1746c08ca18217c32905e462e36ce3be39e772c180e86039b2783a2ec07a28fb5c55df06" + "f4c52c9de2bcbf6955817183995497cea956ae515d2261898fa051015728e5a8aaac42dad33170d04507a" + "33a85521abdf1cba64ecfb850458dbef0a8aea71575d060c7db3970f85a6e1e4c7abf5ae8cdb0933d71e8" + "c94e04a25619dcee3d2261ad2ee6bf12ffa06d98a0864d87602733ec86a64521f2b18177b200cbbe11757" + "7a615d6c770988c0bad946e208e24fa074e5ab3143db5bfce0fd108e4b82d120a92108011a723c12a787e" + "6d788719a10bdba5b2699c327186af4e23c1a946834b6150bda2583e9ca2ad44ce8dbbbc2db04de8ef92e" + "8efc141fbecaa6287c59474e6bc05d99b2964fa090c3a2233ba186515be7ed1f612970cee2d7afb81bdd7" + "62170481cd0069127d5b05aa993b4ea988d8fddc186ffb7dc90a6c08f4df435c934063199" + "ffffffffffffffff" + "91c11c23a5b54389a05127d7fa94e681b7884667d770cd646fa46583d73e0a5ad09e361c30a0f9a9b765a" + "b291e2b8a13ba0088ba4bdb4a838120e649d49ed78db7fdb3397b907d1a42c0531d16796229d7b3a5a369" + "bfc639771ab1dea093ca7756b2b4a4a98f296478b331538533faead12342d6f1fe92817690ae8e2c51cbf" + "dc96c819e480c2170e6be12149691cb2bab218528c40cc5729b8a303bbe95d77eba288f3c4137d316c5b1" + "d9004c8fe3d034b1284d39e57133972700d1933465b4cc656d2dcbef67dc7eeb464f197bd5187abce304b" + "c972768dcaaa703e287f4f1e375c287b474fe0e3df9897a24bdaf06466976becbc8908fd81211537d1a44" + "e63e705d69b5c90f375fa717c978b27a21384769da67cd97e5134f71aef420dd4b36a2c4e05f80cee043f" + "ab873b478804dc126495d35f0a1df041c4b2bf0926e35ae033f6f6808bdc8a5792b7d380e7c7fc95fc80c" + "fea44c92842ed75757230f590b7dd1d9beadbabb78a96c47f21e65f6ea8566820ff9c59d078870fa1d367" + "2ff0b7f9a3d600f3479232e533bc8b64a3909fd61e070b19ac98d50a7c2a33c885294183e82a4e7777f2a" + "423002219a6094fdf51651d48550f771ede87eb95592eda604b85058cd0d8db9a92670e6e0d74400f7cd4" + "b48c8dfbace5c92c39775a60bb4e59d70878a50b609b15ae4a930ee5eb0494902d44869089349fa2ab343" + "028e"); + QCOMPARE(captured.size(), 1028); + + const QByteArray generator = captured.left(2); + const int keyLength = (static_cast(captured.at(2)) << 8) | static_cast(captured.at(3)); + QCOMPARE(keyLength, 512); + const QByteArray prime = captured.mid(4, keyLength); + const QByteArray serverPublicKey = captured.mid(4 + keyLength, keyLength); + QCOMPARE(prime.size(), 512); + QCOMPARE(serverPublicKey.size(), 512); + + const VncAppleDhAuth::Response response = VncAppleDhAuth::computeResponse( + generator, prime, serverPublicKey, QStringLiteral("tester"), QStringLiteral("s3cret")); + QVERIFY(!response.clientPublicKey.isEmpty()); + QCOMPARE(response.clientPublicKey.size(), 512); + QCOMPARE(response.encryptedCredentials.size(), 128); +} + QTEST_GUILESS_MAIN(TestVncSessionBackend) #include "test_vnc_session_backend.moc"