Merge remote-tracking branch 'upstream/master' into mergeback-2018-10-08

This commit is contained in:
Tom Hvitved
2018-10-08 11:48:56 +02:00
151 changed files with 5884 additions and 546 deletions

View File

@@ -13,7 +13,8 @@
| **Query** | **Expected impact** | **Change** |
|----------------------------|------------------------|------------------------------------------------------------------|
| Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. |
| Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. |
| Wrong type of arguments to formatting function | Fewer false positive results | False positive results involving typedefs have been removed. Expected argument types are determined more accurately, especially for wide string and pointer types. Custom (non-standard) formatting functions are also identified more accurately. |
## Changes to QL libraries

View File

@@ -9,22 +9,28 @@
* Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features:
- file system access, for example through [fs-extra](https://github.com/jprichardson/node-fs-extra) or [globby](https://www.npmjs.com/package/globby)
* The type inference now handles nested imports (that is, imports not appearing at the toplevel). This may yield fewer false-positive results on projects that use this non-standard language feature.
## New queries
| **Query** | **Tags** | **Purpose** |
|-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094 | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. |
| Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. |
| Host header poisoning in email generation | security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default. |
| Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. |
| Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. |
| Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. |
## Changes to existing queries
| **Query** | **Expected impact** | **Change** |
|--------------------------------|----------------------------|----------------------------------------------|
| Useless assignment to local variable | Fewer false-positive results | This rule now recognizes additional ways default values can be set. |
| Regular expression injection | Fewer false-positive results | This rule now identifies calls to `String.prototype.search` with more precision. |
| Unbound event handler receiver | Fewer false-positive results | This rule now recognizes additional ways class methods can be bound. |
| Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. |
| Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. |
| Server-side URL redirect | More results | This rule now recognizes redirection calls in more cases. |
| Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. |
## Changes to QL libraries

View File

@@ -22,13 +22,15 @@ import UnsignedGEZero
// #define PRINTMSG(val,msg) { if (val >= PRINTLEVEL) printf(msg); }
//
// So to reduce the number of false positives, we do not report a result if
// the comparison is in a macro expansion.
// the comparison is in a macro expansion. Similarly for template
// instantiations.
from
ComparisonOperation cmp, SmallSide ss,
float left, float right, boolean value,
string reason
where
not cmp.isInMacroExpansion() and
not cmp.isFromTemplateInstantiation(_) and
reachablePointlessComparison(cmp, left, right, value, ss) and
// a comparison between an enum and zero is always valid because whether

View File

@@ -25,7 +25,8 @@ private predicate formattingFunctionCallExpectedType(FormattingFunctionCall ffc,
ffc.getTarget() = f and
f.getFormatParameterIndex() = i and
ffc.getArgument(i) = fl and
fl.getConversionType(pos) = expected
fl.getConversionType(pos) = expected and
count(fl.getConversionType(pos)) = 1
)
}
@@ -66,30 +67,15 @@ predicate formatOtherArgType(FormattingFunctionCall ffc, int pos, Type expected,
class ExpectedType extends Type
{
ExpectedType() {
formatArgType(_, _, this, _, _) or
formatOtherArgType(_, _, this, _, _) or
exists(ExpectedType t |
this = t.(PointerType).getBaseType()
exists(Type t |
(
formatArgType(_, _, t, _, _) or
formatOtherArgType(_, _, t, _, _)
) and this = t.getUnspecifiedType()
)
}
}
/**
* Gets an 'interesting' type that can be reached from `t` by removing
* typedefs and specifiers. Note that this does not always mean removing
* all typedefs and specifiers as `Type.getUnspecifiedType()` would, for
* example if the interesting type is itself a typedef.
*/
ExpectedType getAnUnderlyingExpectedType(Type t) {
(
result = t
) or (
result = getAnUnderlyingExpectedType(t.(TypedefType).getBaseType())
) or (
result = getAnUnderlyingExpectedType(t.(SpecifiedType).getBaseType())
)
}
/**
* Holds if it is safe to display a value of type `actual` when `printf`
* expects a value of type `expected`.
@@ -100,59 +86,48 @@ ExpectedType getAnUnderlyingExpectedType(Type t) {
* are converted to `double`.
*/
predicate trivialConversion(ExpectedType expected, Type actual) {
formatArgType(_, _, expected, _, actual) and
exists(Type actualU |
actualU = actual.getUnspecifiedType() and
exists(Type exp, Type act |
formatArgType(_, _, exp, _, act) and
expected = exp.getUnspecifiedType() and
actual = act.getUnspecifiedType()
) and (
(
(
// allow a pointer type to be displayed with `%p`
expected instanceof VoidPointerType and actualU instanceof PointerType
) or (
// allow a function pointer type to be displayed with `%p`
expected instanceof VoidPointerType and actualU instanceof FunctionPointerType and expected.getSize() = actual.getSize()
) or (
// allow an `enum` type to be displayed with `%i`, `%c` etc
expected instanceof IntegralType and actualU instanceof Enum
) or (
// allow any `char *` type to be displayed with `%s`
expected instanceof CharPointerType and actualU instanceof CharPointerType
) or (
// allow `wchar_t *`, or any pointer to an integral type of the same size, to be displayed
// with `%ws`
expected.(PointerType).getBaseType().hasName("wchar_t") and
exists(Wchar_t t |
actual.getUnspecifiedType().(PointerType).getBaseType().(IntegralType).getSize() = t.getSize()
)
) or (
// allow an `int` (or anything promoted to `int`) to be displayed with `%c`
expected instanceof CharType and actualU instanceof IntType
) or (
// allow an `int` (or anything promoted to `int`) to be displayed with `%wc`
expected instanceof Wchar_t and actualU instanceof IntType
) or (
expected instanceof UnsignedCharType and actualU instanceof IntType
) or (
// allow the underlying type of a `size_t` (e.g. `unsigned long`) for
// `%zu`, since this is the type of a `sizeof` expression
expected instanceof Size_t and
actual.getUnspecifiedType() = expected.getUnspecifiedType()
) or (
// allow the underlying type of a `ssize_t` (e.g. `long`) for `%zd`
expected instanceof Ssize_t and
actual.getUnspecifiedType() = expected.getUnspecifiedType()
) or (
// allow any integral type of the same size
// (this permits signedness changes)
expected.(IntegralType).getSize() = actualU.(IntegralType).getSize()
) or (
// allow a pointer to any integral type of the same size
// (this permits signedness changes)
expected.(PointerType).getBaseType().(IntegralType).getSize() = actualU.(PointerType).getBaseType().(IntegralType).getSize()
) or (
// allow expected, or a typedef or specified version of expected
expected = getAnUnderlyingExpectedType(actual)
// allow a pointer type to be displayed with `%p`
expected instanceof VoidPointerType and actual instanceof PointerType
) or (
// allow a function pointer type to be displayed with `%p`
expected instanceof VoidPointerType and actual instanceof FunctionPointerType and expected.getSize() = actual.getSize()
) or (
// allow an `enum` type to be displayed with `%i`, `%c` etc
expected instanceof IntegralType and actual instanceof Enum
) or (
// allow any `char *` type to be displayed with `%s`
expected instanceof CharPointerType and actual instanceof CharPointerType
) or (
// allow `wchar_t *`, or any pointer to an integral type of the same size, to be displayed
// with `%ws`
expected.(PointerType).getBaseType().hasName("wchar_t") and
exists(Wchar_t t |
actual.getUnspecifiedType().(PointerType).getBaseType().(IntegralType).getSize() = t.getSize()
)
) or (
// allow an `int` (or anything promoted to `int`) to be displayed with `%c`
expected instanceof CharType and actual instanceof IntType
) or (
// allow an `int` (or anything promoted to `int`) to be displayed with `%wc`
expected instanceof Wchar_t and actual instanceof IntType
) or (
expected instanceof UnsignedCharType and actual instanceof IntType
) or (
// allow any integral type of the same size
// (this permits signedness changes)
expected.(IntegralType).getSize() = actual.(IntegralType).getSize()
) or (
// allow a pointer to any integral type of the same size
// (this permits signedness changes)
expected.(PointerType).getBaseType().(IntegralType).getSize() = actual.(PointerType).getBaseType().(IntegralType).getSize()
) or (
expected = actual
)
)
}
@@ -164,16 +139,16 @@ int sizeof_IntType() {
exists(IntType it | result = it.getSize())
}
from FormattingFunctionCall ffc, int n, Expr arg, ExpectedType expected, Type actual
from FormattingFunctionCall ffc, int n, Expr arg, Type expected, Type actual
where (
(
formatArgType(ffc, n, expected, arg, actual) and
not trivialConversion(expected, actual)
not trivialConversion(expected.getUnspecifiedType(), actual.getUnspecifiedType())
)
or
(
formatOtherArgType(ffc, n, expected, arg, actual) and
not actual.getUnderlyingType().(IntegralType).getSize() = sizeof_IntType()
not actual.getUnspecifiedType().(IntegralType).getSize() = sizeof_IntType()
)
)
and not arg.isAffectedByMacro()

View File

@@ -41,11 +41,13 @@ Type stripType(Type t) {
result = stripType(t.(ArrayType).getBaseType()) or
result = stripType(t.(ReferenceType).getBaseType()) or
result = stripType(t.(SpecifiedType).getBaseType()) or
result = stripType(t.(Decltype).getBaseType()) or
(
not t instanceof TypedefType and
not t instanceof ArrayType and
not t instanceof ReferenceType and
not t instanceof SpecifiedType and
not t instanceof Decltype and
result = t
)
}

View File

@@ -0,0 +1,3 @@
wchar_t* pSrc;
pSrc = (wchar_t*)"a"; // casting a byte-string literal "a" to a wide-character string

View File

@@ -0,0 +1,35 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>This rule indicates a potentially incorrect cast from an byte string (<code>char *</code>) to a wide-character string (<code>wchar_t *</code>).</p>
<p>This cast might yield strings that are not correctly terminated; including potential buffer overruns when using such strings with some dangerous APIs.</p>
</overview>
<recommendation>
<p>Do not explicitly cast byte strings to wide-character strings.</p>
<p>For string literals, prepend the literal string with the letter "L" to indicate that the string is a wide-character string (<code>wchar_t *</code>).</p>
<p>For converting a byte literal to a wide-character string literal, you would need to use the appropriate conversion function for the platform you are using. Please see the references section for options according to your platform.</p>
</recommendation>
<example>
<p>In the following example, an byte string literal (<code>"a"</code>) is cast to a wide-character string.</p>
<sample src="WcharCharConversion.cpp" />
<p>To fix this issue, prepend the literal with the letter "L" (<code>L"a"</code>) to define it as a wide-character string.</p>
</example>
<references>
<li>
General resources:
<a href="https://en.cppreference.com/w/cpp/string/multibyte/mbstowcs">std::mbstowcs</a>
</li>
<li>
Microsoft specific resources:
<a href="https://docs.microsoft.com/en-us/windows/desktop/Intl/security-considerations--international-features">Security Considerations: International Features</a>
</li>
</references>
</qhelp>

View File

@@ -0,0 +1,29 @@
/**
* @name Cast from char* to wchar_t*
* @description Casting a byte string to a wide-character string is likely
* to yield a string that is incorrectly terminated or aligned.
* This can lead to undefined behavior, including buffer overruns.
* @kind problem
* @id cpp/incorrect-string-type-conversion
* @problem.severity error
* @precision high
* @tags security
* external/cwe/cwe-704
* external/microsoft/c/c6276
*/
import cpp
class WideCharPointerType extends PointerType {
WideCharPointerType() {
this.getBaseType() instanceof WideCharType
}
}
from Expr e1, Cast e2
where
e2 = e1.getConversion() and
exists(WideCharPointerType w, CharPointerType c |
w = e2.getType().getUnspecifiedType().(PointerType) and
c = e1.getType().getUnspecifiedType().(PointerType)
)
select e1, "Conversion from " + e1.getType().toString() + " to " + e2.getType().toString() + ". Use of invalid string can lead to undefined behavior."

View File

@@ -3,7 +3,7 @@
* @description All functions that are not void should return a value on every exit path.
* @kind problem
* @problem.severity error
* @precision medium
* @precision high
* @id cpp/missing-return
* @tags reliability
* readability

View File

@@ -220,6 +220,32 @@ class FormatLiteral extends Literal {
getUse().getTarget().(FormattingFunction).isWideCharDefault()
}
/**
* Gets the default character type expected for `%s` by this format literal. Typically
* `char` or `wchar_t`.
*/
Type getDefaultCharType() {
result = getUse().getTarget().(FormattingFunction).getDefaultCharType()
}
/**
* Gets the non-default character type expected for `%S` by this format literal. Typically
* `wchar_t` or `char`. On some snapshots there may be multiple results where we can't tell
* which is correct for a particular function.
*/
Type getNonDefaultCharType() {
result = getUse().getTarget().(FormattingFunction).getNonDefaultCharType()
}
/**
* Gets the wide character type for this format literal. This is usually `wchar_t`. On some
* snapshots there may be multiple results where we can't tell which is correct for a
* particular function.
*/
Type getWideCharType() {
result = getUse().getTarget().(FormattingFunction).getWideCharType()
}
/**
* Holds if this `FormatLiteral` is in a context that supports
* Microsoft rules and extensions.
@@ -629,7 +655,6 @@ class FormatLiteral extends Literal {
result = getConversionType2(n) or
result = getConversionType3(n) or
result = getConversionType4(n) or
result = getConversionType5(n) or
result = getConversionType6(n) or
result = getConversionType7(n) or
result = getConversionType8(n) or
@@ -696,33 +721,35 @@ class FormatLiteral extends Literal {
}
/**
* Gets the 'effective' string type character, that is, 's' (meaning a char string) or
* 'S' (meaning a wide string).
* - in the base case this is the same as the format type character.
* - for a `wprintf` or similar function call, the meanings are reversed.
* - the size prefixes 'l'/'w' (long) and 'h' (short) override the
* type character to effectively 'S' or 's' respectively.
* Gets the string type required by the nth conversion specifier.
* - in the base case this is the default for the formatting function
* (e.g. `char` for `printf`, `wchar_t` for `wprintf`).
* - the `%S` format character reverses wideness.
* - the size prefixes 'l'/'w' and 'h' override the type character
* to wide or single-byte characters respectively.
*/
private string getEffectiveStringConversionChar(int n) {
exists(string len, string conv | this.parseConvSpec(n, _, _, _, _, _, len, conv) and (conv = "s" or conv = "S") |
(len = "l" and result = "S") or
(len = "w" and result = "S") or
(len = "h" and result = "s") or
(len != "l" and len != "w" and len != "h" and (result = "s" or result = "S") and (if isWideCharDefault() then result != conv else result = conv))
)
}
private Type getConversionType4(int n) {
exists(string cnv, CharType t | cnv = this.getEffectiveStringConversionChar(n) |
cnv="s" and t = result.(PointerType).getBaseType()
and not t.isExplicitlySigned()
and not t.isExplicitlyUnsigned()
)
}
private Type getConversionType5(int n) {
exists(string cnv | cnv = this.getEffectiveStringConversionChar(n) |
cnv="S" and result.(PointerType).getBaseType().hasName("wchar_t")
exists(string len, string conv |
this.parseConvSpec(n, _, _, _, _, _, len, conv) and
(
(
(conv = "s" or conv = "S") and
len = "h" and
result.(PointerType).getBaseType() instanceof PlainCharType
) or (
(conv = "s" or conv = "S") and
(len = "l" or len = "w") and
result.(PointerType).getBaseType() = getWideCharType()
) or (
conv = "s" and
(len != "l" and len != "w" and len != "h") and
result.(PointerType).getBaseType() = getDefaultCharType()
) or (
conv = "S" and
(len != "l" and len != "w" and len != "h") and
result.(PointerType).getBaseType() = getNonDefaultCharType()
)
)
)
}

View File

@@ -12,7 +12,8 @@ class Printf extends FormattingFunction {
hasGlobalName("wprintf") or
hasGlobalName("wprintf_s") or
hasGlobalName("g_printf")
)
) and
not exists(getDefinition().getFile().getRelativePath())
}
override int getFormatParameterIndex() { result=0 }
@@ -26,7 +27,15 @@ class Printf extends FormattingFunction {
* The standard functions `fprintf`, `fwprintf` and their glib variants.
*/
class Fprintf extends FormattingFunction {
Fprintf() { this instanceof TopLevelFunction and (hasGlobalName("fprintf") or hasGlobalName("fwprintf") or hasGlobalName("g_fprintf"))}
Fprintf() {
this instanceof TopLevelFunction and
(
hasGlobalName("fprintf") or
hasGlobalName("fwprintf") or
hasGlobalName("g_fprintf")
) and
not exists(getDefinition().getFile().getRelativePath())
}
override int getFormatParameterIndex() { result=1 }
override predicate isWideCharDefault() { hasGlobalName("fwprintf") }
@@ -47,7 +56,8 @@ class Sprintf extends FormattingFunction {
hasGlobalName("g_strdup_printf") or
hasGlobalName("g_sprintf") or
hasGlobalName("__builtin___sprintf_chk")
)
) and
not exists(getDefinition().getFile().getRelativePath())
}
override predicate isWideCharDefault() {
@@ -100,7 +110,8 @@ class Snprintf extends FormattingFunction {
or hasGlobalName("g_snprintf")
or hasGlobalName("wnsprintf")
or hasGlobalName("__builtin___snprintf_chk")
)
) and
not exists(getDefinition().getFile().getRelativePath())
}
override int getFormatParameterIndex() {
@@ -133,10 +144,13 @@ class Snprintf extends FormattingFunction {
* in the buffer.
*/
predicate returnsFullFormatLength() {
hasGlobalName("snprintf") or
hasGlobalName("g_snprintf") or
hasGlobalName("__builtin___snprintf_chk") or
hasGlobalName("snprintf_s")
(
hasGlobalName("snprintf") or
hasGlobalName("g_snprintf") or
hasGlobalName("__builtin___snprintf_chk") or
hasGlobalName("snprintf_s")
) and
not exists(getDefinition().getFile().getRelativePath())
}
override int getSizeParameterIndex() {
@@ -158,7 +172,8 @@ class StringCchPrintf extends FormattingFunction {
or hasGlobalName("StringCbPrintfEx")
or hasGlobalName("StringCbPrintf_l")
or hasGlobalName("StringCbPrintf_lEx")
)
) and
not exists(getDefinition().getFile().getRelativePath())
}
override int getFormatParameterIndex() {
@@ -187,7 +202,8 @@ class Syslog extends FormattingFunction {
Syslog() {
this instanceof TopLevelFunction and (
hasGlobalName("syslog")
)
) and
not exists(getDefinition().getFile().getRelativePath())
}
override int getFormatParameterIndex() { result=1 }

View File

@@ -7,6 +7,38 @@
*/
import semmle.code.cpp.Function
private Type stripTopLevelSpecifiersOnly(Type t) {
(
result = stripTopLevelSpecifiersOnly(t.(SpecifiedType).getBaseType())
) or (
result = t and
not t instanceof SpecifiedType
)
}
/**
* A type that is used as a format string by any formatting function.
*/
Type getAFormatterWideType() {
exists(FormattingFunction ff |
result = stripTopLevelSpecifiersOnly(ff.getDefaultCharType()) and
result.getSize() != 1
)
}
/**
* A type that is used as a format string by any formatting function, or `wchar_t` if
* there is none.
*/
private Type getAFormatterWideTypeOrDefault() {
result = getAFormatterWideType() or
(
not exists(getAFormatterWideType()) and
result instanceof Wchar_t
)
}
/**
* A standard library function that uses a `printf`-like formatting string.
*/
@@ -20,6 +52,43 @@ abstract class FormattingFunction extends Function {
*/
predicate isWideCharDefault() { none() }
/**
* Gets the default character type expected for `%s` by this function. Typically
* `char` or `wchar_t`.
*/
Type getDefaultCharType() {
result = stripTopLevelSpecifiersOnly(getParameter(getFormatParameterIndex()).getType().
getUnderlyingType().(PointerType).getBaseType())
}
/**
* Gets the non-default character type expected for `%S` by this function. Typically
* `wchar_t` or `char`. On some snapshots there may be multiple results where we can't tell
* which is correct for a particular function.
*/
Type getNonDefaultCharType() {
(
getDefaultCharType().getSize() = 1 and
result = getAFormatterWideTypeOrDefault()
) or (
getDefaultCharType().getSize() > 1 and
result instanceof PlainCharType
)
}
/**
* Gets the wide character type for this function. This is usually `wchar_t`. On some
* snapshots there may be multiple results where we can't tell which is correct for a
* particular function.
*/
Type getWideCharType() {
(
result = getDefaultCharType() or
result = getNonDefaultCharType()
) and
result.getSize() > 1
}
/**
* Gets the position at which the output parameter, if any, occurs.
*/

View File

@@ -44,6 +44,7 @@
import cpp
private import RangeAnalysisUtils
import RangeSSA
import SimpleRangeAnalysisCached
/**
* This fixed set of lower bounds is used when the lower bounds of an
@@ -406,27 +407,6 @@ deprecated predicate negative_overflow(Expr expr) {
exprMightOverflowNegatively(expr)
}
/**
* Holds if the expression might overflow negatively. This predicate
* does not consider the possibility that the expression might overflow
* due to a conversion.
*/
cached
predicate exprMightOverflowNegatively(Expr expr) {
getLowerBoundsImpl(expr) < exprMinVal(expr)
}
/**
* Holds if the expression might overflow negatively. Conversions
* are also taken into account. For example the expression
* `(int16)(x+y)` might overflow due to the `(int16)` cast, rather than
* due to the addition.
*/
cached
predicate convertedExprMightOverflowNegatively(Expr expr) {
exprMightOverflowNegatively(expr) or
convertedExprMightOverflowNegatively(expr.getConversion())
}
/**
* Holds if the expression might overflow positively. This predicate
@@ -439,39 +419,6 @@ deprecated predicate positive_overflow(Expr expr) {
exprMightOverflowPositively(expr)
}
/**
* Holds if the expression might overflow positively. This predicate
* does not consider the possibility that the expression might overflow
* due to a conversion.
*/
cached
predicate exprMightOverflowPositively(Expr expr) {
getUpperBoundsImpl(expr) > exprMaxVal(expr)
}
/**
* Holds if the expression might overflow positively. Conversions
* are also taken into account. For example the expression
* `(int16)(x+y)` might overflow due to the `(int16)` cast, rather than
* due to the addition.
*/
cached
predicate convertedExprMightOverflowPositively(Expr expr) {
exprMightOverflowPositively(expr) or
convertedExprMightOverflowPositively(expr.getConversion())
}
/**
* Holds if the expression might overflow (either positively or
* negatively). The possibility that the expression might overflow
* due to an implicit or explicit cast is also considered.
*/
cached
predicate convertedExprMightOverflow(Expr expr) {
convertedExprMightOverflowNegatively(expr) or
convertedExprMightOverflowPositively(expr)
}
/** Only to be called by `getTruncatedLowerBounds`. */
private
float getLowerBoundsImpl(Expr expr) {
@@ -921,28 +868,6 @@ float getDefUpperBoundsImpl(RangeSsaDefinition def, LocalScopeVariable v) {
unanalyzableDefBounds(def, v, _, result)
}
/** Holds if the definition might overflow negatively. */
cached
predicate defMightOverflowNegatively(RangeSsaDefinition def, LocalScopeVariable v) {
getDefLowerBoundsImpl(def, v) < varMinVal(v)
}
/** Holds if the definition might overflow positively. */
cached
predicate defMightOverflowPositively(RangeSsaDefinition def, LocalScopeVariable v) {
getDefUpperBoundsImpl(def, v) > varMaxVal(v)
}
/**
* Holds if the definition might overflow (either positively or
* negatively).
*/
cached
predicate defMightOverflow(RangeSsaDefinition def, LocalScopeVariable v) {
defMightOverflowNegatively(def, v) or
defMightOverflowPositively(def, v)
}
/**
* Get the lower bounds for a `RangeSsaDefinition`. Most of the work is
* done by `getDefLowerBoundsImpl`, but this is where widening is applied
@@ -1133,63 +1058,142 @@ predicate exprTypeBounds(Expr expr, float boundValue, boolean isLowerBound) {
(isLowerBound = false and boundValue = exprMaxVal(expr.getFullyConverted()))
}
/**
* Gets the lower bound of the expression.
*
* Note: expressions in C/C++ are often implicitly or explicitly cast to a
* different result type. Such casts can cause the value of the expression
* to overflow or to be truncated. This predicate computes the lower bound
* of the expression without including the effect of the casts. To compute
* the lower bound of the expression after all the casts have been applied,
* call `lowerBound` like this:
*
* `lowerBound(expr.getFullyConverted())`
*/
cached
float lowerBound(Expr expr) {
// Combine the lower bounds returned by getTruncatedLowerBounds into a
// single minimum value.
result = min(float lb | lb = getTruncatedLowerBounds(expr) | lb)
}
private cached module SimpleRangeAnalysisCached {
/**
* Gets the lower bound of the expression.
*
* Note: expressions in C/C++ are often implicitly or explicitly cast to a
* different result type. Such casts can cause the value of the expression
* to overflow or to be truncated. This predicate computes the lower bound
* of the expression without including the effect of the casts. To compute
* the lower bound of the expression after all the casts have been applied,
* call `lowerBound` like this:
*
* `lowerBound(expr.getFullyConverted())`
*/
cached
float lowerBound(Expr expr) {
// Combine the lower bounds returned by getTruncatedLowerBounds into a
// single minimum value.
result = min(float lb | lb = getTruncatedLowerBounds(expr) | lb)
}
/**
* Gets the upper bound of the expression.
*
* Note: expressions in C/C++ are often implicitly or explicitly cast to a
* different result type. Such casts can cause the value of the expression
* to overflow or to be truncated. This predicate computes the upper bound
* of the expression without including the effect of the casts. To compute
* the upper bound of the expression after all the casts have been applied,
* call `upperBound` like this:
*
* `upperBound(expr.getFullyConverted())`
*/
cached
float upperBound(Expr expr) {
// Combine the upper bounds returned by getTruncatedUpperBounds into a
// single maximum value.
result = max(float ub | ub = getTruncatedUpperBounds(expr) | ub)
}
/**
* Holds if `expr` has a provably empty range. For example:
*
* 10 < expr and expr < 5
*
* The range of an expression can only be empty if it can never be
* executed. For example:
*
* if (10 < x) {
* if (x < 5) {
* // Unreachable code
* return x; // x has an empty range: 10 < x && x < 5
* }
* }
*/
cached
predicate exprWithEmptyRange(Expr expr) {
analyzableExpr(expr) and
(not exists(lowerBound(expr)) or
not exists(upperBound(expr)) or
lowerBound(expr) > upperBound(expr))
}
/**
* Gets the upper bound of the expression.
*
* Note: expressions in C/C++ are often implicitly or explicitly cast to a
* different result type. Such casts can cause the value of the expression
* to overflow or to be truncated. This predicate computes the upper bound
* of the expression without including the effect of the casts. To compute
* the upper bound of the expression after all the casts have been applied,
* call `upperBound` like this:
*
* `upperBound(expr.getFullyConverted())`
*/
cached
float upperBound(Expr expr) {
// Combine the upper bounds returned by getTruncatedUpperBounds into a
// single maximum value.
result = max(float ub | ub = getTruncatedUpperBounds(expr) | ub)
}
/**
* Holds if `expr` has a provably empty range. For example:
*
* 10 < expr and expr < 5
*
* The range of an expression can only be empty if it can never be
* executed. For example:
*
* if (10 < x) {
* if (x < 5) {
* // Unreachable code
* return x; // x has an empty range: 10 < x && x < 5
* }
* }
*/
cached
predicate exprWithEmptyRange(Expr expr) {
analyzableExpr(expr) and
(not exists(lowerBound(expr)) or
not exists(upperBound(expr)) or
lowerBound(expr) > upperBound(expr))
/** Holds if the definition might overflow negatively. */
cached
predicate defMightOverflowNegatively(RangeSsaDefinition def, LocalScopeVariable v) {
getDefLowerBoundsImpl(def, v) < varMinVal(v)
}
/** Holds if the definition might overflow positively. */
cached
predicate defMightOverflowPositively(RangeSsaDefinition def, LocalScopeVariable v) {
getDefUpperBoundsImpl(def, v) > varMaxVal(v)
}
/**
* Holds if the definition might overflow (either positively or
* negatively).
*/
cached
predicate defMightOverflow(RangeSsaDefinition def, LocalScopeVariable v) {
defMightOverflowNegatively(def, v) or
defMightOverflowPositively(def, v)
}
/**
* Holds if the expression might overflow negatively. This predicate
* does not consider the possibility that the expression might overflow
* due to a conversion.
*/
cached
predicate exprMightOverflowNegatively(Expr expr) {
getLowerBoundsImpl(expr) < exprMinVal(expr)
}
/**
* Holds if the expression might overflow negatively. Conversions
* are also taken into account. For example the expression
* `(int16)(x+y)` might overflow due to the `(int16)` cast, rather than
* due to the addition.
*/
cached
predicate convertedExprMightOverflowNegatively(Expr expr) {
exprMightOverflowNegatively(expr) or
convertedExprMightOverflowNegatively(expr.getConversion())
}
/**
* Holds if the expression might overflow positively. This predicate
* does not consider the possibility that the expression might overflow
* due to a conversion.
*/
cached
predicate exprMightOverflowPositively(Expr expr) {
getUpperBoundsImpl(expr) > exprMaxVal(expr)
}
/**
* Holds if the expression might overflow positively. Conversions
* are also taken into account. For example the expression
* `(int16)(x+y)` might overflow due to the `(int16)` cast, rather than
* due to the addition.
*/
cached
predicate convertedExprMightOverflowPositively(Expr expr) {
exprMightOverflowPositively(expr) or
convertedExprMightOverflowPositively(expr.getConversion())
}
/**
* Holds if the expression might overflow (either positively or
* negatively). The possibility that the expression might overflow
* due to an implicit or explicit cast is also considered.
*/
cached
predicate convertedExprMightOverflow(Expr expr) {
convertedExprMightOverflowNegatively(expr) or
convertedExprMightOverflowPositively(expr)
}
}

View File

@@ -312,7 +312,7 @@ private predicate analyzableLocalScopeVariable(VariableAccess access) {
strictcount (SsaDefinition def, Variable v | def.getAUse(v) = access | v) = 1 and
count (SsaDefinition def, Variable v
| def.getAUse(v) = access
| def.getDefiningValue(v)) <= 1 and
| def.getDefiningValue(v).getFullyConverted()) <= 1 and
not analyzableConst(access)
}

View File

@@ -32,3 +32,4 @@
| PointlessComparison.c:129:12:129:16 | ... > ... | Comparison is always false because a <= 3. |
| PointlessComparison.c:197:7:197:11 | ... < ... | Comparison is always false because x >= 0. |
| RegressionTests.cpp:57:7:57:22 | ... <= ... | Comparison is always true because * ... <= 4294967295. |
| Templates.cpp:9:10:9:24 | ... <= ... | Comparison is always true because local <= 32767. |

View File

@@ -0,0 +1,17 @@
template<typename T>
bool sometimesPointless(T param) {
return param <= 0xFFFF; // GOOD (hypothetical instantiations are okay)
}
template<typename T>
bool alwaysPointless(T param) {
short local = param;
return local <= 0xFFFF; // BAD (in all instantiations)
}
static int caller(int i) {
return
sometimesPointless<short>(i) ||
alwaysPointless<short>(i) ||
alwaysPointless<int>(i);
}

View File

@@ -1,5 +1,4 @@
| custom_printf.cpp:31:5:31:12 | call to myPrintf | Format expects 2 arguments but given 3 |
| custom_printf.cpp:44:2:44:7 | call to printf | Format expects 0 arguments but given 2 |
| macros.cpp:12:2:12:31 | call to printf | Format expects 2 arguments but given 3 |
| macros.cpp:16:2:16:30 | call to printf | Format expects 2 arguments but given 3 |
| test.c:7:2:7:7 | call to printf | Format expects 0 arguments but given 1 |

View File

@@ -1,5 +1,4 @@
| custom_printf.cpp:29:5:29:12 | call to myPrintf | Format expects 2 arguments but given 1 |
| custom_printf.cpp:45:2:45:7 | call to printf | Format expects 2 arguments but given 0 |
| macros.cpp:14:2:14:37 | call to printf | Format expects 4 arguments but given 3 |
| macros.cpp:21:2:21:36 | call to printf | Format expects 4 arguments but given 3 |
| test.c:9:2:9:7 | call to printf | Format expects 1 arguments but given 0 |

View File

@@ -41,6 +41,6 @@ void test_custom_printf2()
{
// notTheFormat format ...
printf(0, "%i %i", 100, 200); // GOOD
printf("", "%i %i", 100, 200); // GOOD [FALSE POSITIVE]
printf("%i %i", "" ); // GOOD [FALSE POSITIVE]
printf("", "%i %i", 100, 200); // GOOD
printf("%i %i", "" ); // GOOD
}

View File

@@ -0,0 +1,10 @@
| tests.cpp:18:15:18:22 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
| tests.cpp:19:15:19:22 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
| tests.cpp:25:17:25:23 | Hello | This argument should be of type 'wchar_t *' but is of type 'char *' |
| tests.cpp:26:17:26:24 | Hello | This argument should be of type 'wchar_t *' but is of type 'char16_t *' |
| tests.cpp:30:17:30:24 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
| tests.cpp:31:17:31:24 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
| tests.cpp:33:36:33:42 | Hello | This argument should be of type 'char16_t *' but is of type 'char *' |
| tests.cpp:35:36:35:43 | Hello | This argument should be of type 'char16_t *' but is of type 'wchar_t *' |
| tests.cpp:38:36:38:43 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
| tests.cpp:39:36:39:43 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |

View File

@@ -0,0 +1 @@
Likely Bugs/Format/WrongTypeFormatArguments.ql

View File

@@ -0,0 +1,3 @@
| tests.cpp:8:5:8:10 | printf | char | char16_t, wchar_t | char16_t, wchar_t |
| tests.cpp:9:5:9:11 | wprintf | wchar_t | char | wchar_t |
| tests.cpp:10:5:10:12 | swprintf | char16_t | char | char16_t |

View File

@@ -0,0 +1,8 @@
import cpp
from FormattingFunction f
select
f,
concat(f.getDefaultCharType().toString(), ", "),
concat(f.getNonDefaultCharType().toString(), ", "),
concat(f.getWideCharType().toString(), ", ")

View File

@@ -0,0 +1,40 @@
/*
* Test for custom definitions of *wprintf using different types than the
* platform wide character type.
*/
typedef unsigned int size_t;
int printf(const char * format, ...);
int wprintf(const wchar_t * format, ...); // on wchar_t
int swprintf(char16_t * s, size_t n, const char16_t * format, ...); // on char16_t
#define BUF_SIZE (4096)
void tests() {
char16_t buffer[BUF_SIZE];
printf("%s", "Hello"); // GOOD
printf("%s", u"Hello"); // BAD: expecting char
printf("%s", L"Hello"); // BAD: expecting char
printf("%S", "Hello"); // BAD: expecting wchar_t or char16_t [NOT DETECTED]
printf("%S", u"Hello"); // GOOD
printf("%S", L"Hello"); // GOOD
wprintf(L"%s", "Hello"); // BAD: expecting wchar_t
wprintf(L"%s", u"Hello"); // BAD: expecting wchar_t
wprintf(L"%s", L"Hello"); // GOOD
wprintf(L"%S", "Hello"); // GOOD
wprintf(L"%S", u"Hello"); // BAD: expecting char
wprintf(L"%S", L"Hello"); // BAD: expecting char
swprintf(buffer, BUF_SIZE, u"%s", "Hello"); // BAD: expecting char16_t
swprintf(buffer, BUF_SIZE, u"%s", u"Hello"); // GOOD
swprintf(buffer, BUF_SIZE, u"%s", L"Hello"); // BAD: expecting char16_t
swprintf(buffer, BUF_SIZE, u"%S", "Hello"); // GOOD
swprintf(buffer, BUF_SIZE, u"%S", u"Hello"); // BAD: expecting char
swprintf(buffer, BUF_SIZE, u"%S", L"Hello"); // BAD: expecting char
}

View File

@@ -0,0 +1,2 @@
| tests_32.cpp:14:16:14:23 | void_ptr | This argument should be of type 'long' but is of type 'void *' |
| tests_64.cpp:14:16:14:23 | void_ptr | This argument should be of type 'long' but is of type 'void *' |

View File

@@ -0,0 +1 @@
Likely Bugs/Format/WrongTypeFormatArguments.ql

View File

@@ -0,0 +1,17 @@
// semmle-extractor-options: --edg --target --edg linux_i686
/*
* Test for printf in a snapshot that contains multiple word/pointer sizes.
*/
int printf(const char * format, ...);
void test_32()
{
long l;
void *void_ptr;
printf("%li", l); // GOOD
printf("%li", void_ptr); // BAD
printf("%p", l); // BAD [NOT DETECTED]
printf("%p", void_ptr); // GOOD
}

View File

@@ -0,0 +1,17 @@
// semmle-extractor-options: --edg --target --edg linux_x86_64
/*
* Test for printf in a snapshot that contains multiple word/pointer sizes.
*/
int printf(const char * format, ...);
void test_64()
{
long l;
void *void_ptr;
printf("%li", l); // GOOD
printf("%li", void_ptr); // BAD
printf("%p", l); // BAD [NOT DETECTED]
printf("%p", void_ptr); // GOOD
}

View File

@@ -11,14 +11,6 @@
| printf1.h:45:18:45:20 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
| printf1.h:46:18:46:20 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
| printf1.h:47:19:47:21 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
| printf1.h:68:19:68:21 | sst | This argument should be of type 'size_t' but is of type 'long' |
| printf1.h:70:19:70:20 | ul | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:71:19:71:20 | st | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:72:19:72:20 | ST | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:73:19:73:22 | c_st | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:74:19:74:22 | C_ST | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:75:19:75:28 | sizeof(<expr>) | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:83:23:83:35 | ... - ... | This argument should be of type 'size_t' but is of type 'long' |
| real_world.h:61:21:61:22 | & ... | This argument should be of type 'int *' but is of type 'short *' |
| real_world.h:62:22:62:23 | & ... | This argument should be of type 'short *' but is of type 'int *' |
| real_world.h:63:22:63:24 | & ... | This argument should be of type 'short *' but is of type 'unsigned int *' |

View File

@@ -65,14 +65,14 @@ void g()
printf("%zu", c_st); // ok
printf("%zu", C_ST); // ok
printf("%zu", sizeof(ul)); // ok
printf("%zu", sst); // not ok [NOT DETECTED ON MICROSOFT]
printf("%zu", sst); // not ok [NOT DETECTED]
printf("%zd", ul); // not ok
printf("%zd", st); // not ok
printf("%zd", ST); // not ok
printf("%zd", c_st); // not ok
printf("%zd", C_ST); // not ok
printf("%zd", sizeof(ul)); // not ok
printf("%zd", ul); // not ok [NOT DETECTED]
printf("%zd", st); // not ok [NOT DETECTED]
printf("%zd", ST); // not ok [NOT DETECTED]
printf("%zd", c_st); // not ok [NOT DETECTED]
printf("%zd", C_ST); // not ok [NOT DETECTED]
printf("%zd", sizeof(ul)); // not ok [NOT DETECTED]
printf("%zd", sst); // ok
{
char *ptr_a, *ptr_b;
@@ -80,8 +80,8 @@ void g()
printf("%tu", ptr_a - ptr_b); // ok
printf("%td", ptr_a - ptr_b); // ok
printf("%zu", ptr_a - ptr_b); // ok (dubious) [DETECTED ON LINUX ONLY]
printf("%zd", ptr_a - ptr_b); // ok (dubious) [DETECTED ON MICROSOFT ONLY]
printf("%zu", ptr_a - ptr_b); // ok (dubious)
printf("%zd", ptr_a - ptr_b); // ok (dubious)
}
}
@@ -92,3 +92,12 @@ void h(int i, struct some_type *j, int k)
// going on.
printf("%i %R %i", i, j, k); // GOOD (as far as we can tell)
}
typedef long ptrdiff_t;
void fun1(unsigned char* a, unsigned char* b) {
ptrdiff_t pdt;
printf("%td\n", pdt); // GOOD
printf("%td\n", a-b); // GOOD
}

View File

@@ -0,0 +1,2 @@
| printf.cpp:45:29:45:35 | test | This argument should be of type 'char *' but is of type 'char16_t *' |
| printf.cpp:52:29:52:35 | test | This argument should be of type 'char16_t *' but is of type 'wchar_t *' |

View File

@@ -0,0 +1 @@
Likely Bugs/Format/WrongTypeFormatArguments.ql

View File

@@ -0,0 +1,2 @@
| printf.cpp:15:5:15:12 | swprintf | char16_t | char | char16_t |
| printf.cpp:26:5:26:11 | sprintf | char | char16_t | char16_t |

View File

@@ -0,0 +1,8 @@
import cpp
from FormattingFunction f
select
f,
concat(f.getDefaultCharType().toString(), ", "),
concat(f.getNonDefaultCharType().toString(), ", "),
concat(f.getWideCharType().toString(), ", ")

View File

@@ -0,0 +1,53 @@
/*
* Test for custom definitions of *wprintf using different types than the
* platform wide character type.
*/
#define WCHAR char16_t
typedef void *va_list;
#define va_start(va, other)
#define va_end(args)
int vswprintf(WCHAR *dest, WCHAR *format, va_list args) {
return 0;
}
int swprintf(WCHAR *dest, WCHAR *format, ...) {
va_list args;
va_start(args, format);
int ret = vswprintf(dest, format, args);
va_end(args);
return ret;
}
int sprintf(char *dest, char *format, ...);
// ---
void test1() {
WCHAR string[20];
swprintf(string, u"test %s", u"test"); // GOOD
}
void test2() {
char string[20];
sprintf(string, "test %S", u"test"); // GOOD
}
void test3() {
char string[20];
sprintf(string, "test %s", u"test"); // BAD: `char16_t` string parameter read as `char` string
}
void test4() {
char string[20];
sprintf(string, "test %S", L"test"); // BAD: `wchar_t` string parameter read as `char16_t` string
}

View File

@@ -11,14 +11,6 @@
| printf1.h:45:18:45:20 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
| printf1.h:46:18:46:20 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
| printf1.h:47:19:47:21 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
| printf1.h:68:19:68:21 | sst | This argument should be of type 'size_t' but is of type 'long' |
| printf1.h:70:19:70:20 | ul | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:71:19:71:20 | st | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:72:19:72:20 | ST | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:73:19:73:22 | c_st | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:74:19:74:22 | C_ST | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:75:19:75:28 | sizeof(<expr>) | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:83:23:83:35 | ... - ... | This argument should be of type 'size_t' but is of type 'long' |
| real_world.h:61:21:61:22 | & ... | This argument should be of type 'int *' but is of type 'short *' |
| real_world.h:62:22:62:23 | & ... | This argument should be of type 'short *' but is of type 'int *' |
| real_world.h:63:22:63:24 | & ... | This argument should be of type 'short *' but is of type 'unsigned int *' |

View File

@@ -0,0 +1,4 @@
| common.h:12:12:12:17 | printf | char | wchar_t | wchar_t |
| format.h:4:13:4:17 | error | char | wchar_t | wchar_t |
| real_world.h:8:12:8:18 | fprintf | char | wchar_t | wchar_t |
| real_world.h:33:6:33:12 | msg_out | char | wchar_t | wchar_t |

View File

@@ -0,0 +1,8 @@
import cpp
from FormattingFunction f
select
f,
concat(f.getDefaultCharType().toString(), ", "),
concat(f.getNonDefaultCharType().toString(), ", "),
concat(f.getWideCharType().toString(), ", ")

View File

@@ -65,14 +65,14 @@ void g()
printf("%zu", c_st); // ok
printf("%zu", C_ST); // ok
printf("%zu", sizeof(ul)); // ok
printf("%zu", sst); // not ok [NOT DETECTED ON MICROSOFT]
printf("%zu", sst); // not ok [NOT DETECTED]
printf("%zd", ul); // not ok
printf("%zd", st); // not ok
printf("%zd", ST); // not ok
printf("%zd", c_st); // not ok
printf("%zd", C_ST); // not ok
printf("%zd", sizeof(ul)); // not ok
printf("%zd", ul); // not ok [NOT DETECTED]
printf("%zd", st); // not ok [NOT DETECTED]
printf("%zd", ST); // not ok [NOT DETECTED]
printf("%zd", c_st); // not ok [NOT DETECTED]
printf("%zd", C_ST); // not ok [NOT DETECTED]
printf("%zd", sizeof(ul)); // not ok [NOT DETECTED]
printf("%zd", sst); // ok
{
char *ptr_a, *ptr_b;
@@ -80,8 +80,8 @@ void g()
printf("%tu", ptr_a - ptr_b); // ok
printf("%td", ptr_a - ptr_b); // ok
printf("%zu", ptr_a - ptr_b); // ok (dubious) [DETECTED ON LINUX ONLY]
printf("%zd", ptr_a - ptr_b); // ok (dubious) [DETECTED ON MICROSOFT ONLY]
printf("%zu", ptr_a - ptr_b); // ok (dubious)
printf("%zd", ptr_a - ptr_b); // ok (dubious)
}
}
@@ -92,3 +92,12 @@ void h(int i, struct some_type *j, int k)
// going on.
printf("%i %R %i", i, j, k); // GOOD (as far as we can tell)
}
typedef long ptrdiff_t;
void fun1(unsigned char* a, unsigned char* b) {
ptrdiff_t pdt;
printf("%td\n", pdt); // GOOD
printf("%td\n", a-b); // GOOD
}

View File

@@ -11,7 +11,6 @@
| printf1.h:45:18:45:20 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
| printf1.h:46:18:46:20 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
| printf1.h:47:19:47:21 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
| printf1.h:70:19:70:20 | ul | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:71:19:71:20 | st | This argument should be of type 'ssize_t' but is of type 'unsigned long long' |
| printf1.h:72:19:72:20 | ST | This argument should be of type 'ssize_t' but is of type 'unsigned long long' |
| printf1.h:73:19:73:22 | c_st | This argument should be of type 'ssize_t' but is of type 'unsigned long long' |

View File

@@ -65,9 +65,9 @@ void g()
printf("%zu", c_st); // ok
printf("%zu", C_ST); // ok
printf("%zu", sizeof(ul)); // ok
printf("%zu", sst); // not ok [NOT DETECTED ON MICROSOFT]
printf("%zu", sst); // not ok [NOT DETECTED]
printf("%zd", ul); // not ok
printf("%zd", ul); // not ok [NOT DETECTED]
printf("%zd", st); // not ok
printf("%zd", ST); // not ok
printf("%zd", c_st); // not ok
@@ -80,8 +80,8 @@ void g()
printf("%tu", ptr_a - ptr_b); // ok
printf("%td", ptr_a - ptr_b); // ok
printf("%zu", ptr_a - ptr_b); // ok (dubious) [DETECTED ON LINUX ONLY]
printf("%zd", ptr_a - ptr_b); // ok (dubious) [DETECTED ON MICROSOFT ONLY]
printf("%zu", ptr_a - ptr_b); // ok (dubious)
printf("%zd", ptr_a - ptr_b); // ok (dubious) [FALSE POSITIVE]
}
}
@@ -92,3 +92,12 @@ void h(int i, struct some_type *j, int k)
// going on.
printf("%i %R %i", i, j, k); // GOOD (as far as we can tell)
}
typedef long long ptrdiff_t;
void fun1(unsigned char* a, unsigned char* b) {
ptrdiff_t pdt;
printf("%td\n", pdt); // GOOD
printf("%td\n", a-b); // GOOD
}

View File

@@ -43,7 +43,7 @@ void someFunction()
WCHAR filename[MAX_LONGPATH];
int linenum;
msg_out("Source file: %S @ %d\n", filename, linenum); // GOOD
msg_out("Source file: %S @ %d\n", filename, linenum); // GOOD [FALSE POSITIVE]
}
// --------------------------------------------------------------

View File

@@ -11,13 +11,13 @@
| printf1.h:45:18:45:20 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
| printf1.h:46:18:46:20 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
| printf1.h:47:19:47:21 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
| printf1.h:70:19:70:20 | ul | This argument should be of type 'ssize_t' but is of type 'unsigned long' |
| printf1.h:71:19:71:20 | st | This argument should be of type 'ssize_t' but is of type 'unsigned long long' |
| printf1.h:72:19:72:20 | ST | This argument should be of type 'ssize_t' but is of type 'unsigned long long' |
| printf1.h:73:19:73:22 | c_st | This argument should be of type 'ssize_t' but is of type 'unsigned long long' |
| printf1.h:74:19:74:22 | C_ST | This argument should be of type 'ssize_t' but is of type 'unsigned long long' |
| printf1.h:75:19:75:28 | sizeof(<expr>) | This argument should be of type 'ssize_t' but is of type 'unsigned long long' |
| printf1.h:84:23:84:35 | ... - ... | This argument should be of type 'ssize_t' but is of type 'long long' |
| printf1.h:130:18:130:18 | 0 | This argument should be of type 'void *' but is of type 'int' |
| real_world.h:61:21:61:22 | & ... | This argument should be of type 'int *' but is of type 'short *' |
| real_world.h:62:22:62:23 | & ... | This argument should be of type 'short *' but is of type 'int *' |
| real_world.h:63:22:63:24 | & ... | This argument should be of type 'short *' but is of type 'unsigned int *' |

View File

@@ -65,9 +65,9 @@ void g()
printf("%zu", c_st); // ok
printf("%zu", C_ST); // ok
printf("%zu", sizeof(ul)); // ok
printf("%zu", sst); // not ok [NOT DETECTED ON MICROSOFT]
printf("%zu", sst); // not ok [NOT DETECTED]
printf("%zd", ul); // not ok
printf("%zd", ul); // not ok [NOT DETECTED]
printf("%zd", st); // not ok
printf("%zd", ST); // not ok
printf("%zd", c_st); // not ok
@@ -80,8 +80,8 @@ void g()
printf("%tu", ptr_a - ptr_b); // ok
printf("%td", ptr_a - ptr_b); // ok
printf("%zu", ptr_a - ptr_b); // ok (dubious) [DETECTED ON LINUX ONLY]
printf("%zd", ptr_a - ptr_b); // ok (dubious) [DETECTED ON MICROSOFT ONLY]
printf("%zu", ptr_a - ptr_b); // ok (dubious)
printf("%zd", ptr_a - ptr_b); // ok (dubious) [FALSE POSITIVE]
}
}
@@ -92,3 +92,40 @@ void h(int i, struct some_type *j, int k)
// going on.
printf("%i %R %i", i, j, k); // GOOD (as far as we can tell)
}
typedef long long ptrdiff_t;
void fun1(unsigned char* a, unsigned char* b) {
ptrdiff_t pdt;
printf("%td\n", pdt); // GOOD
printf("%td\n", a-b); // GOOD
}
typedef wchar_t WCHAR_T; // WCHAR_T -> wchar_t -> int
typedef int MYCHAR; // MYCHAR -> int (notably not via the wchar_t typedef)
void fun2() {
wchar_t *myString1;
WCHAR_T *myString2;
int *myString3;
MYCHAR *myString4;
printf("%S", myString1); // GOOD
printf("%S", myString2); // GOOD
printf("%S", myString3); // GOOD
printf("%S", myString4); // GOOD
}
typedef void *VOIDPTR;
typedef int (*FUNPTR)(int);
void fun3(void *p1, VOIDPTR p2, FUNPTR p3, char *p4)
{
printf("%p\n", p1); // GOOD
printf("%p\n", p2); // GOOD
printf("%p\n", p3); // GOOD
printf("%p\n", p4); // GOOD
printf("%p\n", p4 + 1); // GOOD
printf("%p\n", 0); // GOOD [FALSE POSITIVE]
}

View File

@@ -171,3 +171,10 @@ void more_tests_2()
memset(iapa, 0, sizeof(iapa)); // GOOD
memset(iapa, 0, sizeof(intArrayPointer *)); // BAD
}
void more_tests_3()
{
myStruct ms;
decltype(&ms) msPtr = &ms;
memset(msPtr, 0, sizeof(myStruct)); // GOOD
}

View File

@@ -0,0 +1,34 @@
#define NULL 0
#define CONST const
typedef wchar_t WCHAR; // wc, 16-bit UNICODE character
typedef char CHAR;
typedef WCHAR *LPWSTR;
typedef CONST WCHAR *LPCWSTR;
typedef CHAR *LPSTR;
typedef CONST CHAR *LPCSTR;
void fconstWChar(LPCWSTR p) {}
void fWChar(LPWSTR p) {}
void Test()
{
char *lpChar = NULL;
wchar_t *lpWchar = NULL;
LPCSTR lpcstr = "b";
lpWchar = (LPWSTR)"a"; // BUG
lpWchar = (LPWSTR)lpcstr; // BUG
lpWchar = (wchar_t*)lpChar; // BUG
fconstWChar((LPCWSTR)lpChar); // BUG
fWChar((LPWSTR)lpChar); // BUG
lpChar = (LPSTR)"a"; // Valid
lpWchar = (LPWSTR)L"a"; // Valid
fconstWChar((LPCWSTR)lpWchar); // Valid
fWChar(lpWchar); // Valid
}

View File

@@ -0,0 +1,5 @@
| WcharCharConversion.cpp:21:20:21:22 | array to pointer conversion | Conversion from const char * to LPWSTR. Use of invalid string can lead to undefined behavior. |
| WcharCharConversion.cpp:22:20:22:25 | lpcstr | Conversion from LPCSTR to LPWSTR. Use of invalid string can lead to undefined behavior. |
| WcharCharConversion.cpp:24:22:24:27 | lpChar | Conversion from char * to wchar_t *. Use of invalid string can lead to undefined behavior. |
| WcharCharConversion.cpp:26:23:26:28 | lpChar | Conversion from char * to LPCWSTR. Use of invalid string can lead to undefined behavior. |
| WcharCharConversion.cpp:27:17:27:22 | lpChar | Conversion from char * to LPWSTR. Use of invalid string can lead to undefined behavior. |

View File

@@ -0,0 +1 @@
Security/CWE/CWE-704/WcharCharConversion.ql

View File

@@ -1 +0,0 @@
<queries language="csharp"/>

View File

@@ -1 +0,0 @@
Files/FLinesOfCommentedCode.ql

View File

@@ -1 +0,0 @@
Files/FLinesOfDuplicatedCode.ql

View File

@@ -0,0 +1 @@
Metrics/Files/FLinesOfCommentedCode.ql

View File

@@ -0,0 +1 @@
Metrics/Files/FLinesOfDuplicatedCode.ql

View File

@@ -18,114 +18,6 @@ private Callable dispatchCand(Call c) {
result = viableImpl_inp(c)
}
/**
* Holds if `p` is the `i`th parameter of a viable dispatch target of `call`.
* The instance parameter is considered to have index `-1`.
*/
pragma[nomagic]
private predicate viableParamCand(Call call, int i, ParameterNode p) {
exists(Callable callable |
callable = dispatchCand(call) and
p.isParameterOf(callable, i)
)
}
/**
* Holds if `arg` is a possible argument to `p` taking virtual dispatch into account.
*/
private predicate viableArgParamCand(ArgumentNode arg, ParameterNode p) {
exists(int i, Call call |
viableParamCand(call, i, p) and
arg.argumentOf(call, i)
)
}
/**
* Holds if data may flow from `n1` to `n2` in a single step through a call or a return.
*/
private predicate callFlowStepCand(Node n1, Node n2) {
exists(ReturnStmt ret, Method m |
ret.getEnclosingCallable() = m and
ret.getResult() = n1.asExpr() and
m = dispatchCand(n2.asExpr())
) or
viableArgParamCand(n1, n2)
}
/**
* Holds if data may flow from `n1` to `n2` in a single step that does not go
* through a call or a return.
*/
private predicate flowStep(Node n1, Node n2) {
exists(BaseSsaVariable v, BaseSsaVariable def |
def.(BaseSsaUpdate).getDefiningExpr().(VariableAssign).getSource() = n1.asExpr() or
def.(BaseSsaImplicitInit).isParameterDefinition(n1.asParameter()) or
exists(EnhancedForStmt for |
for.getVariable() = def.(BaseSsaUpdate).getDefiningExpr() and
for.getExpr() = n1.asExpr() and
n1.getType() instanceof Array
)
|
v.getAnUltimateDefinition() = def and
v.getAUse() = n2.asExpr()
) or
exists(Callable c |
n1.(InstanceParameterNode).getCallable() = c
|
exists(InstanceAccess ia | ia = n2.asExpr() and ia.getEnclosingCallable() = c and ia.isOwnInstanceAccess()) or
n2.(ImplicitInstanceAccess).getInstanceAccess().(OwnInstanceAccess).getEnclosingCallable() = c
) or
exists(Field f |
f.getAnAssignedValue() = n1.asExpr() and
n2.asExpr().(FieldRead).getField() = f
) or
exists(EnumType enum, Method getValue |
enum.getAnEnumConstant().getAnAssignedValue() = n1.asExpr() and
getValue.getDeclaringType() = enum and
(getValue.hasName("values") or getValue.hasName("valueOf")) and
n2.asExpr().(MethodAccess).getMethod() = getValue
) or
n2.asExpr().(ParExpr).getExpr() = n1.asExpr() or
n2.asExpr().(CastExpr).getExpr() = n1.asExpr() or
n2.asExpr().(ConditionalExpr).getTrueExpr() = n1.asExpr() or
n2.asExpr().(ConditionalExpr).getFalseExpr() = n1.asExpr() or
n2.asExpr().(AssignExpr).getSource() = n1.asExpr() or
n2.asExpr().(ArrayInit).getAnInit() = n1.asExpr() or
n2.asExpr().(ArrayCreationExpr).getInit() = n1.asExpr() or
n2.asExpr().(ArrayAccess).getArray() = n1.asExpr() or
exists(Argument arg | n1.asExpr() = arg and arg.isVararg() and n2.(ImplicitVarargsArray).getCall() = arg.getCall()) or
exists(AssignExpr a, Variable v |
a.getSource() = n1.asExpr() and
a.getDest().(ArrayAccess).getArray() = v.getAnAccess() and
n2.asExpr() = v.getAnAccess().(RValue)
) or
exists(Variable v, MethodAccess put, MethodAccess get |
put.getArgument(1) = n1.asExpr() and
put.getMethod().(MapMethod).hasName("put") and
put.getQualifier() = v.getAnAccess() and
get.getQualifier() = v.getAnAccess() and
get.getMethod().(MapMethod).hasName("get") and
n2.asExpr() = get
) or
exists(Variable v, MethodAccess add |
add.getAnArgument() = n1.asExpr() and
add.getMethod().(CollectionMethod).hasName("add") and
add.getQualifier() = v.getAnAccess()
|
exists(MethodAccess get |
get.getQualifier() = v.getAnAccess() and
get.getMethod().(CollectionMethod).hasName("get") and
n2.asExpr() = get
) or
exists(EnhancedForStmt for, BaseSsaVariable ssa, BaseSsaVariable def |
for.getVariable() = def.(BaseSsaUpdate).getDefiningExpr() and
for.getExpr() = v.getAnAccess() and
ssa.getAnUltimateDefinition() = def and
ssa.getAUse() = n2.asExpr()
)
)
}
/**
* Holds if `t` and all its enclosing types are public.
*/
@@ -203,6 +95,130 @@ private predicate dispatchOrigin(ClassInstanceExpr cie, MethodAccess ma, Method
trackedMethodOnType(m, cie.getConstructedType().getSourceDeclaration())
}
/** Holds if `t` is a type that is relevant for dispatch flow. */
private predicate relevant(RefType t) {
exists(ClassInstanceExpr cie | dispatchOrigin(cie, _, _) and t = cie.getConstructedType().getSourceDeclaration()) or
relevant(t.getErasure()) or
exists(RefType r | relevant(r) and t = r.getASourceSupertype()) or
relevant(t.(Array).getComponentType()) or
t instanceof MapType or
t instanceof CollectionType
}
/** A node with a type that is relevant for dispatch flow. */
private class RelevantNode extends Node {
RelevantNode() { relevant(this.getType()) }
}
/**
* Holds if `p` is the `i`th parameter of a viable dispatch target of `call`.
* The instance parameter is considered to have index `-1`.
*/
pragma[nomagic]
private predicate viableParamCand(Call call, int i, ParameterNode p) {
exists(Callable callable |
callable = dispatchCand(call) and
p.isParameterOf(callable, i) and
p instanceof RelevantNode
)
}
/**
* Holds if `arg` is a possible argument to `p` taking virtual dispatch into account.
*/
private predicate viableArgParamCand(ArgumentNode arg, ParameterNode p) {
exists(int i, Call call |
viableParamCand(call, i, p) and
arg.argumentOf(call, i)
)
}
/**
* Holds if data may flow from `n1` to `n2` in a single step through a call or a return.
*/
private predicate callFlowStepCand(RelevantNode n1, RelevantNode n2) {
exists(ReturnStmt ret, Method m |
ret.getEnclosingCallable() = m and
ret.getResult() = n1.asExpr() and
m = dispatchCand(n2.asExpr())
) or
viableArgParamCand(n1, n2)
}
/**
* Holds if data may flow from `n1` to `n2` in a single step that does not go
* through a call or a return.
*/
private predicate flowStep(RelevantNode n1, RelevantNode n2) {
exists(BaseSsaVariable v, BaseSsaVariable def |
def.(BaseSsaUpdate).getDefiningExpr().(VariableAssign).getSource() = n1.asExpr() or
def.(BaseSsaImplicitInit).isParameterDefinition(n1.asParameter()) or
exists(EnhancedForStmt for |
for.getVariable() = def.(BaseSsaUpdate).getDefiningExpr() and
for.getExpr() = n1.asExpr() and
n1.getType() instanceof Array
)
|
v.getAnUltimateDefinition() = def and
v.getAUse() = n2.asExpr()
) or
exists(Callable c |
n1.(InstanceParameterNode).getCallable() = c
|
exists(InstanceAccess ia | ia = n2.asExpr() and ia.getEnclosingCallable() = c and ia.isOwnInstanceAccess()) or
n2.(ImplicitInstanceAccess).getInstanceAccess().(OwnInstanceAccess).getEnclosingCallable() = c
) or
exists(Field f |
f.getAnAssignedValue() = n1.asExpr() and
n2.asExpr().(FieldRead).getField() = f
) or
exists(EnumType enum, Method getValue |
enum.getAnEnumConstant().getAnAssignedValue() = n1.asExpr() and
getValue.getDeclaringType() = enum and
(getValue.hasName("values") or getValue.hasName("valueOf")) and
n2.asExpr().(MethodAccess).getMethod() = getValue
) or
n2.asExpr().(ParExpr).getExpr() = n1.asExpr() or
n2.asExpr().(CastExpr).getExpr() = n1.asExpr() or
n2.asExpr().(ConditionalExpr).getTrueExpr() = n1.asExpr() or
n2.asExpr().(ConditionalExpr).getFalseExpr() = n1.asExpr() or
n2.asExpr().(AssignExpr).getSource() = n1.asExpr() or
n2.asExpr().(ArrayInit).getAnInit() = n1.asExpr() or
n2.asExpr().(ArrayCreationExpr).getInit() = n1.asExpr() or
n2.asExpr().(ArrayAccess).getArray() = n1.asExpr() or
exists(Argument arg | n1.asExpr() = arg and arg.isVararg() and n2.(ImplicitVarargsArray).getCall() = arg.getCall()) or
exists(AssignExpr a, Variable v |
a.getSource() = n1.asExpr() and
a.getDest().(ArrayAccess).getArray() = v.getAnAccess() and
n2.asExpr() = v.getAnAccess().(RValue)
) or
exists(Variable v, MethodAccess put, MethodAccess get |
put.getArgument(1) = n1.asExpr() and
put.getMethod().(MapMethod).hasName("put") and
put.getQualifier() = v.getAnAccess() and
get.getQualifier() = v.getAnAccess() and
get.getMethod().(MapMethod).hasName("get") and
n2.asExpr() = get
) or
exists(Variable v, MethodAccess add |
add.getAnArgument() = n1.asExpr() and
add.getMethod().(CollectionMethod).hasName("add") and
add.getQualifier() = v.getAnAccess()
|
exists(MethodAccess get |
get.getQualifier() = v.getAnAccess() and
get.getMethod().(CollectionMethod).hasName("get") and
n2.asExpr() = get
) or
exists(EnhancedForStmt for, BaseSsaVariable ssa, BaseSsaVariable def |
for.getVariable() = def.(BaseSsaUpdate).getDefiningExpr() and
for.getExpr() = v.getAnAccess() and
ssa.getAnUltimateDefinition() = def and
ssa.getAUse() = n2.asExpr()
)
)
}
/**
* Holds if `n` is forward-reachable from a relevant `ClassInstanceExpr`.
*/

View File

@@ -5,6 +5,7 @@
+ semmlecode-javascript-queries/Expressions/SuspiciousInvocation.ql: /Correctness/Expressions
+ semmlecode-javascript-queries/Expressions/SuspiciousPropAccess.ql: /Correctness/Expressions
+ semmlecode-javascript-queries/Expressions/UnboundEventHandlerReceiver.ql: /Correctness/Expressions
+ semmlecode-javascript-queries/Expressions/UnclearOperatorPrecedence.ql: /Correctness/Expressions
+ semmlecode-javascript-queries/Expressions/UnknownDirective.ql: /Correctness/Expressions
+ semmlecode-javascript-queries/Expressions/WhitespaceContradictsPrecedence.ql: /Correctness/Expressions
+ semmlecode-javascript-queries/LanguageFeatures/IllegalInvocation.ql: /Correctness/Language Features

View File

@@ -22,6 +22,7 @@
+ semmlecode-javascript-queries/Security/CWE-601/ClientSideUrlRedirect.ql: /Security/CWE/CWE-601
+ semmlecode-javascript-queries/Security/CWE-601/ServerSideUrlRedirect.ql: /Security/CWE/CWE-601
+ semmlecode-javascript-queries/Security/CWE-611/Xxe.ql: /Security/CWE/CWE-611
+ semmlecode-javascript-queries/Security/CWE-640/HostHeaderPoisoningInEmailGeneration.ql: /Security/CWE/CWE-640
+ semmlecode-javascript-queries/Security/CWE-643/XpathInjection.ql: /Security/CWE/CWE-643
+ semmlecode-javascript-queries/Security/CWE-730/RegExpInjection.ql: /Security/CWE/CWE-730
+ semmlecode-javascript-queries/Security/CWE-770/MissingRateLimiting.ql: /Security/CWE/CWE-770

View File

