-
-
Notifications
You must be signed in to change notification settings - Fork 417
fix(security): resolve SQL injection protection bypass (942380 PL2) #3720
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
base: main
Are you sure you want to change the base?
Conversation
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.
In your bypass example you mention for example using \s instead of \s+ in regex
.
What is the difference on what you did? Does having
work with no spacing?
|
You could try removing all spaces with a transformation first. |
📊 Quantitative test results for language: |
Ready to review again. |
\bhaving\s*\(?\s*(?:['"]?\s*\d+\s*['"]?|['"]\s*[^\d]*\s*['"]|true|false)\s*(?:[=<>+\-*/%^&!|]+|div|mod) | ||
\bexecute\s*\( | ||
\bexecute\b\s*`?\s*[\w\.$]+\s*`? | ||
\bcreate\b\s+\btable\b\s*`?\s*.+?\s*`?\s*\( |
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 \b
s are redundant since \s+
implies non-word characters between the two strings.
\bcreate\b\s+\btable\b\s*`?\s*.+?\s*`?\s*\( | |
\bcreate\s+table\b\s*`?\s*.+?\s*`?\s*\( |
\bhaving\s*\(?\s*(?:['"]?\s*\d+\s*['"]?|['"]\s*[^\d]*\s*['"]|true|false)\s*(?:[=<>+\-*/%^&!|]+|div|mod) | ||
\bexecute\s*\( | ||
\bexecute\b\s*`?\s*[\w\.$]+\s*`? | ||
\bcreate\b\s+\btable\b\s*`?\s*.+?\s*`?\s*\( |
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.
Too complicated. Break it up into alternations, e.g.:
\bcreate\b\s+\btable\b\s*`?\s*.+?\s*`?\s*\( | |
\bcreate\b\s+\btable\b[^(]+\( | |
\bcreate\b\s+\btable\b\s*`\s*[^\s`]+\s*`\s*\( |
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.
@theseion But it's not the same. Your version will match also this:
create table
Mine is matching, at least, this:
create table (
I don't think this one can be splitted into parts.
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.
How do my proposed expressions match create table
? Both require \(
. ?
can always be written as an alternation (|
), in this case
...(?:`|)...
And [^\(]+
is just a simplification of the wildcards in the original expression. So yes, I'm pretty sure that's correct :).
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.
@theseion Hm, if i understand it correctly, you replaced this line:
\bcreate\b\s+\btable\b\s*`?\s*.+?\s*`?\s*\(
with these two?
\bcreate\b\s+\btable\b[^(]+\(
\bcreate\b\s+\btable\b\s*`\s*[^\s`]+\s*`\s*\(
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.
Yes. The first doesn't have the backtick enclosed part, the second always has backticks. The result would be (spaces added for readability):
\bcreate\b\s+\btable\b[^(]+\( | \bcreate\b\s+\btable\b\s*`\s*[^\s`]+\s*`\s*\(
Shortened:
\bcreate\b\s+\btable\b(?:[^(]+\( | \s*`\s*[^\s`]+\s*`\s*\()
\bhaving\b ?\d{1,10} ?[=<>]+ | ||
\bhaving\b ?[\'\"][^=]{1,10}[\'\" ?[=<>]+ | ||
\bcreate\s+?table.{0,20}?\( | ||
\bhaving\s*\(?\s*(?:['"]?\s*\d+\s*['"]?|['"]\s*[^\d]*\s*['"]|true|false)\s*(?:[=<>+\-*/%^&!|]+|div|mod) |
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.
Too complicated. Break it up.
exists\s\bhaving\b\s+\d{1,10} | ||
exists\s'[^=]{1,10}' | ||
\bexists\s*?\(\s*?select\b | ||
\bselect\b.*?\bcase\b |
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.
\bselect\b.*?\bcase\b | |
\bselect\b.+?\bcase\b |
\bexists\s*?\(\s*?select\b | ||
\bselect\b.*?\bcase\b | ||
\bfrom\b.*?\blimit\b | ||
\bfrom\b.*?\bgroup\b\s+\bby\b |
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.
\bfrom\b.*?\bgroup\b\s+\bby\b | |
\bfrom\b.+?\bgroup\b\s+\bby\b |
tests/regression/tests/REQUEST-942-APPLICATION-ATTACK-SQLI/942380.yaml
Outdated
Show resolved
Hide resolved
tests/regression/tests/REQUEST-942-APPLICATION-ATTACK-SQLI/942380.yaml
Outdated
Show resolved
Hide resolved
\bhaving\b ?\d{1,10} ?[=<>]+ | ||
\bhaving\b ?[\'\"][^=]{1,10}[\'\" ?[=<>]+ | ||
\bcreate\s+?table.{0,20}?\( | ||
\bhaving\s*\(?\s*(?:['"]?\s*\d+\s*['"]?|['"]\s*[^\d]*\s*['"]|true|false)\s*(?:[=<>+\-*/%^&!|]+|div|mod) |
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.
\bhaving\s*\(?\s*(?:['"]?\s*\d+\s*['"]?|['"]\s*[^\d]*\s*['"]|true|false)\s*(?:[=<>+\-*/%^&!|]+|div|mod) | |
\bhaving\s+\(?\s*(?:['"]?\s*\d+\s*['"]?|['"]\s*[^\d]*\s*['"]|true|false)\s*(?:[=<>+\-*/%^&!|]+|div|mod) |
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 think *
is correct here. The parser might accept ...having'...
, for example.
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 msg is SQL Injection Attack
, atleast MySQL expects a space after HAVING, haven't checked others. Maybe @azurit has more info about this.
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 don't think it matters really in terms of the regex. As long as we assume that it can happen we should be fine either way.
I found TONS of ways how to bypass rule
942380
and some of them were extremely easy (for example using\s
instead of\s+
in regex).We are now able to catch these new ways of injection:
Also, we are now catching these:
We are still NOT catching these: