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.
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
Swift: New query: Incomplete regular expression for hostnames #14034
base: main
Are you sure you want to change the base?
Conversation
|
QHelp previews: swift/ql/src/queries/Security/CWE-020/IncompleteHostnameRegex.qhelpIncomplete regular expression for hostnamesSanitizing untrusted URLs is an important technique for preventing attacks such as request forgeries and malicious redirections. Often, this is done by checking that the host of a URL is in a set of allowed hosts. If a regular expression implements such a check, it is easy to accidentally make the check too permissive by not escaping the RecommendationEscape all meta-characters appropriately when constructing regular expressions for security checks, and pay special attention to the ExampleThe following example code checks that a URL redirection will reach the The check is, however, easy to bypass because the unescaped Address this vulnerability by escaping References
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only reviewed the QL code so far.
Co-authored-by: Mathias Vorreiter Pedersen <mathiasvp@github.com>
|
DCA LGTM. It shows a slight (~3%) overall analysis slowdown, which may be wobble or the real effect of adding a new query. I will check the most slowed down project ( |
|
I'll review this on behalf of Docs! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geoffw0 - this LGTM ✨
Added a few editorial suggestions (feel free to ignore the ones you don't agree with).
| @@ -0,0 +1,16 @@ | |||
| /** | |||
| * @name Incomplete regular expression for hostnames | |||
| * @description Matching a URL or hostname against a regular expression that contains an unescaped dot as part of the hostname might match more hostnames than expected. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description looks fine, but perhaps we could replace the second occurrence of "to match" so that there's less repetition. I've used "to return" but perhaps you can find a better synonym. Also used the present for "may".
| * @description Matching a URL or hostname against a regular expression that contains an unescaped dot as part of the hostname might match more hostnames than expected. | |
| * @description Matching a URL or hostname against a regular expression that contains an unescaped dot as part of the hostname may return more hostnames than expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 "may", I've made that change.
I'm not quite happy with the word "return" here since we don't know whether the user code we've identified is really "returning" anything (or whether it's doing something else with the match). Sadly I can't find a good alternative word so I've left the repetition of "match" for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geoffw0 - that's fine, let's lave it as-is. Thanks for addressing my other comments 👍🏻
swift/ql/src/change-notes/2023-08-23-incomplete-hostname-regex.md
Outdated
Show resolved
Hide resolved
swift/ql/src/queries/Security/CWE-020/IncompleteHostnameRegex.qhelp
Outdated
Show resolved
Hide resolved
swift/ql/src/queries/Security/CWE-020/IncompleteHostnameRegex.qhelp
Outdated
Show resolved
Hide resolved
swift/ql/src/queries/Security/CWE-020/IncompleteHostnameRegex.qhelp
Outdated
Show resolved
Hide resolved
Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
|
@mchammer01 thanks for the speedy review! I've accepted or addressed all your comments, with one slight exception (see above). Please say if you have any more suggestions there or elsewhere. |
|
Locally, on the |
|
I think we just need someone from the team to have a quick read of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from an editorial perspective ✨
Adds a new Swift query "Incomplete regular expression for hostnames". This is a port of
js/incomplete-hostname-regexp; Ruby, Go and Python have similar queries as well.shared/regex/codeql/regex/HostnameRegexpfor the query, with a thin layer inswift/ql/lib/codeql/swift/security/regex/HostnameRegex.qllconnecting that to the Swift libraries.swift/ql/lib/codeql/swift/regex/Regex.qll.ParsedStringRegexis changed to extend a new classRegexPatternSourceand ultimatelyDataFlow::Noderather thanExpr, as that is the configuration of classes the shared library expects to find.Before merging: