hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-05-02 04:05:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge branch 'master' into shelljs

Browse Source
This commit is contained in:
Asger F
2019-04-05 14:44:32 +01:00
committed by GitHub
parent d594e55a61 f7dda1b3a4
commit 80f413177a
45 changed files with 636 additions and 400 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
change-notes/1.21/analysis-cpp.md
View File

@@ -18,6 +18,7 @@
| Memory is never freed (`cpp/memory-never-freed`) | More correct results | Support added for more Microsoft-specific allocation functions, including `LocalAlloc`, `GlobalAlloc`, `HeapAlloc` and `CoTaskMemAlloc`. |
| Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | Resource allocation and deallocation functions are now determined more accurately. |
| Comparison result is always the same | Fewer false positive results | The range analysis library is now more conservative about floating point values being possibly `NaN` |
| Use of potentially dangerous function | More correct results | Calls to `localtime`, `ctime` and `asctime` are now detected by this query. |
| Wrong type of arguments to formatting function (`cpp/wrong-type-format-argument`) | More correct results and fewer false positive results | This query now more accurately identifies wide and non-wide string/character format arguments on different platforms. Platform detection has also been made more accurate for the purposes of this query. |
| Wrong type of arguments to formatting function (`cpp/wrong-type-format-argument`) | Fewer false positive results | Non-standard uses of %L are now understood. |

2
change-notes/1.21/analysis-csharp.md
View File

@@ -4,7 +4,7 @@
| **Query** | **Expected impact** | **Change** |
|------------------------------|------------------------|-----------------------------------|
| Class defines a field that uses an ICryptoTransform class in a way that would be unsafe for concurrent threads (`cs/thread-unsafe-icryptotransform-field-in-class`) | Fewer false positive results | The criteria for a result has changed to include nested properties, nested fields and collections. The format of the alert message has changed to highlight the static field. |
## Changes to code extraction

1
change-notes/1.21/analysis-java.md
View File

@@ -9,6 +9,7 @@
| **Query** | **Expected impact** | **Change** |
|----------------------------|------------------------|------------------------------------------------------------------|
| Implicit conversion from array to string (`java/print-array`) | Fewer false positive results | Results in slf4j logging calls are no longer reported as slf4j supports array printing. |
## Changes to QL libraries

2
change-notes/1.21/analysis-javascript.md
View File

@@ -5,7 +5,9 @@
* Support for the following frameworks and libraries has been improved:
- [koa](https://github.com/koajs/koa)
- [socket.io](http://socket.io)
- [Node.js](http://nodejs.org)
- [Firebase](https://firebase.google.com/)
- [Express](https://expressjs.com/)
- [shelljs](https://www.npmjs.com/package/shelljs)
* The security queries now track data flow through Base64 decoders such as the Node.js `Buffer` class, the DOM function `atob`, and a number of npm packages intcluding [`abab`](https://www.npmjs.com/package/abab), [`atob`](https://www.npmjs.com/package/atob), [`btoa`](https://www.npmjs.com/package/btoa), [`base-64`](https://www.npmjs.com/package/base-64), [`js-base64`](https://www.npmjs.com/package/js-base64), [`Base64.js`](https://www.npmjs.com/package/Base64) and [`base64-js`](https://www.npmjs.com/package/base64-js).

26
change-notes/1.21/analysis-python.md Normal file
View File

@@ -0,0 +1,26 @@
# Improvements to Python analysis
## General improvements
> Changes that affect alerts in many files or from many queries
> For example, changes to file classification
## New queries
| **Query** | **Tags** | **Purpose** |
|-----------|----------|-------------|
| Accepting unknown SSH host keys when using Paramiko (`py/paramiko-missing-host-key-validation`) | security, external/cwe/cwe-295 | Finds instances where Paramiko is configured to accept unknown host keys. Results are shown on LGTM by default. |
## Changes to existing queries
| **Query** | **Expected impact** | **Change** |
|-----------|---------------------|------------|
## Changes to code extraction
* *Series of bullet points*
## Changes to QL libraries
* *Series of bullet points*

4
cpp/ql/src/Best Practices/SloppyGlobal.ql
View File

@@ -2,8 +2,8 @@
* @name Short global name
* @description Global variables should have descriptive names, to help document their use, avoid namespace pollution and reduce the risk of shadowing with local variables.
* @kind problem
* @problem.severity warning
* @precision high
* @problem.severity recommendation
* @precision very-high
* @id cpp/short-global-name
* @tags maintainability
*/

103
cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScaling.ql
View File

@@ -9,111 +9,8 @@
* @tags security
* external/cwe/cwe-468
*/
import cpp
import semmle.code.cpp.controlflow.SSA
import IncorrectPointerScalingCommon
private predicate isPointerType(Type t) {
t instanceof PointerType or
t instanceof ArrayType
}
private Type baseType(Type t) {
(
exists (PointerType dt
| dt = t.getUnspecifiedType() and
result = dt.getBaseType().getUnspecifiedType()) or
exists (ArrayType at
| at = t.getUnspecifiedType() and
(not at.getBaseType().getUnspecifiedType() instanceof ArrayType) and
result = at.getBaseType().getUnspecifiedType()) or
exists (ArrayType at, ArrayType at2
| at = t.getUnspecifiedType() and
at2 = at.getBaseType().getUnspecifiedType() and
result = baseType(at2))
)
// Make sure that the type has a size and that it isn't ambiguous.
and strictcount(result.getSize()) = 1
}
/**
* Holds if there is a pointer expression with type `sourceType` at
* location `sourceLoc` which might be the source expression for `use`.
*
* For example, with
* ```
* int intArray[5] = { 1, 2, 3, 4, 5 };
* char *charPointer = (char *)intArray;
* return *(charPointer + i);
* ```
* the array initializer on the first line is a source expression
* for the use of `charPointer` on the third line.
*
* The source will either be an `Expr` or a `Parameter`.
*/
private
predicate exprSourceType(Expr use, Type sourceType, Location sourceLoc) {
// Reaching definitions.
if exists (SsaDefinition def, LocalScopeVariable v | use = def.getAUse(v)) then
exists (SsaDefinition def, LocalScopeVariable v
| use = def.getAUse(v)
| defSourceType(def, v, sourceType, sourceLoc))
// Pointer arithmetic
else if use instanceof PointerAddExpr then
exprSourceType(use.(PointerAddExpr).getLeftOperand(), sourceType, sourceLoc)
else if use instanceof PointerSubExpr then
exprSourceType(use.(PointerSubExpr).getLeftOperand(), sourceType, sourceLoc)
else if use instanceof AddExpr then
exprSourceType(use.(AddExpr).getAnOperand(), sourceType, sourceLoc)
else if use instanceof SubExpr then
exprSourceType(use.(SubExpr).getAnOperand(), sourceType, sourceLoc)
else if use instanceof CrementOperation then
exprSourceType(use.(CrementOperation).getOperand(), sourceType, sourceLoc)
// Conversions are not in the AST, so ignore them.
else if use instanceof Conversion then
none()
// Source expressions
else
(sourceType = use.getType().getUnspecifiedType() and
isPointerType(sourceType) and
sourceLoc = use.getLocation())
}
/**
* Holds if there is a pointer expression with type `sourceType` at
* location `sourceLoc` which might define the value of `v` at `def`.
*/
private
predicate defSourceType(SsaDefinition def, LocalScopeVariable v,
Type sourceType, Location sourceLoc) {
exprSourceType(def.getDefiningValue(v), sourceType, sourceLoc)
or
defSourceType(def.getAPhiInput(v), v, sourceType, sourceLoc)
or
exists (Parameter p
| p = v and
def.definedByParameter(p) and
sourceType = p.getType().getUnspecifiedType() and
strictcount(p.getType()) = 1 and
isPointerType(sourceType) and
sourceLoc = p.getLocation())
}
/**
* Gets the pointer arithmetic expression that `e` is (directly) used
* in, if any.
*
* For example, in `(char*)(p + 1)`, for `p`, ths result is `p + 1`.
*/
private Expr pointerArithmeticParent(Expr e) {
e = result.(PointerAddExpr).getLeftOperand() or
e = result.(PointerSubExpr).getLeftOperand() or
e = result.(PointerDiffExpr).getAnOperand()
}
from Expr dest, Type destType, Type sourceType, Type sourceBase,
Type destBase, Location sourceLoc
where exists(pointerArithmeticParent(dest))

95
cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingChar.ql
View File

@@ -9,103 +9,8 @@
* @tags security
* external/cwe/cwe-468
*/
import cpp
import semmle.code.cpp.controlflow.SSA
import IncorrectPointerScalingCommon
private predicate isPointerType(Type t) {
t instanceof PointerType or
t instanceof ArrayType
}
private Type baseType(Type t) {
exists (DerivedType dt
| dt = t.getUnspecifiedType() and
isPointerType(dt) and
result = dt.getBaseType().getUnspecifiedType())
// Make sure that the type has a size and that it isn't ambiguous.
and strictcount(result.getSize()) = 1
}
/**
* Holds if there is a pointer expression with type `sourceType` at
* location `sourceLoc` which might be the source expression for `use`.
*
* For example, with
* ```
* int intArray[5] = { 1, 2, 3, 4, 5 };
* char *charPointer = (char *)intArray;
* return *(charPointer + i);
* ```
* the array initializer on the first line is a source expression
* for the use of `charPointer` on the third line.
*
* The source will either be an `Expr` or a `Parameter`.
*/
private
predicate exprSourceType(Expr use, Type sourceType, Location sourceLoc) {
// Reaching definitions.
if exists (SsaDefinition def, LocalScopeVariable v | use = def.getAUse(v)) then
exists (SsaDefinition def, LocalScopeVariable v
| use = def.getAUse(v)
| defSourceType(def, v, sourceType, sourceLoc))
// Pointer arithmetic
else if use instanceof PointerAddExpr then
exprSourceType(use.(PointerAddExpr).getLeftOperand(), sourceType, sourceLoc)
else if use instanceof PointerSubExpr then
exprSourceType(use.(PointerSubExpr).getLeftOperand(), sourceType, sourceLoc)
else if use instanceof AddExpr then
exprSourceType(use.(AddExpr).getAnOperand(), sourceType, sourceLoc)
else if use instanceof SubExpr then
exprSourceType(use.(SubExpr).getAnOperand(), sourceType, sourceLoc)
else if use instanceof CrementOperation then
exprSourceType(use.(CrementOperation).getOperand(), sourceType, sourceLoc)
// Conversions are not in the AST, so ignore them.
else if use instanceof Conversion then
none()
// Source expressions
else
(sourceType = use.getType().getUnspecifiedType() and
isPointerType(sourceType) and
sourceLoc = use.getLocation())
}
/**
* Holds if there is a pointer expression with type `sourceType` at
* location `sourceLoc` which might define the value of `v` at `def`.
*/
private
predicate defSourceType(SsaDefinition def, LocalScopeVariable v,
Type sourceType, Location sourceLoc) {
exprSourceType(def.getDefiningValue(v), sourceType, sourceLoc)
or
defSourceType(def.getAPhiInput(v), v, sourceType, sourceLoc)
or
exists (Parameter p
| p = v and
def.definedByParameter(p) and
sourceType = p.getType().getUnspecifiedType() and
strictcount(p.getType()) = 1 and
isPointerType(sourceType) and
sourceLoc = p.getLocation())
}
/**
* Gets the pointer arithmetic expression that `e` is (directly) used
* in, if any.
*
* For example, in `(char*)(p + 1)`, for `p`, ths result is `p + 1`.
*/
private Expr pointerArithmeticParent(Expr e) {
e = result.(PointerAddExpr).getLeftOperand() or
e = result.(PointerSubExpr).getLeftOperand() or
e = result.(PointerDiffExpr).getAnOperand()
}
from Expr dest, Type destType, Type sourceType, Type sourceBase,
Type destBase, Location sourceLoc
where exists(pointerArithmeticParent(dest))

106
cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingCommon.qll
View File

@@ -46,3 +46,109 @@ predicate addWithSizeof(Expr e, Expr sizeofExpr, Type sizeofParam) {
| e = subExpr.getLeftOperand() and
multiplyWithSizeof(subExpr.getRightOperand(), sizeofExpr, sizeofParam))
}
/**
* Holds if `t` is a pointer or array type.
*/
predicate isPointerType(Type t) {
t instanceof PointerType or
t instanceof ArrayType
}
/**
* Gets the base type of a pointer or array type. In the case of an array of
* arrays, the inner base type is returned.
*/
Type baseType(Type t) {
(
exists (PointerType dt
| dt = t.getUnspecifiedType() and
result = dt.getBaseType().getUnspecifiedType()) or
exists (ArrayType at
| at = t.getUnspecifiedType() and
(not at.getBaseType().getUnspecifiedType() instanceof ArrayType) and
result = at.getBaseType().getUnspecifiedType()) or
exists (ArrayType at, ArrayType at2
| at = t.getUnspecifiedType() and
at2 = at.getBaseType().getUnspecifiedType() and
result = baseType(at2))
)
// Make sure that the type has a size and that it isn't ambiguous.
and strictcount(result.getSize()) = 1
}
/**
* Holds if there is a pointer expression with type `sourceType` at
* location `sourceLoc` which might be the source expression for `use`.
*
* For example, with
* ```
* int intArray[5] = { 1, 2, 3, 4, 5 };
* char *charPointer = (char *)intArray;
* return *(charPointer + i);
* ```
* the array initializer on the first line is a source expression
* for the use of `charPointer` on the third line.
*
* The source will either be an `Expr` or a `Parameter`.
*/
predicate exprSourceType(Expr use, Type sourceType, Location sourceLoc) {
// Reaching definitions.
if exists (SsaDefinition def, LocalScopeVariable v | use = def.getAUse(v)) then
exists (SsaDefinition def, LocalScopeVariable v
| use = def.getAUse(v)
| defSourceType(def, v, sourceType, sourceLoc))
// Pointer arithmetic
else if use instanceof PointerAddExpr then
exprSourceType(use.(PointerAddExpr).getLeftOperand(), sourceType, sourceLoc)
else if use instanceof PointerSubExpr then
exprSourceType(use.(PointerSubExpr).getLeftOperand(), sourceType, sourceLoc)
else if use instanceof AddExpr then
exprSourceType(use.(AddExpr).getAnOperand(), sourceType, sourceLoc)
else if use instanceof SubExpr then
exprSourceType(use.(SubExpr).getAnOperand(), sourceType, sourceLoc)
else if use instanceof CrementOperation then
exprSourceType(use.(CrementOperation).getOperand(), sourceType, sourceLoc)
// Conversions are not in the AST, so ignore them.
else if use instanceof Conversion then
none()
// Source expressions
else
(sourceType = use.getType().getUnspecifiedType() and
isPointerType(sourceType) and
sourceLoc = use.getLocation())
}
/**
* Holds if there is a pointer expression with type `sourceType` at
* location `sourceLoc` which might define the value of `v` at `def`.
*/
predicate defSourceType(SsaDefinition def, LocalScopeVariable v,
Type sourceType, Location sourceLoc) {
exprSourceType(def.getDefiningValue(v), sourceType, sourceLoc)
or
defSourceType(def.getAPhiInput(v), v, sourceType, sourceLoc)
or
exists (Parameter p
| p = v and
def.definedByParameter(p) and
sourceType = p.getType().getUnspecifiedType() and
strictcount(p.getType()) = 1 and
isPointerType(sourceType) and
sourceLoc = p.getLocation())
}
/**
* Gets the pointer arithmetic expression that `e` is (directly) used
* in, if any.
*
* For example, in `(char*)(p + 1)`, for `p`, ths result is `p + 1`.
*/
Expr pointerArithmeticParent(Expr e) {
e = result.(PointerAddExpr).getLeftOperand() or
e = result.(PointerSubExpr).getLeftOperand() or
e = result.(PointerDiffExpr).getAnOperand()
}

95
cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingVoid.ql
View File

@@ -9,103 +9,8 @@
* @tags security
* external/cwe/cwe-468
*/
import cpp
import semmle.code.cpp.controlflow.SSA
import IncorrectPointerScalingCommon
private predicate isPointerType(Type t) {
t instanceof PointerType or
t instanceof ArrayType
}
private Type baseType(Type t) {
exists (DerivedType dt
| dt = t.getUnspecifiedType() and
isPointerType(dt) and
result = dt.getBaseType().getUnspecifiedType())
// Make sure that the type has a size and that it isn't ambiguous.
and strictcount(result.getSize()) = 1
}
/**
* Holds if there is a pointer expression with type `sourceType` at
* location `sourceLoc` which might be the source expression for `use`.
*
* For example, with
* ```
* int intArray[5] = { 1, 2, 3, 4, 5 };
* char *charPointer = (char *)intArray;
* return *(charPointer + i);
* ```
* the array initializer on the first line is a source expression
* for the use of `charPointer` on the third line.
*
* The source will either be an `Expr` or a `Parameter`.
*/
private
predicate exprSourceType(Expr use, Type sourceType, Location sourceLoc) {
// Reaching definitions.
if exists (SsaDefinition def, LocalScopeVariable v | use = def.getAUse(v)) then
exists (SsaDefinition def, LocalScopeVariable v
| use = def.getAUse(v)
| defSourceType(def, v, sourceType, sourceLoc))
// Pointer arithmetic
else if use instanceof PointerAddExpr then
exprSourceType(use.(PointerAddExpr).getLeftOperand(), sourceType, sourceLoc)
else if use instanceof PointerSubExpr then
exprSourceType(use.(PointerSubExpr).getLeftOperand(), sourceType, sourceLoc)
else if use instanceof AddExpr then
exprSourceType(use.(AddExpr).getAnOperand(), sourceType, sourceLoc)
else if use instanceof SubExpr then
exprSourceType(use.(SubExpr).getAnOperand(), sourceType, sourceLoc)
else if use instanceof CrementOperation then
exprSourceType(use.(CrementOperation).getOperand(), sourceType, sourceLoc)
// Conversions are not in the AST, so ignore them.
else if use instanceof Conversion then
none()
// Source expressions
else
(sourceType = use.getType().getUnspecifiedType() and
isPointerType(sourceType) and
sourceLoc = use.getLocation())
}
/**
* Holds if there is a pointer expression with type `sourceType` at
* location `sourceLoc` which might define the value of `v` at `def`.
*/
private
predicate defSourceType(SsaDefinition def, LocalScopeVariable v,
Type sourceType, Location sourceLoc) {
exprSourceType(def.getDefiningValue(v), sourceType, sourceLoc)
or
defSourceType(def.getAPhiInput(v), v, sourceType, sourceLoc)
or
exists (Parameter p
| p = v and
def.definedByParameter(p) and
sourceType = p.getType().getUnspecifiedType() and
strictcount(p.getType()) = 1 and
isPointerType(sourceType) and
sourceLoc = p.getLocation())
}
/**
* Gets the pointer arithmetic expression that `e` is (directly) used
* in, if any.
*
* For example, in `(char*)(p + 1)`, for `p`, ths result is `p + 1`.
*/
private Expr pointerArithmeticParent(Expr e) {
e = result.(PointerAddExpr).getLeftOperand() or
e = result.(PointerSubExpr).getLeftOperand() or
e = result.(PointerDiffExpr).getAnOperand()
}
from Expr dest, Type destType, Type sourceType, Type sourceBase,
Type destBase, Location sourceLoc
where exists(pointerArithmeticParent(dest))

15
cpp/ql/src/Security/CWE/CWE-676/PotentiallyDangerousFunction.qhelp
View File

@@ -5,13 +5,15 @@
<overview>
<p>This rule finds calls to functions that are dangerous to
use. Currently, it checks for calls
to <code>gets</code> and <code>gmtime</code>. See <strong>Related rules</strong>
below for rules that identify other dangerous functions.</p>
to <code>gets</code>, <code>gmtime</code>, <code>localtime</code>,
<code>ctime</code> and <code>asctime</code>. See <strong>Related
rules</strong> below for rules that identify other dangerous functions.</p>
<p>The <code>gets</code> function is one of the vulnerabilities exploited by the Internet Worm of 1988, one of the first computer worms to spread through the Internet. The <code>gets</code> function provides no way to limit the amount of data that is read and stored, so without prior knowledge of the input it is impossible to use it safely with any size of buffer.</p>
<p>The <code>gmtime</code> function fills data into a <code>tm</code>
struct in shared memory and then returns a pointer to that struct. If
<p>The time related functions such as <code>gmtime</code>
fill data into a <code>tm</code> struct or <code>char</code> array in
shared memory and then returns a pointer to that memory. If
the function is called from multiple places in the same program, and
especially if it is called from multiple threads in the same program,
then the calls will overwrite each other's data.</p>
@@ -26,6 +28,11 @@ With <code>gmtime_r</code>, the application code manages allocation of
the <code>tm</code> struct. That way, separate calls to the function
can use their own storage.</p>
<p>Similarly replace calls to <code>localtime</code> with
<code>localtime_r</code>, calls to <code>ctime</code> with
<code>ctime_r</code> and calls to <code>asctime</code> with
<code>asctime_r</code>.</p>
</recommendation>
<example>
<p>The following example checks the local time in two ways:</p>

11
cpp/ql/src/Security/CWE/CWE-676/PotentiallyDangerousFunction.ql
View File

@@ -12,9 +12,14 @@
import cpp
predicate potentiallyDangerousFunction(Function f, string message) {
(
f.getQualifiedName() = "gmtime" and
message = "Call to gmtime is potentially dangerous"
exists(string name | name = f.getQualifiedName() |
(
name = "gmtime" or
name = "localtime" or
name = "ctime" or
name = "asctime"
) and
message = "Call to " + name + " is potentially dangerous"
) or (
f.getQualifiedName() = "gets" and
message = "gets does not guard against buffer overflow"

4
cpp/ql/src/jsf/4.13 Functions/AV Rule 107.ql
View File

@@ -2,8 +2,8 @@
* @name Function declared in block
* @description Functions should always be declared at file scope. It is confusing to declare a function at block scope, and the visibility of the function is not what would be expected.
* @kind problem
* @problem.severity warning
* @precision high
* @problem.severity recommendation
* @precision very-high
* @id cpp/function-in-block
* @tags maintainability
* readability

6
cpp/ql/src/semmle/code/cpp/File.qll
View File

@@ -309,6 +309,12 @@ class File extends Container, @file {
) or exists(File parent |
parent.compiledAsMicrosoft() and
parent.getAnIncludedFile() = this
) or (
getAbsolutePath().charAt(1) = ":"
// this is not ideal, it detects compilation on a Windows file system
// as a heuristic approximation to compilation with a Microsoft compiler.
) or (
getAbsolutePath().charAt(1) = "/"
)
}

2
cpp/ql/test/examples/BadLocking/AV Rule 107.expected Normal file
View File

@@ -0,0 +1,2 @@
| UnintendedDeclaration.cpp:51:2:51:22 | declaration | Functions should be declared at file scope, not inside blocks. |
| UnintendedDeclaration.cpp:72:2:72:27 | declaration | Functions should be declared at file scope, not inside blocks. |

1
cpp/ql/test/examples/BadLocking/AV Rule 107.qlref Normal file
View File

@@ -0,0 +1 @@
jsf/4.13 Functions/AV Rule 107.ql

7
cpp/ql/test/examples/BadLocking/DeclStmts.expected Normal file
View File

@@ -0,0 +1,7 @@
| UnintendedDeclaration.cpp:44:2:44:29 | declaration | myLock | Variable |
| UnintendedDeclaration.cpp:51:2:51:22 | declaration | myLock | Function |
| UnintendedDeclaration.cpp:58:2:58:20 | declaration | myLock | Variable |
| UnintendedDeclaration.cpp:65:2:65:22 | declaration | myMutex | Variable |
| UnintendedDeclaration.cpp:72:2:72:27 | declaration | myLock | Function |
| UnintendedDeclaration.cpp:82:3:82:34 | declaration | myLock | Variable |
| UnintendedDeclaration.cpp:89:3:89:27 | declaration | memberMutex | Variable |

18
cpp/ql/test/examples/BadLocking/DeclStmts.ql Normal file
View File

@@ -0,0 +1,18 @@
import cpp
string describe(Declaration d)
{
(
d instanceof Variable and
result = "Variable"
) or (
d instanceof Function and
result = "Function"
)
}
from DeclStmt ds, Declaration d
where
ds.getADeclaration() = d
select
ds, concat(d.getName(), ", "), concat(describe(d), ", ")

1
cpp/ql/test/examples/BadLocking/LocalVariableHidesGlobalVariable.expected Normal file
View File

@@ -0,0 +1 @@
| UnintendedDeclaration.cpp:65:14:65:20 | definition of myMutex | Local variable myMutex hides $@ with the same name. | UnintendedDeclaration.cpp:40:7:40:13 | myMutex | a global variable |

1
cpp/ql/test/examples/BadLocking/LocalVariableHidesGlobalVariable.qlref Normal file
View File

@@ -0,0 +1 @@
Best Practices/Hiding/LocalVariableHidesGlobalVariable.ql

96
cpp/ql/test/examples/BadLocking/UnintendedDeclaration.cpp Normal file
View File

@@ -0,0 +1,96 @@
class Mutex
{
public:
Mutex();
~Mutex();
void lock();
void unlock();
private:
// ...
};
template<class T>
class Lock
{
public:
Lock() : m(0)
{
}
Lock(T &_m) : m(&_m)
{
m->lock();
}
~Lock()
{
if (m)
{
m->unlock();
}
}
private:
T *m;
};
Mutex myMutex;
void test1()
{
Lock<Mutex> myLock(myMutex); // GOOD (creates `myLock` on `myMutex`)
// ...
}
void test2()
{
Lock<Mutex> myLock(); // BAD (interpreted as a function declaration, this does nothing)
// ...
}
void test3()
{
Lock<Mutex> myLock; // GOOD (creates an uninitialized variable called `myLock`, probably intended)
// ...
}
void test4()
{
Lock<Mutex>(myMutex); // BAD (creates an uninitialized variable called `myMutex`, probably not intended)
// ...
}
void test5()
{
Lock<Mutex> myLock(Mutex); // BAD (interpreted as a function declaration, this does nothing)
// ...
}
class MyTestClass
{
public:
void test6()
{
Lock<Mutex> myLock(memberMutex); // GOOD (creates `myLock` on `memberMutex`)
// ...
}
void test7()
{
Lock<Mutex>(memberMutex); // BAD (creates an uninitialized variable called `memberMutex`, probably not intended) [NOT DETECTED]
// ...
}
private:
Mutex memberMutex;
};

1
cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/IncorrectPointerScalingChar.expected
View File

@@ -1,3 +1,4 @@
| test.cpp:13:19:13:29 | charPointer | This pointer might have type $@ (size 4), but the pointer arithmetic here is done with type char * (size 1). | test.cpp:10:31:10:38 | test.cpp:10:31:10:38 | int |
| test.cpp:77:17:77:17 | x | This pointer might have type $@ (size 4), but the pointer arithmetic here is done with type char * (size 1). | test.cpp:72:19:72:19 | test.cpp:72:19:72:19 | int |
| test.cpp:119:26:119:26 | p | This pointer might have type $@ (size 8), but the pointer arithmetic here is done with type char * (size 1). | test.cpp:114:22:114:22 | test.cpp:114:22:114:22 | mystruct |
| test.cpp:147:19:147:29 | charPointer | This pointer might have type $@ (size 4), but the pointer arithmetic here is done with type char * (size 1). | test.cpp:145:31:145:38 | test.cpp:145:31:145:38 | int |

7
cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test.cpp
View File

@@ -139,3 +139,10 @@ void* test17(int* x) {
// BAD: void pointer arithmetic is not portable across compilers
return (void*)x + sizeof(int);
}
int test18(int i) {
int intArray[2][2] = { {1, 2}, {3, 4} };
char *charPointer = (char *)intArray;
// BAD: the pointer arithmetic uses type char*, so the offset is not scaled by sizeof(int).
return *(int *)(charPointer + i);
}

9
cpp/ql/test/query-tests/Security/CWE/CWE-676/semmle/PotentiallyDangerousFunction/PotentiallyDangerousFunction.expected
View File

@@ -1,3 +1,6 @@
| test.c:28:22:28:27 | call to gmtime | Call to gmtime is potentially dangerous |
| test.c:39:2:39:5 | call to gets | gets does not guard against buffer overflow |
| test.c:40:6:40:9 | call to gets | gets does not guard against buffer overflow |
| test.c:31:22:31:27 | call to gmtime | Call to gmtime is potentially dangerous |
| test.c:42:2:42:5 | call to gets | gets does not guard against buffer overflow |
| test.c:43:6:43:9 | call to gets | gets does not guard against buffer overflow |
| test.c:48:19:48:27 | call to localtime | Call to localtime is potentially dangerous |
| test.c:49:22:49:26 | call to ctime | Call to ctime is potentially dangerous |
| test.c:50:23:50:29 | call to asctime | Call to asctime is potentially dangerous |

10
cpp/ql/test/query-tests/Security/CWE/CWE-676/semmle/PotentiallyDangerousFunction/test.c
View File

@@ -21,6 +21,9 @@ struct tm {
struct tm *gmtime(const time_t *timer);
time_t time(time_t *timer);
struct tm *localtime(const time_t *timer);
char *ctime(const time_t *timer);
char *asctime(const struct tm *timeptr);
// Code under test
@@ -39,3 +42,10 @@ void testGets() {
gets(buf1); // BAD: use of gets
s = gets(buf2); // BAD: use of gets
}
void testTime()
{
struct tm *now = localtime(time(NULL)); // BAD: localtime uses shared state
char *time_string = ctime(time(NULL)); // BAD: localtime uses shared state
char *time_string2 = asctime(now); // BAD: localtime uses shared state
}

100
csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.ql
View File

@@ -1,5 +1,5 @@
/**
* @name Class defines a field that uses an ICryptoTransform class in a way that would be unsafe for concurrent threads.
* @name Class defines a field that uses an ICryptoTransform class in a way that would be unsafe for concurrent threads
* @description The class has a field that directly or indirectly make use of a static System.Security.Cryptography.ICryptoTransform object.
* Using this an instance of this class in concurrent threads is dangerous as it may not only result in an error,
* but under some circumstances may also result in incorrect results.
@@ -13,72 +13,44 @@
*/
import csharp
import semmle.code.csharp.frameworks.system.collections.Generic
class ICryptoTransform extends Class {
class UnsafeField extends Field {
UnsafeField() {
this.isStatic() and
not this.getAnAttribute().getType().getQualifiedName() = "System.ThreadStaticAttribute" and
this.getType() instanceof UsesICryptoTransform
}
}
ValueOrRefType getAnEnumeratedType(ValueOrRefType type) {
exists(ConstructedInterface interface |
interface = type.getABaseInterface*() and
interface.getUnboundGeneric() instanceof SystemCollectionsGenericIEnumerableTInterface
|
result = interface.getATypeArgument()
)
}
class UsesICryptoTransform extends ValueOrRefType {
UsesICryptoTransform() {
this instanceof ICryptoTransform
or
this.getAField().getType() instanceof UsesICryptoTransform
or
this.getAProperty().getType() instanceof UsesICryptoTransform
or
getAnEnumeratedType(this) instanceof UsesICryptoTransform
}
}
class ICryptoTransform extends ValueOrRefType {
ICryptoTransform() {
this.getABaseType*().hasQualifiedName("System.Security.Cryptography", "ICryptoTransform")
}
}
predicate usesICryptoTransformType(Type t) {
exists(ICryptoTransform ict |
ict = t or
usesICryptoTransformType(t.getAChild())
)
}
predicate hasICryptoTransformMember(Class c) {
exists(Field f |
f = c.getAMember() and
(
exists(ICryptoTransform ict | ict = f.getType()) or
hasICryptoTransformMember(f.getType()) or
usesICryptoTransformType(f.getType())
)
)
}
predicate hasICryptoTransformStaticMemberNested(Class c) {
exists(Field f | f = c.getAMember() |
hasICryptoTransformStaticMemberNested(f.getType())
or
f.isStatic() and
hasICryptoTransformMember(f.getType()) and
not exists(Attribute a | a = f.getAnAttribute() |
a.getType().getQualifiedName() = "System.ThreadStaticAttribute"
)
)
}
predicate hasICryptoTransformStaticMember(Class c, string msg) {
exists(Field f |
f = c.getAMember*() and
f.isStatic() and
not exists(Attribute a |
a = f.getAnAttribute() and
a.getType().getQualifiedName() = "System.ThreadStaticAttribute"
) and
(
exists(ICryptoTransform ict |
ict = f.getType() and
msg = "Static field " + f + " of type " + f.getType() +
", implements 'System.Security.Cryptography.ICryptoTransform', but it does not have an attribute [ThreadStatic]. The usage of this class is unsafe for concurrent threads."
)
or
not exists(ICryptoTransform ict | ict = f.getType()) and // Avoid dup messages
exists(Type t | t = f.getType() |
usesICryptoTransformType(t) and
msg = "Static field " + f + " of type " + f.getType() +
" makes usage of 'System.Security.Cryptography.ICryptoTransform', but it does not have an attribute [ThreadStatic]. The usage of this class is unsafe for concurrent threads."
)
)
)
or
hasICryptoTransformStaticMemberNested(c) and
msg = "Class" + c +
" implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads."
}
from Class c, string s
where hasICryptoTransformStaticMember(c, s)
select c, s
from UnsafeField field
select field,
"Static field '" + field.getName() +
"' contains a 'System.Security.Cryptography.ICryptoTransform' that could be used in an unsafe way."

9
csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.cs
View File

@@ -54,7 +54,7 @@ public static class StaticMemberChildUsage
SHA256,
}
private static readonly Dictionary<DigestAlgorithm, HashAlgorithm> HashMap = new Dictionary<DigestAlgorithm, HashAlgorithm>
private static readonly IDictionary<DigestAlgorithm, HashAlgorithm> HashMap = new Dictionary<DigestAlgorithm, HashAlgorithm>
{
{ DigestAlgorithm.SHA1, SHA1.Create() },
{ DigestAlgorithm.SHA256, SHA256.Create() },
@@ -112,3 +112,10 @@ public class TokenCacheNonStat
}
}
public class FuncTest
{
/// <summary>
/// Should be OK. Does not store the field.
/// </summary>
public static Func<SHA1> function;
}

10
csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.expected
View File

@@ -1,5 +1,5 @@
| ThreadUnsafeICryptoTransform.cs:39:14:39:19 | Nest03 | ClassNest03 implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads. |
| ThreadUnsafeICryptoTransform.cs:44:14:44:19 | Nest04 | ClassNest04 implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads. |
| ThreadUnsafeICryptoTransform.cs:49:21:49:42 | StaticMemberChildUsage | Static field HashMap of type Dictionary<DigestAlgorithm,HashAlgorithm> makes usage of 'System.Security.Cryptography.ICryptoTransform', but it does not have an attribute [ThreadStatic]. The usage of this class is unsafe for concurrent threads. |
| ThreadUnsafeICryptoTransform.cs:64:14:64:25 | StaticMember | Static field _sha1 of type SHA1, implements 'System.Security.Cryptography.ICryptoTransform', but it does not have an attribute [ThreadStatic]. The usage of this class is unsafe for concurrent threads. |
| ThreadUnsafeICryptoTransform.cs:69:14:69:28 | IndirectStatic2 | ClassIndirectStatic2 implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads. |
| ThreadUnsafeICryptoTransform.cs:41:36:41:37 | _n | Static field '_n' contains a 'System.Security.Cryptography.ICryptoTransform' that could be used in an unsafe way. |
| ThreadUnsafeICryptoTransform.cs:46:26:46:30 | _list | Static field '_list' contains a 'System.Security.Cryptography.ICryptoTransform' that could be used in an unsafe way. |
| ThreadUnsafeICryptoTransform.cs:57:73:57:79 | HashMap | Static field 'HashMap' contains a 'System.Security.Cryptography.ICryptoTransform' that could be used in an unsafe way. |
| ThreadUnsafeICryptoTransform.cs:66:25:66:29 | _sha1 | Static field '_sha1' contains a 'System.Security.Cryptography.ICryptoTransform' that could be used in an unsafe way. |
| ThreadUnsafeICryptoTransform.cs:71:19:71:20 | _n | Static field '_n' contains a 'System.Security.Cryptography.ICryptoTransform' that could be used in an unsafe way. |

6
java/ql/src/Violations of Best Practice/Undesirable Calls/PrintLnArray.ql
View File

@@ -27,7 +27,11 @@ predicate arraysToStringArgument(Expr e) {
from Expr arr
where
arr.getType() instanceof Array and
implicitToStringCall(arr)
implicitToStringCall(arr) and
not exists(FormattingCall fmtcall |
// exclude slf4j formatting as it supports array formatting
fmtcall.getAnArgumentToBeFormatted() = arr and fmtcall.getSyntax().isLogger()
)
or
arr.getType().(Array).getComponentType() instanceof Array and
arraysToStringArgument(arr)

3
java/ql/src/semmle/code/java/StringFormat.qll
View File

@@ -85,6 +85,9 @@ class FmtSyntax extends TFmtSyntax {
or
result = "logger ({}) syntax" and this = TFmtLogger()
}
/** Holds if this syntax is logger ({}) syntax. */
predicate isLogger() { this = TFmtLogger() }
}
/**

44
javascript/ql/src/semmle/javascript/frameworks/Express.qll
View File

@@ -14,10 +14,8 @@ module Express {
* Express application.
*/
DataFlow::SourceNode appCreation() {
exists(DataFlow::ModuleImportNode express | express.getPath() = "express" |
// `app = [new] express()`
result = express.getAnInvocation()
)
// `app = [new] express()`
result = DataFlow::moduleImport("express").getAnInvocation()
or
// `app = express.createServer()`
result = DataFlow::moduleMember("express", "createServer").getAnInvocation()
@@ -40,7 +38,13 @@ module Express {
private predicate isRouter(Expr e, RouterDefinition router) {
router.flowsTo(e)
or
isRouter(e.(RouteSetup).getReceiver(), router)
exists (DataFlow::MethodCallNode chain, DataFlow::Node base, string name |
name = "route" or
name = routeSetupMethodName() |
chain.calls(base, name) and
isRouter(base.asExpr(), router) and
chain.flowsToExpr(e)
)
}
/**
@@ -50,10 +54,7 @@ module Express {
RouterDefinition router;
RouteExpr() {
isRouter(this.getReceiver(), router) and
this.getMethodName() = "route"
or
this.(RouteSetup).getReceiver().(RouteExpr).getRouter() = router
isRouter(this, router)
}
/** Gets the router from which this route was created. */
@@ -61,20 +62,27 @@ module Express {
}
/**
* A call to an Express method that sets up a route.
* Gets the name of an Express router method that sets up a route.
*/
string routeSetupMethodName() {
result = "param" or
result = "all" or
result = "use" or
result = any(HTTP::RequestMethodName m).toLowerCase() or
// deprecated methods
result = "error" or
result = "del"
}
/**
* A call to an Express router method that sets up a route.
*/
class RouteSetup extends HTTP::Servers::StandardRouteSetup, MethodCallExpr {
RouterDefinition router;
RouteSetup() {
exists(string methodName | methodName = getMethodName() |
(isRouter(getReceiver(), router) or getReceiver().(RouteExpr).getRouter() = router) and
(
methodName = "all" or
methodName = "use" or
methodName = any(HTTP::RequestMethodName m).toLowerCase()
)
)
isRouter(getReceiver(), router) and
getMethodName() = routeSetupMethodName()
}
/** Gets the path associated with the route. */

4
javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
View File

@@ -180,7 +180,7 @@ module NodeJSLib {
RouteSetup() {
server.flowsTo(this) and
handler = getArgument(0)
handler = getLastArgument()
or
server.flowsTo(getReceiver()) and
this.(MethodCallExpr).getMethodName().regexpMatch("on(ce)?") and
@@ -663,7 +663,7 @@ module NodeJSLib {
RouteSetupCandidate() {
getMethodName() = "createServer" and
arg = getArgument(0)
arg = getLastArgument()
or
getMethodName().regexpMatch("on(ce)?") and
getArgument(0).mayHaveStringValue("request") and

12
javascript/ql/test/library-tests/frameworks/Express/src/routesetups.js Normal file
View File

@@ -0,0 +1,12 @@
var express = require('express');
express.Router()
.param('', h)
.get('', h);
var app = express.createServer();
app.error(h);
var router = express.Router();
var root = router.route('/');
root.post('', h);

76
javascript/ql/test/library-tests/frameworks/Express/tests.expected
View File

@@ -66,6 +66,10 @@ test_RouteSetup_getLastRouteHandlerExpr
| src/responseExprs.js:13:1:15:2 | app.get ... es4;\\n}) | src/responseExprs.js:13:23:15:1 | functio ... res4;\\n} |
| src/responseExprs.js:16:1:42:2 | app.pos ... }\\n}) | src/responseExprs.js:16:30:42:1 | functio ... }\\n} |
| src/route.js:4:1:5:39 | router. ... xt) {}) | src/route.js:5:12:5:38 | functio ... ext) {} |
| src/routesetups.js:3:1:4:14 | express ... ('', h) | src/routesetups.js:4:13:4:13 | h |
| src/routesetups.js:3:1:5:12 | express ... ('', h) | src/routesetups.js:5:11:5:11 | h |
| src/routesetups.js:8:1:8:12 | app.error(h) | src/routesetups.js:8:11:8:11 | h |
| src/routesetups.js:12:1:12:16 | root.post('', h) | src/routesetups.js:12:15:12:15 | h |
| src/subrouter.js:4:1:4:26 | app.use ... rotect) | src/subrouter.js:4:19:4:25 | protect |
| src/subrouter.js:5:1:5:29 | app.use ... uter()) | src/subrouter.js:5:14:5:28 | makeSubRouter() |
| src/subrouter.js:9:3:9:35 | router. ... ndler1) | src/subrouter.js:9:27:9:34 | handler1 |
@@ -219,6 +223,10 @@ test_RouteSetup_getRouter
| src/responseExprs.js:13:1:15:2 | app.get ... es4;\\n}) | src/responseExprs.js:2:11:2:19 | express() |
| src/responseExprs.js:16:1:42:2 | app.pos ... }\\n}) | src/responseExprs.js:2:11:2:19 | express() |
| src/route.js:4:1:5:39 | router. ... xt) {}) | src/route.js:2:14:2:29 | express.Router() |
| src/routesetups.js:3:1:4:14 | express ... ('', h) | src/routesetups.js:3:1:3:16 | express.Router() |
| src/routesetups.js:3:1:5:12 | express ... ('', h) | src/routesetups.js:3:1:3:16 | express.Router() |
| src/routesetups.js:8:1:8:12 | app.error(h) | src/routesetups.js:7:11:7:32 | express ... erver() |
| src/routesetups.js:12:1:12:16 | root.post('', h) | src/routesetups.js:10:14:10:29 | express.Router() |
| src/subrouter.js:4:1:4:26 | app.use ... rotect) | src/subrouter.js:2:11:2:19 | express() |
| src/subrouter.js:5:1:5:29 | app.use ... uter()) | src/subrouter.js:2:11:2:19 | express() |
| src/subrouter.js:9:3:9:35 | router. ... ndler1) | src/subrouter.js:8:16:8:31 | express.Router() |
@@ -483,6 +491,10 @@ test_RouteHandlerExpr
| src/responseExprs.js:13:23:15:1 | functio ... res4;\\n} | src/responseExprs.js:13:1:15:2 | app.get ... es4;\\n}) | true |
| src/responseExprs.js:16:30:42:1 | functio ... }\\n} | src/responseExprs.js:16:1:42:2 | app.pos ... }\\n}) | true |
| src/route.js:5:12:5:38 | functio ... ext) {} | src/route.js:4:1:5:39 | router. ... xt) {}) | true |
| src/routesetups.js:4:13:4:13 | h | src/routesetups.js:3:1:4:14 | express ... ('', h) | true |
| src/routesetups.js:5:11:5:11 | h | src/routesetups.js:3:1:5:12 | express ... ('', h) | true |
| src/routesetups.js:8:11:8:11 | h | src/routesetups.js:8:1:8:12 | app.error(h) | true |
| src/routesetups.js:12:15:12:15 | h | src/routesetups.js:12:1:12:16 | root.post('', h) | true |
| src/subrouter.js:4:19:4:25 | protect | src/subrouter.js:4:1:4:26 | app.use ... rotect) | false |
| src/subrouter.js:5:14:5:28 | makeSubRouter() | src/subrouter.js:5:1:5:29 | app.use ... uter()) | false |
| src/subrouter.js:9:27:9:34 | handler1 | src/subrouter.js:9:3:9:35 | router. ... ndler1) | true |
@@ -517,6 +529,7 @@ test_appCreation
| src/express4.js:2:11:2:19 | express() |
| src/express.js:2:11:2:19 | express() |
| src/responseExprs.js:2:11:2:19 | express() |
| src/routesetups.js:7:11:7:32 | express ... erver() |
| src/subrouter.js:2:11:2:19 | express() |
test_RouteSetup_getRequestMethod
| src/csurf-example.js:20:1:23:2 | app.get ... ) })\\n}) | GET |
@@ -538,11 +551,56 @@ test_RouteSetup_getRequestMethod
| src/responseExprs.js:10:1:12:2 | app.get ... es3;\\n}) | GET |
| src/responseExprs.js:13:1:15:2 | app.get ... es4;\\n}) | GET |
| src/responseExprs.js:16:1:42:2 | app.pos ... }\\n}) | POST |
| src/routesetups.js:3:1:5:12 | express ... ('', h) | GET |
| src/routesetups.js:12:1:12:16 | root.post('', h) | POST |
| src/subrouter.js:9:3:9:35 | router. ... ndler1) | POST |
| src/subrouter.js:10:3:10:41 | router. ... ndler2) | POST |
test_RouteExpr
| src/auth.js:4:1:4:53 | app.use ... d' }})) | src/auth.js:1:13:1:32 | require('express')() |
| src/csurf-example.js:13:1:13:20 | app.use('/api', api) | src/csurf-example.js:7:11:7:19 | express() |
| src/csurf-example.js:16:1:16:51 | app.use ... lse })) | src/csurf-example.js:7:11:7:19 | express() |
| src/csurf-example.js:17:1:17:23 | app.use ... rser()) | src/csurf-example.js:7:11:7:19 | express() |
| src/csurf-example.js:18:1:18:31 | app.use ... rue })) | src/csurf-example.js:7:11:7:19 | express() |
| src/csurf-example.js:20:1:23:2 | app.get ... ) })\\n}) | src/csurf-example.js:7:11:7:19 | express() |
| src/csurf-example.js:25:1:27:2 | app.pos ... re')\\n}) | src/csurf-example.js:7:11:7:19 | express() |
| src/csurf-example.js:32:3:34:4 | router. ... ')\\n }) | src/csurf-example.js:30:16:30:35 | new express.Router() |
| src/csurf-example.js:39:1:39:48 | app.get ... es) {}) | src/csurf-example.js:7:11:7:19 | express() |
| src/csurf-example.js:40:1:40:49 | app.pos ... es) {}) | src/csurf-example.js:7:11:7:19 | express() |
| src/express2.js:2:14:2:23 | e.Router() | src/express2.js:2:14:2:23 | e.Router() |
| src/express2.js:3:1:3:56 | router. ... res }) | src/express2.js:2:14:2:23 | e.Router() |
| src/express2.js:3:1:4:77 | router. ... sult }) | src/express2.js:2:14:2:23 | e.Router() |
| src/express2.js:6:1:6:15 | app.use(router) | src/express2.js:5:11:5:13 | e() |
| src/express3.js:4:1:7:2 | app.get ... l");\\n}) | src/express3.js:2:11:2:19 | express() |
| src/express3.js:12:1:12:21 | app.use ... dler()) | src/express3.js:2:11:2:19 | express() |
| src/express4.js:4:1:6:2 | app.get ... ery;\\n}) | src/express4.js:2:11:2:19 | express() |
| src/express.js:4:1:9:2 | app.get ... es);\\n}) | src/express.js:2:11:2:19 | express() |
| src/express.js:16:3:18:4 | router. ... );\\n }) | src/express.js:2:11:2:19 | express() |
| src/express.js:22:1:32:2 | app.pos ... r');\\n}) | src/express.js:2:11:2:19 | express() |
| src/express.js:34:1:34:53 | app.get ... andler) | src/express.js:2:11:2:19 | express() |
| src/express.js:39:1:39:21 | app.use ... dler()) | src/express.js:2:11:2:19 | express() |
| src/express.js:44:1:44:26 | app.use ... dler()) | src/express.js:2:11:2:19 | express() |
| src/express.js:46:1:51:2 | app.pos ... me];\\n}) | src/express.js:2:11:2:19 | express() |
| src/responseExprs.js:4:1:6:2 | app.get ... res1\\n}) | src/responseExprs.js:2:11:2:19 | express() |
| src/responseExprs.js:7:1:9:2 | app.get ... es2;\\n}) | src/responseExprs.js:2:11:2:19 | express() |
| src/responseExprs.js:10:1:12:2 | app.get ... es3;\\n}) | src/responseExprs.js:2:11:2:19 | express() |
| src/responseExprs.js:13:1:15:2 | app.get ... es4;\\n}) | src/responseExprs.js:2:11:2:19 | express() |
| src/responseExprs.js:16:1:42:2 | app.pos ... }\\n}) | src/responseExprs.js:2:11:2:19 | express() |
| src/route.js:2:14:2:29 | express.Router() | src/route.js:2:14:2:29 | express.Router() |
| src/route.js:4:1:4:31 | router. ... er_id') | src/route.js:2:14:2:29 | express.Router() |
| src/route.js:4:1:5:39 | router. ... xt) {}) | src/route.js:2:14:2:29 | express.Router() |
| src/routesetups.js:3:1:3:16 | express.Router() | src/routesetups.js:3:1:3:16 | express.Router() |
| src/routesetups.js:3:1:4:14 | express ... ('', h) | src/routesetups.js:3:1:3:16 | express.Router() |
| src/routesetups.js:3:1:5:12 | express ... ('', h) | src/routesetups.js:3:1:3:16 | express.Router() |
| src/routesetups.js:7:11:7:32 | express ... erver() | src/routesetups.js:7:11:7:32 | express ... erver() |
| src/routesetups.js:8:1:8:12 | app.error(h) | src/routesetups.js:7:11:7:32 | express ... erver() |
| src/routesetups.js:10:14:10:29 | express.Router() | src/routesetups.js:10:14:10:29 | express.Router() |
| src/routesetups.js:11:12:11:28 | router.route('/') | src/routesetups.js:10:14:10:29 | express.Router() |
| src/routesetups.js:12:1:12:16 | root.post('', h) | src/routesetups.js:10:14:10:29 | express.Router() |
| src/subrouter.js:4:1:4:26 | app.use ... rotect) | src/subrouter.js:2:11:2:19 | express() |
| src/subrouter.js:5:1:5:29 | app.use ... uter()) | src/subrouter.js:2:11:2:19 | express() |
| src/subrouter.js:8:16:8:31 | express.Router() | src/subrouter.js:8:16:8:31 | express.Router() |
| src/subrouter.js:9:3:9:35 | router. ... ndler1) | src/subrouter.js:8:16:8:31 | express.Router() |
| src/subrouter.js:10:3:10:41 | router. ... ndler2) | src/subrouter.js:8:16:8:31 | express.Router() |
test_RouteHandler_getAResponseExpr
| src/csurf-example.js:20:18:23:1 | functio ... () })\\n} | src/csurf-example.js:22:3:22:5 | res |
| src/csurf-example.js:25:22:27:1 | functio ... ere')\\n} | src/csurf-example.js:26:3:26:5 | res |
@@ -727,6 +785,10 @@ test_RouteSetup_getARouteHandler
| src/responseExprs.js:13:1:15:2 | app.get ... es4;\\n}) | src/responseExprs.js:13:23:15:1 | functio ... res4;\\n} |
| src/responseExprs.js:16:1:42:2 | app.pos ... }\\n}) | src/responseExprs.js:16:30:42:1 | functio ... }\\n} |
| src/route.js:4:1:5:39 | router. ... xt) {}) | src/route.js:5:12:5:38 | functio ... ext) {} |
| src/routesetups.js:3:1:4:14 | express ... ('', h) | src/routesetups.js:4:13:4:13 | h |
| src/routesetups.js:3:1:5:12 | express ... ('', h) | src/routesetups.js:5:11:5:11 | h |
| src/routesetups.js:8:1:8:12 | app.error(h) | src/routesetups.js:8:11:8:11 | h |
| src/routesetups.js:12:1:12:16 | root.post('', h) | src/routesetups.js:12:15:12:15 | h |
| src/subrouter.js:4:1:4:26 | app.use ... rotect) | src/subrouter.js:4:19:4:25 | protect |
| src/subrouter.js:5:1:5:29 | app.use ... uter()) | src/subrouter.js:5:14:5:28 | makeSubRouter() |
| src/subrouter.js:5:1:5:29 | app.use ... uter()) | src/subrouter.js:8:16:8:31 | express.Router() |
@@ -765,6 +827,9 @@ test_isRouterCreation
| src/express.js:2:11:2:19 | express() |
| src/responseExprs.js:2:11:2:19 | express() |
| src/route.js:2:14:2:29 | express.Router() |
| src/routesetups.js:3:1:3:16 | express.Router() |
| src/routesetups.js:7:11:7:32 | express ... erver() |
| src/routesetups.js:10:14:10:29 | express.Router() |
| src/subrouter.js:2:11:2:19 | express() |
| src/subrouter.js:8:16:8:31 | express.Router() |
test_RouteSetup_getRouteHandlerExpr
@@ -797,6 +862,10 @@ test_RouteSetup_getRouteHandlerExpr
| src/responseExprs.js:13:1:15:2 | app.get ... es4;\\n}) | 0 | src/responseExprs.js:13:23:15:1 | functio ... res4;\\n} |
| src/responseExprs.js:16:1:42:2 | app.pos ... }\\n}) | 0 | src/responseExprs.js:16:30:42:1 | functio ... }\\n} |
| src/route.js:4:1:5:39 | router. ... xt) {}) | 0 | src/route.js:5:12:5:38 | functio ... ext) {} |
| src/routesetups.js:3:1:4:14 | express ... ('', h) | 0 | src/routesetups.js:4:13:4:13 | h |
| src/routesetups.js:3:1:5:12 | express ... ('', h) | 0 | src/routesetups.js:5:11:5:11 | h |
| src/routesetups.js:8:1:8:12 | app.error(h) | 0 | src/routesetups.js:8:11:8:11 | h |
| src/routesetups.js:12:1:12:16 | root.post('', h) | 0 | src/routesetups.js:12:15:12:15 | h |
| src/subrouter.js:4:1:4:26 | app.use ... rotect) | 0 | src/subrouter.js:4:19:4:25 | protect |
| src/subrouter.js:5:1:5:29 | app.use ... uter()) | 0 | src/subrouter.js:5:14:5:28 | makeSubRouter() |
| src/subrouter.js:9:3:9:35 | router. ... ndler1) | 0 | src/subrouter.js:9:27:9:34 | handler1 |
@@ -813,6 +882,9 @@ test_RouterDefinition_RouterDefinition
| src/express.js:2:11:2:19 | express() |
| src/responseExprs.js:2:11:2:19 | express() |
| src/route.js:2:14:2:29 | express.Router() |
| src/routesetups.js:3:1:3:16 | express.Router() |
| src/routesetups.js:7:11:7:32 | express ... erver() |
| src/routesetups.js:10:14:10:29 | express.Router() |
| src/subrouter.js:2:11:2:19 | express() |
| src/subrouter.js:8:16:8:31 | express.Router() |
test_RouteHandler_getARequestBodyAccess
@@ -878,6 +950,10 @@ test_RouteSetup_getARouteHandlerExpr
| src/responseExprs.js:13:1:15:2 | app.get ... es4;\\n}) | src/responseExprs.js:13:23:15:1 | functio ... res4;\\n} |
| src/responseExprs.js:16:1:42:2 | app.pos ... }\\n}) | src/responseExprs.js:16:30:42:1 | functio ... }\\n} |
| src/route.js:4:1:5:39 | router. ... xt) {}) | src/route.js:5:12:5:38 | functio ... ext) {} |
| src/routesetups.js:3:1:4:14 | express ... ('', h) | src/routesetups.js:4:13:4:13 | h |
| src/routesetups.js:3:1:5:12 | express ... ('', h) | src/routesetups.js:5:11:5:11 | h |
| src/routesetups.js:8:1:8:12 | app.error(h) | src/routesetups.js:8:11:8:11 | h |
| src/routesetups.js:12:1:12:16 | root.post('', h) | src/routesetups.js:12:15:12:15 | h |
| src/subrouter.js:4:1:4:26 | app.use ... rotect) | src/subrouter.js:4:19:4:25 | protect |
| src/subrouter.js:5:1:5:29 | app.use ... uter()) | src/subrouter.js:5:14:5:28 | makeSubRouter() |
| src/subrouter.js:9:3:9:35 | router. ... ndler1) | src/subrouter.js:9:27:9:34 | handler1 |

3
javascript/ql/test/library-tests/frameworks/NodeJSLib/createServer.js Normal file
View File

@@ -0,0 +1,3 @@
var https = require('https');
https.createServer(function (req, res) {});
https.createServer(o, function (req, res) {});

12
javascript/ql/test/library-tests/frameworks/NodeJSLib/tests.expected
View File

@@ -1,4 +1,6 @@
test_isCreateServer
| createServer.js:2:1:2:42 | https.c ... es) {}) |
| createServer.js:3:1:3:45 | https.c ... es) {}) |
| src/http.js:4:14:10:2 | http.cr ... foo;\\n}) |
| src/http.js:12:1:16:2 | http.cr ... r");\\n}) |
| src/http.js:57:1:57:31 | http.cr ... dler()) |
@@ -47,6 +49,8 @@ test_HeaderDefinition
| src/https.js:7:3:7:42 | res.wri ... rget }) | src/https.js:4:33:10:1 | functio ... .foo;\\n} |
| src/https.js:13:3:13:44 | res.set ... /html') | src/https.js:12:20:16:1 | functio ... ar");\\n} |
test_RouteSetup_getServer
| createServer.js:2:1:2:42 | https.c ... es) {}) | createServer.js:2:1:2:42 | https.c ... es) {}) |
| createServer.js:3:1:3:45 | https.c ... es) {}) | createServer.js:3:1:3:45 | https.c ... es) {}) |
| src/http.js:4:14:10:2 | http.cr ... foo;\\n}) | src/http.js:4:14:10:2 | http.cr ... foo;\\n}) |
| src/http.js:12:1:16:2 | http.cr ... r");\\n}) | src/http.js:12:1:16:2 | http.cr ... r");\\n}) |
| src/http.js:57:1:57:31 | http.cr ... dler()) | src/http.js:57:1:57:31 | http.cr ... dler()) |
@@ -66,6 +70,8 @@ test_HeaderDefinition_getAHeaderName
| src/https.js:7:3:7:42 | res.wri ... rget }) | location |
| src/https.js:13:3:13:44 | res.set ... /html') | content-type |
test_ServerDefinition
| createServer.js:2:1:2:42 | https.c ... es) {}) |
| createServer.js:3:1:3:45 | https.c ... es) {}) |
| src/http.js:4:14:10:2 | http.cr ... foo;\\n}) |
| src/http.js:12:1:16:2 | http.cr ... r");\\n}) |
| src/http.js:57:1:57:31 | http.cr ... dler()) |
@@ -95,6 +101,8 @@ test_RouteHandler_getAResponseExpr
| src/https.js:12:20:16:1 | functio ... ar");\\n} | src/https.js:14:3:14:5 | res |
| src/https.js:12:20:16:1 | functio ... ar");\\n} | src/https.js:15:3:15:5 | res |
test_ServerDefinition_getARouteHandler
| createServer.js:2:1:2:42 | https.c ... es) {}) | createServer.js:2:20:2:41 | functio ... res) {} |
| createServer.js:3:1:3:45 | https.c ... es) {}) | createServer.js:3:23:3:44 | functio ... res) {} |
| src/http.js:4:14:10:2 | http.cr ... foo;\\n}) | src/http.js:4:32:10:1 | functio ... .foo;\\n} |
| src/http.js:12:1:16:2 | http.cr ... r");\\n}) | src/http.js:12:19:16:1 | functio ... ar");\\n} |
| src/http.js:57:1:57:31 | http.cr ... dler()) | src/http.js:55:12:55:30 | function(req,res){} |
@@ -110,6 +118,8 @@ test_ResponseSendArgument
| src/https.js:14:13:14:17 | "foo" | src/https.js:12:20:16:1 | functio ... ar");\\n} |
| src/https.js:15:11:15:15 | "bar" | src/https.js:12:20:16:1 | functio ... ar");\\n} |
test_RouteSetup_getARouteHandler
| createServer.js:2:1:2:42 | https.c ... es) {}) | createServer.js:2:20:2:41 | functio ... res) {} |
| createServer.js:3:1:3:45 | https.c ... es) {}) | createServer.js:3:23:3:44 | functio ... res) {} |
| src/http.js:4:14:10:2 | http.cr ... foo;\\n}) | src/http.js:4:32:10:1 | functio ... .foo;\\n} |
| src/http.js:12:1:16:2 | http.cr ... r");\\n}) | src/http.js:12:19:16:1 | functio ... ar");\\n} |
| src/http.js:57:1:57:31 | http.cr ... dler()) | src/http.js:55:12:55:30 | function(req,res){} |
@@ -137,6 +147,8 @@ test_RemoteFlowSources
| src/https.js:8:3:8:20 | req.headers.cookie |
| src/https.js:9:3:9:17 | req.headers.foo |
test_RouteHandler
| createServer.js:2:20:2:41 | functio ... res) {} | createServer.js:2:1:2:42 | https.c ... es) {}) |
| createServer.js:3:23:3:44 | functio ... res) {} | createServer.js:3:1:3:45 | https.c ... es) {}) |
| src/http.js:4:32:10:1 | functio ... .foo;\\n} | src/http.js:4:14:10:2 | http.cr ... foo;\\n}) |
| src/http.js:12:19:16:1 | functio ... ar");\\n} | src/http.js:12:1:16:2 | http.cr ... r");\\n}) |
| src/http.js:55:12:55:30 | function(req,res){} | src/http.js:57:1:57:31 | http.cr ... dler()) |

