8000 feat(ssrf): move schemes to shared include data by fzipi · Pull Request #2599 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(ssrf): move schemes to shared include data #2599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jun 14, 2022
Merged

feat(ssrf): move schemes to shared include data #2599

merged 8 commits into from
Jun 14, 2022

Conversation

fzipi
Copy link
Member
@fzipi fzipi commented May 23, 2022 •
edited
Loading

Signed-off-by: Felipe Zipitria felipe.zipitria@owasp.org

Move the embedded list of schemes so it can be used by other RFI rules. Also we keep the list centralized.

@fzipi fzipi added the 🚀 enhancement New feature or request label May 23, 2022
@fzipi fzipi force-pushed the move-protocols-include-data branch from b813346 to 7750409 Compare May 23, 2022 18:24
theseion
theseion requested changes May 23, 2022
View reviewed changes
Copy link
Contributor
@theseion theseion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename protocols.data to url-schemes.data. "Scheme" is the term used by the RFC.

theseion
theseion reviewed May 23, 2022
View reviewed changes
@fzipi fzipi changed the title feat(ssrf): move protocols to shared include data feat(ssrf): move schemes to shared include data May 24, 2022
@fzipi fzipi mentioned this pull request Jun 5, 2022
Closed
@fzipi fzipi self-assigned this Jun 7, 2022
@fzipi fzipi force-pushed the move-protocols-include-data branch 2 times, most recently from 206255b to 75ca246 Compare June 11, 2022 14:49
@theseion theseion force-pushed the move-protocols-include-data branch from 75ca246 to 81f4d00 Compare June 12, 2022 06:37
@theseion
Copy link
Contributor

I've replaced the complex IPv6 regular expressions with a single simple one. It doesn't make sense to replicate the specification, we only need to find strings that look like IPv6 (the delimiting brackets [, ] make this simple). In fact, replicating the spec would reduce performance and, potentially, open us up to more bypasses.

I've also expanded the "enclosed alphanumerics" matches to match all existing code points instead of matching only specific strings.

IMPORTANT: this PR relies on two PRs to fix bugs in regexp-assemble: #2624, #2623.

@theseion
Copy link
Contributor

One thing I'm unsure about are evasions of the type http://google.com:80+&@google.com:80#+@127.88.23.245:22/. I don't know what the mechanism behind the evasion is. Currently, the + character is matched literally but I feel like it might actually be a URL encoded space character. Any ideas? @theMiddleBlue maybe?

@fzipi
Copy link
Member Author
fzipi commented Jun 12, 2022

It is a space. See this presentation
image

@fzipi fzipi requested a review from theseion June 12, 2022 11:51
@fzipi
Copy link
Member Author
fzipi commented Jun 12, 2022

Ughhhhh.. We might want to include this one?

python3 -c 'import socket
host = """\\l\\o\\c\\a\\l\\h\\o\\s\\t"""
print(host)
print(socket.gethostbyname(host))'
\l\o\c\a\l\h\o\s\t
127.0.0.1

@fzipi fzipi force-pushed the move-protocols-include-data branch from 890ce6c to 259dd9a Compare June 12, 2022 14:03
@fzipi
Copy link
Member Author
fzipi commented Jun 13, 2022

@theseion You are still appearing as "requested changes" here. Maybe you can approve?

@theseion
Copy link
Contributor

Requires a fix to Regexp::Assemble. Will updated ASAP.

@theseion theseion force-pushed the move-protocols-include-data branch from 259dd9a to 1189d00 Compare June 14, 2022 05:25
@theseion
Copy link
Contributor

@fzipi I have a working version (without having fixed the regexp-assemble bug yet).

@theseion
Copy link
Contributor

I think there's still a duplicate in the tests but I wanted to get this to you as fast as possible.

@fzipi
Copy link
Member Author
fzipi commented Jun 14, 2022 •
edited
Loading

@theseion Awesome! But the bug is affecting the regex creation, right? In that case, are we going to merge?

@theseion
Copy link
Contributor

The bug affects regex creation but only under specific circumstances, which, apart from me, no one is going to run into at the moment. So yes, merge when you think it's good.

@fzipi fzipi merged commit cd973ed into coreruleset:v4.0/dev Jun 14, 2022
@fzipi fzipi deleted the move-protocols-include-data branch June 14, 2022 12:51
@dune73 dune73 mentioned this pull request Jul 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theseion theseion theseion approved these changes

Assignees

@fzipi fzipi

Labels
🚀 enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fzipi @theseion
0