mirror of
https://github.com/github/codeql.git
synced 2025-12-22 11:46:32 +01:00
add suspicious-regexp-range query
This commit is contained in:
Erik Krogh Kristensen
@@ -0,0 +1,302 @@
|
||||
/**
|
||||
* Classes and predicates for working with suspicious character ranges.
|
||||
*/
|
||||
|
||||
// We don't need the NFA utils, just the regexp tree.
|
||||
// but the below is a nice shared library that exposes the API we need.
|
||||
import performance.ReDoSUtil
|
||||
|
||||
/**
|
||||
* Gets a rank for `range` that is unique for ranges in the same file.
|
||||
* Prioritizes ranges that match more characters.
|
||||
*/
|
||||
int rankRange(RegExpCharacterRange range) {
|
||||
range =
|
||||
rank[result](RegExpCharacterRange r, Location l, int low, int high |
|
||||
r.getLocation() = l and
|
||||
isRange(r, low, high)
|
||||
|
|
||||
r order by (high - low) desc, l.getStartLine(), l.getStartColumn()
|
||||
)
|
||||
}
|
||||
|
||||
/** Holds if `range` spans from the unicode code points `low` to `high` (both inclusive). */
|
||||
predicate isRange(RegExpCharacterRange range, int low, int high) {
|
||||
exists(string lowc, string highc |
|
||||
range.isRange(lowc, highc) and
|
||||
low.toUnicode() = lowc and
|
||||
high.toUnicode() = highc
|
||||
)
|
||||
}
|
||||
|
||||
/** Holds if `char` is an alpha-numeric character. */
|
||||
predicate isAlphanumeric(string char) {
|
||||
// written like this to avoid having a bindingset for the predicate
|
||||
char = [[48 .. 57], [65 .. 90], [97 .. 122]].toUnicode() // 0-9, A-Z, a-z
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if the given ranges are from the same character class
|
||||
* and there exists at least one character matched by both ranges.
|
||||
*/
|
||||
predicate overlap(RegExpCharacterRange a, RegExpCharacterRange b) {
|
||||
exists(RegExpCharacterClass clz |
|
||||
a = clz.getAChild() and
|
||||
b = clz.getAChild()
|
||||
|
|
||||
// b contains the lower end of a
|
||||
exists(int alow, int ahigh, int blow, int bhigh |
|
||||
isRange(a, alow, ahigh) and
|
||||
isRange(b, blow, bhigh) and
|
||||
blow <= alow and
|
||||
bhigh >= ahigh
|
||||
)
|
||||
or
|
||||
// b contains the upper end of a
|
||||
exists(int blow, int bhigh, int alow, int ahigh |
|
||||
isRange(a, alow, ahigh) and
|
||||
isRange(b, blow, bhigh) and
|
||||
blow <= ahigh and
|
||||
bhigh >= ahigh
|
||||
)
|
||||
)
|
||||
or
|
||||
// symmetric overlap
|
||||
overlap(b, a)
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `range` overlaps with the char class `escape` from the same character class.
|
||||
*/
|
||||
predicate overlapsWithCharEscape(RegExpCharacterRange range, RegExpCharacterClassEscape escape) {
|
||||
exists(RegExpCharacterClass clz, string low, string high |
|
||||
range = clz.getAChild() and
|
||||
escape = clz.getAChild() and
|
||||
range.isRange(low, high)
|
||||
|
|
||||
escape.getValue() = "w" and
|
||||
inRange(low, high).regexpMatch("\\w")
|
||||
or
|
||||
escape.getValue() = "d" and
|
||||
inRange(low, high).regexpMatch("\\d")
|
||||
or
|
||||
escape.getValue() = "s" and
|
||||
inRange(low, high).regexpMatch("\\s")
|
||||
)
|
||||
}
|
||||
|
||||
/** Gets the unicode code point for a `char`. */
|
||||
bindingset[char]
|
||||
int toCodePoint(string char) { result.toUnicode() = char }
|
||||
|
||||
/** A character range that appears to be overly wide. */
|
||||
class OverlyWideRange extends RegExpCharacterRange {
|
||||
OverlyWideRange() {
|
||||
exists(int low, int high, int numChars |
|
||||
isRange(this, low, high) and
|
||||
numChars = (1 + high - low) and
|
||||
this.getRootTerm().isUsedAsRegExp() and
|
||||
numChars >= 10
|
||||
|
|
||||
// across the Z-a range (which includes backticks)
|
||||
toCodePoint("Z") >= low and
|
||||
toCodePoint("a") <= high
|
||||
or
|
||||
// across the 9-A range (which includes e.g. ; and ?)
|
||||
toCodePoint("9") >= low and
|
||||
toCodePoint("A") <= high
|
||||
or
|
||||
// any non-alpha numeric as part of the range
|
||||
not isAlphanumeric([low, high].toUnicode())
|
||||
) and
|
||||
// some cases I want to exclude from being flagged
|
||||
not this = allowedWideRanges()
|
||||
}
|
||||
|
||||
/** Gets a string representation of a character class that matches the same chars as this range. */
|
||||
string printEquivalent() { result = RangePrinter::printEquivalentCharClass(this) }
|
||||
}
|
||||
|
||||
/** Gets a range that should not be reported as an overly wide range. */
|
||||
RegExpCharacterRange allowedWideRanges() {
|
||||
// ~ is the last printable ASCII character, it's used right in various wide ranges.
|
||||
result.isRange(_, "~")
|
||||
or
|
||||
// the same with " " and "!". " " is the first printable character, and "!" is the first non-white-space printable character.
|
||||
result.isRange([" ", "!"], _)
|
||||
or
|
||||
// I've seen this often enough, looks OK.
|
||||
result.isRange("@", "_")
|
||||
or
|
||||
// starting from the zero byte is a good indication that it's purposely matching a large range.
|
||||
result.isRange(0.toUnicode(), _)
|
||||
}
|
||||
|
||||
/** Gets all chars between (and including) `low` and `high`. */
|
||||
bindingset[low, high]
|
||||
private string inRange(string low, string high) {
|
||||
result = [toCodePoint(low) .. toCodePoint(high)].toUnicode()
|
||||
}
|
||||
|
||||
/** A module computing an equivalent character class for an overly wide range. */
|
||||
module RangePrinter {
|
||||
bindingset[char]
|
||||
private string next(string char) {
|
||||
exists(int prev, int next |
|
||||
prev.toUnicode() = char and
|
||||
next.toUnicode() = result and
|
||||
next = prev + 1
|
||||
)
|
||||
}
|
||||
|
||||
bindingset[char]
|
||||
private string prev(string char) {
|
||||
exists(int prev, int next |
|
||||
prev.toUnicode() = char and
|
||||
next.toUnicode() = result and
|
||||
next = prev - 1
|
||||
)
|
||||
}
|
||||
|
||||
/** Gets the points where the parts of the pretty printed range should be cut off. */
|
||||
private string cutoffs() { result = ["A", "Z", "a", "z", "0", "9"] }
|
||||
|
||||
/** Gets the char to use in the low end of a range for a given `cut` */
|
||||
private string lowCut(string cut) {
|
||||
cut = ["A", "a", "0"] and
|
||||
result = cut
|
||||
or
|
||||
cut = ["Z", "z", "9"] and
|
||||
result = next(cut)
|
||||
}
|
||||
|
||||
/** Gets the char to use in the high end of a range for a given `cut` */
|
||||
private string highCut(string cut) {
|
||||
cut = ["Z", "z", "9"] and
|
||||
result = cut
|
||||
or
|
||||
cut = ["A", "a", "0"] and
|
||||
result = prev(cut)
|
||||
}
|
||||
|
||||
/** Gets the cutoff char used for a given `part` of a range when pretty-printing it. */
|
||||
private string cutoff(OverlyWideRange range, int part) {
|
||||
exists(int low, int high | isRange(range, low, high) |
|
||||
result =
|
||||
rank[part + 1](string cut |
|
||||
cut = cutoffs() and low < toCodePoint(cut) and toCodePoint(cut) < high
|
||||
|
|
||||
cut order by toCodePoint(cut)
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
/** Gets the number of parts we should print for a given `range`. */
|
||||
private int parts(OverlyWideRange range) { result = 1 + strictcount(cutoff(range, _)) }
|
||||
|
||||
/** Holds if the given part of a range should span from `low` to `high`. */
|
||||
private predicate part(OverlyWideRange range, int part, string low, string high) {
|
||||
// first part.
|
||||
part = 0 and
|
||||
(
|
||||
range.isRange(low, high) and
|
||||
parts(range) = 1
|
||||
or
|
||||
parts(range) >= 2 and
|
||||
range.isRange(low, _) and
|
||||
high = highCut(cutoff(range, part))
|
||||
)
|
||||
or
|
||||
// middle
|
||||
part >= 1 and
|
||||
part < parts(range) and
|
||||
low = lowCut(cutoff(range, part - 1)) and
|
||||
high = highCut(cutoff(range, part))
|
||||
or
|
||||
// last.
|
||||
part = parts(range) and
|
||||
low = lowCut(cutoff(range, part - 2)) and
|
||||
range.isRange(_, high)
|
||||
}
|
||||
|
||||
/** Gets an escaped `char` for use in a character class. */
|
||||
bindingset[char]
|
||||
private string escape(string char) {
|
||||
exists(string reg | reg = "(\\[|\\]|\\\\|-|/)" |
|
||||
char.regexpMatch(reg) and
|
||||
result = "\\" + char
|
||||
or
|
||||
not char.regexpMatch(reg) and
|
||||
result = char
|
||||
)
|
||||
}
|
||||
|
||||
/** Gets a part of the equivalent range. */
|
||||
private string printEquivalentCharClass(OverlyWideRange range, int part) {
|
||||
exists(string low, string high | part(range, part, low, high) |
|
||||
if
|
||||
isAlphanumeric(low) and
|
||||
isAlphanumeric(high)
|
||||
then result = low + "-" + high
|
||||
else
|
||||
result = strictconcat(string char | char = inRange(low, high) | escape(char) order by char)
|
||||
)
|
||||
}
|
||||
|
||||
/** Gets the entire pretty printed equivalent range. */
|
||||
string printEquivalentCharClass(OverlyWideRange range) {
|
||||
result =
|
||||
"[" +
|
||||
strictconcat(string r, int part |
|
||||
r = printEquivalentCharClass(range, part)
|
||||
|
|
||||
r order by part
|
||||
) + "]"
|
||||
}
|
||||
}
|
||||
|
||||
/** Gets a char range that is suspiciously because of `reason`. */
|
||||
RegExpCharacterRange getABadRange(string reason, int priority) {
|
||||
priority = 0 and
|
||||
reason = "is equivalent to " + result.(OverlyWideRange).printEquivalent()
|
||||
or
|
||||
priority = 1 and
|
||||
exists(RegExpCharacterRange other |
|
||||
reason = "overlaps with " + other + " in the same character class" and
|
||||
result != other and
|
||||
rankRange(result) < rankRange(other) and
|
||||
overlap(result, other)
|
||||
)
|
||||
or
|
||||
priority = 2 and
|
||||
exists(RegExpCharacterClassEscape escape |
|
||||
reason = "overlaps with " + escape + " in the same character class" and
|
||||
overlapsWithCharEscape(result, escape)
|
||||
)
|
||||
or
|
||||
reason = "is empty" and
|
||||
priority = 3 and
|
||||
exists(int low, int high |
|
||||
isRange(result, low, high) and
|
||||
low > high
|
||||
)
|
||||
}
|
||||
|
||||
/** Holds if `range` matches suspiciously many characters. */
|
||||
predicate problem(RegExpCharacterRange range, string reason) {
|
||||
range = getABadRange(_, _) and
|
||||
reason =
|
||||
strictconcat(string m, int priority |
|
||||
range = getABadRange(m, priority)
|
||||
|
|
||||
m, ", and " order by priority desc
|
||||
) and
|
||||
// specifying a range using an escape is usually OK.
|
||||
not range.getAChild() instanceof RegExpEscape and
|
||||
// Unicode escapes in strings are interpreted before it turns into a regexp,
|
||||
// so e.g. [\u0001-\uFFFF] will just turn up as a range between two constants.
|
||||
// We therefore exclude these ranges.
|
||||
range.getRootTerm().getParent() instanceof RegExpLiteral and
|
||||
// is used as regexp (mostly for JS where regular expressions are parsed eagerly)
|
||||
range.getRootTerm().isUsedAsRegExp()
|
||||
}
|
||||
66
python/ql/src/Security/CWE-020/SuspiciousRegexpRange.qhelp
Normal file
66
python/ql/src/Security/CWE-020/SuspiciousRegexpRange.qhelp
Normal file
@@ -0,0 +1,66 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
|
||||
<overview>
|
||||
<p>
|
||||
A regexp range can by accident match more than was intended.
|
||||
For example, the regular expression <code>/[a-zA-z]/</code> will
|
||||
match every lowercase and uppercase letters, but the same regular
|
||||
expression will also match the chars: <code>[\]^_`</code>.
|
||||
</p>
|
||||
<p>
|
||||
On other occasions it can happen that the dash in a regular
|
||||
expression is not escaped, which will cause it to be interpreted
|
||||
as part of a range. For example in the character class <code>[a-zA-Z0-9%=.,-_]</code>
|
||||
the last character range matches the 55 characters between
|
||||
<code>,</code> and <code>_</code> (both included), which overlaps with the
|
||||
range <code>[0-9]</code> and is thus clearly not intended.
|
||||
</p>
|
||||
</overview>
|
||||
|
||||
<recommendation>
|
||||
<p>
|
||||
|
||||
Don't write character ranges were there might be confusion as to
|
||||
which characters are included in the range.
|
||||
|
||||
</p>
|
||||
</recommendation>
|
||||
|
||||
<example>
|
||||
|
||||
<p>
|
||||
The following example code checks whether a string is a valid 6 digit hex color.
|
||||
</p>
|
||||
|
||||
<sample language="python">
|
||||
import re
|
||||
def is_valid_hex_color(color):
|
||||
return re.match(r'^#[0-9a-fA-f]{6}$', color) is not None
|
||||
</sample>
|
||||
|
||||
<p>
|
||||
However, the <code>A-f</code> range matches every uppercase character, and
|
||||
thus a "color" like <code>#XYZ</code> is considered valid.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
The fix is to use an uppercase <code>A-F</code> range instead.
|
||||
</p>
|
||||
|
||||
<sample language="python">
|
||||
import re
|
||||
def is_valid_hex_color(color):
|
||||
return re.match(r'^#[0-9a-fA-F]{6}$', color) is not None
|
||||
</sample>
|
||||
|
||||
</example>
|
||||
|
||||
<references>
|
||||
<li>Mitre.org: <a href="https://cwe.mitre.org/data/definitions/20.html">CWE-020</a></li>
|
||||
<li>github.com: <a href="https://github.com/advisories/GHSA-g4rg-993r-mgx7">CVE-2021-42740</a></li>
|
||||
<li>wh0.github.io: <a href="https://wh0.github.io/2021/10/28/shell-quote-rce-exploiting.html">Exploiting CVE-2021-42740</a></li>
|
||||
</references>
|
||||
</qhelp>
|
||||
18
python/ql/src/Security/CWE-020/SuspiciousRegexpRange.ql
Normal file
18
python/ql/src/Security/CWE-020/SuspiciousRegexpRange.ql
Normal file
@@ -0,0 +1,18 @@
|
||||
/**
|
||||
* @name Suspicious regexp range
|
||||
* @description Some ranges in regular expression might match more than intended.
|
||||
* @kind problem
|
||||
* @problem.severity warning
|
||||
* @security-severity 5.0
|
||||
* @precision high
|
||||
* @id py/suspicious-regexp-range
|
||||
* @tags correctness
|
||||
* security
|
||||
* external/cwe/cwe-020
|
||||
*/
|
||||
|
||||
import semmle.python.security.SuspiciousRegexpRangeQuery
|
||||
|
||||
from RegExpCharacterRange range, string reason
|
||||
where problem(range, reason)
|
||||
select range, "Suspicious character range that " + reason + "."
|
||||
@@ -0,0 +1,5 @@
|
||||
---
|
||||
category: newQuery
|
||||
---
|
||||
* Added a new query, `py/suspicious-regexp-range`, to detect character ranges in regular expressions that seem to match
|
||||
too many characters.
|
||||
@@ -0,0 +1,10 @@
|
||||
| test.py:3:29:3:31 | 0-9 | Suspicious character range that overlaps with 3-5 in the same character class. |
|
||||
| test.py:5:31:5:33 | A-z | Suspicious character range that overlaps with A-Z in the same character class, and is equivalent to [A-Z\\[\\\\\\]^_`a-z]. |
|
||||
| test.py:7:28:7:30 | z-a | Suspicious character range that is empty. |
|
||||
| test.py:17:38:17:40 | A-f | Suspicious character range that overlaps with a-f in the same character class, and is equivalent to [A-Z\\[\\\\\\]^_`a-f]. |
|
||||
| test.py:19:30:19:32 | $-` | Suspicious character range that is equivalent to [$%&'()*+,\\-.\\/0-9:;<=>?@A-Z\\[\\\\\\]^_`]. |
|
||||
| test.py:21:43:21:45 | +-< | Suspicious character range that is equivalent to [+,\\-.\\/0-9:;<]. |
|
||||
| test.py:23:47:23:49 | .-_ | Suspicious character range that overlaps with 1-9 in the same character class, and is equivalent to [.\\/0-9:;<=>?@A-Z\\[\\\\\\]^_]. |
|
||||
| test.py:25:34:25:36 | 7-F | Suspicious character range that is equivalent to [7-9:;<=>?@A-F]. |
|
||||
| test.py:27:38:27:40 | 0-9 | Suspicious character range that overlaps with \\d in the same character class. |
|
||||
| test.py:29:41:29:43 | .-? | Suspicious character range that overlaps with \\w in the same character class, and is equivalent to [.\\/0-9:;<=>?]. |
|
||||
@@ -0,0 +1 @@
|
||||
Security/CWE-020/SuspiciousRegexpRange.ql
|
||||
@@ -0,0 +1,29 @@
|
||||
import re
|
||||
|
||||
overlap1 = re.compile(r'^[0-93-5]$') # NOT OK
|
||||
|
||||
overlap2 = re.compile(r'[A-ZA-z]') # NOT OK
|
||||
|
||||
isEmpty = re.compile(r'^[z-a]$') # NOT OK
|
||||
|
||||
isAscii = re.compile(r'^[\x00-\x7F]*$') # OK
|
||||
|
||||
printable = re.compile(r'[!-~]') # OK - used to select most printable ASCII characters
|
||||
|
||||
codePoints = re.compile(r'[^\x21-\x7E]|[[\](){}<>/%]') # OK
|
||||
|
||||
NON_ALPHANUMERIC_REGEXP = re.compile(r'([^\#-~| |!])') # OK
|
||||
|
||||
smallOverlap = re.compile(r'[0-9a-fA-f]') # NOT OK
|
||||
|
||||
weirdRange = re.compile(r'[$-`]') # NOT OK
|
||||
|
||||
keywordOperator = re.compile(r'[!\~\*\/%+-<>\^|=&]') # NOT OK
|
||||
|
||||
notYoutube = re.compile(r'youtu\.be\/[a-z1-9.-_]+') # NOT OK
|
||||
|
||||
numberToLetter = re.compile(r'[7-F]') # NOT OK
|
||||
|
||||
overlapsWithClass1 = re.compile(r'[0-9\d]') # NOT OK
|
||||
|
||||
overlapsWithClass2 = re.compile(r'[\w,.-?:*+]') # NOT OK
|
||||
Reference in New Issue
Block a user