fix(ssrf): catch malformed urls#4410
Conversation
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
|
📊 Quantitative test results for language: |
There was a problem hiding this comment.
Pull request overview
This PR addresses a security bypass in SSRF detection where Node.js parsers accept and normalize malformed URLs with missing slashes (e.g., http:127.0.0.1 becomes http://127.0.0.1). The fix modifies the URL scheme pattern to match URLs with 0, 1, or 2 slashes after the colon, preventing attackers from bypassing SSRF protections using malformed URL syntax.
Changes:
- Updated regex pattern from
://to:/?/?to catch malformed URLs with missing or incomplete slashes - Added comprehensive test coverage for various malformed URL formats including dotless decimal, hexadecimal, octal, and IPv6 addresses
- Added negative tests to prevent false positives on invalid formats
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| regex-assembly/934120.ra | Changed URL scheme pattern from :// to :/?/? to match malformed URLs with optional slashes |
| rules/REQUEST-934-APPLICATION-ATTACK-GENERIC.conf | Updated compiled regex for rule 934120 to include the new pattern |
| rules/ssrf.data | Added http:127.0.0.1 as a test case for malformed localhost bypass |
| tests/regression/tests/REQUEST-934-APPLICATION-ATTACK-GENERIC/934120.yaml | Added 8 new tests (5 positive, 3 negative) covering various malformed URL formats |
| tests/regression/tests/REQUEST-934-APPLICATION-ATTACK-GENERIC/934110.yaml | Added test for malformed localhost URL detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
|
@fzipi Wouldn't the RFI rules also be affected by this parsing issue? |
|
Maybe? Started with this for now. |
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
| http://0/ | ||
| http://127.1 | ||
| http://127.0.1 | ||
| http:127.0.0.1 |
There was a problem hiding this comment.
http://0x7f.1 also evaluates to http://127.0.0.1.
Turns out Mixed(legacy) formats of IP addressing is supported, e.g. http://93.0xB8.0330.42/ evaluates to http://93.184.216.42/.
There was a problem hiding this comment.
Damned browsers. Now.. is this only happening in browsers, or frameworks do the same? Can you give me some examples?
There was a problem hiding this comment.
| \d{7,10} | ||
|
|
||
| ##! http://0xA9.0xFE.0xA9.0xFE/ Dotted hexadecimal | ||
| (?:0x[a-f0-9]{2}\.){3}0x[a-f0-9]{2} |
There was a problem hiding this comment.
Should we improve the regex above?
There was a problem hiding this comment.
Hmmm, I would say let's create a mixed legacy one, with extensive testing.
There was a problem hiding this comment.
Yes, but another rule makes some of the regex in this file unnecessary.
There was a problem hiding this comment.
I meant not another rule, but another regexp in the same .ra file. If we need to clean others and the tests keep passing, that is fine with me.
|
@Xhoenix I'm confused. Are you adding to this PR, or you'll wait until it is merged and add on top afterwards? |
|
@Xhoenix Let's merge and you can add on top of it. |
what
why
refs