2
python/ql/src/Security/CWE-094/CodeInjection.ql
View File

@@ -10,7 +10,7 @@
* @tags security
* external/owasp/owasp-a1
* external/cwe/cwe-094
* external/cwe/cwe-079
* external/cwe/cwe-095
* external/cwe/cwe-116
*/

43
python/ql/src/Security/CWE-295/MissingHostKeyValidation.qhelp Normal file
View File

@@ -0,0 +1,43 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
In the Secure Shell (SSH) protocol, host keys are used to verify the identity of
remote hosts. Accepting unknown host keys may leave the connection open to
man-in-the-middle attacks.
</p>
</overview>
<recommendation>
<p>
Do not accept unknown host keys. In particular, do not set the default missing
host key policy for the Paramiko library to either <code>AutoAddPolicy</code> or
<code>WarningPolicy</code>. Both of these policies continue even when the host
key is unknown. The default setting of <code>RejectPolicy</code> is secure
because it throws an exception when it encounters an unknown host key.
</p>
</recommendation>
<example>
<p>
The following example shows two ways of opening an SSH connection to
<code>example.com</code>. The first function sets the missing host key policy to
<code>AutoAddPolicy</code>. If the host key verification fails, the client will
continue to interact with the server, even though the connection may be
compromised. The second function sets the host key policy to
<code>RejectPolicy</code>, and will throw an exception if the host key
verification fails.
</p>
<sample src="examples/paramiko_host_key.py" />
</example>
<references>
<li>
Paramiko documentation: <a href="http://docs.paramiko.org/en/2.4/api/client.html?highlight=set_missing_host_key_policy#paramiko.client.SSHClient.set_missing_host_key_policy">set_missing_host_key_policy</a>.
</li>
</references>
</qhelp>

