From 70ce93744a7cf7467c27f45b36c1060ccc56124e Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 10 Apr 2023 16:52:12 -0400 Subject: [PATCH 1/4] fix(NODE-5042): relax SRV record validation to account for a dot suffix --- src/connection_string.ts | 16 +--------------- src/sdam/srv_polling.ts | 17 +---------------- src/utils.ts | 18 ++++++++++++++++++ test/unit/utils.test.ts | 40 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 31 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index 9b554372739..5e6e3ae5d27 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -33,6 +33,7 @@ import { HostAddress, isRecord, makeClientMetadata, + matchesParentDomain, parseInteger, setDifference } from './utils'; @@ -45,21 +46,6 @@ const LB_REPLICA_SET_ERROR = 'loadBalanced option not supported with a replicaSe const LB_DIRECT_CONNECTION_ERROR = 'loadBalanced option not supported when directConnection is provided'; -/** - * Determines whether a provided address matches the provided parent domain in order - * to avoid certain attack vectors. - * - * @param srvAddress - The address to check against a domain - * @param parentDomain - The domain to check the provided address against - * @returns Whether the provided address matches the parent domain - */ -function matchesParentDomain(srvAddress: string, parentDomain: string): boolean { - const regex = /^.*?\./; - const srv = `.${srvAddress.replace(regex, '')}`; - const parent = `.${parentDomain.replace(regex, '')}`; - return srv.endsWith(parent); -} - /** * Lookup a `mongodb+srv` connection string, combine the parts and reparse it as a normal * connection string. diff --git a/src/sdam/srv_polling.ts b/src/sdam/srv_polling.ts index 253f3a74d23..8606abc059f 100644 --- a/src/sdam/srv_polling.ts +++ b/src/sdam/srv_polling.ts @@ -3,22 +3,7 @@ import { clearTimeout, setTimeout } from 'timers'; import { MongoRuntimeError } from '../error'; import { TypedEventEmitter } from '../mongo_types'; -import { HostAddress } from '../utils'; - -/** - * Determines whether a provided address matches the provided parent domain in order - * to avoid certain attack vectors. - * - * @param srvAddress - The address to check against a domain - * @param parentDomain - The domain to check the provided address against - * @returns Whether the provided address matches the parent domain - */ -function matchesParentDomain(srvAddress: string, parentDomain: string): boolean { - const regex = /^.*?\./; - const srv = `.${srvAddress.replace(regex, '')}`; - const parent = `.${parentDomain.replace(regex, '')}`; - return srv.endsWith(parent); -} +import { HostAddress, matchesParentDomain } from '../utils'; /** * @internal diff --git a/src/utils.ts b/src/utils.ts index dee4bc51ae3..1832d888251 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1277,3 +1277,21 @@ export function parseUnsignedInteger(value: unknown): number | null { return parsedInt != null && parsedInt >= 0 ? parsedInt : null; } + +/** + * Determines whether a provided address matches the provided parent domain. + * + * If a DNS server were to become compromised SRV records would still need to + * advertise addresses that are under the same domain as the srvHost. + * + * @param address - The address to check against a domain + * @param srvHost - The domain to check the provided address against + * @returns Whether the provided address matches the parent domain + */ +export function matchesParentDomain(address: string, srvHost: string): boolean { + const regex = /^.*?\./; + const srvAddress = address.endsWith('.') ? address.slice(0, address.length - 1) : address; + const srv = `.${srvAddress.replace(regex, '')}`; + const parent = `.${srvHost.replace(regex, '')}`; + return srv.endsWith(parent); +} diff --git a/test/unit/utils.test.ts b/test/unit/utils.test.ts index ad587853ba9..8b29a95463f 100644 --- a/test/unit/utils.test.ts +++ b/test/unit/utils.test.ts @@ -9,6 +9,7 @@ import { isHello, LEGACY_HELLO_COMMAND, List, + matchesParentDomain, maybeCallback, MongoDBNamespace, MongoRuntimeError, @@ -899,4 +900,43 @@ describe('driver utils', function () { }); }); }); + + describe('matchesParentDomain()', () => { + const exampleSrvName = 'i-love-javascript.mongodb.io'; + const exampleHostNamesWithoutDots = [ + 'i-love-javascript-00.mongodb.io', + 'i-love-javascript-01.mongodb.io', + 'i-love-javascript-02.mongodb.io' + ]; + const exampleHostNamesWithDots = exampleHostNamesWithoutDots.map(hn => hn + '.'); + const exampleHostNamThatDoNotMatchParent = 'i-love-javascript-00.evilJsHaters.io'; + const exampleHostNamThatDoNotMatchParentWithDot = 'i-love-javascript-00.evilJsHaters.io.'; + + context('when address does not match parent domain', () => { + it('without a trailing dot returns false', () => { + expect(matchesParentDomain(exampleHostNamThatDoNotMatchParent, exampleSrvName)).to.be.false; + }); + + it('with a trailing dot returns false', () => { + expect(matchesParentDomain(exampleHostNamThatDoNotMatchParentWithDot, exampleSrvName)).to.be + .false; + }); + }); + + context('when addresses in SRV record end with dots', () => { + it('accepts address since it is considered to still match the parent domain', () => { + for (const host of exampleHostNamesWithDots) { + expect(matchesParentDomain(host, exampleSrvName)).to.be.true; + } + }); + }); + + context('when addresses in SRV record end without dots', () => { + it('accepts address since it matches the parent domain', () => { + for (const host of exampleHostNamesWithoutDots) { + expect(matchesParentDomain(host, exampleSrvName)).to.be.true; + } + }); + }); + }); }); From d3fbc4ede3d85cee43ed778df5601ff4680891c1 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 11 Apr 2023 14:00:23 -0400 Subject: [PATCH 2/4] fix: comments --- src/utils.ts | 4 ++-- test/unit/utils.test.ts | 16 ++++------------ 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 1832d888251..4cbc83bed26 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1291,7 +1291,7 @@ export function parseUnsignedInteger(value: unknown): number | null { export function matchesParentDomain(address: string, srvHost: string): boolean { const regex = /^.*?\./; const srvAddress = address.endsWith('.') ? address.slice(0, address.length - 1) : address; - const srv = `.${srvAddress.replace(regex, '')}`; - const parent = `.${srvHost.replace(regex, '')}`; + const srv = srvAddress.replace(regex, ''); + const parent = srvHost.replace(regex, ''); return srv.endsWith(parent); } diff --git a/test/unit/utils.test.ts b/test/unit/utils.test.ts index 8b29a95463f..85892650892 100644 --- a/test/unit/utils.test.ts +++ b/test/unit/utils.test.ts @@ -903,12 +903,8 @@ describe('driver utils', function () { describe('matchesParentDomain()', () => { const exampleSrvName = 'i-love-javascript.mongodb.io'; - const exampleHostNamesWithoutDots = [ - 'i-love-javascript-00.mongodb.io', - 'i-love-javascript-01.mongodb.io', - 'i-love-javascript-02.mongodb.io' - ]; - const exampleHostNamesWithDots = exampleHostNamesWithoutDots.map(hn => hn + '.'); + const exampleHostNameWithoutDot = 'i-love-javascript-00.mongodb.io'; + const exampleHostNamesWithDot = exampleHostNameWithoutDot + '.'; const exampleHostNamThatDoNotMatchParent = 'i-love-javascript-00.evilJsHaters.io'; const exampleHostNamThatDoNotMatchParentWithDot = 'i-love-javascript-00.evilJsHaters.io.'; @@ -925,17 +921,13 @@ describe('driver utils', function () { context('when addresses in SRV record end with dots', () => { it('accepts address since it is considered to still match the parent domain', () => { - for (const host of exampleHostNamesWithDots) { - expect(matchesParentDomain(host, exampleSrvName)).to.be.true; - } + expect(matchesParentDomain(exampleHostNamesWithDot, exampleSrvName)).to.be.true; }); }); context('when addresses in SRV record end without dots', () => { it('accepts address since it matches the parent domain', () => { - for (const host of exampleHostNamesWithoutDots) { - expect(matchesParentDomain(host, exampleSrvName)).to.be.true; - } + expect(matchesParentDomain(exampleHostNamesWithDot, exampleSrvName)).to.be.true; }); }); }); From 355d8b19d0fe9c1f40c99de91add5b5357457baf Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 11 Apr 2023 15:10:22 -0400 Subject: [PATCH 3/4] fix: leading dot, and test coverage --- src/utils.ts | 4 ++-- test/unit/utils.test.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 4cbc83bed26..1832d888251 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1291,7 +1291,7 @@ export function parseUnsignedInteger(value: unknown): number | null { export function matchesParentDomain(address: string, srvHost: string): boolean { const regex = /^.*?\./; const srvAddress = address.endsWith('.') ? address.slice(0, address.length - 1) : address; - const srv = srvAddress.replace(regex, ''); - const parent = srvHost.replace(regex, ''); + const srv = `.${srvAddress.replace(regex, '')}`; + const parent = `.${srvHost.replace(regex, '')}`; return srv.endsWith(parent); } diff --git a/test/unit/utils.test.ts b/test/unit/utils.test.ts index 85892650892..b5c8c5c19de 100644 --- a/test/unit/utils.test.ts +++ b/test/unit/utils.test.ts @@ -905,8 +905,8 @@ describe('driver utils', function () { const exampleSrvName = 'i-love-javascript.mongodb.io'; const exampleHostNameWithoutDot = 'i-love-javascript-00.mongodb.io'; const exampleHostNamesWithDot = exampleHostNameWithoutDot + '.'; - const exampleHostNamThatDoNotMatchParent = 'i-love-javascript-00.evilJsHaters.io'; - const exampleHostNamThatDoNotMatchParentWithDot = 'i-love-javascript-00.evilJsHaters.io.'; + const exampleHostNamThatDoNotMatchParent = 'i-love-javascript-00.evil-mongodb.io'; + const exampleHostNamThatDoNotMatchParentWithDot = 'i-love-javascript-00.evil-mongodb.io.'; context('when address does not match parent domain', () => { it('without a trailing dot returns false', () => { From cfbcaf2222f253410463f70d43d7e20d8397aaee Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 11 Apr 2023 17:17:37 -0400 Subject: [PATCH 4/4] permit srvHost to end with a dot --- src/utils.ts | 18 +++++++++++++----- test/unit/utils.test.ts | 13 ++++++++++++- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 1832d888251..2533f8cc05d 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1289,9 +1289,17 @@ export function parseUnsignedInteger(value: unknown): number | null { * @returns Whether the provided address matches the parent domain */ export function matchesParentDomain(address: string, srvHost: string): boolean { - const regex = /^.*?\./; - const srvAddress = address.endsWith('.') ? address.slice(0, address.length - 1) : address; - const srv = `.${srvAddress.replace(regex, '')}`; - const parent = `.${srvHost.replace(regex, '')}`; - return srv.endsWith(parent); + // Remove trailing dot if exists on either the resolved address or the srv hostname + const normalizedAddress = address.endsWith('.') ? address.slice(0, address.length - 1) : address; + const normalizedSrvHost = srvHost.endsWith('.') ? srvHost.slice(0, srvHost.length - 1) : srvHost; + + const allCharacterBeforeFirstDot = /^.*?\./; + // Remove all characters before first dot + // Add leading dot back to string so + // an srvHostDomain = '.trusted.site' + // will not satisfy an addressDomain that endsWith '.fake-trusted.site' + const addressDomain = `.${normalizedAddress.replace(allCharacterBeforeFirstDot, '')}`; + const srvHostDomain = `.${normalizedSrvHost.replace(allCharacterBeforeFirstDot, '')}`; + + return addressDomain.endsWith(srvHostDomain); } diff --git a/test/unit/utils.test.ts b/test/unit/utils.test.ts index b5c8c5c19de..df0efc599eb 100644 --- a/test/unit/utils.test.ts +++ b/test/unit/utils.test.ts @@ -903,6 +903,7 @@ describe('driver utils', function () { describe('matchesParentDomain()', () => { const exampleSrvName = 'i-love-javascript.mongodb.io'; + const exampleSrvNameWithDot = 'i-love-javascript.mongodb.io.'; const exampleHostNameWithoutDot = 'i-love-javascript-00.mongodb.io'; const exampleHostNamesWithDot = exampleHostNameWithoutDot + '.'; const exampleHostNamThatDoNotMatchParent = 'i-love-javascript-00.evil-mongodb.io'; @@ -919,12 +920,22 @@ describe('driver utils', function () { }); }); - context('when addresses in SRV record end with dots', () => { + context('when addresses in SRV record end with a dot', () => { it('accepts address since it is considered to still match the parent domain', () => { expect(matchesParentDomain(exampleHostNamesWithDot, exampleSrvName)).to.be.true; }); }); + context('when SRV host ends with a dot', () => { + it('accepts address if it ends with a dot', () => { + expect(matchesParentDomain(exampleHostNamesWithDot, exampleSrvNameWithDot)).to.be.true; + }); + + it('accepts address if it does not end with a dot', () => { + expect(matchesParentDomain(exampleHostNameWithoutDot, exampleSrvName)).to.be.true; + }); + }); + context('when addresses in SRV record end without dots', () => { it('accepts address since it matches the parent domain', () => { expect(matchesParentDomain(exampleHostNamesWithDot, exampleSrvName)).to.be.true;