From 8a7089c0c68f18bb0dde29cd7b0422ef31421a21 Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 18 Aug 2022 13:57:17 +0200 Subject: [PATCH 1/8] Remove redundant security result tests The event is the desired behaviour. RFB._fail() being called is just an internal detail that we shouldn't care about. --- tests/test.rfb.js | 32 +++++--------------------------- 1 file changed, 5 insertions(+), 27 deletions(-) diff --git a/tests/test.rfb.js b/tests/test.rfb.js index 3f79bc04..2b9d5e7f 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -1592,31 +1592,6 @@ describe('Remote Frame Buffer Protocol Client', function () { expect(client._rfbInitState).to.equal('ServerInitialisation'); }); - it('should fail on an error code of 1 with the given message for versions >= 3.8', function () { - client._rfbVersion = 3.8; - sinon.spy(client, '_fail'); - const failureData = [0, 0, 0, 1, 0, 0, 0, 6, 119, 104, 111, 111, 112, 115]; - client._sock._websocket._receiveData(new Uint8Array(failureData)); - expect(client._fail).to.have.been.calledWith( - 'Security negotiation failed on security result (reason: whoops)'); - }); - - it('should fail on an error code of 1 with a standard message for version < 3.8', function () { - sinon.spy(client, '_fail'); - client._rfbVersion = 3.7; - client._sock._websocket._receiveData(new Uint8Array([0, 0, 0, 1])); - expect(client._fail).to.have.been.calledWith( - 'Security handshake failed'); - }); - - it('should result in securityfailure event when receiving a non zero status', function () { - const spy = sinon.spy(); - client.addEventListener("securityfailure", spy); - client._sock._websocket._receiveData(new Uint8Array([0, 0, 0, 2])); - expect(spy).to.have.been.calledOnce; - expect(spy.args[0][0].detail.status).to.equal(2); - }); - it('should include reason when provided in securityfailure event', function () { client._rfbVersion = 3.8; const spy = sinon.spy(); @@ -1624,25 +1599,28 @@ describe('Remote Frame Buffer Protocol Client', function () { const failureData = [0, 0, 0, 1, 0, 0, 0, 12, 115, 117, 99, 104, 32, 102, 97, 105, 108, 117, 114, 101]; client._sock._websocket._receiveData(new Uint8Array(failureData)); + expect(spy).to.have.been.calledOnce; expect(spy.args[0][0].detail.status).to.equal(1); expect(spy.args[0][0].detail.reason).to.equal('such failure'); }); it('should not include reason when length is zero in securityfailure event', function () { - client._rfbVersion = 3.9; + client._rfbVersion = 3.8; const spy = sinon.spy(); client.addEventListener("securityfailure", spy); const failureData = [0, 0, 0, 1, 0, 0, 0, 0]; client._sock._websocket._receiveData(new Uint8Array(failureData)); + expect(spy).to.have.been.calledOnce; expect(spy.args[0][0].detail.status).to.equal(1); expect('reason' in spy.args[0][0].detail).to.be.false; }); it('should not include reason in securityfailure event for version < 3.8', function () { - client._rfbVersion = 3.6; + client._rfbVersion = 3.7; const spy = sinon.spy(); client.addEventListener("securityfailure", spy); client._sock._websocket._receiveData(new Uint8Array([0, 0, 0, 2])); + expect(spy).to.have.been.calledOnce; expect(spy.args[0][0].detail.status).to.equal(2); expect('reason' in spy.args[0][0].detail).to.be.false; }); From 05d68e118d9bdb429ca1da49accbb098afd91523 Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 18 Aug 2022 14:15:36 +0200 Subject: [PATCH 2/8] Abstract resuming the authentication We now do this in multiple places, so make sure things are handled the same way in all cases. --- core/rfb.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/core/rfb.js b/core/rfb.js index 578a8984..18b19156 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -402,7 +402,7 @@ export default class RFB extends EventTargetMixin { sendCredentials(creds) { this._rfbCredentials = creds; - setTimeout(this._initMsg.bind(this), 0); + this._resumeAuthentication(); } sendCtrlAltDel() { @@ -1661,7 +1661,7 @@ export default class RFB extends EventTargetMixin { this._rfbCredentials.ardCredentials = encrypted; this._rfbCredentials.ardPublicKey = clientPublicKey; - setTimeout(this._initMsg.bind(this), 0); + this._resumeAuthentication(); } _negotiateTightUnixAuth() { @@ -2052,6 +2052,14 @@ export default class RFB extends EventTargetMixin { } } + // Resume authentication handshake after it was paused for some + // reason, e.g. waiting for a password from the user + _resumeAuthentication() { + // We use setTimeout() so it's run in its own context, just like + // it originally did via the WebSocket's event handler + setTimeout(this._initMsg.bind(this), 0); + } + _handleSetColourMapMsg() { Log.Debug("SetColorMapEntries"); From 084030fe68b137e20043d112e7876e0094b4d761 Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 18 Aug 2022 14:26:18 +0200 Subject: [PATCH 3/8] Handle connection init loop at the top Avoid the mess of having lots of functions call back to _initMsg() just because they might be able to continue right away. Instead loop at the top level until we're either done, or we need more data. --- core/rfb.js | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/core/rfb.js b/core/rfb.js index 18b19156..8fd5b796 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -922,8 +922,15 @@ export default class RFB extends EventTargetMixin { } } break; + case 'connecting': + while (this._rfbConnectionState === 'connecting') { + if (!this._initMsg()) { + break; + } + } + break; default: - this._initMsg(); + Log.Error("Got data while in an invalid state"); break; } } @@ -1342,7 +1349,7 @@ export default class RFB extends EventTargetMixin { this._rfbInitState = "SecurityReason"; this._securityContext = "no security types"; this._securityStatus = 1; - return this._initMsg(); + return true; } const types = this._sock.rQshiftBytes(numTypes); @@ -1377,14 +1384,14 @@ export default class RFB extends EventTargetMixin { this._rfbInitState = "SecurityReason"; this._securityContext = "authentication scheme"; this._securityStatus = 1; - return this._initMsg(); + return true; } } this._rfbInitState = 'Authentication'; Log.Debug('Authenticating using scheme: ' + this._rfbAuthScheme); - return this._initMsg(); // jump to authentication + return true; } _handleSecurityReason() { @@ -1773,10 +1780,10 @@ export default class RFB extends EventTargetMixin { return true; case 'STDVVNCAUTH_': // VNC auth this._rfbAuthScheme = 2; - return this._initMsg(); + return true; case 'TGHTULGNAUTH': // UNIX auth this._rfbAuthScheme = 129; - return this._initMsg(); + return true; default: return this._fail("Unsupported tiny auth scheme " + "(scheme: " + authType + ")"); @@ -1813,7 +1820,7 @@ export default class RFB extends EventTargetMixin { }).then(() => { this.dispatchEvent(new CustomEvent('securityresult')); this._rfbInitState = "SecurityResult"; - this._initMsg(); + return true; }).finally(() => { this._rfbRSAAESAuthenticationState.removeEventListener( "serververification", this._eventHandlers.handleRSAAESServerVerification); @@ -1833,7 +1840,7 @@ export default class RFB extends EventTargetMixin { return true; } this._rfbInitState = 'ClientInitialisation'; - return this._initMsg(); + return true; case 22: // XVP auth return this._negotiateXvpAuth(); @@ -1870,13 +1877,13 @@ export default class RFB extends EventTargetMixin { if (status === 0) { // OK this._rfbInitState = 'ClientInitialisation'; Log.Debug('Authentication OK'); - return this._initMsg(); + return true; } else { if (this._rfbVersion >= 3.8) { this._rfbInitState = "SecurityReason"; this._securityContext = "security result"; this._securityStatus = status; - return this._initMsg(); + return true; } else { this.dispatchEvent(new CustomEvent( "securityfailure", From 5671072dfe109193bbd2570d95882480c7aea011 Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 18 Aug 2022 14:33:48 +0200 Subject: [PATCH 4/8] Expect security result for RFB 3.7 The cut off was wrong here. 3.7 will send a security result, but not a security reason. It also fixes the issue that < 3.7 (e.g. 3.3) supports VNC authentication as well. --- core/rfb.js | 13 ++++++++----- tests/test.rfb.js | 12 ++++++------ 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/core/rfb.js b/core/rfb.js index 8fd5b796..f524411b 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -1835,11 +1835,7 @@ export default class RFB extends EventTargetMixin { _negotiateAuthentication() { switch (this._rfbAuthScheme) { case 1: // no auth - if (this._rfbVersion >= 3.8) { - this._rfbInitState = 'SecurityResult'; - return true; - } - this._rfbInitState = 'ClientInitialisation'; + this._rfbInitState = 'SecurityResult'; return true; case 22: // XVP auth @@ -1870,6 +1866,13 @@ export default class RFB extends EventTargetMixin { } _handleSecurityResult() { + // There is no security choice, and hence no security result + // until RFB 3.7 + if (this._rfbVersion < 3.7) { + this._rfbInitState = 'ClientInitialisation'; + return true; + } + if (this._sock.rQwait('VNC auth response ', 4)) { return false; } const status = this._sock.rQshift32(); diff --git a/tests/test.rfb.js b/tests/test.rfb.js index 2b9d5e7f..4f168618 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -1144,7 +1144,7 @@ describe('Remote Frame Buffer Protocol Client', function () { const authSchemes = [2, 1, 3]; client._sock._websocket._receiveData(new Uint8Array(authSchemes)); expect(client._rfbAuthScheme).to.equal(1); - expect(client._sock).to.have.sent(new Uint8Array([1, 1])); + expect(client._sock).to.have.sent(new Uint8Array([1])); }); it('should choose for the most prefered scheme possible for versions >= 3.7', function () { @@ -1209,15 +1209,15 @@ describe('Remote Frame Buffer Protocol Client', function () { 'Security negotiation failed on authentication scheme (reason: Whoopsies)'); }); - it('should transition straight to SecurityResult on "no auth" (1) for versions >= 3.8', function () { - client._rfbVersion = 3.8; + it('should transition straight to SecurityResult on "no auth" (1) for versions >= 3.7', function () { + client._rfbVersion = 3.7; sendSecurity(1, client); expect(client._rfbInitState).to.equal('SecurityResult'); }); - it('should transition straight to ServerInitialisation on "no auth" for versions < 3.8', function () { - client._rfbVersion = 3.7; - sendSecurity(1, client); + it('should transition straight to ServerInitialisation on "no auth" for versions < 3.7', function () { + client._rfbVersion = 3.6; + client._sock._websocket._receiveData(new Uint8Array([0, 0, 0, 1])); expect(client._rfbInitState).to.equal('ServerInitialisation'); }); From 6719b932cf95b7356385b7ca3ed0a0271be4134b Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 18 Aug 2022 13:31:44 +0200 Subject: [PATCH 5/8] Avoiding internal variables for security tests A good test uses only input and output, so let's avoid assuming internal variable names or behaviours. --- tests/test.rfb.js | 139 ++++++++++++++++++++-------------------------- 1 file changed, 60 insertions(+), 79 deletions(-) diff --git a/tests/test.rfb.js b/tests/test.rfb.js index 4f168618..e7d6040a 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -1026,17 +1026,21 @@ describe('Remote Frame Buffer Protocol Client', function () { client._rfbConnectionState = 'connecting'; }); - describe('ProtocolVersion', function () { - function sendVer(ver, client) { - const arr = new Uint8Array(12); - for (let i = 0; i < ver.length; i++) { - arr[i+4] = ver.charCodeAt(i); - } - arr[0] = 'R'; arr[1] = 'F'; arr[2] = 'B'; arr[3] = ' '; - arr[11] = '\n'; - client._sock._websocket._receiveData(arr); + function sendVer(ver, client) { + const arr = new Uint8Array(12); + for (let i = 0; i < ver.length; i++) { + arr[i+4] = ver.charCodeAt(i); } + arr[0] = 'R'; arr[1] = 'F'; arr[2] = 'B'; arr[3] = ' '; + arr[11] = '\n'; + client._sock._websocket._receiveData(arr); + } + function sendSecurity(type, cl) { + cl._sock._websocket._receiveData(new Uint8Array([1, type])); + } + + describe('ProtocolVersion', function () { describe('version parsing', function () { it('should interpret version 003.003 as version 3.3', function () { sendVer('003.003', client); @@ -1127,44 +1131,32 @@ describe('Remote Frame Buffer Protocol Client', function () { describe('Security', function () { beforeEach(function () { - client._rfbInitState = 'Security'; - }); - - it('should simply receive the auth scheme when for versions < 3.7', function () { - client._rfbVersion = 3.6; - const authSchemeRaw = [1, 2, 3, 4]; - const authScheme = (authSchemeRaw[0] << 24) + (authSchemeRaw[1] << 16) + - (authSchemeRaw[2] << 8) + authSchemeRaw[3]; - client._sock._websocket._receiveData(new Uint8Array(authSchemeRaw)); - expect(client._rfbAuthScheme).to.equal(authScheme); + sendVer('003.008\n', client); + client._sock._websocket._getSentData(); }); it('should prefer no authentication is possible', function () { - client._rfbVersion = 3.7; const authSchemes = [2, 1, 3]; client._sock._websocket._receiveData(new Uint8Array(authSchemes)); expect(client._rfbAuthScheme).to.equal(1); expect(client._sock).to.have.sent(new Uint8Array([1])); }); - it('should choose for the most prefered scheme possible for versions >= 3.7', function () { - client._rfbVersion = 3.7; + it('should choose for the most prefered scheme possible', function () { const authSchemes = [2, 22, 16]; client._sock._websocket._receiveData(new Uint8Array(authSchemes)); expect(client._rfbAuthScheme).to.equal(22); expect(client._sock).to.have.sent(new Uint8Array([22])); }); - it('should fail if there are no supported schemes for versions >= 3.7', function () { + it('should fail if there are no supported schemes', function () { sinon.spy(client, "_fail"); - client._rfbVersion = 3.7; const authSchemes = [1, 32]; client._sock._websocket._receiveData(new Uint8Array(authSchemes)); expect(client._fail).to.have.been.calledOnce; }); - it('should fail with the appropriate message if no types are sent for versions >= 3.7', function () { - client._rfbVersion = 3.7; + it('should fail with the appropriate message if no types are sent', function () { const failureData = [0, 0, 0, 0, 6, 119, 104, 111, 111, 112, 115]; sinon.spy(client, '_fail'); client._sock._websocket._receiveData(new Uint8Array(failureData)); @@ -1175,7 +1167,6 @@ describe('Remote Frame Buffer Protocol Client', function () { }); it('should transition to the Authentication state and continue on successful negotiation', function () { - client._rfbVersion = 3.7; const authSchemes = [1, 1]; client._negotiateAuthentication = sinon.spy(); client._sock._websocket._receiveData(new Uint8Array(authSchemes)); @@ -1184,17 +1175,8 @@ describe('Remote Frame Buffer Protocol Client', function () { }); }); - describe('Authentication', function () { - beforeEach(function () { - client._rfbInitState = 'Security'; - }); - - function sendSecurity(type, cl) { - cl._sock._websocket._receiveData(new Uint8Array([1, type])); - } - + describe('Legacy Authentication', function () { it('should fail on auth scheme 0 (pre 3.7) with the given message', function () { - client._rfbVersion = 3.6; const errMsg = "Whoopsies"; const data = [0, 0, 0, 0]; const errLen = errMsg.length; @@ -1203,37 +1185,42 @@ describe('Remote Frame Buffer Protocol Client', function () { data.push(errMsg.charCodeAt(i)); } + sendVer('003.006\n', client); + client._sock._websocket._getSentData(); + sinon.spy(client, '_fail'); client._sock._websocket._receiveData(new Uint8Array(data)); expect(client._fail).to.have.been.calledWith( 'Security negotiation failed on authentication scheme (reason: Whoopsies)'); }); - it('should transition straight to SecurityResult on "no auth" (1) for versions >= 3.7', function () { - client._rfbVersion = 3.7; + it('should transition straight to ServerInitialisation on "no auth" for versions < 3.7', function () { + sendVer('003.006\n', client); + client._sock._websocket._getSentData(); + + client._sock._websocket._receiveData(new Uint8Array([0, 0, 0, 1])); + expect(client._rfbInitState).to.equal('ServerInitialisation'); + }); + }); + + describe('Authentication', function () { + beforeEach(function () { + sendVer('003.008\n', client); + client._sock._websocket._getSentData(); + }); + + it('should transition straight to SecurityResult on "no auth" (1)', function () { sendSecurity(1, client); expect(client._rfbInitState).to.equal('SecurityResult'); }); - it('should transition straight to ServerInitialisation on "no auth" for versions < 3.7', function () { - client._rfbVersion = 3.6; - client._sock._websocket._receiveData(new Uint8Array([0, 0, 0, 1])); - expect(client._rfbInitState).to.equal('ServerInitialisation'); - }); - it('should fail on an unknown auth scheme', function () { sinon.spy(client, "_fail"); - client._rfbVersion = 3.8; sendSecurity(57, client); expect(client._fail).to.have.been.calledOnce; }); describe('VNC Authentication (type 2) Handler', function () { - beforeEach(function () { - client._rfbInitState = 'Security'; - client._rfbVersion = 3.8; - }); - it('should fire the credentialsrequired event if missing a password', function () { const spy = sinon.spy(); client.addEventListener("credentialsrequired", spy); @@ -1274,12 +1261,6 @@ describe('Remote Frame Buffer Protocol Client', function () { }); describe('ARD Authentication (type 30) Handler', function () { - - beforeEach(function () { - client._rfbInitState = 'Security'; - client._rfbVersion = 3.8; - }); - it('should fire the credentialsrequired event if all credentials are missing', function () { const spy = sinon.spy(); client.addEventListener("credentialsrequired", spy); @@ -1347,11 +1328,6 @@ describe('Remote Frame Buffer Protocol Client', function () { }); describe('XVP Authentication (type 22) Handler', function () { - beforeEach(function () { - client._rfbInitState = 'Security'; - client._rfbVersion = 3.8; - }); - it('should fall through to standard VNC authentication upon completion', function () { client._rfbCredentials = { username: 'user', target: 'target', @@ -1400,8 +1376,6 @@ describe('Remote Frame Buffer Protocol Client', function () { describe('TightVNC Authentication (type 16) Handler', function () { beforeEach(function () { - client._rfbInitState = 'Security'; - client._rfbVersion = 3.8; sendSecurity(16, client); client._sock._websocket._getSentData(); // skip the security reply }); @@ -1487,8 +1461,6 @@ describe('Remote Frame Buffer Protocol Client', function () { describe('VeNCrypt Authentication (type 19) Handler', function () { beforeEach(function () { - client._rfbInitState = 'Security'; - client._rfbVersion = 3.8; sendSecurity(19, client); expect(client._sock).to.have.sent(new Uint8Array([19])); }); @@ -1582,9 +1554,30 @@ describe('Remote Frame Buffer Protocol Client', function () { }); }); + describe('Legacy SecurityResult', function () { + beforeEach(function () { + sendVer('003.007\n', client); + client._sock._websocket._getSentData(); + sendSecurity(1, client); + client._sock._websocket._getSentData(); + }); + + it('should not include reason in securityfailure event', function () { + const spy = sinon.spy(); + client.addEventListener("securityfailure", spy); + client._sock._websocket._receiveData(new Uint8Array([0, 0, 0, 2])); + expect(spy).to.have.been.calledOnce; + expect(spy.args[0][0].detail.status).to.equal(2); + expect('reason' in spy.args[0][0].detail).to.be.false; + }); + }); + describe('SecurityResult', function () { beforeEach(function () { - client._rfbInitState = 'SecurityResult'; + sendVer('003.008\n', client); + client._sock._websocket._getSentData(); + sendSecurity(1, client); + client._sock._websocket._getSentData(); }); it('should fall through to ServerInitialisation on a response code of 0', function () { @@ -1593,7 +1586,6 @@ describe('Remote Frame Buffer Protocol Client', function () { }); it('should include reason when provided in securityfailure event', function () { - client._rfbVersion = 3.8; const spy = sinon.spy(); client.addEventListener("securityfailure", spy); const failureData = [0, 0, 0, 1, 0, 0, 0, 12, 115, 117, 99, 104, @@ -1605,7 +1597,6 @@ describe('Remote Frame Buffer Protocol Client', function () { }); it('should not include reason when length is zero in securityfailure event', function () { - client._rfbVersion = 3.8; const spy = sinon.spy(); client.addEventListener("securityfailure", spy); const failureData = [0, 0, 0, 1, 0, 0, 0, 0]; @@ -1614,16 +1605,6 @@ describe('Remote Frame Buffer Protocol Client', function () { expect(spy.args[0][0].detail.status).to.equal(1); expect('reason' in spy.args[0][0].detail).to.be.false; }); - - it('should not include reason in securityfailure event for version < 3.8', function () { - client._rfbVersion = 3.7; - const spy = sinon.spy(); - client.addEventListener("securityfailure", spy); - client._sock._websocket._receiveData(new Uint8Array([0, 0, 0, 2])); - expect(spy).to.have.been.calledOnce; - expect(spy.args[0][0].detail.status).to.equal(2); - expect('reason' in spy.args[0][0].detail).to.be.false; - }); }); describe('ClientInitialisation', function () { From e1174e813b617062c77491c01130c38b45f15311 Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 18 Aug 2022 14:53:10 +0200 Subject: [PATCH 6/8] Use constants for security types Makes everything much more readable. --- core/rfb.js | 72 ++++++++++++++++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 29 deletions(-) diff --git a/core/rfb.js b/core/rfb.js index f524411b..2d7e77fa 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -54,6 +54,21 @@ const GESTURE_SCRLSENS = 50; const DOUBLE_TAP_TIMEOUT = 1000; const DOUBLE_TAP_THRESHOLD = 50; +// Security types +const securityTypeNone = 1; +const securityTypeVNCAuth = 2; +const securityTypeRA2ne = 6; +const securityTypeTight = 16; +const securityTypeVeNCrypt = 19; +const securityTypeXVP = 22; +const securityTypeARD = 30; + +// Special Tight security types +const securityTypeUnixLogon = 129; + +// VeNCrypt security types +const securityTypePlain = 256; + // Extended clipboard pseudo-encoding formats const extendedClipboardFormatText = 1; /*eslint-disable no-unused-vars */ @@ -1356,20 +1371,20 @@ export default class RFB extends EventTargetMixin { Log.Debug("Server security types: " + types); // Look for each auth in preferred order - if (types.includes(1)) { - this._rfbAuthScheme = 1; // None - } else if (types.includes(22)) { - this._rfbAuthScheme = 22; // XVP - } else if (types.includes(16)) { - this._rfbAuthScheme = 16; // Tight - } else if (types.includes(6)) { - this._rfbAuthScheme = 6; // RA2ne Auth - } else if (types.includes(2)) { - this._rfbAuthScheme = 2; // VNC Auth - } else if (types.includes(30)) { - this._rfbAuthScheme = 30; // ARD Auth - } else if (types.includes(19)) { - this._rfbAuthScheme = 19; // VeNCrypt Auth + if (types.includes(securityTypeNone)) { + this._rfbAuthScheme = securityTypeNone; + } else if (types.includes(securityTypeXVP)) { + this._rfbAuthScheme = securityTypeXVP; + } else if (types.includes(securityTypeTight)) { + this._rfbAuthScheme = securityTypeTight; + } else if (types.includes(securityTypeRA2ne)) { + this._rfbAuthScheme = securityTypeRA2ne; + } else if (types.includes(securityTypeVNCAuth)) { + this._rfbAuthScheme = securityTypeVNCAuth; + } else if (types.includes(securityTypeARD)) { + this._rfbAuthScheme = securityTypeARD; + } else if (types.includes(securityTypeVeNCrypt)) { + this._rfbAuthScheme = securityTypeVeNCrypt; } else { return this._fail("Unsupported security types (types: " + types + ")"); } @@ -1441,7 +1456,7 @@ export default class RFB extends EventTargetMixin { this._rfbCredentials.username + this._rfbCredentials.target; this._sock.sendString(xvpAuthStr); - this._rfbAuthScheme = 2; + this._rfbAuthScheme = securityTypeVNCAuth; return this._negotiateAuthentication(); } @@ -1499,8 +1514,7 @@ export default class RFB extends EventTargetMixin { subtypes.push(this._sock.rQshift32()); } - // 256 = Plain subtype - if (subtypes.indexOf(256) != -1) { + if (subtypes.indexOf(securityTypePlain) != -1) { // 0x100 = 256 this._sock.send([0, 0, 1, 0]); this._rfbVeNCryptState = 4; @@ -1778,11 +1792,11 @@ export default class RFB extends EventTargetMixin { case 'STDVNOAUTH__': // no auth this._rfbInitState = 'SecurityResult'; return true; - case 'STDVVNCAUTH_': // VNC auth - this._rfbAuthScheme = 2; + case 'STDVVNCAUTH_': + this._rfbAuthScheme = securityTypeVNCAuth; return true; - case 'TGHTULGNAUTH': // UNIX auth - this._rfbAuthScheme = 129; + case 'TGHTULGNAUTH': + this._rfbAuthScheme = securityTypeUnixLogon; return true; default: return this._fail("Unsupported tiny auth scheme " + @@ -1834,29 +1848,29 @@ export default class RFB extends EventTargetMixin { _negotiateAuthentication() { switch (this._rfbAuthScheme) { - case 1: // no auth + case securityTypeNone: this._rfbInitState = 'SecurityResult'; return true; - case 22: // XVP auth + case securityTypeXVP: return this._negotiateXvpAuth(); - case 30: // ARD auth + case securityTypeARD: return this._negotiateARDAuth(); - case 2: // VNC authentication + case securityTypeVNCAuth: return this._negotiateStdVNCAuth(); - case 16: // TightVNC Security Type + case securityTypeTight: return this._negotiateTightAuth(); - case 19: // VeNCrypt Security Type + case securityTypeVeNCrypt: return this._negotiateVeNCryptAuth(); - case 129: // TightVNC UNIX Security Type + case securityTypeUnixLogon: return this._negotiateTightUnixAuth(); - case 6: // RA2ne Security Type + case securityTypeRA2ne: return this._negotiateRA2neAuth(); default: From 795494ade1bab6a14fd45e02dbaba52301df65f1 Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 18 Aug 2022 15:01:52 +0200 Subject: [PATCH 7/8] Prefer security types in the server's order This is how TigerVNC has been behaving for years and has worked well there, so let's follow them. --- core/rfb.js | 42 ++++++++++++++++++++++++++---------------- tests/test.rfb.js | 14 +++----------- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/core/rfb.js b/core/rfb.js index 2d7e77fa..b10b502c 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -1354,6 +1354,21 @@ export default class RFB extends EventTargetMixin { this._rfbInitState = 'Security'; } + _isSupportedSecurityType(type) { + const clientTypes = [ + securityTypeNone, + securityTypeVNCAuth, + securityTypeRA2ne, + securityTypeTight, + securityTypeVeNCrypt, + securityTypeXVP, + securityTypeARD, + securityTypePlain, + ]; + + return clientTypes.includes(type); + } + _negotiateSecurity() { if (this._rfbVersion >= 3.7) { // Server sends supported list, client decides @@ -1370,22 +1385,17 @@ export default class RFB extends EventTargetMixin { const types = this._sock.rQshiftBytes(numTypes); Log.Debug("Server security types: " + types); - // Look for each auth in preferred order - if (types.includes(securityTypeNone)) { - this._rfbAuthScheme = securityTypeNone; - } else if (types.includes(securityTypeXVP)) { - this._rfbAuthScheme = securityTypeXVP; - } else if (types.includes(securityTypeTight)) { - this._rfbAuthScheme = securityTypeTight; - } else if (types.includes(securityTypeRA2ne)) { - this._rfbAuthScheme = securityTypeRA2ne; - } else if (types.includes(securityTypeVNCAuth)) { - this._rfbAuthScheme = securityTypeVNCAuth; - } else if (types.includes(securityTypeARD)) { - this._rfbAuthScheme = securityTypeARD; - } else if (types.includes(securityTypeVeNCrypt)) { - this._rfbAuthScheme = securityTypeVeNCrypt; - } else { + // Look for a matching security type in the order that the + // server prefers + this._rfbAuthScheme = -1; + for (let type of types) { + if (this._isSupportedSecurityType(type)) { + this._rfbAuthScheme = type; + break; + } + } + + if (this._rfbAuthScheme === -1) { return this._fail("Unsupported security types (types: " + types + ")"); } diff --git a/tests/test.rfb.js b/tests/test.rfb.js index e7d6040a..0e46ff4c 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -1135,18 +1135,10 @@ describe('Remote Frame Buffer Protocol Client', function () { client._sock._websocket._getSentData(); }); - it('should prefer no authentication is possible', function () { - const authSchemes = [2, 1, 3]; + it('should respect server preference order', function () { + const authSchemes = [ 6, 79, 30, 188, 16, 6, 1 ]; client._sock._websocket._receiveData(new Uint8Array(authSchemes)); - expect(client._rfbAuthScheme).to.equal(1); - expect(client._sock).to.have.sent(new Uint8Array([1])); - }); - - it('should choose for the most prefered scheme possible', function () { - const authSchemes = [2, 22, 16]; - client._sock._websocket._receiveData(new Uint8Array(authSchemes)); - expect(client._rfbAuthScheme).to.equal(22); - expect(client._sock).to.have.sent(new Uint8Array([22])); + expect(client._sock).to.have.sent(new Uint8Array([30])); }); it('should fail if there are no supported schemes', function () { From df8d005de960e2133ded0610656af591e92fc6b7 Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 18 Aug 2022 16:15:43 +0200 Subject: [PATCH 8/8] VeNCrypt should handle classical types VeNCrypt is a superset of the original security types, so it should be fine to send any of the classical values here as well. --- core/rfb.js | 87 +++++++++++++++++++++++++++++------------------ tests/test.rfb.js | 58 +++++++++++++++++++++++++++++-- 2 files changed, 109 insertions(+), 36 deletions(-) diff --git a/core/rfb.js b/core/rfb.js index b10b502c..4b3526f9 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -1524,48 +1524,66 @@ export default class RFB extends EventTargetMixin { subtypes.push(this._sock.rQshift32()); } - if (subtypes.indexOf(securityTypePlain) != -1) { - // 0x100 = 256 - this._sock.send([0, 0, 1, 0]); - this._rfbVeNCryptState = 4; - } else { - return this._fail("VeNCrypt Plain subtype not offered by server"); - } - } + // Look for a matching security type in the order that the + // server prefers + this._rfbAuthScheme = -1; + for (let type of subtypes) { + // Avoid getting in to a loop + if (type === securityTypeVeNCrypt) { + continue; + } - // negotiated Plain subtype, server waits for password - if (this._rfbVeNCryptState == 4) { - if (this._rfbCredentials.username === undefined || - this._rfbCredentials.password === undefined) { - this.dispatchEvent(new CustomEvent( - "credentialsrequired", - { detail: { types: ["username", "password"] } })); - return false; + if (this._isSupportedSecurityType(type)) { + this._rfbAuthScheme = type; + break; + } } - const user = encodeUTF8(this._rfbCredentials.username); - const pass = encodeUTF8(this._rfbCredentials.password); + if (this._rfbAuthScheme === -1) { + return this._fail("Unsupported security types (types: " + subtypes + ")"); + } - this._sock.send([ - (user.length >> 24) & 0xFF, - (user.length >> 16) & 0xFF, - (user.length >> 8) & 0xFF, - user.length & 0xFF - ]); - this._sock.send([ - (pass.length >> 24) & 0xFF, - (pass.length >> 16) & 0xFF, - (pass.length >> 8) & 0xFF, - pass.length & 0xFF - ]); - this._sock.sendString(user); - this._sock.sendString(pass); + this._sock.send([this._rfbAuthScheme >> 24, + this._rfbAuthScheme >> 16, + this._rfbAuthScheme >> 8, + this._rfbAuthScheme]); - this._rfbInitState = "SecurityResult"; + this._rfbVeNCryptState == 4; return true; } } + _negotiatePlainAuth() { + if (this._rfbCredentials.username === undefined || + this._rfbCredentials.password === undefined) { + this.dispatchEvent(new CustomEvent( + "credentialsrequired", + { detail: { types: ["username", "password"] } })); + return false; + } + + const user = encodeUTF8(this._rfbCredentials.username); + const pass = encodeUTF8(this._rfbCredentials.password); + + this._sock.send([ + (user.length >> 24) & 0xFF, + (user.length >> 16) & 0xFF, + (user.length >> 8) & 0xFF, + user.length & 0xFF + ]); + this._sock.send([ + (pass.length >> 24) & 0xFF, + (pass.length >> 16) & 0xFF, + (pass.length >> 8) & 0xFF, + pass.length & 0xFF + ]); + this._sock.sendString(user); + this._sock.sendString(pass); + + this._rfbInitState = "SecurityResult"; + return true; + } + _negotiateStdVNCAuth() { if (this._sock.rQwait("auth challenge", 16)) { return false; } @@ -1877,6 +1895,9 @@ export default class RFB extends EventTargetMixin { case securityTypeVeNCrypt: return this._negotiateVeNCryptAuth(); + case securityTypePlain: + return this._negotiatePlainAuth(); + case securityTypeUnixLogon: return this._negotiateTightUnixAuth(); diff --git a/tests/test.rfb.js b/tests/test.rfb.js index 0e46ff4c..75d1e118 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -1463,18 +1463,70 @@ describe('Remote Frame Buffer Protocol Client', function () { expect(client._fail).to.have.been.calledOnce; }); - it('should fail if the Plain authentication is not present', function () { + it('should fail if there are no supported subtypes', function () { // VeNCrypt version client._sock._websocket._receiveData(new Uint8Array([0, 2])); expect(client._sock).to.have.sent(new Uint8Array([0, 2])); // Server ACK. client._sock._websocket._receiveData(new Uint8Array([0])); - // Subtype list, only list subtype 1. + // Subtype list sinon.spy(client, "_fail"); - client._sock._websocket._receiveData(new Uint8Array([1, 0, 0, 0, 1])); + client._sock._websocket._receiveData(new Uint8Array([2, 0, 0, 0, 9, 0, 0, 1, 4])); expect(client._fail).to.have.been.calledOnce; }); + it('should support standard types', function () { + // VeNCrypt version + client._sock._websocket._receiveData(new Uint8Array([0, 2])); + expect(client._sock).to.have.sent(new Uint8Array([0, 2])); + // Server ACK. + client._sock._websocket._receiveData(new Uint8Array([0])); + // Subtype list + client._sock._websocket._receiveData(new Uint8Array([2, 0, 0, 0, 2, 0, 0, 1, 4])); + + let expectedResponse = []; + push32(expectedResponse, 2); // Chosen subtype. + + expect(client._sock).to.have.sent(new Uint8Array(expectedResponse)); + }); + + it('should respect server preference order', function () { + // VeNCrypt version + client._sock._websocket._receiveData(new Uint8Array([0, 2])); + expect(client._sock).to.have.sent(new Uint8Array([0, 2])); + // Server ACK. + client._sock._websocket._receiveData(new Uint8Array([0])); + // Subtype list + let subtypes = [ 6 ]; + push32(subtypes, 79); + push32(subtypes, 30); + push32(subtypes, 188); + push32(subtypes, 256); + push32(subtypes, 6); + push32(subtypes, 1); + client._sock._websocket._receiveData(new Uint8Array(subtypes)); + + let expectedResponse = []; + push32(expectedResponse, 30); // Chosen subtype. + + expect(client._sock).to.have.sent(new Uint8Array(expectedResponse)); + }); + + it('should ignore redundant VeNCrypt subtype', function () { + // VeNCrypt version + client._sock._websocket._receiveData(new Uint8Array([0, 2])); + expect(client._sock).to.have.sent(new Uint8Array([0, 2])); + // Server ACK. + client._sock._websocket._receiveData(new Uint8Array([0])); + // Subtype list + client._sock._websocket._receiveData(new Uint8Array([2, 0, 0, 0, 19, 0, 0, 0, 2])); + + let expectedResponse = []; + push32(expectedResponse, 2); // Chosen subtype. + + expect(client._sock).to.have.sent(new Uint8Array(expectedResponse)); + }); + it('should support Plain authentication', function () { client._rfbCredentials = { username: 'username', password: 'password' }; // VeNCrypt version