36
python/ql/src/Security/CWE-295/MissingHostKeyValidation.ql Normal file
View File

@@ -0,0 +1,36 @@
/**
* @name Accepting unknown SSH host keys when using Paramiko
* @description Accepting unknown host keys can allow man-in-the-middle attacks.
* @kind problem
* @problem.severity error
* @precision high
* @id py/paramiko-missing-host-key-validation
* @tags security
* external/cwe/cwe-295
*/
import python
private ModuleObject theParamikoClientModule() { result = ModuleObject::named("paramiko.client") }
private ClassObject theParamikoSSHClientClass() {
result = theParamikoClientModule().attr("SSHClient")
}
private ClassObject unsafe_paramiko_policy(string name) {
(name = "AutoAddPolicy" or name = "WarningPolicy") and
result = theParamikoClientModule().attr(name)
}
from CallNode call, ControlFlowNode arg, string name
where
call = theParamikoSSHClientClass()
.lookupAttribute("set_missing_host_key_policy")
.(FunctionObject)
.getACall() and
arg = call.getAnArg() and
(
arg.refersTo(unsafe_paramiko_policy(name)) or
arg.refersTo(_, unsafe_paramiko_policy(name), _)
)
select call, "Setting missing host key policy to " + name + " may be unsafe."

19
python/ql/src/Security/CWE-295/examples/paramiko_host_key.py Normal file
View File

