Refactor ssrf module for enhanced security and configuration#6
Conversation
Refactor ssrf module to include configuration options for blacklist, allowlist, and path behavior. Enhance URL validation and DNS checks to improve security against SSRF attacks.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the SSRF (Server-Side Request Forgery) protection module to provide more robust security controls through comprehensive configuration options. The changes replace a basic hostname blacklist approach with a sophisticated multi-layered validation system that includes allowlists, CIDR-based network filtering, port restrictions, and enhanced URL parsing.
Key changes:
- Replaced legacy
urlmodule parsing with WHATWG URL API for better URL handling and security - Added support for allowlist/whitelist, CIDR allow/deny rules, custom IP range blocking, and port allowlisting
- Enhanced detection of obfuscated IPv4 addresses (hex, octal, dword notations) to prevent bypasses
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const address = await new Promise((resolve, reject) => { | ||
| dns.lookup(hostname, { all: false, family: 0, hints: dns.ADDRCONFIG | dns.V4MAPPED }, (err, address) => { | ||
| if (err) return reject(err) | ||
| resolve(address) | ||
| }) | ||
| }) | ||
| if (typeof address === 'string') results.push(address) | ||
| else if (address && address.address) results.push(address.address) |
There was a problem hiding this comment.
The dns.lookup call uses all: false but should use all: true to retrieve all addresses for the hostname. With all: false, only a single address is returned, which may miss other IPs that could be malicious. This creates a security vulnerability where an attacker could use DNS round-robin to bypass IP checks.
| const address = await new Promise((resolve, reject) => { | |
| dns.lookup(hostname, { all: false, family: 0, hints: dns.ADDRCONFIG | dns.V4MAPPED }, (err, address) => { | |
| if (err) return reject(err) | |
| resolve(address) | |
| }) | |
| }) | |
| if (typeof address === 'string') results.push(address) | |
| else if (address && address.address) results.push(address.address) | |
| const addresses = await new Promise((resolve, reject) => { | |
| dns.lookup(hostname, { all: true, family: 0, hints: dns.ADDRCONFIG | dns.V4MAPPED }, (err, addresses) => { | |
| if (err) return reject(err) | |
| resolve(addresses) | |
| }) | |
| }) | |
| if (Array.isArray(addresses)) { | |
| for (const addrObj of addresses) { | |
| if (addrObj && addrObj.address) results.push(addrObj.address) | |
| } | |
| } |
| if (typeof address === 'string') results.push(address) | ||
| else if (address && address.address) results.push(address.address) |
There was a problem hiding this comment.
When all: true is used with dns.lookup, the callback receives an array of address objects, not a single address. This code incorrectly handles the result by checking if it's a string or object, when it should iterate over an array. The correct handling should be: if (Array.isArray(address)) { results.push(...address.map(a => a.address)) }
| if (nums.every(x => x !== null && x >= 0)) { | ||
| let dword = null | ||
| if (nums.length === 4) { | ||
| if (nums.every(x => x <= 255)) dword = (nums[0] << 24) | (nums[1] << 16) | (nums[2] << 8) | nums[3] |
There was a problem hiding this comment.
Bitwise shift operators in JavaScript convert operands to 32-bit signed integers, which can produce negative values when the most significant bit is set. For example, 255 << 24 produces -16777216. Use >>> 0 after the expression to convert to unsigned: dword = ((nums[0] << 24) | (nums[1] << 16) | (nums[2] << 8) | nums[3]) >>> 0
| if (nums.length === 4) { | ||
| if (nums.every(x => x <= 255)) dword = (nums[0] << 24) | (nums[1] << 16) | (nums[2] << 8) | nums[3] | ||
| } else if (nums.length === 3) { | ||
| if (nums[0] <= 255 && nums[1] <= 255 && nums[2] <= 0xFFFF) dword = (nums[0] << 24) | (nums[1] << 16) | nums[2] |
There was a problem hiding this comment.
Same bitwise shift issue as above - the result can be a negative signed integer. Apply unsigned conversion: dword = ((nums[0] << 24) | (nums[1] << 16) | nums[2]) >>> 0
| } else if (nums.length === 3) { | ||
| if (nums[0] <= 255 && nums[1] <= 255 && nums[2] <= 0xFFFF) dword = (nums[0] << 24) | (nums[1] << 16) | nums[2] | ||
| } else if (nums.length === 2) { | ||
| if (nums[0] <= 255 && nums[1] <= 0xFFFFFF) dword = (nums[0] << 24) | nums[1] |
There was a problem hiding this comment.
Same bitwise shift issue - needs unsigned conversion: dword = ((nums[0] << 24) | nums[1]) >>> 0
| if (nums[0] <= 255 && nums[1] <= 0xFFFFFF) dword = (nums[0] << 24) | nums[1] | |
| if (nums[0] <= 255 && nums[1] <= 0xFFFFFF) dword = ((nums[0] << 24) | nums[1]) >>> 0 |
Refactor ssrf module to include configuration options for blacklist, allowlist, and path behavior. Enhance URL validation and DNS checks to improve security against SSRF attacks.