Skip to content

Commit 211846a

Browse files
authored
fix: prevent subdomain confusion attacks in RPC domain validation (#17234)
Replace vulnerable `endsWith()` with secure dot-prefix validation to prevent malicious domains like 'evilinfura.io' from being misclassified. - Add `ALLOWED_PROVIDER_DOMAINS` constant for clear business logic - Add `isAllowedProviderDomain()` with secure `endsWith('.${domain}')` validation - Simplify `extractRpcDomain()` provider validation logic - Add comprehensive security tests and edge case coverage <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** This PR fixes a security vulnerability where `extractRpcDomain()` used vulnerable `endsWith()` checks that allowed subdomain confusion attacks. Malicious domains like `evilinfura.io` or `fakealchemyapi.io` were incorrectly classified as legitimate provider domains. **What is the reason for the change?** The security audit flagged "Incomplete URL substring sanitization" because `endsWith('infura.io')` accepts any domain ending with those characters, including malicious domains. **What is the improvement/solution?** Replaced vulnerable `endsWith('infura.io')` with secure `endsWith('.infura.io')` validation. The dot prefix ensures only legitimate subdomains are accepted: - ✅ `mainnet.infura.io` ends with `.infura.io` → legitimate subdomain - ❌ `evilinfura.io` does NOT end with `.infura.io` → blocked as malicious ## **Changelog** CHANGELOG entry: Fixed security vulnerability in RPC domain validation that could allow malicious domains to be misclassified as legitimate providers ## **Related issues** Fixes: Security audit finding - "Incomplete URL substring sanitization" ## **Manual testing steps** 1. **Verify existing RPC domain tracking works**: - Submit transactions using default Infura RPC - Check logs show `"rpc_domain": "mainnet.infura.io"` in MetaMetrics events - Confirm both "Transaction Approved" and "Transaction Finalized" events include the property 2. **Security validation through unit tests**: - Run `yarn test app/util/rpc-domain-utils.test.ts` to verify: - Legitimate domains work: `mainnet.infura.io` → `"mainnet.infura.io"` - Malicious domains blocked: `evilinfura.io` → `"private"` - The security fix prevents subdomain confusion attacks 3. **Verification summary**: - Manual testing confirms existing functionality is preserved - Unit tests validate the security vulnerability is fixed ## **Screenshots/Recordings** https://github.com/user-attachments/assets/dcb9f1f0-a908-4de1-8a61-ab4aa41cde77 ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
1 parent 1b6ee57 commit 211846a

File tree

3 files changed

+143
-7
lines changed

3 files changed

+143
-7
lines changed

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
- fix: Security vulnerability in RPC domain validation that could allow malicious domains to be misclassified as legitimate providers ([#17234](https://github.com/MetaMask/metamask-mobile/pull/17234))
11+
1012
## [7.50.1]
1113

1214
### Fixed

app/util/rpc-domain-utils.test.ts

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,7 +303,122 @@ describe('rpc-domain-utils', () => {
303303
expect(result).toBe('known-domain.com');
304304
});
305305
});
306+
307+
describe('Security tests for provider domain validation', () => {
308+
beforeEach(async () => {
309+
setupTestEnvironment();
310+
const mockChains: SafeChain[] = [
311+
{
312+
chainId: '1',
313+
name: 'Test Chain',
314+
nativeCurrency: { symbol: 'TEST' },
315+
rpc: ['https://known-domain.com/api'],
316+
},
317+
];
318+
(StorageWrapper.getItem as jest.Mock).mockResolvedValue(
319+
JSON.stringify(mockChains),
320+
);
321+
await initializeRpcProviderDomains();
322+
});
323+
324+
describe('Infura provider domain validation', () => {
325+
it('returns domain for legitimate Infura domains and subdomains', () => {
326+
expect(extractRpcDomain('https://infura.io')).toBe('infura.io');
327+
expect(extractRpcDomain('https://mainnet.infura.io')).toBe(
328+
'mainnet.infura.io',
329+
);
330+
expect(extractRpcDomain('https://goerli.infura.io')).toBe(
331+
'goerli.infura.io',
332+
);
333+
expect(extractRpcDomain('https://api.infura.io')).toBe(
334+
'api.infura.io',
335+
);
336+
expect(extractRpcDomain('https://v1.api.infura.io')).toBe(
337+
'v1.api.infura.io',
338+
);
339+
});
340+
341+
it('returns private for malicious domains that end with infura.io but are not subdomains', () => {
342+
expect(extractRpcDomain('https://evilinfura.io')).toBe('private');
343+
expect(extractRpcDomain('https://malicious-infura.io')).toBe(
344+
'private',
345+
);
346+
expect(extractRpcDomain('https://fakeinfura.io')).toBe('private');
347+
expect(extractRpcDomain('https://notinfura.io')).toBe('private');
348+
});
349+
});
350+
351+
describe('Alchemy provider domain validation', () => {
352+
it('returns domain for legitimate Alchemy domains and subdomains', () => {
353+
expect(extractRpcDomain('https://alchemyapi.io')).toBe(
354+
'alchemyapi.io',
355+
);
356+
expect(extractRpcDomain('https://eth-mainnet.alchemyapi.io')).toBe(
357+
'eth-mainnet.alchemyapi.io',
358+
);
359+
expect(
360+
extractRpcDomain('https://polygon-mainnet.alchemyapi.io'),
361+
).toBe('polygon-mainnet.alchemyapi.io');
362+
expect(extractRpcDomain('https://eth.v2.alchemyapi.io')).toBe(
363+
'eth.v2.alchemyapi.io',
364+
);
365+
});
366+
367+
it('returns private for malicious domains that end with alchemyapi.io but are not subdomains', () => {
368+
expect(extractRpcDomain('https://evilalchemyapi.io')).toBe('private');
369+
expect(extractRpcDomain('https://malicious-alchemyapi.io')).toBe(
370+
'private',
371+
);
372+
expect(extractRpcDomain('https://fakealchemyapi.io')).toBe('private');
373+
expect(extractRpcDomain('https://notalchemyapi.io')).toBe('private');
374+
});
375+
});
376+
377+
describe('Edge cases for provider domain validation', () => {
378+
it('handles case sensitivity correctly', () => {
379+
expect(extractRpcDomain('https://MAINNET.INFURA.IO')).toBe(
380+
'mainnet.infura.io',
381+
);
382+
expect(extractRpcDomain('https://ETH-MAINNET.ALCHEMYAPI.IO')).toBe(
383+
'eth-mainnet.alchemyapi.io',
384+
);
385+
});
386+
387+
it('returns private for domains with similar but different TLDs', () => {
388+
expect(extractRpcDomain('https://mainnet.infura.com')).toBe(
389+
'private',
390+
);
391+
expect(extractRpcDomain('https://eth-mainnet.alchemyapi.com')).toBe(
392+
'private',
393+
);
394+
});
395+
396+
it('prevents subdomain confusion attacks (security fix validation)', () => {
397+
// These domains would have passed the old endsWith check but should be blocked
398+
const attackDomains = [
399+
'https://evilinfura.io',
400+
'https://maliciousinfura.io',
401+
'https://fakeinfura.io',
402+
'https://evilalchemyapi.io',
403+
'https://maliciousalchemyapi.io',
404+
'https://fakealchemyapi.io',
405+
];
406+
407+
attackDomains.forEach((domain) => {
408+
expect(extractRpcDomain(domain)).toBe('private');
409+
});
410+
});
411+
412+
it('handles exact base domain matches', () => {
413+
expect(extractRpcDomain('https://infura.io')).toBe('infura.io');
414+
expect(extractRpcDomain('https://alchemyapi.io')).toBe(
415+
'alchemyapi.io',
416+
);
417+
});
418+
});
419+
});
306420
});
421+
307422
describe('getNetworkRpcUrl', () => {
308423
describe('when retrieving RPC URLs', () => {
309424
it('returns RPC URL from legacy format', () => {

app/util/rpc-domain-utils.ts

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,26 @@ function parseDomain(url: string): string | undefined {
116116
}
117117
}
118118

119+
// Allowed provider domains for RPC endpoint validation
120+
const ALLOWED_PROVIDER_DOMAINS = new Set(['infura.io', 'alchemyapi.io']);
121+
122+
/**
123+
* Check if a hostname is an allowed provider domain or legitimate subdomain
124+
* @param hostname - The hostname to check
125+
* @returns True if the hostname is allowed, false otherwise
126+
*/
127+
function isAllowedProviderDomain(hostname: string): boolean {
128+
// Check exact match first
129+
if (ALLOWED_PROVIDER_DOMAINS.has(hostname)) {
130+
return true;
131+
}
132+
133+
// Check if it's a legitimate subdomain of any allowed domain
134+
return [...ALLOWED_PROVIDER_DOMAINS].some((allowedDomain) =>
135+
hostname.endsWith(`.${allowedDomain}`),
136+
);
137+
}
138+
119139
/**
120140
* Extracts the domain from an RPC URL for analytics tracking
121141
* @param rpcUrl - The RPC URL to extract domain from
@@ -126,23 +146,22 @@ export function extractRpcDomain(rpcUrl: string): RpcDomainStatus | string {
126146
if (!domain) {
127147
return RpcDomainStatus.Invalid;
128148
}
149+
129150
// Check if this is a known domain
130151
if (isKnownDomain(domain)) {
131152
return domain;
132153
}
133-
// Special case for Infura subdomains - always return the actual domain
134-
// even if not in the known domains list
135-
if (domain.includes('infura.io')) {
136-
return domain;
137-
}
138-
// Special case for Alchemy subdomains
139-
if (domain.endsWith('alchemyapi.io')) {
154+
155+
// Check if it's an allowed provider domain (Infura, Alchemy, etc.)
156+
if (isAllowedProviderDomain(domain)) {
140157
return domain;
141158
}
159+
142160
// Special case for local/development nodes
143161
if (domain === 'localhost' || domain === '127.0.0.1') {
144162
return RpcDomainStatus.Private;
145163
}
164+
146165
// For all other domains, return "private" for privacy
147166
return RpcDomainStatus.Private;
148167
}

0 commit comments

Comments
 (0)