@@ -0,0 +1,19 @@
from paramiko.client import SSHClient, AutoAddPolicy, RejectPolicy
def unsafe_connect():
client = SSHClient()
client.set_missing_host_key_policy(AutoAddPolicy)
client.connect("example.com")
# ... interaction with server
client.close()
def safe_connect():
client = SSHClient()
client.set_missing_host_key_policy(RejectPolicy)
client.connect("example.com")
# ... interaction with server
client.close()

4
python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.expected Normal file
View File

@@ -0,0 +1,4 @@
| paramiko_host_key.py:5:1:5:49 | ControlFlowNode for Attribute() | Setting missing host key policy to AutoAddPolicy may be unsafe. |
| paramiko_host_key.py:7:1:7:49 | ControlFlowNode for Attribute() | Setting missing host key policy to WarningPolicy may be unsafe. |
| paramiko_host_key.py:11:1:11:51 | ControlFlowNode for Attribute() | Setting missing host key policy to AutoAddPolicy may be unsafe. |
| paramiko_host_key.py:13:1:13:51 | ControlFlowNode for Attribute() | Setting missing host key policy to WarningPolicy may be unsafe. |

1
python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.qlref Normal file
View File

@@ -0,0 +1 @@
Security/CWE-295/MissingHostKeyValidation.ql

13
python/ql/test/query-tests/Security/CWE-295/paramiko_host_key.py Normal file
View File

@@ -0,0 +1,13 @@
from paramiko.client import AutoAddPolicy, WarningPolicy, RejectPolicy, SSHClient
client = SSHClient()
client.set_missing_host_key_policy(AutoAddPolicy) # bad
client.set_missing_host_key_policy(RejectPolicy) # good
client.set_missing_host_key_policy(WarningPolicy) # bad
# Using instances
client.set_missing_host_key_policy(AutoAddPolicy()) # bad
client.set_missing_host_key_policy(RejectPolicy()) # good
client.set_missing_host_key_policy(WarningPolicy()) # bad

0
python/ql/test/query-tests/Security/lib/paramiko/__init__.py Normal file
View File

15
python/ql/test/query-tests/Security/lib/paramiko/client.py Normal file
View File

@@ -0,0 +1,15 @@
class SSHClient(object):
def __init__(self, *args, **kwargs):
pass
def set_missing_host_key_policy(self, *args, **kwargs):
pass
class AutoAddPolicy(object):
pass
class WarningPolicy(object):
pass
class RejectPolicy(object):
pass