@@ -33,12 +33,15 @@ predicate deadStoreOfLocal(VarDef vd, PurelyLocalVariable v) {
* is itself an expression evaluating to `null` or `undefined`.
*/
predicate isNullOrUndef(Expr e) {
// `null` or `undefined`
e instanceof NullLiteral or
e.(VarAccess).getName() = "undefined" or
e instanceof VoidExpr or
// recursive case to catch multi-assignments of the form `x = y = null`
isNullOrUndef(e.(AssignExpr).getRhs())
exists (Expr inner |
inner = e.stripParens() |
// `null` or `undefined`
inner instanceof NullLiteral or
inner.(VarAccess).getName() = "undefined" or
inner instanceof VoidExpr or
// recursive case to catch multi-assignments of the form `x = y = null`
isNullOrUndef(inner.(AssignExpr).getRhs())
)
}
/**

View File

@@ -39,7 +39,7 @@ property of the name stored in variable <code>member</code>:
<p>
However, this test is ineffective as written: the operator <code>!</code> binds more
tighly than <code>in</code>, so it is applied first. Applying <code>!</code> to a
tightly than <code>in</code>, so it is applied first. Applying <code>!</code> to a
non-empty string yields <code>false</code>, so the <code>in</code> operator actually
ends up checking whether <code>obj</code> contains a property called <code>"false"</code>.
</p>

View File

@@ -0,0 +1,46 @@
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
<qhelp>
<overview>
<p>
Nested expressions that rely on less well-known operator precedence rules can be
hard to read and understand. They could even indicate a bug where the author of the
code misunderstood the precedence rules.
</p>
</overview>
<recommendation>
<p>
Use parentheses or additional whitespace to clarify grouping.
</p>
</recommendation>
<example>
<p>
Consider the following snippet of code:
</p>
<sample src="examples/UnclearOperatorPrecedence.js" />
<p>
It might look like this tests whether <code>x</code> and <code>y</code> have any bits in
common, but in fact <code>==</code> binds more tightly than <code>&amp;</code>, so the test
is equivalent to <code>x &amp; (y == 0)</code>.
</p>
<p>
If this is the intended interpretation, parentheses should be used to clarify this. You could
also consider adding extra whitespace around <code>&amp;</code> or removing whitespace
around <code>==</code> to make it visually apparent that it binds less tightly:
<code>x &amp; y==0</code>.
</p>
<p>
Probably the best approach in this case, though, would be to use the <code>&amp;&amp;</code>
operator instead to clarify the intended interpretation: <code>x &amp;&amp; y == 0</code>.
</p>
</example>
<references>
<li>Mozilla Developer Network, <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence">Operator precedence</a>.</li>
</references>
</qhelp>

View File

@@ -0,0 +1,29 @@
/**
* @name Unclear precedence of nested operators
* @description Nested expressions involving binary bitwise operators and comparisons are easy
* to misunderstand without additional disambiguating parentheses or whitespace.
* @kind problem
* @problem.severity recommendation
* @id js/unclear-operator-precedence
* @tags maintainability
* correctness
* statistical
* non-attributable
* external/cwe/cwe-783
* @precision very-high
*/
import javascript
from BitwiseBinaryExpr bit, Comparison rel, Expr other
where bit.hasOperands(rel, other) and
// only flag if whitespace doesn't clarify the nesting (note that if `bit` has less
// whitespace than `rel`, it will be reported by `js/whitespace-contradicts-precedence`)
bit.getWhitespaceAroundOperator() = rel.getWhitespaceAroundOperator() and
// don't flag if the other operand is itself a comparison,
// since the nesting tends to be visually more obvious in such cases
not other instanceof Comparison and
// don't flag occurrences in minified code
not rel.getTopLevel().isMinified()
select rel, "The '" + rel.getOperator() + "' operator binds more tightly than " +
"'" + bit.getOperator() + "', which may not be obvious in this case."

View File

@@ -54,29 +54,6 @@ class HarmlessNestedExpr extends BinaryExpr {
}
}
/** Holds if the right operand of `expr` starts on line `line`, at column `col`. */
predicate startOfBinaryRhs(BinaryExpr expr, int line, int col) {
exists(Location rloc | rloc = expr.getRightOperand().getLocation() |
rloc.getStartLine() = line and rloc.getStartColumn() = col
)
}
/** Holds if the left operand of `expr` ends on line `line`, at column `col`. */
predicate endOfBinaryLhs(BinaryExpr expr, int line, int col) {
exists(Location lloc | lloc = expr.getLeftOperand().getLocation() |
lloc.getEndLine() = line and lloc.getEndColumn() = col
)
}
/** Gets the number of whitespace characters around the operator of `expr`. */
int operatorWS(BinaryExpr expr) {
exists(int line, int lcol, int rcol |
endOfBinaryLhs(expr, line, lcol) and
startOfBinaryRhs(expr, line, rcol) and
result = rcol - lcol + 1 - expr.getOperator().length()
)
}
/**
* Holds if `inner` is an operand of `outer`, and the relative precedence
* may not be immediately clear, but is important for the semantics of
@@ -88,10 +65,8 @@ predicate interestingNesting(BinaryExpr inner, BinaryExpr outer) {
not inner instanceof HarmlessNestedExpr
}
from BinaryExpr inner, BinaryExpr outer, int wsouter, int wsinner
from BinaryExpr inner, BinaryExpr outer
where interestingNesting(inner, outer) and
wsinner = operatorWS(inner) and wsouter = operatorWS(outer) and
wsinner % 2 = 0 and wsouter % 2 = 0 and
wsinner > wsouter and
inner.getWhitespaceAroundOperator() > outer.getWhitespaceAroundOperator() and
not outer.getTopLevel().isMinified()
select outer, "Whitespace around nested operators contradicts precedence."

View File

@@ -0,0 +1,3 @@
if (x & y == 0) {
// ...
}

View File

@@ -6,7 +6,7 @@ express().get('/list-directory', function(req, res) {
var list = '<ul>';
fileNames.forEach(fileName => {
// BAD: `fileName` can contain HTML elements
list += '<li>' + fileName '</li>';
list += '<li>' + fileName + '</li>';
});
list += '</ul>'
res.send(list);

View File

@@ -7,7 +7,7 @@ express().get('/list-directory', function(req, res) {
var list = '<ul>';
fileNames.forEach(fileName => {
// GOOD: escaped `fileName` can not contain HTML elements
list += '<li>' + escape(fileName) '</li>';
list += '<li>' + escape(fileName) + '</li>';
});
list += '</ul>'
res.send(list);

View File

@@ -0,0 +1,16 @@
/**
* @name File Access data flows to Http POST/PUT
* @description Writing data from file directly to http body or request header can be an indication to data exfiltration or unauthorized information disclosure.
* @kind problem
* @problem.severity warning
* @id js/file-access-to-http
* @tags security
* external/cwe/cwe-200
*/
import javascript
import semmle.javascript.security.dataflow.FileAccessToHttp
from FileAccessToHttpDataFlow::Configuration config, DataFlow::Node src, DataFlow::Node sink
where config.hasFlow (src, sink)
select src, "$@ flows directly to Http request body", sink, "File access"

View File

@@ -0,0 +1,46 @@
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
<qhelp>
<overview>
<p>
Using the HTTP Host header to construct a link in an email can facilitate phishing attacks and leak password reset tokens.
A malicious user can send an HTTP request to the targeted web site, but with a Host header that refers to his own web site.
This means the emails will be sent out to potential victims, originating from a server they trust, but with
links leading to a malicious web site.
</p>
<p>
If the email contains a password reset link, and should the victim click the link, the secret reset token will be leaked to the attacker.
Using the leaked token, the attacker can then construct the real reset link and use it to change the victim's password.
</p>
</overview>
<recommendation>
<p>
Obtain the server's host name from a configuration file and avoid relying on the Host header.
</p>
</recommendation>
<example>
<p>
The following example uses the <code>req.host</code> to generate a password reset link.
This value is derived from the Host header, and can thus be set to anything by an attacker:
</p>
<sample src="examples/HostHeaderPoisoningInEmailGeneration.js"/>
<p>
To ensure the link refers to the correct web site, get the host name from a configuration file:
</p>
<sample src="examples/HostHeaderPoisoningInEmailGenerationGood.js"/>
</example>
<references>
<li>
Mitre:
<a href="https://cwe.mitre.org/data/definitions/640.html">CWE-640: Weak Password Recovery Mechanism for Forgotten Password</a>.
</li>
<li>
Ian Muscat:
<a href="https://www.acunetix.com/blog/articles/automated-detection-of-host-header-attacks/">What is a Host Header Attack?</a>.
</li>
</references>
</qhelp>

View File

@@ -0,0 +1,29 @@
/**
* @name Host header poisoning in email generation
* @description Using the HTTP Host header to construct a link in an email can facilitate phishing attacks and leak password reset tokens.
* @kind problem
* @problem.severity error
* @precision high
* @id js/host-header-forgery-in-email-generation
* @tags security
* external/cwe/cwe-640
*/
import javascript
class TaintedHostHeader extends TaintTracking::Configuration {
TaintedHostHeader() { this = "TaintedHostHeader" }
override predicate isSource(DataFlow::Node node) {
exists (HTTP::RequestHeaderAccess input | node = input |
input.getKind() = "header" and
input.getAHeaderName() = "host")
}
override predicate isSink(DataFlow::Node node) {
exists (EmailSender email | node = email.getABody())
}
}
from TaintedHostHeader taint, DataFlow::Node src, DataFlow::Node sink
where taint.hasFlow(src, sink)
select sink, "Links in this email can be hijacked by poisoning the HTTP host header $@.", src, "here"

View File

@@ -0,0 +1,19 @@
let nodemailer = require('nodemailer');
let express = require('express');
let backend = require('./backend');
let app = express();
let config = JSON.parse(fs.readFileSync('config.json', 'utf8'));
app.post('/resetpass', (req, res) => {
let email = req.query.email;
let transport = nodemailer.createTransport(config.smtp);
let token = backend.getUserSecretResetToken(email);
transport.sendMail({
from: 'webmaster@example.com',
to: email,
subject: 'Forgot password',
text: `Click to reset password: https://${req.host}/resettoken/${token}`,
});
});

View File

@@ -0,0 +1,19 @@
let nodemailer = require('nodemailer');
let express = require('express');
let backend = require('./backend');
let app = express();
let config = JSON.parse(fs.readFileSync('config.json', 'utf8'));
app.post('/resetpass', (req, res) => {
let email = req.query.email;
let transport = nodemailer.createTransport(config.smtp);
let token = backend.getUserSecretResetToken(email);
transport.sendMail({
from: 'webmaster@example.com',
to: email,
subject: 'Forgot password',
text: `Click to reset password: https://${config.hostname}/resettoken/${token}`,
});
});

View File

@@ -0,0 +1,16 @@
/**
* @name Http response data flows to File Access
* @description Writing data from an HTTP request directly to the file system allows arbitrary file upload and might indicate a backdoor.
* @kind problem
* @problem.severity warning
* @id js/http-to-file-access
* @tags security
* external/cwe/cwe-912
*/
import javascript
import semmle.javascript.security.dataflow.HttpToFileAccess
from HttpToFileAccessFlow::Configuration configuration, DataFlow::Node src, DataFlow::Node sink
where configuration.hasFlow(src, sink)
select sink, "$@ flows to file system", src, "Untrusted data received from Http response"

View File

@@ -13,6 +13,7 @@ import semmle.javascript.Constants
import semmle.javascript.DataFlow
import semmle.javascript.DefUse
import semmle.javascript.DOM
import semmle.javascript.EmailClients
import semmle.javascript.Errors
import semmle.javascript.ES2015Modules
import semmle.javascript.Expr

View File

