Skip to content

Commit

Permalink
fix(NODE-5042): relax SRV record validation to account for a dot suff…
Browse files Browse the repository at this point in the history
…ix (#3633)
  • Loading branch information
nbbeeken committed Apr 12, 2023
1 parent 8a7ce1f commit ad15881
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 31 deletions.
16 changes: 1 addition & 15 deletions src/connection_string.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
HostAddress,
isRecord,
makeClientMetadata,
matchesParentDomain,
parseInteger,
setDifference
} from './utils';
Expand All @@ -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.
Expand Down
17 changes: 1 addition & 16 deletions src/sdam/srv_polling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 26 additions & 0 deletions src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1277,3 +1277,29 @@ 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 {
// 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);
}
43 changes: 43 additions & 0 deletions test/unit/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
isHello,
LEGACY_HELLO_COMMAND,
List,
matchesParentDomain,
maybeCallback,
MongoDBNamespace,
MongoRuntimeError,
Expand Down Expand Up @@ -899,4 +900,46 @@ 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';
const exampleHostNamThatDoNotMatchParentWithDot = 'i-love-javascript-00.evil-mongodb.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 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;
});
});
});
});

0 comments on commit ad15881

Please sign in to comment.