@@ -281,7 +281,7 @@ import javascript
*/
class ControlFlowNode extends @cfg_node, Locatable {
/** Gets a node succeeding this node in the CFG. */
ControlFlowNode getASuccessor() {
cached ControlFlowNode getASuccessor() {
successor(this, result)
}
@@ -457,3 +457,47 @@ class ConcreteControlFlowNode extends ControlFlowNode {
not this instanceof SyntheticControlFlowNode
}
}
/**
* A CFG node corresponding to a nested (that is, non-toplevel) import declaration.
*
* This is a non-standard language feature that is not currently supported very well
* by the extractor, in particular such imports do not appear in the control flow graph
* generated by the extractor. We patch them in by overriding `getASuccessor`; once an
* extractor fix becomes available, this class can be removed.
*/
private class NestedImportDeclaration extends ImportDeclaration {
NestedImportDeclaration() {
exists (ASTNode p | p = getParent() |
not p instanceof TopLevel
) and
// if there are no specifiers, the default control flow graph is fine
exists(getASpecifier())
}
override ControlFlowNode getASuccessor() {
result = getSpecifier(0).getFirstControlFlowNode()
}
}
/**
* A CFG node corresponding to an import specifier in a nested import declaration.
*
* As for `NestedImportDeclaration` above, this is a temporary workaround that will be
* removed once extractor support for this non-standard language feature becomes available.
*/
private class NestedImportSpecifier extends ImportSpecifier {
NestedImportDeclaration decl;
int i;
NestedImportSpecifier() {
this = decl.getSpecifier(i)
}
override ControlFlowNode getASuccessor() {
result = decl.getSpecifier(i+1).getFirstControlFlowNode()
or
not exists(decl.getSpecifier(i+1)) and
successor(decl, result)
}
}

View File

@@ -22,13 +22,27 @@ abstract class SystemCommandExecution extends DataFlow::Node {
}
/**
* A data flow node that performs a file system access.
* A data flow node that performs a file system access (read, write, copy, permissions, stats, etc).
*/
abstract class FileSystemAccess extends DataFlow::Node {
/** Gets an argument to this file system access that is interpreted as a path. */
abstract DataFlow::Node getAPathArgument();
/** Gets a node that represents file system access data, such as buffer the data is copied to. */
abstract DataFlow::Node getDataNode();
}
/**
* A data flow node that performs read file system access.
*/
abstract class FileSystemReadAccess extends FileSystemAccess { }
/**
* A data flow node that performs write file system access.
*/
abstract class FileSystemWriteAccess extends FileSystemAccess { }
/**
* A data flow node that contains a file name or an array of file names from the local file system.
*/

View File

@@ -27,7 +27,7 @@ class ES2015Module extends Module {
/** Gets an export declaration in this module. */
ExportDeclaration getAnExport() {
result.getContainer() = this
result.getTopLevel() = this
}
override Module getAnImportedModule() {
@@ -55,7 +55,7 @@ class ES2015Module extends Module {
/** An import declaration. */
class ImportDeclaration extends Stmt, Import, @importdeclaration {
override ES2015Module getEnclosingModule() {
result = getContainer()
result = getTopLevel()
}
override PathExprInModule getImportedPath() {
@@ -254,7 +254,7 @@ class BulkReExportDeclaration extends ReExportDeclaration, @exportalldeclaration
* but we ignore this subtlety.
*/
private predicate isShadowedFromBulkExport(BulkReExportDeclaration reExport, string name) {
exists (ExportNamedDeclaration other | other.getContainer() = reExport.getEnclosingModule() |
exists (ExportNamedDeclaration other | other.getTopLevel() = reExport.getEnclosingModule() |
other.getAnExportedDecl().getName() = name
or
other.getASpecifier().getExportedName() = name)

View File

@@ -0,0 +1,68 @@
import javascript
/**
* An operation that sends an email.
*/
abstract class EmailSender extends DataFlow::DefaultSourceNode {
/**
* Gets a data flow node holding the plaintext version of the email body.
*/
abstract DataFlow::Node getPlainTextBody();
/**
* Gets a data flow node holding the HTML body of the email.
*/
abstract DataFlow::Node getHtmlBody();
/**
* Gets a data flow node holding the address of the email recipient(s).
*/
abstract DataFlow::Node getTo();
/**
* Gets a data flow node holding the address of the email sender.
*/
abstract DataFlow::Node getFrom();
/**
* Gets a data flow node holding the email subject.
*/
abstract DataFlow::Node getSubject();
/**
* Gets a data flow node that refers to the HTML body or plaintext body of the email.
*/
DataFlow::Node getABody() {
result = getPlainTextBody() or
result = getHtmlBody()
}
}
/**
* An email-sending call based on the `nodemailer` package.
*/
private class NodemailerEmailSender extends EmailSender, DataFlow::MethodCallNode {
NodemailerEmailSender() {
this = DataFlow::moduleMember("nodemailer", "createTransport").getACall().getAMethodCall("sendMail")
}
override DataFlow::Node getPlainTextBody() {
result = getOptionArgument(0, "text")
}
override DataFlow::Node getHtmlBody() {
result = getOptionArgument(0, "html")
}
override DataFlow::Node getTo() {
result = getOptionArgument(0, "to")
}
override DataFlow::Node getFrom() {
result = getOptionArgument(0, "from")
}
override DataFlow::Node getSubject() {
result = getOptionArgument(0, "subject")
}
}

View File

@@ -1008,6 +1008,25 @@ class BinaryExpr extends @binaryexpr, Expr {
override ControlFlowNode getFirstControlFlowNode() {
result = getLeftOperand().getFirstControlFlowNode()
}
/**
* Gets the number of whitespace characters around the operator of this expression.
*
* This predicate is only defined if both operands are on the same line, and if the
* amount of whitespace before and after the operator are the same.
*/
int getWhitespaceAroundOperator() {
exists (Token lastLeft, Token operator, Token firstRight, int l, int c1, int c2, int c3, int c4 |
lastLeft = getLeftOperand().getLastToken() and
operator = lastLeft.getNextToken() and
firstRight = operator.getNextToken() and
lastLeft.getLocation().hasLocationInfo(_, _, _, l, c1) and
operator.getLocation().hasLocationInfo(_, l, c2, l, c3) and
firstRight.getLocation().hasLocationInfo(_, l, c4, _, _) and
result = c2-c1-1 and
result = c4-c3-1
)
}
}
/**

View File

@@ -283,6 +283,18 @@ abstract class AdditionalSink extends DataFlow::Node {
abstract predicate isSinkFor(Configuration cfg);
}
/**
* An invocation that is modeled as a partial function application.
*
* This contributes additional argument-passing flow edges that should be added to all data flow configurations.
*/
cached abstract class AdditionalPartialInvokeNode extends DataFlow::InvokeNode {
/**
* Holds if `argument` is passed as argument `index` to the function in `callback`.
*/
cached abstract predicate isPartialArgument(DataFlow::Node callback, DataFlow::Node argument, int index);
}
/**
* Additional flow step to model flow from import specifiers into the SSA variable
* corresponding to the imported variable.
@@ -299,6 +311,37 @@ private class FlowStepThroughImport extends AdditionalFlowStep, DataFlow::ValueN
}
}
/**
* A partial call through the built-in `Function.prototype.bind`.
*/
private class BindPartialCall extends AdditionalPartialInvokeNode, DataFlow::MethodCallNode {
BindPartialCall() {
getMethodName() = "bind"
}
override predicate isPartialArgument(DataFlow::Node callback, DataFlow::Node argument, int index) {
callback = getReceiver() and
argument = getArgument(index + 1)
}
}
/**
* A partial call through `_.partial` or a function with a similar interface.
*/
private class LibraryPartialCall extends AdditionalPartialInvokeNode {
LibraryPartialCall() {
this = LodashUnderscore::member("partial").getACall() or
this = DataFlow::moduleMember("ramda", "partial").getACall()
}
override predicate isPartialArgument(DataFlow::Node callback, DataFlow::Node argument, int index) {
callback = getArgument(0) and
exists (DataFlow::ArrayLiteralNode array |
array.flowsTo(getArgument(1)) and
argument = array.getElement(index))
}
}
/**
* Holds if there is a flow step from `pred` to `succ` described by `summary`
* under configuration `cfg`.
@@ -395,16 +438,18 @@ private predicate isRelevant(DataFlow::Node nd, DataFlow::Configuration cfg) {
* either `pred` is an argument of `f` and `succ` the corresponding parameter, or
* `pred` is a variable definition whose value is captured by `f` at `succ`.
*/
pragma[noopt]
private predicate callInputStep(Function f, DataFlow::Node invk,
DataFlow::Node pred, DataFlow::Node succ,
DataFlow::Configuration cfg) {
isRelevant(pred, cfg) and
(
isRelevant(pred, cfg) and
exists (Parameter parm |
argumentPassing(invk, pred, f, parm) and
succ = DataFlow::parameterNode(parm)
)
or
isRelevant(pred, cfg) and
exists (SsaDefinition prevDef, SsaDefinition def |
pred = DataFlow::ssaDefinitionNode(prevDef) and
calls(invk, f) and captures(f, prevDef, def) and
@@ -445,6 +490,7 @@ private predicate flowThroughCall(DataFlow::Node input, DataFlow::Node invk,
DataFlow::Configuration cfg, boolean valuePreserving) {
exists (Function f, DataFlow::ValueNode ret |
ret.asExpr() = f.getAReturnedExpr() and
calls(invk, f) and // Do not consider partial calls
reachableFromInput(f, invk, input, ret, cfg, PathSummary::level(valuePreserving))
)
}

View File

@@ -307,7 +307,7 @@ class ArrayLiteralNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode
/** A data flow node corresponding to a `new Array()` or `Array()` invocation. */
class ArrayConstructorInvokeNode extends DataFlow::InvokeNode {
ArrayConstructorInvokeNode() {
getCallee() = DataFlow::globalVarRef("Array")
getCalleeNode() = DataFlow::globalVarRef("Array")
}
/** Gets the `i`th initial element of this array, if one is provided. */

View File

@@ -27,6 +27,21 @@ predicate calls(DataFlow::InvokeNode invk, Function f) {
f = invk.getACallee()
}
/**
* Holds if `invk` may invoke `f` indirectly through the given `callback` argument.
*
* This only holds for explicitly modeled partial calls.
*/
private predicate partiallyCalls(DataFlow::AdditionalPartialInvokeNode invk, DataFlow::AnalyzedNode callback, Function f) {
invk.isPartialArgument(callback, _, _) and
exists (AbstractFunction callee | callee = callback.getAValue() |
if callback.getAValue().isIndefinite("global") then
(f = callee.getFunction() and f.getFile() = invk.getFile())
else
f = callee.getFunction()
)
}
/**
* Holds if `f` captures the variable defined by `def` in `cap`.
*/
@@ -69,6 +84,11 @@ predicate argumentPassing(DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Fu
f.getParameter(i) = parm and not parm.isRestParameter() and
arg = invk.getArgument(i)
)
or
exists (DataFlow::Node callback, int i |
invk.(DataFlow::AdditionalPartialInvokeNode).isPartialArgument(callback, arg, i) and
partiallyCalls(invk, callback, f) and
parm = f.getParameter(i) and not parm.isRestParameter())
}

View File

@@ -301,7 +301,7 @@ private class AnalyzedExportAssign extends AnalyzedPropertyWrite, DataFlow::Valu
}
override predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source) {
baseVal = TAbstractModuleObject(exportAssign.getContainer()) and
baseVal = TAbstractModuleObject(exportAssign.getTopLevel()) and
propName = "exports" and
source = this
}

View File

@@ -471,29 +471,14 @@ module Express {
propName = "originalUrl"
)
or
exists (string methodName |
// `req.get(...)` or `req.header(...)`
kind = "header" and
this.(DataFlow::MethodCallNode).calls(request, methodName) |
methodName = "get" or
methodName = "header"
)
or
exists (DataFlow::PropRead headers |
// `req.headers.name`
kind = "header" and
headers.accesses(request, "headers") and
this = headers.getAPropertyRead())
or
exists (string propName | propName = "host" or propName = "hostname" |
// `req.host` and `req.hostname` are derived from headers
kind = "header" and
this.(DataFlow::PropRead).accesses(request, propName))
or
// `req.cookies`
kind = "cookie" and
this.(DataFlow::PropRef).accesses(request, "cookies")
)
or
exists (RequestHeaderAccess access | this = access |
rh = access.getRouteHandler() and
kind = "header")
}
override RouteHandler getRouteHandler() {
@@ -505,6 +490,53 @@ module Express {
}
}
/**
* An access to a header on an Express request.
*/
private class RequestHeaderAccess extends HTTP::RequestHeaderAccess {
RouteHandler rh;
RequestHeaderAccess() {
exists (DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |
exists (string methodName |
// `req.get(...)` or `req.header(...)`
this.(DataFlow::MethodCallNode).calls(request, methodName) |
methodName = "get" or
methodName = "header"
)
or
exists (DataFlow::PropRead headers |
// `req.headers.name`
headers.accesses(request, "headers") and
this = headers.getAPropertyRead())
or
exists (string propName | propName = "host" or propName = "hostname" |
// `req.host` and `req.hostname` are derived from headers
this.(DataFlow::PropRead).accesses(request, propName))
)
}
override string getAHeaderName() {
exists (string name |
name = this.(DataFlow::PropRead).getPropertyName()
or
this.(DataFlow::CallNode).getArgument(0).mayHaveStringValue(name)
|
if name = "hostname" then
result = "host"
else
result = name.toLowerCase())
}
override RouteHandler getRouteHandler() {
result = rh
}
override string getKind() {
result = "header"
}
}
/**
* HTTP headers created by Express calls
*/
@@ -600,9 +632,9 @@ module Express {
astNode.getMethodName() = any(string n | n = "set" or n = "header") and
astNode.getNumArgument() = 1
}
/**
* Gets a reference to the multiple headers object that is to be set.
* Gets a reference to the multiple headers object that is to be set.
*/
private DataFlow::SourceNode getAHeaderSource() {
result.flowsToExpr(astNode.getArgument(0))
@@ -618,12 +650,12 @@ module Express {
override RouteHandler getRouteHandler() {
result = rh
}
override Expr getNameExpr() {
exists (DataFlow::PropWrite write |
exists (DataFlow::PropWrite write |
getAHeaderSource().flowsTo(write.getBase()) and
result = write.getPropertyNameExpr()
)
)
}
}
@@ -800,6 +832,10 @@ module Express {
asExpr().(MethodCallExpr).calls(any(ResponseExpr res), name))
}
override DataFlow::Node getDataNode() {
result = DataFlow::valueNode(astNode)
}
override DataFlow::Node getAPathArgument() {
result = DataFlow::valueNode(astNode.getArgument(0))
}

View File

@@ -72,9 +72,9 @@ module HTTP {
* Holds if the header with (lower-case) name `headerName` is set to the value of `headerValue`.
*/
abstract predicate definesExplicitly(string headerName, Expr headerValue);
/**
* Returns the expression used to compute the header name.
* Returns the expression used to compute the header name.
*/
abstract Expr getNameExpr();
}
@@ -132,6 +132,11 @@ module HTTP {
result = "http" or result = "https"
}
/**
* An expression whose value is sent as (part of) the body of an HTTP request (POST, PUT).
*/
abstract class RequestBody extends DataFlow::Node {}
/**
* An expression whose value is sent as (part of) the body of an HTTP response.
*/
@@ -354,9 +359,9 @@ module HTTP {
headerName = getNameExpr().getStringValue().toLowerCase() and
headerValue = astNode.getArgument(1)
}
override Expr getNameExpr() {
result = astNode.getArgument(0)
result = astNode.getArgument(0)
}
}
@@ -400,7 +405,20 @@ module HTTP {
*/
abstract string getKind();
}
/**
* An access to a header on an incoming HTTP request.
*/
abstract class RequestHeaderAccess extends RequestInputAccess {
/**
* Gets the lower-case name of an HTTP header from which this input is derived,
* if this can be determined.
*
* When the name of the header is unknown, this has no result.
*/
abstract string getAHeaderName();
}
/**
* A node that looks like a route setup on a server.
*

View File

@@ -121,13 +121,6 @@ module Hapi {
this.asExpr().(PropAccess).accesses(url, "path")
)
or
exists (PropAccess headers |
// `request.headers.<name>`
kind = "header" and
headers.accesses(request, "headers") and
this.asExpr().(PropAccess).accesses(headers, _)
)
or
exists (PropAccess state |
// `request.state.<name>`
kind = "cookie" and
@@ -135,6 +128,10 @@ module Hapi {
this.asExpr().(PropAccess).accesses(state, _)
)
)
or
exists (RequestHeaderAccess access | this = access |
rh = access.getRouteHandler() and
kind = "header")
}
override RouteHandler getRouteHandler() {
@@ -146,6 +143,35 @@ module Hapi {
}
}
/**
* An access to an HTTP header on a Hapi request.
*/
private class RequestHeaderAccess extends HTTP::RequestHeaderAccess {
RouteHandler rh;
RequestHeaderAccess() {
exists (Expr request | request = rh.getARequestExpr() |
exists (PropAccess headers |
// `request.headers.<name>`
headers.accesses(request, "headers") and
this.asExpr().(PropAccess).accesses(headers, _)
)
)
}
override string getAHeaderName() {
result = this.(DataFlow::PropRead).getPropertyName().toLowerCase()
}
override RouteHandler getRouteHandler() {
result = rh
}
override string getKind() {
result = "header"
}
}
/**
* An HTTP header defined in a Hapi server.
*/

View File

@@ -182,19 +182,6 @@ module Koa {
propName = "originalUrl" or
propName = "href"
)
or
exists (string propName, PropAccess headers |
// `ctx.request.header.<name>`, `ctx.request.headers.<name>`
kind = "header" and
headers.accesses(request, propName) and
this.asExpr().(PropAccess).accesses(headers, _) |
propName = "header" or
propName = "headers"
)
or
// `ctx.request.get(<name>)`
kind = "header" and
this.asExpr().(MethodCallExpr).calls(request, "get")
)
or
exists (PropAccess cookies |
@@ -203,6 +190,10 @@ module Koa {
cookies.accesses(rh.getAContextExpr(), "cookies") and
this.asExpr().(MethodCallExpr).calls(cookies, "get")
)
or
exists (RequestHeaderAccess access | access = this |
rh = access.getRouteHandler() and
kind = "header")
}
override RouteHandler getRouteHandler() {
@@ -214,6 +205,44 @@ module Koa {
}
}
/**
* An access to an HTTP header on a Koa request.
*/
private class RequestHeaderAccess extends HTTP::RequestHeaderAccess {
RouteHandler rh;
RequestHeaderAccess() {
exists (Expr request | request = rh.getARequestExpr() |
exists (string propName, PropAccess headers |
// `ctx.request.header.<name>`, `ctx.request.headers.<name>`
headers.accesses(request, propName) and
this.asExpr().(PropAccess).accesses(headers, _) |
propName = "header" or
propName = "headers"
)
or
// `ctx.request.get(<name>)`
this.asExpr().(MethodCallExpr).calls(request, "get")
)
}
override string getAHeaderName() {
result = this.(DataFlow::PropRead).getPropertyName().toLowerCase()
or
exists (string name |
this.(DataFlow::CallNode).getArgument(0).mayHaveStringValue(name) and
result = name.toLowerCase())
}
override RouteHandler getRouteHandler() {
result = rh
}
override string getKind() {
result = "header"
}
}
/**
* A call to a Koa method that sets up a route.
*/

View File

@@ -146,12 +146,16 @@ module NodeJSLib {
kind = "url" and
this.asExpr().(PropAccess).accesses(request, "url")
or
exists (PropAccess headers, string name |
// `req.headers.<name>`
if name = "cookie" then kind = "cookie" else kind= "header" |
exists (PropAccess headers |
// `req.headers.cookie`
kind = "cookie" and
headers.accesses(request, "headers") and
this.asExpr().(PropAccess).accesses(headers, name)
this.asExpr().(PropAccess).accesses(headers, "cookie")
)
or
exists (RequestHeaderAccess access | this = access |
request = access.getRequest() and
kind = "header")
}
override RouteHandler getRouteHandler() {
@@ -163,6 +167,38 @@ module NodeJSLib {
}
}
/**
* An access to an HTTP header (other than "Cookie") on an incoming Node.js request object.
*/
private class RequestHeaderAccess extends HTTP::RequestHeaderAccess {
RequestExpr request;
RequestHeaderAccess() {
exists (PropAccess headers, string name |
// `req.headers.<name>`
name != "cookie" and
headers.accesses(request, "headers") and
this.asExpr().(PropAccess).accesses(headers, name)
)
}
override string getAHeaderName() {
result = this.(DataFlow::PropRead).getPropertyName().toLowerCase()
}
override RouteHandler getRouteHandler() {
result = request.getRouteHandler()
}
override string getKind() {
result = "header"
}
RequestExpr getRequest() {
result = request
}
}
class RouteSetup extends CallExpr, HTTP::Servers::StandardRouteSetup {
ServerDefinition server;
Expr handler;
@@ -336,6 +372,23 @@ module NodeJSLib {
)
}
/**
* Holds if the `i`th parameter of method `methodName` of the Node.js
* `fs` module might represent a data parameter or buffer or a callback
* that receives the data.
*
* We determine this by looking for an externs declaration for
* `fs.methodName` where the `i`th parameter's name is `data` or
* `buffer` or a 'callback'.
*/
private predicate fsDataParam(string methodName, int i, string n) {
exists (ExternalMemberDecl decl, Function f, JSDocParamTag p |
decl.hasQualifiedName("fs", methodName) and f = decl.getInit() and
p.getDocumentedParameter() = f.getParameter(i).getAVariable() and
n = p.getName().toLowerCase() |
n = "data" or n = "buffer" or n = "callback"
)
}
/**
* A member `member` from module `fs` or its drop-in replacements `graceful-fs` or `fs-extra`.
*/
@@ -348,21 +401,161 @@ module NodeJSLib {
)
}
/**
* A call to a method from module `fs`, `graceful-fs` or `fs-extra`.
*/
private class NodeJSFileSystemAccess extends FileSystemAccess, DataFlow::CallNode {
private class NodeJSFileSystemAccessCall extends FileSystemAccess, DataFlow::CallNode {
string methodName;
NodeJSFileSystemAccess() {
NodeJSFileSystemAccessCall() {
this = fsModuleMember(methodName).getACall()
}
string getMethodName() {
result = methodName
}
override DataFlow::Node getDataNode() {
(
methodName = "readFileSync" and
result = this
)
or
exists (int i, string paramName | fsDataParam(methodName, i, paramName) |
(
paramName = "callback" and
exists (DataFlow::ParameterNode p, string n |
p = getCallback(i).getAParameter() and
n = p.getName().toLowerCase() and
result = p |
n = "data" or n = "buffer" or n = "string"
)
)
or
result = getArgument(i))
}
override DataFlow::Node getAPathArgument() {
exists (int i | fsFileParam(methodName, i) |
result = getArgument(i)
exists (int i | fsFileParam(methodName, i) |
result = getArgument(i))
}
}
/** Only NodeJSSystemFileAccessCalls that write data to 'fs' */
private class NodeJSFileSystemAccessWriteCall extends FileSystemWriteAccess, NodeJSFileSystemAccessCall {
NodeJSFileSystemAccessWriteCall () {
this.getMethodName() = "appendFile" or
this.getMethodName() = "appendFileSync" or
this.getMethodName() = "write" or
this.getMethodName() = "writeFile" or
this.getMethodName() = "writeFileSync" or
this.getMethodName() = "writeSync"
}
}
/** Only NodeJSSystemFileAccessCalls that read data from 'fs' */
private class NodeJSFileSystemAccessReadCall extends FileSystemReadAccess, NodeJSFileSystemAccessCall {
NodeJSFileSystemAccessReadCall () {
this.getMethodName() = "read" or
this.getMethodName() = "readSync" or
this.getMethodName() = "readFile" or
this.getMethodName() = "readFileSync"
}
}
/**
* A call to write corresponds to a pattern where file stream is open first with 'createWriteStream', followed by 'write' or 'end' call
*/
private class NodeJSFileSystemWrite extends FileSystemWriteAccess, DataFlow::CallNode {
NodeJSFileSystemAccessCall init;
NodeJSFileSystemWrite() {
exists (NodeJSFileSystemAccessCall n |
n.getCalleeName() = "createWriteStream" and init = n |
this = n.getAMemberCall("write") or
this = n.getAMemberCall("end")
)
}
override DataFlow::Node getDataNode() {
result = this.getArgument(0)
}
override DataFlow::Node getAPathArgument() {
result = init.getAPathArgument()
}
}
/**
* A call to read corresponds to a pattern where file stream is open first with createReadStream, followed by 'read' call
*/
private class NodeJSFileSystemRead extends FileSystemReadAccess, DataFlow::CallNode {
NodeJSFileSystemAccessCall init;
NodeJSFileSystemRead() {
exists (NodeJSFileSystemAccessCall n |
n.getCalleeName() = "createReadStream" and init = n |
this = n.getAMemberCall("read")
)
}
override DataFlow::Node getDataNode() {
result = this
}
override DataFlow::Node getAPathArgument() {
result = init.getAPathArgument()
}
}
/**
* A call to read corresponds to a pattern where file stream is open first with createReadStream, followed by 'pipe' call
*/
private class NodeJSFileSystemPipe extends FileSystemReadAccess, DataFlow::CallNode {
NodeJSFileSystemAccessCall init;
NodeJSFileSystemPipe() {
exists (NodeJSFileSystemAccessCall n |
n.getCalleeName() = "createReadStream" and init = n |
this = n.getAMemberCall("pipe")
)
}
override DataFlow::Node getDataNode() {
result = this.getArgument(0)
}
override DataFlow::Node getAPathArgument() {
result = init.getAPathArgument()
}
}
/**
* An 'on' event where data comes in as a parameter (usage: readstream.on('data', chunk))
*/
private class NodeJSFileSystemReadDataEvent extends FileSystemReadAccess, DataFlow::CallNode {
NodeJSFileSystemAccessCall init;
NodeJSFileSystemReadDataEvent() {
exists(NodeJSFileSystemAccessCall n |
n.getCalleeName() = "createReadStream" and init = n |
this = n.getAMethodCall("on") and
this.getArgument(0).mayHaveStringValue("data")
)
}
override DataFlow::Node getDataNode() {
result = this.getCallback(1).getParameter(0)
}
override DataFlow::Node getAPathArgument() {
result = init.getAPathArgument()
}
}
/**
@@ -380,7 +573,7 @@ module NodeJSLib {
}
}
/**
* A call to a method from module `child_process`.
*/
@@ -476,21 +669,21 @@ module NodeJSLib {
}
}
/**
* A call to a method from module `vm`
*/
class VmModuleMethodCall extends DataFlow::CallNode {
string methodName;
VmModuleMethodCall() {
this = DataFlow::moduleMember("vm", methodName).getACall()
}
/**
* Gets the code to be executed as part of this call.
*/
DataFlow::Node getACodeArgument() {
DataFlow::Node getACodeArgument() {
(
methodName = "runInContext" or
methodName = "runInNewContext" or
@@ -543,7 +736,7 @@ module NodeJSLib {
}
}
/**
* A model of a URL request in the Node.js `http` library.
*/
@@ -569,7 +762,7 @@ module NodeJSLib {
}
}
/**
* A data flow node that is the parameter of a result callback for an HTTP or HTTPS request made by a Node.js process, for example `res` in `https.request(url, (res) => {})`.
*/
@@ -579,12 +772,12 @@ module NodeJSLib {
this = req.(DataFlow::MethodCallNode).getCallback(1).getParameter(0)
)
}
override string getSourceType() {
result = "NodeJSClientRequest callback parameter"
}
}
/**
* A data flow node that is the parameter of a data callback for an HTTP or HTTPS request made by a Node.js process, for example `body` in `http.request(url, (res) => {res.on('data', (body) => {})})`.
*/
@@ -596,20 +789,31 @@ module NodeJSLib {
this = mcn.getCallback(1).getParameter(0)
)
}
override string getSourceType() {
result = "http.request data parameter"
}
}
/**
* An argument to client request.write () method, can be used to write body to a HTTP or HTTPS POST/PUT request,
* or request option (like headers, cookies, even url)
*/
class HttpRequestWriteArgument extends HTTP::RequestBody, DataFlow::Node {
HttpRequestWriteArgument () {
exists(CustomClientRequest req |
this = req.getAMethodCall("write").getArgument(0) or
this = req.getArgument(0))
}
}
/**
* A data flow node that is registered as a callback for an HTTP or HTTPS request made by a Node.js process, for example the function `handler` in `http.request(url).on(message, handler)`.
*/
class ClientRequestHandler extends DataFlow::FunctionNode {
string handledEvent;
NodeJSClientRequest clientRequest;
ClientRequestHandler() {
exists(DataFlow::MethodCallNode mcn |
clientRequest.getAMethodCall("on") = mcn and
@@ -617,14 +821,14 @@ module NodeJSLib {
flowsTo(mcn.getArgument(1))
)
}
/**
* Gets the name of an event this callback is registered for.
*/
string getAHandledEvent() {
result = handledEvent
}
/**
* Gets a request this callback is registered for.
*/
@@ -632,7 +836,7 @@ module NodeJSLib {
result = clientRequest
}
}
/**
* A data flow node that is the parameter of a response callback for an HTTP or HTTPS request made by a Node.js process, for example `res` in `http.request(url).on('response', (res) => {})`.
*/
@@ -643,12 +847,12 @@ module NodeJSLib {
handler.getAHandledEvent() = "response"
)
}
override string getSourceType() {
result = "NodeJSClientRequest response event"
}
}
/**
* A data flow node that is the parameter of a data callback for an HTTP or HTTPS request made by a Node.js process, for example `chunk` in `http.request(url).on('response', (res) => {res.on('data', (chunk) => {})})`.
*/
@@ -660,12 +864,12 @@ module NodeJSLib {
this = mcn.getCallback(1).getParameter(0)
)
}
override string getSourceType() {
result = "NodeJSClientRequest data event"
}
}
/**
* A data flow node that is a login callback for an HTTP or HTTPS request made by a Node.js process.
*/
@@ -674,7 +878,7 @@ module NodeJSLib {
getAHandledEvent() = "login"
}
}
/**
* A data flow node that is a parameter of a login callback for an HTTP or HTTPS request made by a Node.js process, for example `res` in `http.request(url).on('login', (res, callback) => {})`.
*/
@@ -684,12 +888,12 @@ module NodeJSLib {
this = handler.getParameter(0)
)
}
override string getSourceType() {
result = "NodeJSClientRequest login event"
}
}
/**
* A data flow node that is the login callback provided by an HTTP or HTTPS request made by a Node.js process, for example `callback` in `http.request(url).on('login', (res, callback) => {})`.
*/
@@ -700,7 +904,7 @@ module NodeJSLib {
)
}
}
/**
* A data flow node that is the username passed to the login callback provided by an HTTP or HTTPS request made by a Node.js process, for example `username` in `http.request(url).on('login', (res, cb) => {cb(username, password)})`.
*/
@@ -710,14 +914,14 @@ module NodeJSLib {
this = callback.getACall().getArgument(0).asExpr()
)
}
override string getCredentialsKind() {
result = "Node.js http(s) client login username"
}
}
/**
* A data flow node that is the password passed to the login callback provided by an HTTP or HTTPS request made by a Node.js process, for example `password` in `http.request(url).on('login', (res, cb) => {cb(username, password)})`.
* A data flow node that is the password passed to the login callback provided by an HTTP or HTTPS request made by a Node.js process, for example `password` in `http.request(url).on('login', (res, cb) => {cb(username, password)})`.
*/
private class ClientRequestLoginPassword extends CredentialsExpr {
ClientRequestLoginPassword() {
@@ -725,13 +929,13 @@ module NodeJSLib {
this = callback.getACall().getArgument(1).asExpr()
)
}
override string getCredentialsKind() {
result = "Node.js http(s) client login password"
}
}
/**
* A data flow node that is the parameter of an error callback for an HTTP or HTTPS request made by a Node.js process, for example `err` in `http.request(url).on('error', (err) => {})`.
*/
@@ -742,7 +946,7 @@ module NodeJSLib {
handler.getAHandledEvent() = "error"
)
}
override string getSourceType() {
result = "NodeJSClientRequest error event"
}

Some files were not shown because too many files have changed in this diff Show More