diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll index e729c3cb0a4..7e55868b847 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll @@ -50,19 +50,18 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid } override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { - this.getName() = ["strncat", "wcsncat", "_mbsncat", "_mbsncat_l"] and - input.isParameter(2) and - output.isParameterDeref(0) - or - this.getName() = ["_mbsncat_l", "_mbsnbcat_l"] and - input.isParameter(3) and - output.isParameterDeref(0) - or - input.isParameterDeref(0) and - output.isParameterDeref(0) - or - input.isParameterDeref(1) and - output.isParameterDeref(0) + ( + this.getName() = ["strncat", "wcsncat", "_mbsncat", "_mbsncat_l"] and + input.isParameter(2) + or + this.getName() = ["_mbsncat_l", "_mbsnbcat_l"] and + input.isParameter(3) + or + input.isParameterDeref(0) + or + input.isParameterDeref(1) + ) and + (output.isParameterDeref(0) or output.isReturnValueDeref()) } override predicate hasArrayInput(int param) { diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/true_upon_entry.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/true_upon_entry.cpp index d08e8b1ded3..923a7c0513d 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/true_upon_entry.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/true_upon_entry.cpp @@ -5,7 +5,7 @@ int source(); void sink(...); bool random(); -int test1() { +void test1() { int x = source(); for (int i = 0; i < 10; i++) { x = 0; @@ -13,7 +13,7 @@ int test1() { sink(x); // $ SPURIOUS: ir } -int test2(int iterations) { +void test2(int iterations) { int x = source(); for (int i = 0; i < iterations; i++) { x = 0; @@ -21,7 +21,7 @@ int test2(int iterations) { sink(x); // $ ast,ir } -int test3() { +void test3() { int x = 0; for (int i = 0; i < 10; i++) { x = source(); @@ -29,7 +29,7 @@ int test3() { sink(x); // $ ast,ir } -int test4() { +void test4() { int x = source(); for (int i = 0; i < 10; i++) { if (random()) @@ -39,7 +39,7 @@ int test4() { sink(x); // $ ast,ir } -int test5() { +void test5() { int x = source(); for (int i = 0; i < 10; i++) { if (random()) @@ -49,7 +49,7 @@ int test5() { sink(x); // $ ast,ir } -int test6() { +void test6() { int y; int x = source(); for (int i = 0; i < 10 && (y = 1); i++) { @@ -57,7 +57,7 @@ int test6() { sink(x); // $ ast,ir } -int test7() { +void test7() { int y; int x = source(); for (int i = 0; i < 10 && (y = 1); i++) { @@ -66,7 +66,7 @@ int test7() { sink(x); // $ SPURIOUS: ir } -int test8() { +void test8() { int x = source(); // It appears to the analysis that the condition can exit after `i < 10` // without having assigned to `x`. That is an effect of how the @@ -78,7 +78,7 @@ int test8() { sink(x); // $ SPURIOUS: ast,ir } -int test9() { +void test9() { int y; int x = source(); for (int i = 0; (y = 1) && i < 10; i++) { @@ -86,21 +86,21 @@ int test9() { sink(x); // $ ast,ir } -int test10() { +void test10() { int x = source(); for (int i = 0; (x = 1) && i < 10; i++) { } sink(x); // no flow } -int test10(int b, int d) { +void test10(int b, int d) { int i = 0; int x = source(); if (b) goto L; for (; i < 10; i += d) { x = 0; - L: + L: ; } sink(x); // $ ir MISSING: ast } diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index 07bf9a85cf4..20f18d62201 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -5964,6 +5964,7 @@ | taint.cpp:172:10:172:15 | buffer | taint.cpp:172:3:172:8 | call to strcat | | | taint.cpp:172:10:172:15 | buffer | taint.cpp:172:10:172:15 | ref arg buffer | TAINT | | taint.cpp:172:10:172:15 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | +| taint.cpp:172:18:172:24 | tainted | taint.cpp:172:3:172:8 | call to strcat | TAINT | | taint.cpp:172:18:172:24 | tainted | taint.cpp:172:10:172:15 | ref arg buffer | TAINT | | taint.cpp:180:19:180:19 | p | taint.cpp:180:19:180:19 | p | | | taint.cpp:180:19:180:19 | p | taint.cpp:181:9:181:9 | p | | @@ -6373,12 +6374,14 @@ | taint.cpp:561:9:561:13 | dest1 | taint.cpp:561:9:561:13 | ref arg dest1 | TAINT | | taint.cpp:561:9:561:13 | ref arg dest1 | taint.cpp:560:24:560:28 | dest1 | | | taint.cpp:561:9:561:13 | ref arg dest1 | taint.cpp:562:7:562:11 | dest1 | | +| taint.cpp:561:16:561:21 | source | taint.cpp:561:2:561:7 | call to strcat | TAINT | | taint.cpp:561:16:561:21 | source | taint.cpp:561:9:561:13 | ref arg dest1 | TAINT | | taint.cpp:562:7:562:11 | ref arg dest1 | taint.cpp:560:24:560:28 | dest1 | | | taint.cpp:564:9:564:13 | dest2 | taint.cpp:564:2:564:7 | call to strcat | | | taint.cpp:564:9:564:13 | dest2 | taint.cpp:564:9:564:13 | ref arg dest2 | TAINT | | taint.cpp:564:9:564:13 | ref arg dest2 | taint.cpp:560:37:560:41 | dest2 | | | taint.cpp:564:9:564:13 | ref arg dest2 | taint.cpp:565:7:565:11 | dest2 | | +| taint.cpp:564:16:564:20 | clean | taint.cpp:564:2:564:7 | call to strcat | TAINT | | taint.cpp:564:16:564:20 | clean | taint.cpp:564:9:564:13 | ref arg dest2 | TAINT | | taint.cpp:565:7:565:11 | ref arg dest2 | taint.cpp:560:37:560:41 | dest2 | | | taint.cpp:572:37:572:41 | dest1 | taint.cpp:572:37:572:41 | dest1 | | @@ -6405,9 +6408,12 @@ | taint.cpp:574:36:574:40 | ref arg dest1 | taint.cpp:572:37:572:41 | dest1 | | | taint.cpp:574:36:574:40 | ref arg dest1 | taint.cpp:575:7:575:11 | dest1 | | | taint.cpp:574:36:574:40 | ref arg dest1 | taint.cpp:576:8:576:12 | dest1 | | +| taint.cpp:574:43:574:45 | ptr | taint.cpp:574:25:574:34 | call to _mbsncat_l | TAINT | | taint.cpp:574:43:574:45 | ptr | taint.cpp:574:36:574:40 | ref arg dest1 | TAINT | +| taint.cpp:574:48:574:48 | n | taint.cpp:574:25:574:34 | call to _mbsncat_l | TAINT | | taint.cpp:574:48:574:48 | n | taint.cpp:574:36:574:40 | ref arg dest1 | TAINT | | taint.cpp:574:51:574:56 | ref arg source | taint.cpp:573:49:573:54 | source | | +| taint.cpp:574:51:574:56 | source | taint.cpp:574:25:574:34 | call to _mbsncat_l | TAINT | | taint.cpp:574:51:574:56 | source | taint.cpp:574:36:574:40 | ref arg dest1 | TAINT | | taint.cpp:575:7:575:11 | ref arg dest1 | taint.cpp:572:37:572:41 | dest1 | | | taint.cpp:575:7:575:11 | ref arg dest1 | taint.cpp:576:8:576:12 | dest1 | | @@ -6421,8 +6427,11 @@ | taint.cpp:580:36:580:40 | ref arg dest3 | taint.cpp:572:85:572:89 | dest3 | | | taint.cpp:580:36:580:40 | ref arg dest3 | taint.cpp:581:7:581:11 | dest3 | | | taint.cpp:580:36:580:40 | ref arg dest3 | taint.cpp:582:8:582:12 | dest3 | | +| taint.cpp:580:43:580:45 | ptr | taint.cpp:580:25:580:34 | call to _mbsncat_l | TAINT | | taint.cpp:580:43:580:45 | ptr | taint.cpp:580:36:580:40 | ref arg dest3 | TAINT | +| taint.cpp:580:48:580:48 | n | taint.cpp:580:25:580:34 | call to _mbsncat_l | TAINT | | taint.cpp:580:48:580:48 | n | taint.cpp:580:36:580:40 | ref arg dest3 | TAINT | +| taint.cpp:580:51:580:55 | clean | taint.cpp:580:25:580:34 | call to _mbsncat_l | TAINT | | taint.cpp:580:51:580:55 | clean | taint.cpp:580:36:580:40 | ref arg dest3 | TAINT | | taint.cpp:580:51:580:55 | ref arg clean | taint.cpp:573:32:573:36 | clean | | | taint.cpp:581:7:581:11 | ref arg dest3 | taint.cpp:572:85:572:89 | dest3 | | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp index 2ae093098d2..0271b8205e4 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp @@ -574,8 +574,8 @@ void test__mbsncat_l(unsigned char* dest1, unsigned const char* ptr, unsigned ch unsigned char* dest2 = _mbsncat_l(dest1, ptr, n, source); sink(dest1); // $ SPURIOUS: ast,ir sink(*dest1); // $ ast,ir - sink(dest2); // $ SPURIOUS: ir - sink(*dest2); // $ ir + sink(dest2); // $ SPURIOUS: ast,ir + sink(*dest2); // $ ast,ir unsigned char* dest4 = _mbsncat_l(dest3, ptr, n, clean); sink(dest3); diff --git a/docs/codeql/ql-language-reference/ql-language-specification.rst b/docs/codeql/ql-language-reference/ql-language-specification.rst index 46912d2b083..765a5fda4ab 100644 --- a/docs/codeql/ql-language-reference/ql-language-specification.rst +++ b/docs/codeql/ql-language-reference/ql-language-specification.rst @@ -819,13 +819,15 @@ A class definition has the following syntax: :: - class ::= qldoc? annotations "class" classname "extends" type ("," type)* "{" member* "}" + class ::= qldoc? annotations "class" classname ("extends" type ("," type)*)? ("instanceof" type ("," type)*)? "{" member* "}" The identifier following the ``class`` keyword is the name of the class. The types specified after the ``extends`` keyword are the *base types* of the class. -A class domain type is said to *inherit* from the base types of the associated class type, a character type is said to *inherit* from its associated class domain type and a class type is said to *inherit* from its associated character type. In addition, inheritance is transitive: If a type ``A`` inherits from a type ``B``, and ``B`` inherits from a type ``C``, then ``A`` inherits from ``C``. +The types specified after the ``instanceof`` keyword are the *instanceof types* of the class. + +A class type is said to *inherit* from the base types. In addition, inheritance is transitive: If a type ``A`` inherits from a type ``B``, and ``B`` inherits from a type ``C``, then ``A`` inherits from ``C``. A class adds a mapping from the class name to the class declaration to the current module's declared type environment. @@ -833,6 +835,20 @@ A valid class can be annotated with ``abstract``, ``final``, ``library``, and `` A valid class may not inherit from a final class, from itself, or from more than one primitive type. +A valid class must have at least one base type or instanceof type. + +Class dependencies +~~~~~~~~~~~~~~~~~~ + +The program is invalid if there is a cycle of class dependencies. + +The following are class dependencies: +- ``C`` depends on ``C.C`` +- ``C.C`` depends on ``C.extends`` +- If ``C`` is abstract then it depends on all classes ``D`` such that ``C`` is a base type of ``D``. +- ``C.extends`` depends on ``D.D`` for each base type ``D`` of ``C``. +- ``C.extends`` depends on ``D`` for each instanceof type ``D`` of ``C``. + Class environments ~~~~~~~~~~~~~~~~~~ @@ -848,7 +864,9 @@ For each of member predicates and fields a class *inherits* and *declares*, and The program is invalid if any of these environments is not definite. -For each of member predicates and fields a domain type *exports* an environment. This is the union of the exported ``X`` environments of types the class inherits from, excluding any elements that are ``overridden`` by another element. +For each of member predicates and fields a domain type *exports* an environment. We say the *exported X extends environment* is the union of the exported ``X`` environments of types the class inherits from, excluding any elements that are ``overridden`` by another element. +We say the *exported X instanceof environement* is the union of the exported ``X`` environments of types that a instanceof type inherits from, excluding any elements that are ``overridden`` by another element. +The *exported X environment* of the domain type is the union of the exported ``X`` extends environment and the exported ``X`` instanceof environement. Members ~~~~~~~ @@ -1090,11 +1108,7 @@ A super expression has the following syntax: super_expr ::= "super" | type "." "super" -For a super expression to be valid, the ``this`` keyword must have a type and value in the typing environment. The type of the expression is the same as the type of ``this`` in the typing environment. - -A super expression may only occur in a QL program as the receiver expression for a predicate call. - -If a super expression includes a ``type``, then that type must be a class that the enclosing class inherits from. +For a super expression to be valid, the ``this`` keyword must have a type and value in the typing environment. The type of the expression is the same as the domain type of the type of ``this`` in the typing environment. The value of a super expression is the same as the value of ``this`` in the named tuple. @@ -1147,13 +1161,6 @@ A valid call with results *resolves* to a set of predicates. The ways a call can - If the call has no receiver and the predicate name is a selection identifier, then the qualifier is resolved as a module (see "`Module resolution <#module-resolution>`__"). The identifier is then resolved in the exported predicate environment of the qualifier module. -- If the call has a super expression as the receiver, then it resolves to a member predicate in a class that the enclosing class inherits from: - - If the super expression is unqualified and there is a single class that the current class inherits from, then the super-class is that class. - - If the super expression is unqualified and there are multiple classes that the current class inherits from, then the super-class is the domain type. - - Otherwise, the super-class is the class named by the qualifier of the super expression. - - The predicate is resolved by looking up its name and arity in the exported predicate environment of the super-class. - - If the type of the receiver is the same as the enclosing class, the predicate is resolved by looking up its name and arity in the visible predicate environment of the class. - If the type of the receiver is not the same as the enclosing class, the predicate is resolved by looking up its name and arity in the exported predicate environment of the class or domain type. @@ -1177,11 +1184,20 @@ If the resolved predicate is built in, then the call may not include a closure. If the call includes a closure, then all declared predicate arguments, the enclosing type of the declaration (if it exists), and the result type of the declaration (if it exists) must be compatible. If one of those types is a subtype of ``int``, then all the other arguments must be a subtype of ``int``. +A call to a member predicate may be a *direct* call: + - If the receiver is not a super expression it is not direct. + - If the receiver is ``A.super`` and ``A`` is an instanceof type and not a base type then it is not direct. + - If the receiver is ``A.super`` and ``A`` is a base type type and not an instanceof type then it is direct. + - If the receiver is ``A.super`` and ``A`` is a base type and an instanceof type then the call is not valid. + - If the receiver is ``super`` and the member predicate is in the exported member predicate environment of an instanceof type and not in the exported member predicate environment of a base type then it isn't direct. + - If the receiver is ``super`` and the member predicate is in the exported member predicate environment of a base type and not in the exported member predicate environment of an instanceof type then it is direct. + - If the receiver is ``super`` and the member predicate is in the exported member predicate environment of a base type and in the exported member predicate environment of an instanceof type then the call is not valid. + If the call resolves to a member predicate, then the *receiver values* are as follows. If the call has a receiver, then the receiver values are the values of that receiver. If the call does not have a receiver, then the single receiver value is the value of ``this`` in the contextual named tuple. The *tuple prefixes* of a call with results include one value from each of the argument expressions' values, in the same order as the order of the arguments. If the call resolves to a non-member predicate, then those values are exactly the tuple prefixes of the call. If the call instead resolves to a member predicate, then the tuple prefixes additionally include a receiver value, ordered before the argument values. -The *matching tuples* of a call with results are all ordered tuples that are one of the tuple prefixes followed by any value of the same type as the call. If the call has no closure, then all matching tuples must additionally satisfy the resolved predicate of the call, unless the receiver is a super expression, in which case they must *directly* satisfy the resolved predicate of the call. If the call has a ``*`` or ``+`` closure, then the matching tuples must satisfy or directly satisfy the associated closure of the resolved predicate. +The *matching tuples* of a call with results are all ordered tuples that are one of the tuple prefixes followed by any value of the same type as the call. If the call has no closure, then all matching tuples must additionally satisfy the resolved predicate of the call, unless the call is direct in which case they must *directly* satisfy the resolved predicate of the call. If the call has a ``*`` or ``+`` closure, then the matching tuples must satisfy or directly satisfy the associated closure of the resolved predicate. The values of a call with results are the final elements of each of the call's matching tuples. @@ -1534,13 +1550,15 @@ The identifier is called the *predicate name* of the call. A call must resolve to a predicate, using the same definition of resolve as for calls with results (see "`Calls with results <#calls-with-results>`__"). +A call may be direct using the same definition of direct as for calls with results (see "`Calls with results <#calls-with-results>`__"). + The resolved predicate must not have a result type. If the resolved predicate is a built-in member predicate of a primitive type, then the call may not include a closure. If the call does have a closure, then the call must resolve to a predicate with relational arity of 2. The *candidate tuples* of a call are the ordered tuples formed by selecting a value from each of the arguments of the call. -If the call has no closure, then it matches whenever one of the candidate tuples satisfies the resolved predicate of the call, unless the call has a super expression as a receiver, in which case the candidate tuple must *directly* satisfy the resolved predicate. If the call has ``*`` or ``+`` closure, then the call matches whenever one of the candidate tuples satisfies or directly satisfies the associated closure of the resolved predicate. +If the call has no closure, then it matches whenever one of the candidate tuples satisfies the resolved predicate of the call, unless the call is direct, in which case the candidate tuple must *directly* satisfy the resolved predicate. If the call has ``*`` or ``+`` closure, then the call matches whenever one of the candidate tuples satisfies or directly satisfies the associated closure of the resolved predicate. Disambiguation of formulas ~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -1926,10 +1944,12 @@ Predicates, and types can *depend* and *strictly depend* on each other. Such dep - A predicate containing a predicate call depends on the predicate to which the call resolves. If the call has negative or zero polarity then the dependency is strict. -- A predicate containing a predicate call, which resolves to a member predicate and does not have a ``super`` expression as a qualifier, depends on the dispatch dependencies of the root definitions of the target of the call. If the call has negative or zero polarity then the dependencies are strict. The predicate strictly depends on the strict dispatch dependencies of the root definitions. +- A predicate containing a predicate call, which resolves to a member predicate, where the call is not direct, depends on the dispatch dependencies of the root definitions of the target of the call. If the call has negative or zero polarity then the dependencies are strict. The predicate strictly depends on the strict dispatch dependencies of the root definitions. - For each class ``C`` in the program, for each base class ``B`` of ``C``, ``C.extends`` depends on ``B.B``. +- For each class ``C`` in the program, for each instanceof type ``B`` of ``C``, ``C.extends`` depends on ``B``. + - For each class ``C`` in the program, for each base type ``B`` of ``C`` that is not a class type, ``C.extends`` depends on ``B``. - For each class ``C`` in the program, ``C.class`` depends on ``C.C``. @@ -1976,6 +1996,7 @@ Each layer of the stratification is *populated* in order. To populate a layer, e - To populate the type ``C.extends`` for a class ``C``, identify each named tuple that has the following properties: - The value of ``this`` is in all non-class base types of ``C``. + - The value of ``this`` is in all instanceof types of ``C``. - The keys of the tuple are ``this`` and the union of the public fields from each base type. - For each class base type ``B`` of ``C`` there is a named tuple with variables from the public fields of ``B`` and ``this`` that the given tuple and some tuple in ``B.B`` both extend. @@ -2060,7 +2081,7 @@ The complete grammar for QL is as follows: | "{" formula "}" | "=" literalId "(" (predicateRef "/" int ("," predicateRef "/" int)*)? ")" "(" (exprs)? ")" - class ::= qldoc? annotations "class" classname "extends" type ("," type)* "{" member* "}" + class ::= qldoc? annotations "class" classname ("extends" type ("," type)*)? ("instanceof" type ("," type)*)? "{" member* "}" member ::= character | predicate | field diff --git a/java/ql/lib/change-notes/2022-10-19-android-startactivities-summaries.md b/java/ql/lib/change-notes/2022-10-19-android-startactivities-summaries.md new file mode 100644 index 00000000000..4716fb2ac41 --- /dev/null +++ b/java/ql/lib/change-notes/2022-10-19-android-startactivities-summaries.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added data flow summaries for tainted Android intents sent to activities via `Activity.startActivities`. \ No newline at end of file diff --git a/java/ql/lib/semmle/code/java/dataflow/FlowSummary.qll b/java/ql/lib/semmle/code/java/dataflow/FlowSummary.qll index ed9b0de165d..6b790e487c1 100644 --- a/java/ql/lib/semmle/code/java/dataflow/FlowSummary.qll +++ b/java/ql/lib/semmle/code/java/dataflow/FlowSummary.qll @@ -26,6 +26,9 @@ module SummaryComponent { /** Gets a summary component for `Element`. */ SummaryComponent element() { result = content(any(CollectionContent c)) } + /** Gets a summary component for `ArrayElement`. */ + SummaryComponent arrayElement() { result = content(any(ArrayContent c)) } + /** Gets a summary component for `MapValue`. */ SummaryComponent mapValue() { result = content(any(MapValueContent c)) } @@ -52,6 +55,11 @@ module SummaryComponentStack { result = push(SummaryComponent::element(), object) } + /** Gets a stack representing `ArrayElement` of `object`. */ + SummaryComponentStack arrayElementOf(SummaryComponentStack object) { + result = push(SummaryComponent::arrayElement(), object) + } + /** Gets a stack representing `MapValue` of `object`. */ SummaryComponentStack mapValueOf(SummaryComponentStack object) { result = push(SummaryComponent::mapValue(), object) diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index be38b83e5a7..e37e7f350b8 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -1,7 +1,10 @@ import java +private import semmle.code.java.frameworks.android.Android private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSteps +private import semmle.code.java.dataflow.FlowSummary +private import semmle.code.java.dataflow.internal.BaseSSA as BaseSsa /** The class `android.content.Intent`. */ class TypeIntent extends Class { @@ -242,19 +245,52 @@ private class StartComponentMethodAccess extends MethodAccess { /** Gets the intent argument of this call. */ Argument getIntentArg() { - result.getType() instanceof TypeIntent and + ( + result.getType() instanceof TypeIntent or + result.getType().(Array).getElementType() instanceof TypeIntent + ) and result = this.getAnArgument() } /** Holds if this targets a component of type `targetType`. */ - predicate targetsComponentType(RefType targetType) { + predicate targetsComponentType(AndroidComponent targetType) { exists(NewIntent newIntent | - DataFlow::localExprFlow(newIntent, this.getIntentArg()) and + reaches(newIntent, this.getIntentArg()) and newIntent.getClassArg().getType().(ParameterizedType).getATypeArgument() = targetType ) } } +/** + * Holds if `src` reaches the intent argument `arg` of `StartComponentMethodAccess` + * through intra-procedural steps. + */ +private predicate reaches(Expr src, Argument arg) { + any(StartComponentMethodAccess ma).getIntentArg() = arg and + src = arg + or + exists(Expr mid, BaseSsa::BaseSsaVariable ssa, BaseSsa::BaseSsaUpdate upd | + reaches(mid, arg) and + mid = ssa.getAUse() and + upd = ssa.getAnUltimateLocalDefinition() and + src = upd.getDefiningExpr().(VariableAssign).getSource() + ) + or + exists(CastingExpr e | e.getExpr() = src | reaches(e, arg)) + or + exists(ChooseExpr e | e.getAResultExpr() = src | reaches(e, arg)) + or + exists(AssignExpr e | e.getSource() = src | reaches(e, arg)) + or + exists(ArrayCreationExpr e | e.getInit().getAnInit() = src | reaches(e, arg)) + or + exists(StmtExpr e | e.getResultExpr() = src | reaches(e, arg)) + or + exists(NotNullExpr e | e.getExpr() = src | reaches(e, arg)) + or + exists(WhenExpr e | e.getBranch(_).getAResult() = src | reaches(e, arg)) +} + /** * A value-preserving step from the intent argument of a `startActivity` call to * a `getIntent` call in the activity the intent targeted in its constructor. @@ -271,6 +307,87 @@ private class StartActivityIntentStep extends AdditionalValueStep { } } +/** + * Holds if `targetType` is targeted by an existing `StartComponentMethodAccess` call + * and it's identified by `id`. + */ +private predicate isTargetableType(AndroidComponent targetType, string id) { + exists(StartComponentMethodAccess ma | ma.targetsComponentType(targetType)) and + targetType.getQualifiedName() = id +} + +private class StartActivitiesSyntheticCallable extends SyntheticCallable { + AndroidComponent targetType; + + StartActivitiesSyntheticCallable() { + exists(string id | + this = "android.content.Activity.startActivities()+" + id and + isTargetableType(targetType, id) + ) + } + + override StartComponentMethodAccess getACall() { + result.getMethod().hasName("startActivities") and + result.targetsComponentType(targetType) + } + + override predicate propagatesFlow( + SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue + ) { + exists(ActivityIntentSyntheticGlobal glob | glob.getTargetType() = targetType | + input = SummaryComponentStack::arrayElementOf(SummaryComponentStack::argument(0)) and + output = SummaryComponentStack::singleton(SummaryComponent::syntheticGlobal(glob)) and + preservesValue = true + ) + } +} + +private class GetIntentSyntheticCallable extends SyntheticCallable { + AndroidComponent targetType; + + GetIntentSyntheticCallable() { + exists(string id | + this = "android.content.Activity.getIntent()+" + id and + isTargetableType(targetType, id) + ) + } + + override Call getACall() { + result.getCallee() instanceof AndroidGetIntentMethod and + result.getEnclosingCallable().getDeclaringType() = targetType + } + + override predicate propagatesFlow( + SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue + ) { + exists(ActivityIntentSyntheticGlobal glob | glob.getTargetType() = targetType | + input = SummaryComponentStack::singleton(SummaryComponent::syntheticGlobal(glob)) and + output = SummaryComponentStack::return() and + preservesValue = true + ) + } +} + +private class ActivityIntentSyntheticGlobal extends SummaryComponent::SyntheticGlobal { + AndroidComponent targetType; + + ActivityIntentSyntheticGlobal() { + exists(string id | + this = "ActivityIntentSyntheticGlobal+" + id and + isTargetableType(targetType, id) + ) + } + + AndroidComponent getTargetType() { result = targetType } +} + +private class RequiredComponentStackForStartActivities extends RequiredSummaryComponentStack { + override predicate required(SummaryComponent head, SummaryComponentStack tail) { + head = SummaryComponent::arrayElement() and + tail = SummaryComponentStack::argument(0) + } +} + /** * A value-preserving step from the intent argument of a `sendBroadcast` call to * the intent parameter in the `onReceive` method of the receiver the diff --git a/java/ql/src/Violations of Best Practice/Dead Code/UnreadLocal.ql b/java/ql/src/Violations of Best Practice/Dead Code/UnreadLocal.ql index c2052344b7b..31697e561ed 100644 --- a/java/ql/src/Violations of Best Practice/Dead Code/UnreadLocal.ql +++ b/java/ql/src/Violations of Best Practice/Dead Code/UnreadLocal.ql @@ -26,5 +26,10 @@ where not exists(getARead(v)) and // Discarded exceptions are covered by another query. not exists(CatchClause cc | cc.getVariable().getVariable() = v) and + // Exclude common Kotlin pattern to do something n times: `for(i in 1..n) { doSomething() } + not exists(EnhancedForStmt f | + f.getVariable().getVariable() = v and + f.getExpr().getType().(RefType).hasQualifiedName("kotlin.ranges", ["IntRange", "LongRange"]) + ) and not readImplicitly(v) select v, "Variable '" + v + "' is never read." diff --git a/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql b/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql index e3ac1384243..f355cd5f219 100644 --- a/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql +++ b/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql @@ -57,6 +57,8 @@ private predicate candidateMethod(RefType t, Method m, string name, int numParam m.getNumberOfParameters() = numParam and m = m.getSourceDeclaration() and not m.getAnAnnotation() instanceof DeprecatedAnnotation and + // Exclude compiler generated methods, such as Kotlin `$default` methods: + not m.isCompilerGenerated() and not whitelist(name) } diff --git a/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/Test.kt b/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/Test.kt index e8ead8d323e..b802d9f76a0 100644 --- a/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/Test.kt +++ b/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/Test.kt @@ -7,3 +7,8 @@ class C { prop(this) } } + +class A { + fun fn(value: T, i: Int = 1) {} + fun fn(value: String, i: Int = 1) {} +} diff --git a/java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.expected b/java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.expected new file mode 100644 index 00000000000..3c83d812b43 --- /dev/null +++ b/java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.expected @@ -0,0 +1,3 @@ +| test.kt:8:10:8:10 | int e | Variable 'int e' is never read. | +| test.kt:14:11:14:13 | int idx | Variable 'int idx' is never read. | +| test.kt:14:16:14:16 | int e | Variable 'int e' is never read. | diff --git a/java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.qlref b/java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.qlref new file mode 100644 index 00000000000..5a77117711e --- /dev/null +++ b/java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.qlref @@ -0,0 +1 @@ +Violations of Best Practice/Dead Code/UnreadLocal.ql diff --git a/java/ql/test/kotlin/query-tests/UnreadLocal/test.kt b/java/ql/test/kotlin/query-tests/UnreadLocal/test.kt new file mode 100644 index 00000000000..e1663d7c116 --- /dev/null +++ b/java/ql/test/kotlin/query-tests/UnreadLocal/test.kt @@ -0,0 +1,25 @@ +fun fn0(size: Int) { + for (idx in 1..size) { + println() + } +} + +fun fn1(a: Array) { + for (e in a) { + println() + } +} + +fun fn2(a: Array) { + for ((idx, e) in a.withIndex()) { + println() + } +} + +fun fn3() { + for (i in 1 until 10) { + println() + } +} + +// Diagnostic Matches: % Couldn't find a Java equivalent function to kotlin.Int.rangeTo in java.lang.Integer% diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java index 35884a23a58..09e785e5b6c 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java @@ -24,6 +24,12 @@ public class TestStartActivityToGetIntent { Intent[] intents = new Intent[] {intent}; ctx.startActivities(intents); } + { + Intent intent = new Intent(null, AnotherActivity.class); + intent.putExtra("data", (String) source("ctx-start-acts-2")); + Intent[] intents = new Intent[] {intent}; + ctx.startActivities(intents); + } { Intent intent = new Intent(null, SomeActivity.class); intent.putExtra("data", (String) source("act-start")); @@ -35,6 +41,12 @@ public class TestStartActivityToGetIntent { Intent[] intents = new Intent[] {intent}; act.startActivities(intents); } + { + Intent intent = new Intent(null, Object.class); + intent.putExtra("data", (String) source("start-activities-should-not-reach")); + Intent[] intents = new Intent[] {intent}; + act.startActivities(intents); + } { Intent intent = new Intent(null, SomeActivity.class); intent.putExtra("data", (String) source("start-for-result")); @@ -79,9 +91,16 @@ public class TestStartActivityToGetIntent { static class SomeActivity extends Activity { public void test() { - sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-start hasValueFlow=act-start hasValueFlow=start-for-result hasValueFlow=start-if-needed hasValueFlow=start-matching hasValueFlow=start-from-child hasValueFlow=start-from-frag hasValueFlow=4-arg MISSING: hasValueFlow=ctx-start-acts hasValueFlow=act-start-acts + // @formatter:off + sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-start hasValueFlow=act-start hasValueFlow=start-for-result hasValueFlow=start-if-needed hasValueFlow=start-matching hasValueFlow=start-from-child hasValueFlow=start-from-frag hasValueFlow=4-arg hasValueFlow=ctx-start-acts hasValueFlow=act-start-acts + // @formatter:on } + } + static class AnotherActivity extends Activity { + public void test() { + sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-start-acts-2 + } } static class SafeActivity extends Activity { diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml b/javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml index 3bb1669cdff..e0571f38255 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml @@ -1,5 +1,5 @@ name: codeql/javascript-experimental-atm-lib -version: 0.3.7 +version: 0.4.1 extractor: javascript library: true groups: diff --git a/javascript/ql/experimental/adaptivethreatmodeling/model/qlpack.yml b/javascript/ql/experimental/adaptivethreatmodeling/model/qlpack.yml index 4cf8ca1dbef..40b611fc72a 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/model/qlpack.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/model/qlpack.yml @@ -1,5 +1,5 @@ name: codeql/javascript-experimental-atm-model -version: 0.2.1 +version: 0.3.1 groups: - javascript - experimental diff --git a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/codeql-pack.lock.yml b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/codeql-pack.lock.yml index 46630e15fd9..fc9f87b3bbb 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/codeql-pack.lock.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/codeql-pack.lock.yml @@ -1,6 +1,6 @@ --- dependencies: codeql/javascript-experimental-atm-model: - version: 0.2.1-2022-09-06-08h55m54s.bubbly-basin-xpztl8fh.f3c3c9360a727959e428ecc6932257e6a546dc65d8a9baac525a49247123822d + version: 0.3.0 compiled: false lockVersion: 1.0.0 diff --git a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/qlpack.yml b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/qlpack.yml index 0c2521e5411..e6657138f1c 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/qlpack.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/qlpack.yml @@ -6,4 +6,4 @@ groups: - experimental dependencies: codeql/javascript-experimental-atm-lib: ${workspace} - codeql/javascript-experimental-atm-model: "0.2.1-2022-09-06-08h55m54s.bubbly-basin-xpztl8fh.f3c3c9360a727959e428ecc6932257e6a546dc65d8a9baac525a49247123822d" + codeql/javascript-experimental-atm-model: "0.3.0" diff --git a/javascript/ql/experimental/adaptivethreatmodeling/src/codeql-pack.lock.yml b/javascript/ql/experimental/adaptivethreatmodeling/src/codeql-pack.lock.yml index 46630e15fd9..fc9f87b3bbb 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/src/codeql-pack.lock.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/src/codeql-pack.lock.yml @@ -1,6 +1,6 @@ --- dependencies: codeql/javascript-experimental-atm-model: - version: 0.2.1-2022-09-06-08h55m54s.bubbly-basin-xpztl8fh.f3c3c9360a727959e428ecc6932257e6a546dc65d8a9baac525a49247123822d + version: 0.3.0 compiled: false lockVersion: 1.0.0 diff --git a/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml b/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml index 1b2e3da82e9..cab87ce0e33 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml @@ -1,6 +1,6 @@ name: codeql/javascript-experimental-atm-queries language: javascript -version: 0.3.7 +version: 0.4.1 suites: codeql-suites defaultSuiteFile: codeql-suites/javascript-atm-code-scanning.qls groups: @@ -8,4 +8,4 @@ groups: - experimental dependencies: codeql/javascript-experimental-atm-lib: ${workspace} - codeql/javascript-experimental-atm-model: "0.2.1-2022-09-06-08h55m54s.bubbly-basin-xpztl8fh.f3c3c9360a727959e428ecc6932257e6a546dc65d8a9baac525a49247123822d" + codeql/javascript-experimental-atm-model: "0.3.0" diff --git a/javascript/ql/experimental/adaptivethreatmodeling/test/codeql-pack.lock.yml b/javascript/ql/experimental/adaptivethreatmodeling/test/codeql-pack.lock.yml index 46630e15fd9..fc9f87b3bbb 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/test/codeql-pack.lock.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/test/codeql-pack.lock.yml @@ -1,6 +1,6 @@ --- dependencies: codeql/javascript-experimental-atm-model: - version: 0.2.1-2022-09-06-08h55m54s.bubbly-basin-xpztl8fh.f3c3c9360a727959e428ecc6932257e6a546dc65d8a9baac525a49247123822d + version: 0.3.0 compiled: false lockVersion: 1.0.0 diff --git a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll index e617f597870..dc28e44ecdd 100644 --- a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll +++ b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll @@ -233,6 +233,8 @@ module AccessPath { baseName = fromReference(write.getBase(), root) or baseName = fromRhs(write.getBase(), root) + or + baseName = fromRhs(GetLaterAccess::getLaterBaseAccess(write), root) ) or exists(GlobalVariable var | @@ -266,6 +268,100 @@ module AccessPath { ) } + /** A module for computing an access to a variable that happens after a property has been written onto it */ + private module GetLaterAccess { + /** + * Gets an access to a variable that is written to in `write`, where the access is after the write. + * + * This allows `fromRhs` to compute an access path for e.g. the below example: + * ```JavaScript + * function foo(x) { + * var obj = { + * bar: x // `x` has the access path "foo.bar" starting from the root `this`. + * }; + * this.foo = obj; + * } + * ``` + */ + pragma[noopt] + DataFlow::Node getLaterBaseAccess(DataFlow::PropWrite write) { + exists( + ControlFlowNode writeNode, BindingPattern access, VarRef otherAccess, Variable variable, + StmtContainer container + | + access = getBaseVar(write) and + writeNode = write.getWriteNode() and + access = getAnAccessInContainer(variable, container, true) and + variable = getARelevantVariable() and // manual magic + otherAccess = getAnAccessInContainer(variable, container, false) and + access != otherAccess and + result.asExpr() = otherAccess + | + exists(BasicBlock bb, int i, int j | + bb.getNode(i) = writeNode and + bb.getNode(j) = otherAccess and + i < j + ) + or + otherAccess.getBasicBlock() = getASuccessorBBThatReadsVar(write) // more manual magic - outlined into a helper predicate. + ) + } + + /** Gets a variable ref that `write` writes a property to. */ + VarRef getBaseVar(DataFlow::PropWrite write) { + result = write.getBase().asExpr() + or + exists(Assignment assign | + write.getBase().asExpr() = assign.getRhs() and + result = assign.getLhs() + ) + or + exists(VariableDeclarator decl | + write.getBase().asExpr() = decl.getInit() and + result = decl.getBindingPattern() + ) + } + + /** Gets an access to `var` inside `container` where `usedInWrite` indicates whether the access is the base of a property write. */ + private VarRef getAnAccessInContainer(Variable var, StmtContainer container, boolean usedInWrite) { + result.getVariable() = var and + result.getContainer() = container and + var.isLocal() and + if result = getBaseVar(_) then usedInWrite = true else usedInWrite = false + } + + /** Gets a variable that is relevant for the computations in the `GetLaterAccess` module. */ + private Variable getARelevantVariable() { + // The variable might be used where `getLaterBaseAccess()` is called. + exists(DataFlow::Node node | + exists(fromRhs(node, _)) and + node.asExpr().(VarAccess).getVariable() = result + ) and + // There is a write that writes to the variable. + getBaseVar(_).getVariable() = result and + // It's local. + result.isLocal() and // we skip global variables, because that turns messy quick. + // There is both a "write" and "read" in the same container of the variable. + exists(StmtContainer container | + exists(getAnAccessInContainer(result, container, true)) and // a "write", an access to the variable that is the base of a property reference. + exists(getAnAccessInContainer(result, container, false)) // a "read", an access to the variable that is not the base of a property reference. + ) + } + + /** Gets a basic-block that has a read of the variable that is written to by `write`, where the basicblock occurs after `start`. */ + private ReachableBasicBlock getASuccessorBBThatReadsVar(DataFlow::PropWrite write) { + exists(VarAccess baseExpr, Variable var, ControlFlowNode writeNode | + baseExpr = getBaseVar(write) and + var = baseExpr.getVariable() and + var = getARelevantVariable() and + writeNode = write.getWriteNode() and + writeNode.getBasicBlock().(ReachableBasicBlock).strictlyDominates(result) and + // manual magic. + result = getAnAccessInContainer(getARelevantVariable(), _, false).getBasicBlock() + ) + } + } + /** * Gets a node that refers to the given access path relative to the given `root` node, * or `root` itself if the access path is empty. diff --git a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll index 20366a8f2b5..2404fecbd30 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll @@ -1692,10 +1692,10 @@ module DataFlow { */ predicate localFieldStep(DataFlow::Node pred, DataFlow::Node succ) { exists(ClassNode cls, string prop | - pred = cls.getADirectSuperClass*().getAReceiverNode().getAPropertyWrite(prop).getRhs() or + pred = AccessPath::getAnAssignmentTo(cls.getADirectSuperClass*().getAReceiverNode(), prop) or pred = cls.getInstanceMethod(prop) | - succ = cls.getAReceiverNode().getAPropertyRead(prop) + succ = AccessPath::getAReferenceTo(cls.getAReceiverNode(), prop) ) } diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected index 4cf79a4aedd..c97e4dc650e 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected @@ -170,6 +170,10 @@ nodes | lib/lib.js:277:23:277:26 | opts | | lib/lib.js:277:23:277:30 | opts.bla | | lib/lib.js:277:23:277:30 | opts.bla | +| lib/lib.js:279:19:279:22 | opts | +| lib/lib.js:279:19:279:26 | opts.bla | +| lib/lib.js:281:23:281:35 | this.opts.bla | +| lib/lib.js:281:23:281:35 | this.opts.bla | | lib/lib.js:307:39:307:42 | name | | lib/lib.js:307:39:307:42 | name | | lib/lib.js:308:23:308:26 | name | @@ -504,8 +508,13 @@ edges | lib/lib.js:268:22:268:24 | obj | lib/lib.js:268:22:268:32 | obj.version | | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:26 | opts | | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:26 | opts | +| lib/lib.js:276:8:276:11 | opts | lib/lib.js:279:19:279:22 | opts | +| lib/lib.js:276:8:276:11 | opts | lib/lib.js:279:19:279:22 | opts | | lib/lib.js:277:23:277:26 | opts | lib/lib.js:277:23:277:30 | opts.bla | | lib/lib.js:277:23:277:26 | opts | lib/lib.js:277:23:277:30 | opts.bla | +| lib/lib.js:279:19:279:22 | opts | lib/lib.js:279:19:279:26 | opts.bla | +| lib/lib.js:279:19:279:26 | opts.bla | lib/lib.js:281:23:281:35 | this.opts.bla | +| lib/lib.js:279:19:279:26 | opts.bla | lib/lib.js:281:23:281:35 | this.opts.bla | | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | @@ -714,6 +723,7 @@ edges | lib/lib.js:261:11:261:33 | "rm -rf ... + name | lib/lib.js:257:35:257:38 | name | lib/lib.js:261:30:261:33 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:257:35:257:38 | name | library input | lib/lib.js:261:3:261:34 | cp.exec ... + name) | shell command | | lib/lib.js:268:10:268:32 | "rm -rf ... version | lib/lib.js:267:46:267:48 | obj | lib/lib.js:268:22:268:32 | obj.version | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:267:46:267:48 | obj | library input | lib/lib.js:268:2:268:33 | cp.exec ... ersion) | shell command | | lib/lib.js:277:11:277:30 | "rm -rf " + opts.bla | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:30 | opts.bla | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:276:8:276:11 | opts | library input | lib/lib.js:277:3:277:31 | cp.exec ... ts.bla) | shell command | +| lib/lib.js:281:11:281:35 | "rm -rf ... pts.bla | lib/lib.js:276:8:276:11 | opts | lib/lib.js:281:23:281:35 | this.opts.bla | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:276:8:276:11 | opts | library input | lib/lib.js:281:3:281:36 | cp.exec ... ts.bla) | shell command | | lib/lib.js:308:11:308:26 | "rm -rf " + name | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:307:39:307:42 | name | library input | lib/lib.js:308:3:308:27 | cp.exec ... + name) | shell command | | lib/lib.js:315:10:315:25 | "rm -rf " + name | lib/lib.js:314:40:314:43 | name | lib/lib.js:315:22:315:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:314:40:314:43 | name | library input | lib/lib.js:315:2:315:26 | cp.exec ... + name) | shell command | | lib/lib.js:320:11:320:26 | "rm -rf " + name | lib/lib.js:314:40:314:43 | name | lib/lib.js:320:23:320:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:314:40:314:43 | name | library input | lib/lib.js:320:3:320:27 | cp.exec ... + name) | shell command | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js index d94f430c57f..64b279d7d05 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js @@ -278,7 +278,7 @@ module.exports.Foo = class Foo { this.opts = {}; this.opts.bla = opts.bla - cp.exec("rm -rf " + this.opts.bla); // NOT OK - but FN [INCONSISTENCY] + cp.exec("rm -rf " + this.opts.bla); // NOT OK } } diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/UnsafeCodeConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/UnsafeCodeConstruction.expected index a4486d680f6..14733b4274d 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/UnsafeCodeConstruction.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/UnsafeCodeConstruction.expected @@ -15,6 +15,41 @@ nodes | lib/index.js:19:26:19:29 | data | | lib/index.js:22:7:22:10 | data | | lib/index.js:22:7:22:10 | data | +| lib/index.js:41:32:41:35 | opts | +| lib/index.js:41:32:41:35 | opts | +| lib/index.js:42:3:42:19 | opts | +| lib/index.js:42:10:42:13 | opts | +| lib/index.js:42:10:42:19 | opts \|\| {} | +| lib/index.js:44:21:44:24 | opts | +| lib/index.js:44:21:44:32 | opts.varName | +| lib/index.js:51:21:51:32 | opts.varName | +| lib/index.js:51:21:51:32 | opts.varName | +| lib/index.js:51:21:51:32 | opts.varName | +| lib/index.js:86:15:86:19 | taint | +| lib/index.js:86:15:86:19 | taint | +| lib/index.js:87:18:87:22 | taint | +| lib/index.js:89:36:89:40 | taint | +| lib/index.js:93:32:93:36 | taint | +| lib/index.js:98:30:98:34 | taint | +| lib/index.js:103:21:103:47 | this.op ... dOption | +| lib/index.js:103:21:103:47 | this.op ... dOption | +| lib/index.js:104:21:104:47 | this.op ... dOption | +| lib/index.js:104:21:104:47 | this.op ... dOption | +| lib/index.js:105:21:105:47 | this.op ... dOption | +| lib/index.js:105:21:105:47 | this.op ... dOption | +| lib/index.js:106:21:106:30 | this.taint | +| lib/index.js:106:21:106:30 | this.taint | +| lib/index.js:112:17:112:21 | taint | +| lib/index.js:112:17:112:21 | taint | +| lib/index.js:113:20:113:24 | taint | +| lib/index.js:121:34:121:38 | taint | +| lib/index.js:129:32:129:36 | taint | +| lib/index.js:136:23:136:49 | this.op ... dOption | +| lib/index.js:136:23:136:49 | this.op ... dOption | +| lib/index.js:137:23:137:49 | this.op ... dOption | +| lib/index.js:137:23:137:49 | this.op ... dOption | +| lib/index.js:138:23:138:32 | this.taint | +| lib/index.js:138:23:138:32 | this.taint | edges | lib/index.js:1:35:1:38 | data | lib/index.js:2:21:2:24 | data | | lib/index.js:1:35:1:38 | data | lib/index.js:2:21:2:24 | data | @@ -32,8 +67,53 @@ edges | lib/index.js:19:26:19:29 | data | lib/index.js:22:7:22:10 | data | | lib/index.js:19:26:19:29 | data | lib/index.js:22:7:22:10 | data | | lib/index.js:19:26:19:29 | data | lib/index.js:22:7:22:10 | data | +| lib/index.js:41:32:41:35 | opts | lib/index.js:42:10:42:13 | opts | +| lib/index.js:41:32:41:35 | opts | lib/index.js:42:10:42:13 | opts | +| lib/index.js:42:3:42:19 | opts | lib/index.js:44:21:44:24 | opts | +| lib/index.js:42:10:42:13 | opts | lib/index.js:42:10:42:19 | opts \|\| {} | +| lib/index.js:42:10:42:19 | opts \|\| {} | lib/index.js:42:3:42:19 | opts | +| lib/index.js:44:21:44:24 | opts | lib/index.js:44:21:44:32 | opts.varName | +| lib/index.js:44:21:44:32 | opts.varName | lib/index.js:51:21:51:32 | opts.varName | +| lib/index.js:44:21:44:32 | opts.varName | lib/index.js:51:21:51:32 | opts.varName | +| lib/index.js:44:21:44:32 | opts.varName | lib/index.js:51:21:51:32 | opts.varName | +| lib/index.js:86:15:86:19 | taint | lib/index.js:87:18:87:22 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:87:18:87:22 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:89:36:89:40 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:89:36:89:40 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:93:32:93:36 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:93:32:93:36 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:98:30:98:34 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:98:30:98:34 | taint | +| lib/index.js:87:18:87:22 | taint | lib/index.js:106:21:106:30 | this.taint | +| lib/index.js:87:18:87:22 | taint | lib/index.js:106:21:106:30 | this.taint | +| lib/index.js:89:36:89:40 | taint | lib/index.js:103:21:103:47 | this.op ... dOption | +| lib/index.js:89:36:89:40 | taint | lib/index.js:103:21:103:47 | this.op ... dOption | +| lib/index.js:93:32:93:36 | taint | lib/index.js:104:21:104:47 | this.op ... dOption | +| lib/index.js:93:32:93:36 | taint | lib/index.js:104:21:104:47 | this.op ... dOption | +| lib/index.js:98:30:98:34 | taint | lib/index.js:105:21:105:47 | this.op ... dOption | +| lib/index.js:98:30:98:34 | taint | lib/index.js:105:21:105:47 | this.op ... dOption | +| lib/index.js:112:17:112:21 | taint | lib/index.js:113:20:113:24 | taint | +| lib/index.js:112:17:112:21 | taint | lib/index.js:113:20:113:24 | taint | +| lib/index.js:112:17:112:21 | taint | lib/index.js:121:34:121:38 | taint | +| lib/index.js:112:17:112:21 | taint | lib/index.js:121:34:121:38 | taint | +| lib/index.js:112:17:112:21 | taint | lib/index.js:129:32:129:36 | taint | +| lib/index.js:112:17:112:21 | taint | lib/index.js:129:32:129:36 | taint | +| lib/index.js:113:20:113:24 | taint | lib/index.js:138:23:138:32 | this.taint | +| lib/index.js:113:20:113:24 | taint | lib/index.js:138:23:138:32 | this.taint | +| lib/index.js:121:34:121:38 | taint | lib/index.js:136:23:136:49 | this.op ... dOption | +| lib/index.js:121:34:121:38 | taint | lib/index.js:136:23:136:49 | this.op ... dOption | +| lib/index.js:129:32:129:36 | taint | lib/index.js:137:23:137:49 | this.op ... dOption | +| lib/index.js:129:32:129:36 | taint | lib/index.js:137:23:137:49 | this.op ... dOption | #select | lib/index.js:2:21:2:24 | data | lib/index.js:1:35:1:38 | data | lib/index.js:2:21:2:24 | data | This string concatenation which depends on $@ is later $@. | lib/index.js:1:35:1:38 | data | library input | lib/index.js:2:15:2:30 | "(" + data + ")" | interpreted as code | | lib/index.js:6:26:6:29 | name | lib/index.js:5:35:5:38 | name | lib/index.js:6:26:6:29 | name | This string concatenation which depends on $@ is later $@. | lib/index.js:5:35:5:38 | name | library input | lib/index.js:6:17:6:29 | "obj." + name | interpreted as code | | lib/index.js:14:21:14:24 | data | lib/index.js:13:38:13:41 | data | lib/index.js:14:21:14:24 | data | This string concatenation which depends on $@ is later $@. | lib/index.js:13:38:13:41 | data | library input | lib/index.js:14:15:14:30 | "(" + data + ")" | interpreted as code | | lib/index.js:22:7:22:10 | data | lib/index.js:19:26:19:29 | data | lib/index.js:22:7:22:10 | data | This string concatenation which depends on $@ is later $@. | lib/index.js:19:26:19:29 | data | library input | lib/index.js:25:24:25:26 | str | interpreted as code | +| lib/index.js:51:21:51:32 | opts.varName | lib/index.js:41:32:41:35 | opts | lib/index.js:51:21:51:32 | opts.varName | This string concatenation which depends on $@ is later $@. | lib/index.js:41:32:41:35 | opts | library input | lib/index.js:51:10:51:52 | " var ... ing();" | interpreted as code | +| lib/index.js:103:21:103:47 | this.op ... dOption | lib/index.js:86:15:86:19 | taint | lib/index.js:103:21:103:47 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:86:15:86:19 | taint | library input | lib/index.js:103:10:103:67 | " var ... ing();" | interpreted as code | +| lib/index.js:104:21:104:47 | this.op ... dOption | lib/index.js:86:15:86:19 | taint | lib/index.js:104:21:104:47 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:86:15:86:19 | taint | library input | lib/index.js:104:10:104:67 | " var ... ing();" | interpreted as code | +| lib/index.js:105:21:105:47 | this.op ... dOption | lib/index.js:86:15:86:19 | taint | lib/index.js:105:21:105:47 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:86:15:86:19 | taint | library input | lib/index.js:105:10:105:67 | " var ... ing();" | interpreted as code | +| lib/index.js:106:21:106:30 | this.taint | lib/index.js:86:15:86:19 | taint | lib/index.js:106:21:106:30 | this.taint | This string concatenation which depends on $@ is later $@. | lib/index.js:86:15:86:19 | taint | library input | lib/index.js:106:10:106:50 | " var ... ing();" | interpreted as code | +| lib/index.js:136:23:136:49 | this.op ... dOption | lib/index.js:112:17:112:21 | taint | lib/index.js:136:23:136:49 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:112:17:112:21 | taint | library input | lib/index.js:136:12:136:69 | " var ... ing();" | interpreted as code | +| lib/index.js:137:23:137:49 | this.op ... dOption | lib/index.js:112:17:112:21 | taint | lib/index.js:137:23:137:49 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:112:17:112:21 | taint | library input | lib/index.js:137:12:137:69 | " var ... ing();" | interpreted as code | +| lib/index.js:138:23:138:32 | this.taint | lib/index.js:112:17:112:21 | taint | lib/index.js:138:23:138:32 | this.taint | This string concatenation which depends on $@ is later $@. | lib/index.js:112:17:112:21 | taint | library input | lib/index.js:138:12:138:52 | " var ... ing();" | interpreted as code | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/lib/index.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/lib/index.js index 4289ebfc686..9df334c56dc 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/lib/index.js +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/lib/index.js @@ -33,3 +33,109 @@ export function greySink(data) { } }); } + +function codeIsAlive() { + new Template().compile(); +} + +export function Template(text, opts) { + opts = opts || {}; + var options = {}; + options.varName = opts.varName; + this.opts = options; +} + +Template.prototype = { + compile: function () { + var opts = this.opts; + eval(" var " + opts.varName + " = something();"); // NOT OK + }, + // The below are justs tests that ensure the global-access-path computations terminate. + pathsTerminate1: function (node, prev) { + node.tree = { + ancestor: node, + number: rand ? prev.tree.number + 1 : 0, + }; + }, + pathsTerminate2: function (A) { + try { + var B = A.p1; + var C = B.p2; + C.p5 = C; + } catch (ex) {} + }, + pathsTerminate3: function (A) { + var x = foo(); + while (Math.random()) { + x.r = x; + } + }, + pathsTerminate4: function () { + var dest = foo(); + var range = foo(); + while (Math.random() < 0.5) { + range.tabstop = dest; + if (Math.random() < 0.5) { + dest.firstNonLinked = range; + } + } + }, +}; + +export class AccessPathClass { + constructor(taint) { + this.taint = taint; + + var options1 = {taintedOption: taint}; + this.options1 = options1; + + var options2; + options2 = {taintedOption: taint}; + this.options2 = options2; + + var options3; + options3 = {}; + options3.taintedOption = taint; + this.options3 = options3; + } + + doesTaint() { + eval(" var " + this.options1.taintedOption + " = something();"); // NOT OK + eval(" var " + this.options2.taintedOption + " = something();"); // NOT OK + eval(" var " + this.options3.taintedOption + " = something();"); // NOT OK + eval(" var " + this.taint + " = something();"); // NOT OK + } +} + + +export class AccessPathClassBB { + constructor(taint) { + this.taint = taint; + + var options1 = {taintedOption: taint}; + if (Math.random() < 0.5) { console.log("foo"); } + this.options1 = options1; + + var options2; + if (Math.random() < 0.5) { console.log("foo"); } + options2 = {taintedOption: taint}; + if (Math.random() < 0.5) { console.log("foo"); } + this.options2 = options2; + + var options3; + if (Math.random() < 0.5) { console.log("foo"); } + options3 = {}; + if (Math.random() < 0.5) { console.log("foo"); } + options3.taintedOption = taint; + if (Math.random() < 0.5) { console.log("foo"); } + this.options3 = options3; + } + + doesTaint() { + eval(" var " + this.options1.taintedOption + " = something();"); // NOT OK + eval(" var " + this.options2.taintedOption + " = something();"); // NOT OK + eval(" var " + this.options3.taintedOption + " = something();"); // NOT OK + eval(" var " + this.taint + " = something();"); // NOT OK + } + } + \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected index 7705d224298..92cd6bd6c45 100644 --- a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected +++ b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected @@ -248,7 +248,9 @@ edges | lib.js:55:15:55:21 | path[0] | lib.js:55:11:55:22 | obj[path[0]] | | lib.js:59:18:59:18 | s | lib.js:61:17:61:17 | s | | lib.js:59:18:59:18 | s | lib.js:61:17:61:17 | s | +| lib.js:61:17:61:17 | s | lib.js:68:11:68:26 | path | | lib.js:61:17:61:17 | s | lib.js:68:18:68:26 | this.path | +| lib.js:61:17:61:17 | s | lib.js:70:17:70:20 | path | | lib.js:68:11:68:26 | path | lib.js:70:17:70:20 | path | | lib.js:68:18:68:26 | this.path | lib.js:68:11:68:26 | path | | lib.js:70:17:70:20 | path | lib.js:70:17:70:23 | path[0] | diff --git a/misc/bazel/workspace.bzl b/misc/bazel/workspace.bzl index 9d1b533bb4e..4248a09d47a 100644 --- a/misc/bazel/workspace.bzl +++ b/misc/bazel/workspace.bzl @@ -22,6 +22,10 @@ def codeql_workspace(repository_name = "codeql"): _swift_prebuilt_version, repo_arch, ), + patches = [ + "@%s//swift/third_party/swift-llvm-support:patches/remove_getFallthrougDest_assert.patch" % repository_name, + ], + patch_args = ["-p1"], build_file = "@%s//swift/third_party/swift-llvm-support:BUILD.swift-prebuilt.bazel" % repository_name, sha256 = sha256, ) diff --git a/ruby/ql/lib/change-notes/2022-10-28-try-code-execution.md b/ruby/ql/lib/change-notes/2022-10-28-try-code-execution.md new file mode 100644 index 00000000000..af5b1cb59e4 --- /dev/null +++ b/ruby/ql/lib/change-notes/2022-10-28-try-code-execution.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `ActiveSupport` extensions `Object#try` and `Object#try!` are now recognised as code executions. diff --git a/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll b/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll index 9ce0bf32fd7..30045054503 100644 --- a/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll +++ b/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll @@ -252,6 +252,30 @@ private module Cached { cfn.isJoin() or cfn.getAPredecessor().isBranch() + or + /* + * In cases such as + * + * ```rb + * if x or y + * foo + * else + * bar + * ``` + * + * we have a CFG that looks like + * + * x --false--> [false] x or y --false--> bar + * \ | + * --true--> y --false-- + * \ + * --true--> [true] x or y --true--> foo + * + * and we want to ensure that both `foo` and `bar` start a new basic block, + * in order to get a `ConditionalBlock` out of the disjunction. + */ + + exists(cfn.getAPredecessor(any(SuccessorTypes::ConditionalSuccessor s))) } /** diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll index 7b56f2e6a93..5a6883a5fe4 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll @@ -147,6 +147,9 @@ class ExprNode extends Node, TExprNode { class ParameterNode extends Node, TParameterNode instanceof ParameterNodeImpl { /** Gets the parameter corresponding to this node, if any. */ final Parameter getParameter() { result = super.getParameter() } + + /** Gets the name of the parameter, if any. */ + final string getName() { result = this.getParameter().(NamedParameter).getName() } } /** diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll index 6c57b31f6fa..88fe072395d 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll @@ -81,6 +81,29 @@ module ActiveSupport { preservesValue = true } } + + /** + * A call to `Object#try`, which may execute its first argument as a Ruby + * method call. + * ```rb + * x = "abc" + * x.try(:upcase) # => "ABC" + * y = nil + * y.try(:upcase) # => nil + * ``` + * `Object#try!` behaves similarly but raises `NoMethodError` if the + * receiver is not `nil` and does not respond to the method. + */ + class TryCallCodeExecution extends CodeExecution::Range instanceof DataFlow::CallNode { + TryCallCodeExecution() { + this.asExpr().getExpr() instanceof UnknownMethodCall and + this.getMethodName() = ["try", "try!"] + } + + override DataFlow::Node getCode() { result = super.getArgument(0) } + + override predicate runsArbitraryCode() { none() } + } } /** diff --git a/ruby/ql/lib/codeql/ruby/frameworks/Core.qll b/ruby/ql/lib/codeql/ruby/frameworks/Core.qll index 0e3336f81d6..6411dad64b5 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/Core.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/Core.qll @@ -7,6 +7,7 @@ private import codeql.ruby.DataFlow private import codeql.ruby.dataflow.FlowSummary import core.BasicObject::BasicObject import core.Object::Object +import core.Gem::Gem import core.Kernel::Kernel import core.Module import core.Array diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll new file mode 100644 index 00000000000..9e417ad371a --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll @@ -0,0 +1,102 @@ +/** + * Provides modeling for the `Gem` module and `.gemspec` files. + */ + +private import ruby +private import Ast +private import codeql.ruby.ApiGraphs + +/** Provides modeling for the `Gem` module and `.gemspec` files. */ +module Gem { + /** + * A .gemspec file that lists properties of a Ruby gem. + * The contents of a .gemspec file generally look like: + * ```Ruby + * Gem::Specification.new do |s| + * s.name = 'library-name' + * s.require_path = "lib" + * # more properties + * end + * ``` + */ + class GemSpec instanceof File { + API::Node specCall; + + GemSpec() { + this.getExtension() = "gemspec" and + specCall = API::root().getMember("Gem").getMember("Specification").getMethod("new") and + specCall.getLocation().getFile() = this + } + + /** Gets the name of this .gemspec file. */ + string toString() { result = File.super.getBaseName() } + + /** + * Gets a value of the `name` property of this .gemspec file. + * These properties are set using the `Gem::Specification.new` method. + */ + private Expr getSpecProperty(string name) { + exists(Expr rhs | + rhs = + specCall + .getBlock() + .getParameter(0) + .getMethod(name + "=") + .getParameter(0) + .asSink() + .asExpr() + .getExpr() + .(Ast::AssignExpr) + .getRightOperand() + | + result = rhs + or + // some properties are arrays, we just unfold them + result = rhs.(ArrayLiteral).getAnElement() + ) + } + + /** Gets the name of the gem */ + string getName() { result = getSpecProperty("name").getConstantValue().getString() } + + /** Gets a path that is loaded when the gem is required */ + private string getARequirePath() { + result = getSpecProperty(["require_paths", "require_path"]).getConstantValue().getString() + or + not exists(getSpecProperty(["require_paths", "require_path"]).getConstantValue().getString()) and + result = "lib" // the default is "lib" + } + + /** Gets a file that could be loaded when the gem is required. */ + private File getAPossiblyRequiredFile() { + result = File.super.getParentContainer().getFolder(getARequirePath()).getAChildContainer*() + } + + /** Gets a class/module that is exported by this gem. */ + private ModuleBase getAPublicModule() { + result.(Toplevel).getLocation().getFile() = getAPossiblyRequiredFile() + or + result = getAPublicModule().getAModule() + or + result = getAPublicModule().getAClass() + or + result = getAPublicModule().getStmt(_).(SingletonClass) + } + + /** Gets a parameter from an exported method, which is an input to this gem. */ + DataFlow::ParameterNode getAnInputParameter() { + exists(MethodBase method | method = getAPublicModule().getAMethod() | + result.getParameter() = method.getAParameter() and + method.isPublic() + ) + } + } + + /** Gets a parameter that is an input to a named gem. */ + DataFlow::ParameterNode getALibraryInput() { + exists(GemSpec spec | + exists(spec.getName()) and // we only consider `.gemspec` files that have a name + result = spec.getAnInputParameter() + ) + } +} diff --git a/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpConfiguration.qll b/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpConfiguration.qll index 3c451b15b78..5c08054ac14 100644 --- a/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpConfiguration.qll +++ b/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpConfiguration.qll @@ -7,8 +7,7 @@ private import codeql.ruby.dataflow.internal.DataFlowImplForRegExp private import codeql.ruby.typetracking.TypeTracker private import codeql.ruby.ApiGraphs private import codeql.ruby.dataflow.internal.DataFlowPrivate as DataFlowPrivate -private import codeql.ruby.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl -private import codeql.ruby.dataflow.FlowSummary as FlowSummary +private import codeql.ruby.TaintTracking private import codeql.ruby.frameworks.core.String class RegExpConfiguration extends Configuration { @@ -38,8 +37,8 @@ class RegExpConfiguration extends Configuration { } override predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { - // include taint flow through `String` summaries, - FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, false) and + // include taint flow through `String` summaries + TaintTracking::localTaintStep(nodeFrom, nodeTo) and nodeFrom.(DataFlowPrivate::SummaryNode).getSummarizedCallable() instanceof String::SummarizedCallable or diff --git a/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll index e67ae044a00..20cc8d5b26b 100644 --- a/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll @@ -49,6 +49,9 @@ module CommandInjection { class ShellwordsEscapeAsSanitizer extends Sanitizer { ShellwordsEscapeAsSanitizer() { this = API::getTopLevelMember("Shellwords").getAMethodCall(["escape", "shellescape"]) + or + // The method is also added as `String#shellescape`. + this.(DataFlow::CallNode).getMethodName() = "shellescape" } } } diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll new file mode 100644 index 00000000000..515b563c1e1 --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll @@ -0,0 +1,105 @@ +/** + * Provides default sources, sinks and sanitizers for reasoning about + * shell command constructed from library input vulnerabilities, as + * well as extension points for adding your own. + */ + +private import codeql.ruby.DataFlow +private import codeql.ruby.DataFlow2 +private import codeql.ruby.ApiGraphs +private import codeql.ruby.frameworks.core.Gem::Gem as Gem +private import codeql.ruby.AST as Ast +private import codeql.ruby.Concepts as Concepts + +/** + * Module containing sources, sinks, and sanitizers for shell command constructed from library input. + */ +module UnsafeShellCommandConstruction { + /** A source for shell command constructed from library input vulnerabilities. */ + abstract class Source extends DataFlow::Node { } + + /** An input parameter to a gem seen as a source. */ + private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode { + LibraryInputAsSource() { + this = Gem::getALibraryInput() and + // we exclude arguments named `cmd` or similar, as they seem to execute commands on purpose + not exists(string name | name = super.getName() | + name = ["cmd", "command"] + or + name.regexpMatch(".*(Cmd|Command)$") + ) + } + } + + /** A sink for shell command constructed from library input vulnerabilities. */ + abstract class Sink extends DataFlow::Node { + /** Gets a description of how the string in this sink was constructed. */ + abstract string describe(); + + /** Gets the dataflow node where the string is constructed. */ + DataFlow::Node getStringConstruction() { result = this } + + /** Gets the dataflow node that executed the string as a shell command. */ + abstract DataFlow::Node getCommandExecution(); + } + + /** Holds if the string constructed at `source` is executed at `shellExec` */ + predicate isUsedAsShellCommand(DataFlow::Node source, Concepts::SystemCommandExecution shellExec) { + source = backtrackShellExec(TypeTracker::TypeBackTracker::end(), shellExec) + } + + import codeql.ruby.typetracking.TypeTracker as TypeTracker + + private DataFlow::LocalSourceNode backtrackShellExec( + TypeTracker::TypeBackTracker t, Concepts::SystemCommandExecution shellExec + ) { + t.start() and + result = any(DataFlow::Node n | shellExec.isShellInterpreted(n)).getALocalSource() + or + exists(TypeTracker::TypeBackTracker t2 | + result = backtrackShellExec(t2, shellExec).backtrack(t2, t) + ) + } + + /** + * A string constructed from a string-literal (e.g. `"foo #{sink}"`), + * where the resulting string ends up being executed as a shell command. + */ + class StringInterpolationAsSink extends Sink { + Concepts::SystemCommandExecution s; + Ast::StringLiteral lit; + + StringInterpolationAsSink() { + isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr().getExpr() = lit), s) and + this.asExpr().getExpr() = lit.getComponent(_) + } + + override string describe() { result = "string construction" } + + override DataFlow::Node getCommandExecution() { result = s } + + override DataFlow::Node getStringConstruction() { result.asExpr().getExpr() = lit } + } + + import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat + + /** + * A string constructed from a printf-style call, + * where the resulting string ends up being executed as a shell command. + */ + class TaintedFormatStringAsSink extends Sink { + Concepts::SystemCommandExecution s; + TaintedFormat::PrintfStyleCall call; + + TaintedFormatStringAsSink() { + isUsedAsShellCommand(call, s) and + this = [call.getFormatArgument(_), call.getFormatString()] + } + + override string describe() { result = "formatted string" } + + override DataFlow::Node getCommandExecution() { result = s } + + override DataFlow::Node getStringConstruction() { result = call } + } +} diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll new file mode 100644 index 00000000000..87b602911ae --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll @@ -0,0 +1,35 @@ +/** + * Provides a taint tracking configuration for reasoning about shell command + * constructed from library input vulnerabilities + * + * Note, for performance reasons: only import this file if `Configuration` is needed, + * otherwise `UnsafeShellCommandConstructionCustomizations` should be imported instead. + */ + +import codeql.ruby.DataFlow +import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction +private import codeql.ruby.TaintTracking +private import CommandInjectionCustomizations::CommandInjection as CommandInjection +private import codeql.ruby.dataflow.BarrierGuards + +/** + * A taint-tracking configuration for detecting shell command constructed from library input vulnerabilities. + */ +class Configuration extends TaintTracking::Configuration { + Configuration() { this = "UnsafeShellCommandConstruction" } + + override predicate isSource(DataFlow::Node source) { source instanceof Source } + + override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + override predicate isSanitizer(DataFlow::Node node) { + node instanceof CommandInjection::Sanitizer or // using all sanitizers from `rb/command-injection` + node instanceof StringConstCompareBarrier or + node instanceof StringConstArrayInclusionCallBarrier + } + + // override to require the path doesn't have unmatched return steps + override DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureHasSourceCallContext + } +} diff --git a/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md b/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md new file mode 100644 index 00000000000..fba6a9304cf --- /dev/null +++ b/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* Added a new query, `rb/shell-command-constructed-from-input`, to detect libraries that unsafely construct shell commands from their inputs. diff --git a/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp new file mode 100644 index 00000000000..4231f7cb0b4 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp @@ -0,0 +1,73 @@ + + + +

+ Dynamically constructing a shell command with inputs from exported + functions may inadvertently change the meaning of the shell command. + + Clients using the exported function may use inputs containing + characters that the shell interprets in a special way, for instance + quotes and spaces. + + This can result in the shell command misbehaving, or even + allowing a malicious user to execute arbitrary commands on the system. +

+ + + + + +

+ If possible, provide the dynamic arguments to the shell as an array + to APIs such as system(..) to avoid interpretation by the shell. +

+ +

+ Alternatively, if the shell command must be constructed + dynamically, then add code to ensure that special characters + do not alter the shell command unexpectedly. +

+ + + + +

+ The following example shows a dynamically constructed shell + command that downloads a file from a remote URL. +

+ + + +

+ The shell command will, however, fail to work as intended if the + input contains spaces or other special characters interpreted in a + special way by the shell. +

+ +

+ Even worse, a client might pass in user-controlled + data, not knowing that the input is interpreted as a shell command. + This could allow a malicious user to provide the input http://example.org; cat /etc/passwd + in order to execute the command cat /etc/passwd. +

+ +

+ To avoid such potentially catastrophic behaviors, provide the + input from exported functions as an argument that does not + get interpreted by a shell: +

+ + + + + + +
  • + OWASP: + Command Injection. +
    • + +     +     diff --git a/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql new file mode 100644 index 00000000000..53c71cfc314 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql @@ -0,0 +1,26 @@ +/** + * @name Unsafe shell command constructed from library input + * @description Using externally controlled strings in a command line may allow a malicious + * user to change the meaning of the command. + * @kind path-problem + * @problem.severity error + * @security-severity 6.3 + * @precision high + * @id rb/shell-command-constructed-from-input + * @tags correctness + * security + * external/cwe/cwe-078 + * external/cwe/cwe-088 + * external/cwe/cwe-073 + */ + +import codeql.ruby.security.UnsafeShellCommandConstructionQuery +import DataFlow::PathGraph + +from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode +where + config.hasFlowPath(source, sink) and + sinkNode = sink.getNode() +select sinkNode.getStringConstruction(), source, sink, + "This " + sinkNode.describe() + " which depends on $@ is later used in a $@.", source.getNode(), + "library input", sinkNode.getCommandExecution(), "shell command" diff --git a/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb new file mode 100644 index 00000000000..500bd49e890 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb @@ -0,0 +1,5 @@ +module Utils + def download(path) + system("wget #{path}") # NOT OK + end +end \ No newline at end of file diff --git a/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb new file mode 100644 index 00000000000..cb8730eee09 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb @@ -0,0 +1,6 @@ +module Utils + def download(path) + # using an array to call `system` is safe + system("wget", path) # OK + end +end \ No newline at end of file diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected index 783d414dad6..a50542e8df3 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected @@ -1,4 +1,4 @@ -WARNING: Type BarrierGuard has been deprecated and may be removed in future (barrier-guards.ql:8,3-15) +WARNING: Type BarrierGuard has been deprecated and may be removed in future (barrier-guards.ql:9,3-15) oldStyleBarrierGuards | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | barrier-guards.rb:3:4:3:6 | foo | true | | barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:10:5:10:7 | foo | barrier-guards.rb:9:21:9:23 | foo | true | @@ -20,3 +20,50 @@ newStyleBarrierGuards | barrier-guards.rb:71:5:71:7 | foo | | barrier-guards.rb:83:5:83:7 | foo | | barrier-guards.rb:91:5:91:7 | foo | +controls +| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | true | +| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:6:5:6:7 | foo | false | +| barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:10:5:10:7 | foo | true | +| barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:12:5:12:7 | foo | false | +| barrier-guards.rb:15:4:15:15 | ... != ... | barrier-guards.rb:16:5:16:7 | foo | true | +| barrier-guards.rb:15:4:15:15 | ... != ... | barrier-guards.rb:18:5:18:7 | foo | false | +| barrier-guards.rb:21:8:21:19 | ... == ... | barrier-guards.rb:22:5:22:7 | foo | false | +| barrier-guards.rb:21:8:21:19 | ... == ... | barrier-guards.rb:24:5:24:7 | foo | true | +| barrier-guards.rb:27:8:27:19 | ... != ... | barrier-guards.rb:28:5:28:7 | foo | false | +| barrier-guards.rb:27:8:27:19 | ... != ... | barrier-guards.rb:30:5:30:7 | foo | true | +| barrier-guards.rb:37:4:37:20 | call to include? | barrier-guards.rb:38:5:38:7 | foo | true | +| barrier-guards.rb:37:4:37:20 | call to include? | barrier-guards.rb:40:5:40:7 | foo | false | +| barrier-guards.rb:43:4:43:15 | ... == ... | barrier-guards.rb:44:5:46:5 | self | true | +| barrier-guards.rb:49:4:49:15 | ... == ... | barrier-guards.rb:50:5:53:5 | self | true | +| barrier-guards.rb:56:4:56:15 | ... == ... | barrier-guards.rb:57:5:57:13 | my_lambda | true | +| barrier-guards.rb:70:4:70:21 | call to include? | barrier-guards.rb:71:5:71:7 | foo | true | +| barrier-guards.rb:70:4:70:21 | call to include? | barrier-guards.rb:73:5:73:7 | foo | false | +| barrier-guards.rb:76:4:76:21 | call to include? | barrier-guards.rb:77:5:77:7 | foo | true | +| barrier-guards.rb:76:4:76:21 | call to include? | barrier-guards.rb:79:5:79:7 | foo | false | +| barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:83:5:83:7 | foo | true | +| barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:85:5:85:7 | foo | false | +| barrier-guards.rb:88:4:88:25 | ... == ... | barrier-guards.rb:89:5:89:7 | foo | true | +| barrier-guards.rb:88:4:88:25 | ... == ... | barrier-guards.rb:91:5:91:7 | foo | false | +| barrier-guards.rb:96:4:96:12 | call to condition | barrier-guards.rb:97:5:97:8 | bars | true | +| barrier-guards.rb:100:4:100:21 | call to include? | barrier-guards.rb:101:5:101:7 | foo | true | +| barrier-guards.rb:100:4:100:21 | call to include? | barrier-guards.rb:103:5:103:7 | foo | false | +| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | +| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:9:106:9 | self | false | +| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:109:5:109:8 | bars | false | +| barrier-guards.rb:106:4:106:9 | [false] ... or ... | barrier-guards.rb:109:5:109:8 | bars | false | +| barrier-guards.rb:106:4:106:9 | [true] ... or ... | barrier-guards.rb:107:5:107:7 | foo | true | +| barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | +| barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:109:5:109:8 | bars | false | +| barrier-guards.rb:112:4:112:4 | call to x | barrier-guards.rb:112:4:112:10 | [true] ... and ... | true | +| barrier-guards.rb:112:4:112:4 | call to x | barrier-guards.rb:112:10:112:10 | self | true | +| barrier-guards.rb:112:4:112:4 | call to x | barrier-guards.rb:113:5:113:7 | foo | true | +| barrier-guards.rb:112:4:112:10 | [false] ... and ... | barrier-guards.rb:115:5:115:8 | bars | false | +| barrier-guards.rb:112:4:112:10 | [true] ... and ... | barrier-guards.rb:113:5:113:7 | foo | true | +| barrier-guards.rb:112:10:112:10 | call to y | barrier-guards.rb:112:4:112:10 | [true] ... and ... | true | +| barrier-guards.rb:112:10:112:10 | call to y | barrier-guards.rb:113:5:113:7 | foo | true | +| barrier-guards.rb:118:4:118:8 | [false] not ... | barrier-guards.rb:121:5:121:8 | bars | false | +| barrier-guards.rb:118:4:118:8 | [true] not ... | barrier-guards.rb:119:5:119:7 | foo | true | +| barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:118:4:118:8 | [false] not ... | true | +| barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:118:4:118:8 | [true] not ... | false | +| barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:119:5:119:7 | foo | false | +| barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:121:5:121:8 | bars | true | diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql index 84a962ade35..7998164b6da 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql @@ -2,6 +2,7 @@ import codeql.ruby.dataflow.internal.DataFlowPublic import codeql.ruby.dataflow.BarrierGuards import codeql.ruby.controlflow.CfgNodes import codeql.ruby.controlflow.ControlFlowGraph +import codeql.ruby.controlflow.BasicBlocks import codeql.ruby.DataFlow query predicate oldStyleBarrierGuards( @@ -14,3 +15,10 @@ query predicate newStyleBarrierGuards(DataFlow::Node n) { n instanceof StringConstCompareBarrier or n instanceof StringConstArrayInclusionCallBarrier } + +query predicate controls(CfgNode condition, BasicBlock bb, SuccessorTypes::ConditionalSuccessor s) { + exists(ConditionBlock cb | + cb.controls(bb, s) and + condition = cb.getLastNode() + ) +} diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb index bc9599fd926..5ead7bc04cf 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb @@ -102,3 +102,21 @@ if bars.include?(foo) else foo end + +if x or y then + foo +else + bars +end + +if x and y then + foo +else + bars +end + +if not x then + foo +else + bars +end diff --git a/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.expected b/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.expected index c99abbeacf3..bca8bf08959 100644 --- a/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.expected +++ b/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.expected @@ -5,3 +5,12 @@ constantizeCalls loggerInstantiations | active_support.rb:6:1:6:33 | call to new | | active_support.rb:7:1:7:40 | call to new | +codeExecutions +| active_support.rb:1:1:1:22 | call to constantize | +| active_support.rb:3:1:3:13 | call to constantize | +| active_support.rb:4:1:4:18 | call to safe_constantize | +| active_support.rb:296:5:296:18 | call to try | +| active_support.rb:297:5:297:17 | call to try | +| active_support.rb:298:5:298:19 | call to try! | +| active_support.rb:298:5:298:35 | call to try! | +| active_support.rb:299:5:299:18 | call to try! | diff --git a/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.ql b/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.ql index 149113851be..239a47434e2 100644 --- a/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.ql +++ b/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.ql @@ -1,9 +1,12 @@ import codeql.ruby.frameworks.ActiveSupport import codeql.ruby.DataFlow import codeql.ruby.frameworks.stdlib.Logger +import codeql.ruby.Concepts query DataFlow::Node constantizeCalls(ActiveSupport::CoreExtensions::String::Constantize c) { result = c.getCode() } query predicate loggerInstantiations(Logger::LoggerInstantiation l) { any() } + +query predicate codeExecutions(CodeExecution c) { any() } diff --git a/ruby/ql/test/library-tests/frameworks/active_support/active_support.rb b/ruby/ql/test/library-tests/frameworks/active_support/active_support.rb index 023f9724ce8..fe0256080d1 100644 --- a/ruby/ql/test/library-tests/frameworks/active_support/active_support.rb +++ b/ruby/ql/test/library-tests/frameworks/active_support/active_support.rb @@ -290,3 +290,11 @@ def m_deep_dup x = source "a" sink x.deep_dup # $hasValueFlow=a end + +def m_try(method) + x = "abc" + x.try(:upcase) + x.try(method) + x.try!(:upcase).try!(:downcase) + x.try!(method) +end diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected index b2e4990daec..1d00f293dfd 100644 --- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected +++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected @@ -15,6 +15,8 @@ edges | CommandInjection.rb:81:20:81:25 | **args : | CommandInjection.rb:82:22:82:25 | args : | | CommandInjection.rb:82:22:82:25 | args : | CommandInjection.rb:82:22:82:37 | ...[...] : | | CommandInjection.rb:82:22:82:37 | ...[...] : | CommandInjection.rb:82:14:82:39 | "echo #{...}" | +| CommandInjection.rb:94:16:94:21 | call to params : | CommandInjection.rb:94:16:94:28 | ...[...] : | +| CommandInjection.rb:94:16:94:28 | ...[...] : | CommandInjection.rb:95:16:95:28 | "cat #{...}" | nodes | CommandInjection.rb:6:15:6:20 | call to params : | semmle.label | call to params : | | CommandInjection.rb:6:15:6:26 | ...[...] : | semmle.label | ...[...] : | @@ -37,6 +39,9 @@ nodes | CommandInjection.rb:82:14:82:39 | "echo #{...}" | semmle.label | "echo #{...}" | | CommandInjection.rb:82:22:82:25 | args : | semmle.label | args : | | CommandInjection.rb:82:22:82:37 | ...[...] : | semmle.label | ...[...] : | +| CommandInjection.rb:94:16:94:21 | call to params : | semmle.label | call to params : | +| CommandInjection.rb:94:16:94:28 | ...[...] : | semmle.label | ...[...] : | +| CommandInjection.rb:95:16:95:28 | "cat #{...}" | semmle.label | "cat #{...}" | subpaths #select | CommandInjection.rb:7:10:7:15 | #{...} | CommandInjection.rb:6:15:6:20 | call to params : | CommandInjection.rb:7:10:7:15 | #{...} | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value | @@ -51,3 +56,4 @@ subpaths | CommandInjection.rb:65:14:65:29 | "echo #{...}" | CommandInjection.rb:64:18:64:23 | number : | CommandInjection.rb:65:14:65:29 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:64:18:64:23 | number | user-provided value | | CommandInjection.rb:73:14:73:34 | "echo #{...}" | CommandInjection.rb:72:23:72:33 | blah_number : | CommandInjection.rb:73:14:73:34 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:72:23:72:33 | blah_number | user-provided value | | CommandInjection.rb:82:14:82:39 | "echo #{...}" | CommandInjection.rb:81:20:81:25 | **args : | CommandInjection.rb:82:14:82:39 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:81:20:81:25 | **args | user-provided value | +| CommandInjection.rb:95:16:95:28 | "cat #{...}" | CommandInjection.rb:94:16:94:21 | call to params : | CommandInjection.rb:95:16:95:28 | "cat #{...}" | This command depends on a $@. | CommandInjection.rb:94:16:94:21 | call to params | user-provided value | diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb index ed9750128cc..75219e2131c 100644 --- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb +++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb @@ -88,3 +88,13 @@ module Types end end end + +class Foo < ActionController::Base + def create + file = params[:file] + system("cat #{file}") + # .shellescape + system("cat #{file.shellescape}") # OK, because file is shell escaped + + end +end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected new file mode 100644 index 00000000000..964fb4433ce --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected @@ -0,0 +1,44 @@ +edges +| impl/sub/notImported.rb:2:12:2:17 | target : | impl/sub/notImported.rb:3:19:3:27 | #{...} | +| impl/sub/other2.rb:2:12:2:17 | target : | impl/sub/other2.rb:3:19:3:27 | #{...} | +| impl/sub/other.rb:2:12:2:17 | target : | impl/sub/other.rb:3:19:3:27 | #{...} | +| impl/unsafeShell.rb:2:12:2:17 | target : | impl/unsafeShell.rb:3:19:3:27 | #{...} | +| impl/unsafeShell.rb:6:12:6:12 | x : | impl/unsafeShell.rb:7:32:7:32 | x | +| impl/unsafeShell.rb:15:47:15:64 | innocent_file_path : | impl/unsafeShell.rb:20:21:20:41 | #{...} | +| impl/unsafeShell.rb:23:15:23:23 | file_path : | impl/unsafeShell.rb:26:19:26:30 | #{...} | +| impl/unsafeShell.rb:33:12:33:17 | target : | impl/unsafeShell.rb:34:19:34:27 | #{...} | +| impl/unsafeShell.rb:37:10:37:10 | x : | impl/unsafeShell.rb:38:19:38:22 | #{...} | +| impl/unsafeShell.rb:47:16:47:21 | target : | impl/unsafeShell.rb:48:19:48:27 | #{...} | +nodes +| impl/sub/notImported.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} | +| impl/sub/other2.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/sub/other2.rb:3:19:3:27 | #{...} | semmle.label | #{...} | +| impl/sub/other.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/sub/other.rb:3:19:3:27 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/unsafeShell.rb:3:19:3:27 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:6:12:6:12 | x : | semmle.label | x : | +| impl/unsafeShell.rb:7:32:7:32 | x | semmle.label | x | +| impl/unsafeShell.rb:15:47:15:64 | innocent_file_path : | semmle.label | innocent_file_path : | +| impl/unsafeShell.rb:20:21:20:41 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:23:15:23:23 | file_path : | semmle.label | file_path : | +| impl/unsafeShell.rb:26:19:26:30 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:33:12:33:17 | target : | semmle.label | target : | +| impl/unsafeShell.rb:34:19:34:27 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:37:10:37:10 | x : | semmle.label | x : | +| impl/unsafeShell.rb:38:19:38:22 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:47:16:47:21 | target : | semmle.label | target : | +| impl/unsafeShell.rb:48:19:48:27 | #{...} | semmle.label | #{...} | +subpaths +#select +| impl/sub/notImported.rb:3:14:3:28 | "cat #{...}" | impl/sub/notImported.rb:2:12:2:17 | target : | impl/sub/notImported.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/notImported.rb:2:12:2:17 | target | library input | impl/sub/notImported.rb:3:5:3:34 | call to popen | shell command | +| impl/sub/other2.rb:3:14:3:28 | "cat #{...}" | impl/sub/other2.rb:2:12:2:17 | target : | impl/sub/other2.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/other2.rb:2:12:2:17 | target | library input | impl/sub/other2.rb:3:5:3:34 | call to popen | shell command | +| impl/sub/other.rb:3:14:3:28 | "cat #{...}" | impl/sub/other.rb:2:12:2:17 | target : | impl/sub/other.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/other.rb:2:12:2:17 | target | library input | impl/sub/other.rb:3:5:3:34 | call to popen | shell command | +| impl/unsafeShell.rb:3:14:3:28 | "cat #{...}" | impl/unsafeShell.rb:2:12:2:17 | target : | impl/unsafeShell.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:2:12:2:17 | target | library input | impl/unsafeShell.rb:3:5:3:34 | call to popen | shell command | +| impl/unsafeShell.rb:7:14:7:33 | call to sprintf | impl/unsafeShell.rb:6:12:6:12 | x : | impl/unsafeShell.rb:7:32:7:32 | x | This formatted string which depends on $@ is later used in a $@. | impl/unsafeShell.rb:6:12:6:12 | x | library input | impl/unsafeShell.rb:8:5:8:25 | call to popen | shell command | +| impl/unsafeShell.rb:20:14:20:42 | "which #{...}" | impl/unsafeShell.rb:15:47:15:64 | innocent_file_path : | impl/unsafeShell.rb:20:21:20:41 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:15:47:15:64 | innocent_file_path | library input | impl/unsafeShell.rb:20:5:20:48 | call to popen | shell command | +| impl/unsafeShell.rb:26:14:26:31 | "cat #{...}" | impl/unsafeShell.rb:23:15:23:23 | file_path : | impl/unsafeShell.rb:26:19:26:30 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:23:15:23:23 | file_path | library input | impl/unsafeShell.rb:26:5:26:37 | call to popen | shell command | +| impl/unsafeShell.rb:34:14:34:28 | "cat #{...}" | impl/unsafeShell.rb:33:12:33:17 | target : | impl/unsafeShell.rb:34:19:34:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:33:12:33:17 | target | library input | impl/unsafeShell.rb:34:5:34:34 | call to popen | shell command | +| impl/unsafeShell.rb:38:14:38:23 | "cat #{...}" | impl/unsafeShell.rb:37:10:37:10 | x : | impl/unsafeShell.rb:38:19:38:22 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:37:10:37:10 | x | library input | impl/unsafeShell.rb:38:5:38:29 | call to popen | shell command | +| impl/unsafeShell.rb:48:14:48:28 | "cat #{...}" | impl/unsafeShell.rb:47:16:47:21 | target : | impl/unsafeShell.rb:48:19:48:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:47:16:47:21 | target | library input | impl/unsafeShell.rb:48:5:48:34 | call to popen | shell command | diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref new file mode 100644 index 00000000000..99292da7663 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref @@ -0,0 +1 @@ +queries/security/cwe-078/UnsafeShellCommandConstruction.ql \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb new file mode 100644 index 00000000000..0a385f5f6bc --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb @@ -0,0 +1,6 @@ +class Foobar + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK - everything assumed to be imported... + end +end + \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb new file mode 100644 index 00000000000..22eaa13bcc0 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb @@ -0,0 +1,7 @@ +class Foobar + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK + end +end + +require 'sub/other2' \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb new file mode 100644 index 00000000000..007dae343ff --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb @@ -0,0 +1,5 @@ +class Foobar + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK + end +end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb new file mode 100644 index 00000000000..f1fd9f685a8 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb @@ -0,0 +1,50 @@ +class Foobar + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK + end + + def foo2(x) + format = sprintf("cat %s", x) # NOT OK + IO.popen(format, "w") + end + + def fileRead1(path) + File.read(path) # OK + end + + def my_exec(cmd, command, myCmd, myCommand, innocent_file_path) + IO.popen("which #{cmd}", "w") # OK - the parameter is named `cmd`, so it's meant to be a command + IO.popen("which #{command}", "w") # OK - the parameter is named `command`, so it's meant to be a command + IO.popen("which #{myCmd}", "w") # OK - the parameter is named `myCmd`, so it's meant to be a command + IO.popen("which #{myCommand}", "w") # OK - the parameter is named `myCommand`, so it's meant to be a command + IO.popen("which #{innocent_file_path}", "w") # NOT OK - the parameter is named `innocent_file_path`, so it's not meant to be a command + end + + def escaped(file_path) + IO.popen("cat #{file_path.shellescape}", "w") # OK - the parameter is escaped + + IO.popen("cat #{file_path}", "w") # NOT OK - the parameter is not escaped + end +end + +require File.join(File.dirname(__FILE__), 'sub', 'other') + +class Foobar2 + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK + end + + def id(x) + IO.popen("cat #{x}", "w") # NOT OK - the parameter is not a constant. + return x + end + + def thisIsSafe() + IO.popen("echo #{id('foo')}", "w") # OK - only using constants. + end + + # class methods + def self.foo(target) + IO.popen("cat #{target}", "w") # NOT OK + end +end diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec new file mode 100644 index 00000000000..545bc14a7a6 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec @@ -0,0 +1,5 @@ +Gem::Specification.new do |s| + s.name = 'unsafe-shell' + s.require_path = "impl" + end + \ No newline at end of file diff --git a/swift/codegen/generators/cppgen.py b/swift/codegen/generators/cppgen.py index 0ea5fbec2b8..45f0d98949b 100644 --- a/swift/codegen/generators/cppgen.py +++ b/swift/codegen/generators/cppgen.py @@ -12,15 +12,14 @@ Each class in the schema gets a corresponding `struct` in `TrapClasses.h`, where """ import functools -import pathlib -from typing import Dict +import typing import inflection from swift.codegen.lib import cpp, schema -def _get_type(t: str) -> str: +def _get_type(t: str, add_or_none_except: typing.Optional[str] = None) -> str: if t is None: # this is a predicate return "bool" @@ -29,11 +28,15 @@ def _get_type(t: str) -> str: if t == "boolean": return "bool" if t[0].isupper(): - return f"TrapLabel<{t}Tag>" + if add_or_none_except is not None and t != add_or_none_except: + suffix = "OrNone" + else: + suffix = "" + return f"TrapLabel<{t}{suffix}Tag>" return t -def _get_field(cls: schema.Class, p: schema.Property) -> cpp.Field: +def _get_field(cls: schema.Class, p: schema.Property, add_or_none_except: typing.Optional[str] = None) -> cpp.Field: trap_name = None if not p.is_single: trap_name = inflection.camelize(f"{cls.name}_{p.name}") @@ -41,7 +44,7 @@ def _get_field(cls: schema.Class, p: schema.Property) -> cpp.Field: trap_name = inflection.pluralize(trap_name) args = dict( field_name=p.name + ("_" if p.name in cpp.cpp_keywords else ""), - type=_get_type(p.type), + base_type=_get_type(p.type, add_or_none_except), is_optional=p.is_optional, is_repeated=p.is_repeated, is_predicate=p.is_predicate, @@ -52,8 +55,13 @@ def _get_field(cls: schema.Class, p: schema.Property) -> cpp.Field: class Processor: - def __init__(self, data: Dict[str, schema.Class]): - self._classmap = data + def __init__(self, data: schema.Schema): + self._classmap = data.classes + if data.null: + root_type = next(iter(data.classes)) + self._add_or_none_except = root_type + else: + self._add_or_none_except = None @functools.lru_cache(maxsize=None) def _get_class(self, name: str) -> cpp.Class: @@ -64,7 +72,10 @@ class Processor: return cpp.Class( name=name, bases=[self._get_class(b) for b in cls.bases], - fields=[_get_field(cls, p) for p in cls.properties if "cpp_skip" not in p.pragmas], + fields=[ + _get_field(cls, p, self._add_or_none_except) + for p in cls.properties if "cpp_skip" not in p.pragmas + ], final=not cls.derived, trap_name=trap_name, ) @@ -78,8 +89,8 @@ class Processor: def generate(opts, renderer): assert opts.cpp_output - processor = Processor(schema.load_file(opts.schema).classes) + processor = Processor(schema.load_file(opts.schema)) out = opts.cpp_output for dir, classes in processor.get_classes().items(): renderer.render(cpp.ClassList(classes, opts.schema, - include_parent=bool(dir)), out / dir / "TrapClasses") + include_parent=bool(dir)), out / dir / "TrapClasses") diff --git a/swift/codegen/generators/dbschemegen.py b/swift/codegen/generators/dbschemegen.py index 5e04b412646..b750a9d6e12 100755 --- a/swift/codegen/generators/dbschemegen.py +++ b/swift/codegen/generators/dbschemegen.py @@ -13,6 +13,7 @@ Moreover: as columns The type hierarchy will be translated to corresponding `union` declarations. """ +import typing import inflection @@ -23,14 +24,21 @@ from typing import Set, List log = logging.getLogger(__name__) -def dbtype(typename): - """ translate a type to a dbscheme counterpart, using `@lower_underscore` format for classes """ +def dbtype(typename: str, add_or_none_except: typing.Optional[str] = None) -> str: + """ translate a type to a dbscheme counterpart, using `@lower_underscore` format for classes. + For class types, appends an underscore followed by `null` if provided + """ if typename[0].isupper(): - return "@" + inflection.underscore(typename) + underscored = inflection.underscore(typename) + if add_or_none_except is not None and typename != add_or_none_except: + suffix = "_or_none" + else: + suffix = "" + return f"@{underscored}{suffix}" return typename -def cls_to_dbscheme(cls: schema.Class): +def cls_to_dbscheme(cls: schema.Class, add_or_none_except: typing.Optional[str] = None): """ Yield all dbscheme entities needed to model class `cls` """ if cls.derived: yield Union(dbtype(cls.name), (dbtype(c) for c in cls.derived)) @@ -48,7 +56,7 @@ def cls_to_dbscheme(cls: schema.Class): columns=[ Column("id", type=dbtype(cls.name), binding=binding), ] + [ - Column(f.name, dbtype(f.type)) for f in cls.properties if f.is_single + Column(f.name, dbtype(f.type, add_or_none_except)) for f in cls.properties if f.is_single ], dir=dir, ) @@ -61,7 +69,7 @@ def cls_to_dbscheme(cls: schema.Class): columns=[ Column("id", type=dbtype(cls.name)), Column("index", type="int"), - Column(inflection.singularize(f.name), dbtype(f.type)), + Column(inflection.singularize(f.name), dbtype(f.type, add_or_none_except)), ], dir=dir, ) @@ -71,7 +79,7 @@ def cls_to_dbscheme(cls: schema.Class): name=inflection.tableize(f"{cls.name}_{f.name}"), columns=[ Column("id", type=dbtype(cls.name)), - Column(f.name, dbtype(f.type)), + Column(f.name, dbtype(f.type, add_or_none_except)), ], dir=dir, ) @@ -87,7 +95,17 @@ def cls_to_dbscheme(cls: schema.Class): def get_declarations(data: schema.Schema): - return [d for cls in data.classes.values() for d in cls_to_dbscheme(cls)] + add_or_none_except = data.root_class.name if data.null else None + declarations = [d for cls in data.classes.values() for d in cls_to_dbscheme(cls, add_or_none_except)] + if data.null: + property_classes = { + prop.type for cls in data.classes.values() for prop in cls.properties + if cls.name != data.null and prop.type and prop.type[0].isupper() + } + declarations += [ + Union(dbtype(t, data.null), [dbtype(t), dbtype(data.null)]) for t in sorted(property_classes) + ] + return declarations def get_includes(data: schema.Schema, include_dir: pathlib.Path, swift_dir: pathlib.Path): diff --git a/swift/codegen/generators/qlgen.py b/swift/codegen/generators/qlgen.py index de2cddd981b..2172a9517e6 100755 --- a/swift/codegen/generators/qlgen.py +++ b/swift/codegen/generators/qlgen.py @@ -147,7 +147,7 @@ def get_ql_property(cls: schema.Class, prop: schema.Property, prev_child: str = return ql.Property(**args) -def get_ql_class(cls: schema.Class, lookup: typing.Dict[str, schema.Class]): +def get_ql_class(cls: schema.Class): pragmas = {k: True for k in cls.pragmas if k.startswith("ql")} prev_child = "" properties = [] @@ -314,7 +314,7 @@ def generate(opts, renderer): data = schema.load_file(input) - classes = {name: get_ql_class(cls, data.classes) for name, cls in data.classes.items()} + classes = {name: get_ql_class(cls) for name, cls in data.classes.items()} if not classes: raise NoClasses root = next(iter(classes.values())) diff --git a/swift/codegen/generators/trapgen.py b/swift/codegen/generators/trapgen.py index 22c46e6e146..f01203e9272 100755 --- a/swift/codegen/generators/trapgen.py +++ b/swift/codegen/generators/trapgen.py @@ -41,10 +41,10 @@ def get_cpp_type(schema_type: str): def get_field(c: dbscheme.Column): args = { "field_name": c.schema_name, - "type": c.type, + "base_type": c.type, } args.update(cpp.get_field_override(c.schema_name)) - args["type"] = get_cpp_type(args["type"]) + args["base_type"] = get_cpp_type(args["base_type"]) return cpp.Field(**args) diff --git a/swift/codegen/lib/cpp.py b/swift/codegen/lib/cpp.py index acebe616d7b..27bac12231b 100644 --- a/swift/codegen/lib/cpp.py +++ b/swift/codegen/lib/cpp.py @@ -16,7 +16,7 @@ cpp_keywords = {"alignas", "alignof", "and", "and_eq", "asm", "atomic_cancel", " "xor", "xor_eq"} _field_overrides = [ - (re.compile(r"(start|end)_(line|column)|(.*_)?index|width|num_.*"), {"type": "unsigned"}), + (re.compile(r"(start|end)_(line|column)|(.*_)?index|width|num_.*"), {"base_type": "unsigned"}), (re.compile(r"(.*)_"), lambda m: {"field_name": m[1]}), ] @@ -32,7 +32,7 @@ def get_field_override(field: str): @dataclass class Field: field_name: str - type: str + base_type: str is_optional: bool = False is_repeated: bool = False is_predicate: bool = False @@ -40,13 +40,18 @@ class Field: first: bool = False def __post_init__(self): - if self.is_optional: - self.type = f"std::optional<{self.type}>" - if self.is_repeated: - self.type = f"std::vector<{self.type}>" if self.field_name in cpp_keywords: self.field_name += "_" + @property + def type(self) -> str: + type = self.base_type + if self.is_optional: + type = f"std::optional<{type}>" + if self.is_repeated: + type = f"std::vector<{type}>" + return type + # using @property breaks pystache internals here def get_streamer(self): if self.type == "std::string": @@ -60,6 +65,10 @@ class Field: def is_single(self): return not (self.is_optional or self.is_repeated or self.is_predicate) + @property + def is_label(self): + return self.base_type.startswith("TrapLabel<") + @dataclass class Trap: diff --git a/swift/codegen/lib/schema/defs.py b/swift/codegen/lib/schema/defs.py index 6470fb789a1..9657d587f85 100644 --- a/swift/codegen/lib/schema/defs.py +++ b/swift/codegen/lib/schema/defs.py @@ -115,6 +115,8 @@ child = _ChildModifier() doc = _DocModifier desc = _DescModifier +use_for_null = _annotate(null=True) + qltest = _Namespace( skip=_Pragma("qltest_skip"), collapse_hierarchy=_Pragma("qltest_collapse_hierarchy"), diff --git a/swift/codegen/lib/schema/schema.py b/swift/codegen/lib/schema/schema.py index e92af734a1c..72b56ad19af 100644 --- a/swift/codegen/lib/schema/schema.py +++ b/swift/codegen/lib/schema/schema.py @@ -55,6 +55,14 @@ class Property: def is_predicate(self) -> bool: return self.kind == self.Kind.PREDICATE + @property + def has_class_type(self) -> bool: + return bool(self.type) and self.type[0].isupper() + + @property + def has_builtin_type(self) -> bool: + return bool(self.type) and self.type[0].islower() + SingleProperty = functools.partial(Property, Property.Kind.SINGLE) OptionalProperty = functools.partial(Property, Property.Kind.OPTIONAL) @@ -104,6 +112,16 @@ class Class: class Schema: classes: Dict[str, Class] = field(default_factory=dict) includes: Set[str] = field(default_factory=set) + null: Optional[str] = None + + @property + def root_class(self): + # always the first in the dictionary + return next(iter(self.classes.values())) + + @property + def null_class(self): + return self.classes[self.null] if self.null else None predicate_marker = object() @@ -195,6 +213,8 @@ def _get_class(cls: type) -> Class: raise Error(f"Class name must be capitalized, found {cls.__name__}") if len({b._group for b in cls.__bases__ if hasattr(b, "_group")}) > 1: raise Error(f"Bases with mixed groups for {cls.__name__}") + if any(getattr(b, "_null", False) for b in cls.__bases__): + raise Error(f"Null class cannot be derived") return Class(name=cls.__name__, bases=[b.__name__ for b in cls.__bases__ if b is not object], derived={d.__name__ for d in cls.__subclasses__()}, @@ -233,6 +253,7 @@ def load(m: types.ModuleType) -> Schema: known = {"int", "string", "boolean"} known.update(n for n in m.__dict__ if not n.startswith("__")) import swift.codegen.lib.schema.defs as defs + null = None for name, data in m.__dict__.items(): if hasattr(defs, name): continue @@ -247,8 +268,13 @@ def load(m: types.ModuleType) -> Schema: f"Only one root class allowed, found second root {name}") cls.check_types(known) classes[name] = cls + if getattr(data, "_null", False): + if null is not None: + raise Error(f"Null class {null} already defined, second null class {name} not allowed") + null = name + cls.is_null_class = True - return Schema(includes=includes, classes=_toposort_classes_by_group(classes)) + return Schema(includes=includes, classes=_toposort_classes_by_group(classes), null=null) def load_file(path: pathlib.Path) -> Schema: diff --git a/swift/codegen/templates/cpp_classes_h.mustache b/swift/codegen/templates/cpp_classes_h.mustache index 5176a8209de..a4a22170b2f 100644 --- a/swift/codegen/templates/cpp_classes_h.mustache +++ b/swift/codegen/templates/cpp_classes_h.mustache @@ -17,6 +17,8 @@ namespace codeql { {{#classes}} struct {{name}}{{#has_bases}} : {{#bases}}{{^first}}, {{/first}}{{ref.name}}{{/bases}}{{/has_bases}} { + static constexpr const char* NAME = "{{name}}"; + {{#final}} explicit {{name}}(TrapLabel<{{name}}Tag> id) : id{id} {} @@ -33,6 +35,41 @@ struct {{name}}{{#has_bases}} : {{#bases}}{{^first}}, {{/first}}{{ref.name}}{{/b } {{/final}} + {{^final}} + protected: + {{/final}} + template + void forEachLabel(F f) { + {{#final}} + f("id", -1, id); + {{/final}} + {{#bases}} + {{ref.name}}::forEachLabel(f); + {{/bases}} + {{#fields}} + {{#is_label}} + {{#is_repeated}} + for (auto i = 0u; i < {{field_name}}.size(); ++i) { + {{#is_optional}} + if ({{field_name}}[i]) f("{{field_name}}", i, *{{field_name}}[i]); + {{/is_optional}} + {{^is_optional}} + f("{{field_name}}", i, {{field_name}}[i]); + {{/is_optional}} + } + {{/is_repeated}} + {{^is_repeated}} + {{#is_optional}} + if ({{field_name}}) f("{{field_name}}", -1, *{{field_name}}); + {{/is_optional}} + {{^is_optional}} + f("{{field_name}}", -1, {{field_name}}); + {{/is_optional}} + {{/is_repeated}} + {{/is_label}} + {{/fields}} + } + protected: void emit({{^final}}TrapLabel<{{name}}Tag> id, {{/final}}std::ostream& out) const; }; diff --git a/swift/codegen/templates/trap_traps_h.mustache b/swift/codegen/templates/trap_traps_h.mustache index 297acb56d87..d3bf7769bd7 100644 --- a/swift/codegen/templates/trap_traps_h.mustache +++ b/swift/codegen/templates/trap_traps_h.mustache @@ -14,12 +14,24 @@ namespace codeql { // {{table_name}} struct {{name}}Trap { -{{#fields}} + static constexpr const char* NAME = "{{name}}Trap"; + + {{#fields}} {{type}} {{field_name}}{}; -{{/fields}} + {{/fields}} + + template + void forEachLabel(F f) { + {{#fields}} + {{#is_label}} + f("{{field_name}}", -1, {{field_name}}); + {{/is_label}} + {{/fields}} + } }; std::ostream &operator<<(std::ostream &out, const {{name}}Trap &e); + {{#id}} namespace detail { diff --git a/swift/codegen/test/test_cppgen.py b/swift/codegen/test/test_cppgen.py index c2214d7775a..df4925cb70e 100644 --- a/swift/codegen/test/test_cppgen.py +++ b/swift/codegen/test/test_cppgen.py @@ -80,6 +80,25 @@ def test_class_with_field(generate, type, expected, property_cls, optional, repe ] +def test_class_field_with_null(generate, input): + input.null = "Null" + a = cpp.Class(name="A") + assert generate([ + schema.Class(name="A", derived={"B"}), + schema.Class(name="B", bases=["A"], properties=[ + schema.SingleProperty("x", "A"), + schema.SingleProperty("y", "B"), + ]) + ]) == [ + a, + cpp.Class(name="B", bases=[a], final=True, trap_name="Bs", + fields=[ + cpp.Field("x", "TrapLabel"), + cpp.Field("y", "TrapLabel"), + ]), + ] + + def test_class_with_predicate(generate): assert generate([ schema.Class(name="MyClass", properties=[ diff --git a/swift/codegen/test/test_dbschemegen.py b/swift/codegen/test/test_dbschemegen.py index a302ba8f155..0fb71fbc35b 100644 --- a/swift/codegen/test/test_dbschemegen.py +++ b/swift/codegen/test/test_dbschemegen.py @@ -18,8 +18,9 @@ def dir_param(request): @pytest.fixture def generate(opts, input, renderer): - def func(classes): + def func(classes, null=None): input.classes = {cls.name: cls for cls in classes} + input.null = null (out, data), = run_generation(dbschemegen.generate, opts, renderer).items() assert out is opts.dbscheme return data @@ -359,5 +360,114 @@ def test_class_with_derived_and_repeated_property(generate, dir_param): ) +def test_null_class(generate): + assert generate([ + schema.Class( + name="Base", + derived={"W", "X", "Y", "Z", "Null"}, + ), + schema.Class( + name="W", + bases=["Base"], + properties=[ + schema.SingleProperty("w", "W"), + schema.SingleProperty("x", "X"), + schema.OptionalProperty("y", "Y"), + schema.RepeatedProperty("z", "Z"), + ] + ), + schema.Class( + name="X", + bases=["Base"], + ), + schema.Class( + name="Y", + bases=["Base"], + ), + schema.Class( + name="Z", + bases=["Base"], + ), + schema.Class( + name="Null", + bases=["Base"], + ), + ], null="Null") == dbscheme.Scheme( + src=schema_file, + includes=[], + declarations=[ + dbscheme.Union( + lhs="@base", + rhs=["@null", "@w", "@x", "@y", "@z"], + ), + dbscheme.Table( + name="ws", + columns=[ + dbscheme.Column('id', '@w', binding=True), + dbscheme.Column('w', '@w_or_none'), + dbscheme.Column('x', '@x_or_none'), + ], + ), + dbscheme.Table( + name="w_ies", + keyset=dbscheme.KeySet(["id"]), + columns=[ + dbscheme.Column('id', '@w'), + dbscheme.Column('y', '@y_or_none'), + ], + ), + dbscheme.Table( + name="w_zs", + keyset=dbscheme.KeySet(["id", "index"]), + columns=[ + dbscheme.Column('id', '@w'), + dbscheme.Column('index', 'int'), + dbscheme.Column('z', '@z_or_none'), + ], + ), + dbscheme.Table( + name="xes", + columns=[ + dbscheme.Column('id', '@x', binding=True), + ], + ), + dbscheme.Table( + name="ys", + columns=[ + dbscheme.Column('id', '@y', binding=True), + ], + ), + dbscheme.Table( + name="zs", + columns=[ + dbscheme.Column('id', '@z', binding=True), + ], + ), + dbscheme.Table( + name="nulls", + columns=[ + dbscheme.Column('id', '@null', binding=True), + ], + ), + dbscheme.Union( + lhs="@w_or_none", + rhs=["@w", "@null"], + ), + dbscheme.Union( + lhs="@x_or_none", + rhs=["@x", "@null"], + ), + dbscheme.Union( + lhs="@y_or_none", + rhs=["@y", "@null"], + ), + dbscheme.Union( + lhs="@z_or_none", + rhs=["@z", "@null"], + ), + ], + ) + + if __name__ == '__main__': sys.exit(pytest.main([__file__] + sys.argv[1:])) diff --git a/swift/codegen/test/test_schema.py b/swift/codegen/test/test_schema.py index 457b60ce680..98f3c8aee41 100644 --- a/swift/codegen/test/test_schema.py +++ b/swift/codegen/test/test_schema.py @@ -13,6 +13,8 @@ def test_empty_schema(): assert data.classes == {} assert data.includes == set() + assert data.null is None + assert data.null_class is None def test_one_empty_class(): @@ -24,6 +26,7 @@ def test_one_empty_class(): assert data.classes == { 'MyClass': schema.Class('MyClass'), } + assert data.root_class is data.classes['MyClass'] def test_two_empty_classes(): @@ -39,6 +42,7 @@ def test_two_empty_classes(): 'MyClass1': schema.Class('MyClass1', derived={'MyClass2'}), 'MyClass2': schema.Class('MyClass2', bases=['MyClass1']), } + assert data.root_class is data.classes['MyClass1'] def test_no_external_bases(): @@ -452,7 +456,8 @@ def test_property_docstring_newline(): property.""") assert data.classes == { - 'A': schema.Class('A', properties=[schema.SingleProperty('x', 'int', description=["very important", "property."])]) + 'A': schema.Class('A', + properties=[schema.SingleProperty('x', 'int', description=["very important", "property."])]) } @@ -566,5 +571,54 @@ def test_class_default_doc_name(): } +def test_null_class(): + @schema.load + class data: + class Root: + pass + + @defs.use_for_null + class Null(Root): + pass + + assert data.classes == { + 'Root': schema.Class('Root', derived={'Null'}), + 'Null': schema.Class('Null', bases=['Root']), + } + assert data.null == 'Null' + assert data.null_class is data.classes[data.null] + + +def test_null_class_cannot_be_derived(): + with pytest.raises(schema.Error): + @schema.load + class data: + class Root: + pass + + @defs.use_for_null + class Null(Root): + pass + + class Impossible(Null): + pass + + +def test_null_class_cannot_be_defined_multiple_times(): + with pytest.raises(schema.Error): + @schema.load + class data: + class Root: + pass + + @defs.use_for_null + class Null1(Root): + pass + + @defs.use_for_null + class Null2(Root): + pass + + if __name__ == '__main__': sys.exit(pytest.main([__file__] + sys.argv[1:])) diff --git a/swift/extractor/infra/SwiftDispatcher.h b/swift/extractor/infra/SwiftDispatcher.h index 127fb7c1c3e..c26a66b65e9 100644 --- a/swift/extractor/infra/SwiftDispatcher.h +++ b/swift/extractor/infra/SwiftDispatcher.h @@ -60,20 +60,39 @@ class SwiftDispatcher { } template - void emit(const Entry& entry) { - trap.emit(entry); + void emit(Entry&& entry) { + bool valid = true; + entry.forEachLabel([&valid, &entry, this](const char* field, int index, auto& label) { + using Label = std::remove_reference_t; + if (!label.valid()) { + std::cerr << entry.NAME << " has undefined " << field; + if (index >= 0) { + std::cerr << '[' << index << ']'; + } + if constexpr (std::is_base_of_v) { + std::cerr << ", replacing with unspecified element\n"; + label = emitUnspecified(idOf(entry), field, index); + } else { + std::cerr << ", skipping emission\n"; + valid = false; + } + } + }); + if (valid) { + trap.emit(entry); + } } template - void emit(const std::optional& entry) { + void emit(std::optional&& entry) { if (entry) { - emit(*entry); + emit(std::move(*entry)); } } template - void emit(const std::variant& entry) { - std::visit([this](const auto& e) { this->emit(e); }, entry); + void emit(std::variant&& entry) { + std::visit([this](auto&& e) { this->emit(std::move(e)); }, std::move(entry)); } // This is a helper method to emit TRAP entries for AST nodes that we don't fully support yet. @@ -88,13 +107,39 @@ class SwiftDispatcher { emit(ElementIsUnknownTrap{label}); } + TrapLabel emitUnspecified(std::optional>&& parent, + const char* property, + int index) { + UnspecifiedElement entry{trap.createLabel()}; + entry.error = "element was unspecified by the extractor"; + entry.parent = std::move(parent); + entry.property = property; + if (index >= 0) { + entry.index = index; + } + trap.emit(entry); + return entry.id; + } + + template + std::optional> idOf(const E& entry) { + if constexpr (HasId::value) { + return entry.id; + } else { + return std::nullopt; + } + } + // This method gives a TRAP label for already emitted AST node. // If the AST node was not emitted yet, then the emission is dispatched to a corresponding // visitor (see `visit(T *)` methods below). template >* = nullptr> TrapLabelOf fetchLabel(const E& e, Args&&... args) { if constexpr (std::is_constructible_v) { - assert(e && "fetching a label on a null entity, maybe fetchOptionalLabel is to be used?"); + if (!e) { + // this will be treated on emission + return undefined_label; + } } // this is required so we avoid any recursive loop: a `fetchLabel` during the visit of `e` might // end up calling `fetchLabel` on `e` itself, so we want the visit of `e` to call `fetchLabel` @@ -205,17 +250,18 @@ class SwiftDispatcher { return std::nullopt; } - // map `fetchLabel` on the iterable `arg`, returning a vector of all labels + // map `fetchLabel` on the iterable `arg` // universal reference `Arg&&` is used to catch both temporary and non-const references, not // for perfect forwarding template auto fetchRepeatedLabels(Iterable&& arg) { - std::vector ret; + using Label = decltype(fetchLabel(*arg.begin())); + TrapLabelVectorWrapper ret; if constexpr (HasSize::value) { - ret.reserve(arg.size()); + ret.data.reserve(arg.size()); } for (auto&& e : arg) { - ret.push_back(fetchLabel(e)); + ret.data.push_back(fetchLabel(e)); } return ret; } @@ -270,6 +316,12 @@ class SwiftDispatcher { template struct HasSize().size(), void())> : std::true_type {}; + template + struct HasId : std::false_type {}; + + template + struct HasId().id, void())> : std::true_type {}; + void attachLocation(swift::SourceLoc start, swift::SourceLoc end, TrapLabel locatableLabel) { @@ -293,19 +345,20 @@ class SwiftDispatcher { TrapLabel fetchLabelFromUnion(const llvm::PointerUnion u) { TrapLabel ret{}; // with logical op short-circuiting, this will stop trying on the first successful fetch - // don't feel tempted to replace the variable with the expression inside the `assert`, or - // building with `NDEBUG` will not trigger the fetching bool unionCaseFound = (... || fetchLabelFromUnionCase(u, ret)); - assert(unionCaseFound && "llvm::PointerUnion not set to a known case"); + if (!unionCaseFound) { + // TODO emit error/warning here + return undefined_label; + } return ret; } template bool fetchLabelFromUnionCase(const llvm::PointerUnion u, TrapLabel& output) { // we rely on the fact that when we extract `ASTNode` instances (which only happens - // on `BraceStmt` elements), we cannot encounter a standalone `TypeRepr` there, so we skip - // this case; extracting `TypeRepr`s here would be problematic as we would not be able to - // provide the corresponding type + // on `BraceStmt`/`IfConfigDecl` elements), we cannot encounter a standalone `TypeRepr` there, + // so we skip this case; extracting `TypeRepr`s here would be problematic as we would not be + // able to provide the corresponding type if constexpr (!std::is_same_v) { if (auto e = u.template dyn_cast()) { output = fetchLabel(e); diff --git a/swift/extractor/trap/TrapLabel.h b/swift/extractor/trap/TrapLabel.h index 024c58fa829..784d322b213 100644 --- a/swift/extractor/trap/TrapLabel.h +++ b/swift/extractor/trap/TrapLabel.h @@ -4,9 +4,14 @@ #include #include #include +#include namespace codeql { +struct UndefinedTrapLabel {}; + +constexpr UndefinedTrapLabel undefined_label{}; + class UntypedTrapLabel { uint64_t id_; @@ -18,14 +23,17 @@ class UntypedTrapLabel { protected: UntypedTrapLabel() : id_{undefined} {} - UntypedTrapLabel(uint64_t id) : id_{id} {} + UntypedTrapLabel(uint64_t id) : id_{id} { assert(id != undefined); } public: + bool valid() const { return id_ != undefined; } + explicit operator bool() const { return valid(); } + friend std::ostream& operator<<(std::ostream& out, UntypedTrapLabel l) { // TODO: this is a temporary fix to catch us from outputting undefined labels to trap // this should be moved to a validity check, probably aided by code generation and carried out // by `SwiftDispatcher` - assert(l.id_ != undefined && "outputting an undefined label!"); + assert(l && "outputting an undefined label!"); out << '#' << std::hex << l.id_ << std::dec; return out; } @@ -44,14 +52,37 @@ class TrapLabel : public UntypedTrapLabel { using Tag = TagParam; TrapLabel() = default; + TrapLabel(UndefinedTrapLabel) : TrapLabel() {} + + TrapLabel& operator=(UndefinedTrapLabel) { + *this = TrapLabel{}; + return *this; + } // The caller is responsible for ensuring ID uniqueness. static TrapLabel unsafeCreateFromExplicitId(uint64_t id) { return {id}; } static TrapLabel unsafeCreateFromUntyped(UntypedTrapLabel label) { return {label.id_}; } - template - TrapLabel(const TrapLabel& other) : UntypedTrapLabel(other) { - static_assert(std::is_base_of_v, "wrong label assignment!"); + template + TrapLabel(const TrapLabel& other) : UntypedTrapLabel(other) { + static_assert(std::is_base_of_v, "wrong label assignment!"); + } +}; + +// wrapper class to allow directly assigning a vector of TrapLabel to a vector of +// TrapLabel if B is a base of A, using move semantics rather than copying +template +struct TrapLabelVectorWrapper { + using Tag = TagParam; + + std::vector> data; + + template + operator std::vector>() && { + static_assert(std::is_base_of_v, "wrong label assignment!"); + // reinterpret_cast is safe because TrapLabel instances differ only on the type, not the + // underlying data + return std::move(reinterpret_cast>&>(data)); } }; diff --git a/swift/ql/lib/codeql/swift/elements.qll b/swift/ql/lib/codeql/swift/elements.qll index e7b1a6a299a..6e229c07741 100644 --- a/swift/ql/lib/codeql/swift/elements.qll +++ b/swift/ql/lib/codeql/swift/elements.qll @@ -11,6 +11,7 @@ import codeql.swift.elements.Location import codeql.swift.elements.UnknownFile import codeql.swift.elements.UnknownLocation import codeql.swift.elements.UnresolvedElement +import codeql.swift.elements.UnspecifiedElement import codeql.swift.elements.decl.AbstractFunctionDecl import codeql.swift.elements.decl.AbstractStorageDecl import codeql.swift.elements.decl.AbstractTypeParamDecl diff --git a/swift/ql/lib/codeql/swift/elements/UnspecifiedElement.qll b/swift/ql/lib/codeql/swift/elements/UnspecifiedElement.qll new file mode 100644 index 00000000000..7680fa7cca9 --- /dev/null +++ b/swift/ql/lib/codeql/swift/elements/UnspecifiedElement.qll @@ -0,0 +1,22 @@ +private import codeql.swift.generated.UnspecifiedElement +import codeql.swift.elements.Location + +class UnspecifiedElement extends Generated::UnspecifiedElement { + override string toString() { + exists(string source, string index | + ( + source = " from " + this.getParent().getPrimaryQlClasses() + or + not this.hasParent() and source = "" + ) and + ( + index = "[" + this.getIndex() + "]" + or + not this.hasIndex() and index = "" + ) and + result = "missing " + this.getProperty() + index + source + ) + } + + override Location getImmediateLocation() { result = this.getParent().(Locatable).getLocation() } +} diff --git a/swift/ql/lib/codeql/swift/elements/UnspecifiedElementConstructor.qll b/swift/ql/lib/codeql/swift/elements/UnspecifiedElementConstructor.qll new file mode 100644 index 00000000000..8c87ad1281f --- /dev/null +++ b/swift/ql/lib/codeql/swift/elements/UnspecifiedElementConstructor.qll @@ -0,0 +1,4 @@ +// generated by codegen/codegen.py, remove this comment if you wish to edit this file +private import codeql.swift.generated.Raw + +predicate constructUnspecifiedElement(Raw::UnspecifiedElement id) { any() } diff --git a/swift/ql/lib/codeql/swift/elements/expr/BitwiseOperation.qll b/swift/ql/lib/codeql/swift/elements/expr/BitwiseOperation.qll new file mode 100644 index 00000000000..d1e0a0bc8b6 --- /dev/null +++ b/swift/ql/lib/codeql/swift/elements/expr/BitwiseOperation.qll @@ -0,0 +1,108 @@ +private import codeql.swift.elements.expr.BinaryExpr +private import codeql.swift.elements.expr.Expr +private import codeql.swift.elements.expr.PrefixUnaryExpr + +/** + * A bitwise operation, such as: + * ``` + * a & b + * ``` + */ +class BitwiseOperation extends Expr { + BitwiseOperation() { + this instanceof BinaryBitwiseOperation or + this instanceof UnaryBitwiseOperation + } + + /** + * Gets an operand of this bitwise operation. + */ + Expr getAnOperand() { + result = + [this.(BinaryBitwiseOperation).getAnOperand(), this.(UnaryBitwiseOperation).getOperand()] + } +} + +/** + * A binary bitwise operation, such as: + * ``` + * a & b + * ``` + */ +class BinaryBitwiseOperation extends BinaryExpr { + BinaryBitwiseOperation() { + this instanceof AndBitwiseExpr or + this instanceof OrBitwiseExpr or + this instanceof XorBitwiseExpr or + this instanceof ShiftLeftBitwiseExpr or + this instanceof ShiftRightBitwiseExpr + } +} + +/** + * A bitwise AND expression. + * ``` + * a & b + * ``` + */ +class AndBitwiseExpr extends BinaryExpr { + AndBitwiseExpr() { this.getStaticTarget().getName() = "&(_:_:)" } +} + +/** + * A bitwise OR expression. + * ``` + * a | b + * ``` + */ +class OrBitwiseExpr extends BinaryExpr { + OrBitwiseExpr() { this.getStaticTarget().getName() = "|(_:_:)" } +} + +/** + * A bitwise XOR expression. + * ``` + * a ^ b + * ``` + */ +class XorBitwiseExpr extends BinaryExpr { + XorBitwiseExpr() { this.getStaticTarget().getName() = "^(_:_:)" } +} + +/** + * A bitwise shift left expression. + * ``` + * a << b + * ``` + */ +class ShiftLeftBitwiseExpr extends BinaryExpr { + ShiftLeftBitwiseExpr() { this.getStaticTarget().getName() = "<<(_:_:)" } +} + +/** + * A bitwise shift right expression. + * ``` + * a >> b + * ``` + */ +class ShiftRightBitwiseExpr extends BinaryExpr { + ShiftRightBitwiseExpr() { this.getStaticTarget().getName() = ">>(_:_:)" } +} + +/** + * A unary bitwise operation, such as: + * ``` + * ~a + * ``` + */ +class UnaryBitwiseOperation extends PrefixUnaryExpr instanceof NotBitwiseExpr { } + +/** + * A bitwise NOT expression. + * ``` + * ~a + * ``` + */ +class NotBitwiseExpr extends PrefixUnaryExpr { + NotBitwiseExpr() { this.getStaticTarget().getName() = "~(_:)" } +} diff --git a/swift/ql/lib/codeql/swift/generated/ParentChild.qll b/swift/ql/lib/codeql/swift/generated/ParentChild.qll index eb76c02e23e..591c32a2fb5 100644 --- a/swift/ql/lib/codeql/swift/generated/ParentChild.qll +++ b/swift/ql/lib/codeql/swift/generated/ParentChild.qll @@ -165,6 +165,21 @@ private module Impl { ) } + private Element getImmediateChildOfUnspecifiedElement( + UnspecifiedElement e, int index, string partialPredicateCall + ) { + exists(int b, int bLocatable, int n | + b = 0 and + bLocatable = b + 1 + max(int i | i = -1 or exists(getImmediateChildOfLocatable(e, i, _)) | i) and + n = bLocatable and + ( + none() + or + result = getImmediateChildOfLocatable(e, index - b, partialPredicateCall) + ) + ) + } + private Element getImmediateChildOfDecl(Decl e, int index, string partialPredicateCall) { exists(int b, int bAstNode, int n | b = 0 and @@ -4871,6 +4886,8 @@ private module Impl { or result = getImmediateChildOfUnknownLocation(e, index, partialAccessor) or + result = getImmediateChildOfUnspecifiedElement(e, index, partialAccessor) + or result = getImmediateChildOfEnumCaseDecl(e, index, partialAccessor) or result = getImmediateChildOfExtensionDecl(e, index, partialAccessor) diff --git a/swift/ql/lib/codeql/swift/generated/Raw.qll b/swift/ql/lib/codeql/swift/generated/Raw.qll index 37233a66b01..d6340128034 100644 --- a/swift/ql/lib/codeql/swift/generated/Raw.qll +++ b/swift/ql/lib/codeql/swift/generated/Raw.qll @@ -51,6 +51,18 @@ module Raw { override string toString() { result = "DbLocation" } } + class UnspecifiedElement extends @unspecified_element, Locatable { + override string toString() { result = "UnspecifiedElement" } + + Element getParent() { unspecified_element_parents(this, result) } + + string getProperty() { unspecified_elements(this, result, _) } + + int getIndex() { unspecified_element_indices(this, result) } + + string getError() { unspecified_elements(this, _, result) } + } + class Decl extends @decl, AstNode { ModuleDecl getModule() { decls(this, result) } } diff --git a/swift/ql/lib/codeql/swift/generated/Synth.qll b/swift/ql/lib/codeql/swift/generated/Synth.qll index 1baf1fd0f3b..f9fca8aab66 100644 --- a/swift/ql/lib/codeql/swift/generated/Synth.qll +++ b/swift/ql/lib/codeql/swift/generated/Synth.qll @@ -10,6 +10,7 @@ module Synth { TDbLocation(Raw::DbLocation id) { constructDbLocation(id) } or TUnknownFile() or TUnknownLocation() or + TUnspecifiedElement(Raw::UnspecifiedElement id) { constructUnspecifiedElement(id) } or TAccessorDecl(Raw::AccessorDecl id) { constructAccessorDecl(id) } or TAssociatedTypeDecl(Raw::AssociatedTypeDecl id) { constructAssociatedTypeDecl(id) } or TClassDecl(Raw::ClassDecl id) { constructClassDecl(id) } or @@ -321,7 +322,7 @@ module Synth { class TFile = TDbFile or TUnknownFile; - class TLocatable = TArgument or TAstNode or TComment; + class TLocatable = TArgument or TAstNode or TComment or TUnspecifiedElement; class TLocation = TDbLocation or TUnknownLocation; @@ -499,6 +500,11 @@ module Synth { cached TUnknownLocation convertUnknownLocationFromRaw(Raw::Element e) { none() } + cached + TUnspecifiedElement convertUnspecifiedElementFromRaw(Raw::Element e) { + result = TUnspecifiedElement(e) + } + cached TAccessorDecl convertAccessorDeclFromRaw(Raw::Element e) { result = TAccessorDecl(e) } @@ -1464,6 +1470,8 @@ module Synth { result = convertAstNodeFromRaw(e) or result = convertCommentFromRaw(e) + or + result = convertUnspecifiedElementFromRaw(e) } cached @@ -2199,6 +2207,11 @@ module Synth { cached Raw::Element convertUnknownLocationToRaw(TUnknownLocation e) { none() } + cached + Raw::Element convertUnspecifiedElementToRaw(TUnspecifiedElement e) { + e = TUnspecifiedElement(result) + } + cached Raw::Element convertAccessorDeclToRaw(TAccessorDecl e) { e = TAccessorDecl(result) } @@ -3162,6 +3175,8 @@ module Synth { result = convertAstNodeToRaw(e) or result = convertCommentToRaw(e) + or + result = convertUnspecifiedElementToRaw(e) } cached diff --git a/swift/ql/lib/codeql/swift/generated/SynthConstructors.qll b/swift/ql/lib/codeql/swift/generated/SynthConstructors.qll index b8752923a8d..f35b5fdc630 100644 --- a/swift/ql/lib/codeql/swift/generated/SynthConstructors.qll +++ b/swift/ql/lib/codeql/swift/generated/SynthConstructors.qll @@ -2,6 +2,7 @@ import codeql.swift.elements.CommentConstructor import codeql.swift.elements.DbFileConstructor import codeql.swift.elements.DbLocationConstructor +import codeql.swift.elements.UnspecifiedElementConstructor import codeql.swift.elements.decl.AccessorDeclConstructor import codeql.swift.elements.decl.AssociatedTypeDeclConstructor import codeql.swift.elements.decl.ClassDeclConstructor diff --git a/swift/ql/lib/codeql/swift/generated/UnspecifiedElement.qll b/swift/ql/lib/codeql/swift/generated/UnspecifiedElement.qll new file mode 100644 index 00000000000..8f6efd926d8 --- /dev/null +++ b/swift/ql/lib/codeql/swift/generated/UnspecifiedElement.qll @@ -0,0 +1,60 @@ +// generated by codegen/codegen.py +private import codeql.swift.generated.Synth +private import codeql.swift.generated.Raw +import codeql.swift.elements.Element +import codeql.swift.elements.Locatable + +module Generated { + class UnspecifiedElement extends Synth::TUnspecifiedElement, Locatable { + override string getAPrimaryQlClass() { result = "UnspecifiedElement" } + + /** + * Gets the parent of this unspecified element, if it exists. + * + * This includes nodes from the "hidden" AST. It can be overridden in subclasses to change the + * behavior of both the `Immediate` and non-`Immediate` versions. + */ + Element getImmediateParent() { + result = + Synth::convertElementFromRaw(Synth::convertUnspecifiedElementToRaw(this) + .(Raw::UnspecifiedElement) + .getParent()) + } + + /** + * Gets the parent of this unspecified element, if it exists. + */ + final Element getParent() { result = getImmediateParent().resolve() } + + /** + * Holds if `getParent()` exists. + */ + final predicate hasParent() { exists(getParent()) } + + /** + * Gets the property of this unspecified element. + */ + string getProperty() { + result = Synth::convertUnspecifiedElementToRaw(this).(Raw::UnspecifiedElement).getProperty() + } + + /** + * Gets the index of this unspecified element, if it exists. + */ + int getIndex() { + result = Synth::convertUnspecifiedElementToRaw(this).(Raw::UnspecifiedElement).getIndex() + } + + /** + * Holds if `getIndex()` exists. + */ + final predicate hasIndex() { exists(getIndex()) } + + /** + * Gets the error of this unspecified element. + */ + string getError() { + result = Synth::convertUnspecifiedElementToRaw(this).(Raw::UnspecifiedElement).getError() + } + } +} diff --git a/swift/ql/lib/swift.dbscheme b/swift/ql/lib/swift.dbscheme index 5483094d942..9b503cdd7fc 100644 --- a/swift/ql/lib/swift.dbscheme +++ b/swift/ql/lib/swift.dbscheme @@ -35,20 +35,20 @@ element_is_unknown( #keyset[id] callable_self_params( int id: @callable ref, - int self_param: @param_decl ref + int self_param: @param_decl_or_none ref ); #keyset[id, index] callable_params( int id: @callable ref, int index: int ref, - int param: @param_decl ref + int param: @param_decl_or_none ref ); #keyset[id] callable_bodies( int id: @callable ref, - int body: @brace_stmt ref + int body: @brace_stmt_or_none ref ); @file = @@ -66,12 +66,13 @@ files( @argument | @ast_node | @comment +| @unspecified_element ; #keyset[id] locatable_locations( int id: @locatable ref, - int location: @location ref + int location: @location_or_none ref ); @location = @@ -82,7 +83,7 @@ locatable_locations( #keyset[id] locations( int id: @location ref, - int file: @file ref, + int file: @file_or_none ref, int start_line: int ref, int start_column: int ref, int end_line: int ref, @@ -132,6 +133,24 @@ unknown_locations( unique int id: @unknown_location ); +unspecified_elements( + unique int id: @unspecified_element, + string property: string ref, + string error: string ref +); + +#keyset[id] +unspecified_element_parents( + int id: @unspecified_element ref, + int parent: @element ref +); + +#keyset[id] +unspecified_element_indices( + int id: @unspecified_element ref, + int index: int ref +); + @decl = @enum_case_decl | @extension_decl @@ -149,7 +168,7 @@ unknown_locations( #keyset[id] decls( //dir=decl int id: @decl ref, - int module: @module_decl ref + int module: @module_decl_or_none ref ); @generic_context = @@ -163,7 +182,7 @@ decls( //dir=decl generic_context_generic_type_params( //dir=decl int id: @generic_context ref, int index: int ref, - int generic_type_param: @generic_type_param_decl ref + int generic_type_param: @generic_type_param_decl_or_none ref ); @iterable_decl_context = @@ -175,7 +194,7 @@ generic_context_generic_type_params( //dir=decl iterable_decl_context_members( //dir=decl int id: @iterable_decl_context ref, int index: int ref, - int member: @decl ref + int member: @decl_or_none ref ); enum_case_decls( //dir=decl @@ -186,12 +205,12 @@ enum_case_decls( //dir=decl enum_case_decl_elements( //dir=decl int id: @enum_case_decl ref, int index: int ref, - int element: @enum_element_decl ref + int element: @enum_element_decl_or_none ref ); extension_decls( //dir=decl unique int id: @extension_decl, - int extended_type_decl: @nominal_type_decl ref + int extended_type_decl: @nominal_type_decl_or_none ref ); if_config_decls( //dir=decl @@ -202,7 +221,7 @@ if_config_decls( //dir=decl if_config_decl_active_elements( //dir=decl int id: @if_config_decl ref, int index: int ref, - int active_element: @ast_node ref + int active_element: @ast_node_or_none ref ); import_decls( //dir=decl @@ -217,14 +236,14 @@ import_decl_is_exported( //dir=decl #keyset[id] import_decl_imported_modules( //dir=decl int id: @import_decl ref, - int imported_module: @module_decl ref + int imported_module: @module_decl_or_none ref ); #keyset[id, index] import_decl_declarations( //dir=decl int id: @import_decl ref, int index: int ref, - int declaration: @value_decl ref + int declaration: @value_decl_or_none ref ); missing_member_decls( //dir=decl @@ -251,14 +270,14 @@ pattern_binding_decls( //dir=decl pattern_binding_decl_inits( //dir=decl int id: @pattern_binding_decl ref, int index: int ref, - int init: @expr ref + int init: @expr_or_none ref ); #keyset[id, index] pattern_binding_decl_patterns( //dir=decl int id: @pattern_binding_decl ref, int index: int ref, - int pattern: @pattern ref + int pattern: @pattern_or_none ref ); pound_diagnostic_decls( //dir=decl @@ -271,7 +290,7 @@ precedence_group_decls( //dir=decl top_level_code_decls( //dir=decl unique int id: @top_level_code_decl, - int body: @brace_stmt ref + int body: @brace_stmt_or_none ref ); @value_decl = @@ -284,7 +303,7 @@ top_level_code_decls( //dir=decl #keyset[id] value_decls( //dir=decl int id: @value_decl ref, - int interface_type: @type ref + int interface_type: @type_or_none ref ); @abstract_function_decl = @@ -308,7 +327,7 @@ abstract_function_decls( //dir=decl abstract_storage_decl_accessor_decls( //dir=decl int id: @abstract_storage_decl ref, int index: int ref, - int accessor_decl: @accessor_decl ref + int accessor_decl: @accessor_decl_or_none ref ); enum_element_decls( //dir=decl @@ -320,7 +339,7 @@ enum_element_decls( //dir=decl enum_element_decl_params( //dir=decl int id: @enum_element_decl ref, int index: int ref, - int param: @param_decl ref + int param: @param_decl_or_none ref ); infix_operator_decls( //dir=decl @@ -330,7 +349,7 @@ infix_operator_decls( //dir=decl #keyset[id] infix_operator_decl_precedence_groups( //dir=decl int id: @infix_operator_decl ref, - int precedence_group: @precedence_group_decl ref + int precedence_group: @precedence_group_decl_or_none ref ); postfix_operator_decls( //dir=decl @@ -357,7 +376,7 @@ type_decls( //dir=decl type_decl_base_types( //dir=decl int id: @type_decl ref, int index: int ref, - int base_type: @type ref + int base_type: @type_or_none ref ); @abstract_type_param_decl = @@ -402,26 +421,26 @@ module_decl_is_system_module( //dir=decl module_decl_imported_modules( //dir=decl int id: @module_decl ref, int index: int ref, - int imported_module: @module_decl ref + int imported_module: @module_decl_or_none ref ); #keyset[id, index] module_decl_exported_modules( //dir=decl int id: @module_decl ref, int index: int ref, - int exported_module: @module_decl ref + int exported_module: @module_decl_or_none ref ); subscript_decls( //dir=decl unique int id: @subscript_decl, - int element_type: @type ref + int element_type: @type_or_none ref ); #keyset[id, index] subscript_decl_params( //dir=decl int id: @subscript_decl ref, int index: int ref, - int param: @param_decl ref + int param: @param_decl_or_none ref ); @var_decl = @@ -433,25 +452,25 @@ subscript_decl_params( //dir=decl var_decls( //dir=decl int id: @var_decl ref, string name: string ref, - int type_: @type ref + int type_: @type_or_none ref ); #keyset[id] var_decl_attached_property_wrapper_types( //dir=decl int id: @var_decl ref, - int attached_property_wrapper_type: @type ref + int attached_property_wrapper_type: @type_or_none ref ); #keyset[id] var_decl_parent_patterns( //dir=decl int id: @var_decl ref, - int parent_pattern: @pattern ref + int parent_pattern: @pattern_or_none ref ); #keyset[id] var_decl_parent_initializers( //dir=decl int id: @var_decl ref, - int parent_initializer: @expr ref + int parent_initializer: @expr_or_none ref ); accessor_decls( //dir=decl @@ -505,7 +524,7 @@ generic_type_param_decls( //dir=decl #keyset[id] nominal_type_decls( //dir=decl int id: @nominal_type_decl ref, - int type_: @type ref + int type_: @type_or_none ref ); opaque_type_decls( //dir=decl @@ -544,7 +563,7 @@ struct_decls( //dir=decl arguments( //dir=expr unique int id: @argument, string label: string ref, - int expr: @expr ref + int expr: @expr_or_none ref ); @expr = @@ -606,7 +625,7 @@ arguments( //dir=expr #keyset[id] expr_types( //dir=expr int id: @expr ref, - int type_: @type ref + int type_: @type_or_none ref ); @abstract_closure_expr = @@ -623,7 +642,7 @@ expr_types( //dir=expr #keyset[id] any_try_exprs( //dir=expr int id: @any_try_expr ref, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); applied_property_wrapper_exprs( //dir=expr @@ -641,14 +660,14 @@ applied_property_wrapper_exprs( //dir=expr #keyset[id] apply_exprs( //dir=expr int id: @apply_expr ref, - int function: @expr ref + int function: @expr_or_none ref ); #keyset[id, index] apply_expr_arguments( //dir=expr int id: @apply_expr ref, int index: int ref, - int argument: @argument ref + int argument: @argument_or_none ref ); arrow_exprs( //dir=expr @@ -657,25 +676,25 @@ arrow_exprs( //dir=expr assign_exprs( //dir=expr unique int id: @assign_expr, - int dest: @expr ref, - int source: @expr ref + int dest: @expr_or_none ref, + int source: @expr_or_none ref ); bind_optional_exprs( //dir=expr unique int id: @bind_optional_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); capture_list_exprs( //dir=expr unique int id: @capture_list_expr, - int closure_body: @closure_expr ref + int closure_body: @closure_expr_or_none ref ); #keyset[id, index] capture_list_expr_binding_decls( //dir=expr int id: @capture_list_expr ref, int index: int ref, - int binding_decl: @pattern_binding_decl ref + int binding_decl: @pattern_binding_decl_or_none ref ); code_completion_exprs( //dir=expr @@ -689,14 +708,14 @@ code_completion_exprs( //dir=expr decl_ref_exprs( //dir=expr unique int id: @decl_ref_expr, - int decl: @decl ref + int decl: @decl_or_none ref ); #keyset[id, index] decl_ref_expr_replacement_types( //dir=expr int id: @decl_ref_expr ref, int index: int ref, - int replacement_type: @type ref + int replacement_type: @type_or_none ref ); #keyset[id] @@ -716,14 +735,14 @@ decl_ref_expr_has_ordinary_semantics( //dir=expr default_argument_exprs( //dir=expr unique int id: @default_argument_expr, - int param_decl: @param_decl ref, + int param_decl: @param_decl_or_none ref, int param_index: int ref ); #keyset[id] default_argument_expr_caller_side_defaults( //dir=expr int id: @default_argument_expr ref, - int caller_side_default: @expr ref + int caller_side_default: @expr_or_none ref ); discard_assignment_exprs( //dir=expr @@ -732,13 +751,13 @@ discard_assignment_exprs( //dir=expr dot_syntax_base_ignored_exprs( //dir=expr unique int id: @dot_syntax_base_ignored_expr, - int qualifier: @expr ref, - int sub_expr: @expr ref + int qualifier: @expr_or_none ref, + int sub_expr: @expr_or_none ref ); dynamic_type_exprs( //dir=expr unique int id: @dynamic_type_expr, - int base: @expr ref + int base: @expr_or_none ref ); editor_placeholder_exprs( //dir=expr @@ -747,8 +766,8 @@ editor_placeholder_exprs( //dir=expr enum_is_case_exprs( //dir=expr unique int id: @enum_is_case_expr, - int sub_expr: @expr ref, - int element: @enum_element_decl ref + int sub_expr: @expr_or_none ref, + int element: @enum_element_decl_or_none ref ); error_exprs( //dir=expr @@ -763,12 +782,12 @@ error_exprs( //dir=expr #keyset[id] explicit_cast_exprs( //dir=expr int id: @explicit_cast_expr ref, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); force_value_exprs( //dir=expr unique int id: @force_value_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); @identity_expr = @@ -781,14 +800,14 @@ force_value_exprs( //dir=expr #keyset[id] identity_exprs( //dir=expr int id: @identity_expr ref, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); if_exprs( //dir=expr unique int id: @if_expr, - int condition: @expr ref, - int then_expr: @expr ref, - int else_expr: @expr ref + int condition: @expr_or_none ref, + int then_expr: @expr_or_none ref, + int else_expr: @expr_or_none ref ); @implicit_conversion_expr = @@ -829,18 +848,18 @@ if_exprs( //dir=expr #keyset[id] implicit_conversion_exprs( //dir=expr int id: @implicit_conversion_expr ref, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); in_out_exprs( //dir=expr unique int id: @in_out_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); key_path_application_exprs( //dir=expr unique int id: @key_path_application_expr, - int base: @expr ref, - int key_path: @expr ref + int base: @expr_or_none ref, + int key_path: @expr_or_none ref ); key_path_dot_exprs( //dir=expr @@ -854,18 +873,18 @@ key_path_exprs( //dir=expr #keyset[id] key_path_expr_roots( //dir=expr int id: @key_path_expr ref, - int root: @type_repr ref + int root: @type_repr_or_none ref ); #keyset[id] key_path_expr_parsed_paths( //dir=expr int id: @key_path_expr ref, - int parsed_path: @expr ref + int parsed_path: @expr_or_none ref ); lazy_initializer_exprs( //dir=expr unique int id: @lazy_initializer_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); @literal_expr = @@ -886,31 +905,31 @@ lazy_initializer_exprs( //dir=expr #keyset[id] lookup_exprs( //dir=expr int id: @lookup_expr ref, - int base: @expr ref + int base: @expr_or_none ref ); #keyset[id] lookup_expr_members( //dir=expr int id: @lookup_expr ref, - int member: @decl ref + int member: @decl_or_none ref ); make_temporarily_escapable_exprs( //dir=expr unique int id: @make_temporarily_escapable_expr, - int escaping_closure: @opaque_value_expr ref, - int nonescaping_closure: @expr ref, - int sub_expr: @expr ref + int escaping_closure: @opaque_value_expr_or_none ref, + int nonescaping_closure: @expr_or_none ref, + int sub_expr: @expr_or_none ref ); obj_c_selector_exprs( //dir=expr unique int id: @obj_c_selector_expr, - int sub_expr: @expr ref, - int method: @abstract_function_decl ref + int sub_expr: @expr_or_none ref, + int method: @abstract_function_decl_or_none ref ); one_way_exprs( //dir=expr unique int id: @one_way_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); opaque_value_exprs( //dir=expr @@ -919,19 +938,19 @@ opaque_value_exprs( //dir=expr open_existential_exprs( //dir=expr unique int id: @open_existential_expr, - int sub_expr: @expr ref, - int existential: @expr ref, - int opaque_expr: @opaque_value_expr ref + int sub_expr: @expr_or_none ref, + int existential: @expr_or_none ref, + int opaque_expr: @opaque_value_expr_or_none ref ); optional_evaluation_exprs( //dir=expr unique int id: @optional_evaluation_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); other_constructor_decl_ref_exprs( //dir=expr unique int id: @other_constructor_decl_ref_expr, - int constructor_decl: @constructor_decl ref + int constructor_decl: @constructor_decl_or_none ref ); @overload_set_ref_expr = @@ -948,8 +967,8 @@ property_wrapper_value_placeholder_exprs( //dir=expr rebind_self_in_constructor_exprs( //dir=expr unique int id: @rebind_self_in_constructor_expr, - int sub_expr: @expr ref, - int self: @var_decl ref + int sub_expr: @expr_or_none ref, + int self: @var_decl_or_none ref ); sequence_exprs( //dir=expr @@ -960,29 +979,29 @@ sequence_exprs( //dir=expr sequence_expr_elements( //dir=expr int id: @sequence_expr ref, int index: int ref, - int element: @expr ref + int element: @expr_or_none ref ); super_ref_exprs( //dir=expr unique int id: @super_ref_expr, - int self: @var_decl ref + int self: @var_decl_or_none ref ); tap_exprs( //dir=expr unique int id: @tap_expr, - int body: @brace_stmt ref, - int var: @var_decl ref + int body: @brace_stmt_or_none ref, + int var: @var_decl_or_none ref ); #keyset[id] tap_expr_sub_exprs( //dir=expr int id: @tap_expr ref, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); tuple_element_exprs( //dir=expr unique int id: @tuple_element_expr, - int sub_expr: @expr ref, + int sub_expr: @expr_or_none ref, int index: int ref ); @@ -994,7 +1013,7 @@ tuple_exprs( //dir=expr tuple_expr_elements( //dir=expr int id: @tuple_expr ref, int index: int ref, - int element: @expr ref + int element: @expr_or_none ref ); type_exprs( //dir=expr @@ -1004,7 +1023,7 @@ type_exprs( //dir=expr #keyset[id] type_expr_type_reprs( //dir=expr int id: @type_expr ref, - int type_repr: @type_repr ref + int type_repr: @type_repr_or_none ref ); unresolved_decl_ref_exprs( //dir=expr @@ -1019,7 +1038,7 @@ unresolved_decl_ref_expr_names( //dir=expr unresolved_dot_exprs( //dir=expr unique int id: @unresolved_dot_expr, - int base: @expr ref, + int base: @expr_or_none ref, string name: string ref ); @@ -1030,7 +1049,7 @@ unresolved_member_exprs( //dir=expr unresolved_pattern_exprs( //dir=expr unique int id: @unresolved_pattern_expr, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); unresolved_specialize_exprs( //dir=expr @@ -1039,7 +1058,7 @@ unresolved_specialize_exprs( //dir=expr vararg_expansion_exprs( //dir=expr unique int id: @vararg_expansion_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); any_hashable_erasure_exprs( //dir=expr @@ -1058,7 +1077,7 @@ array_exprs( //dir=expr array_expr_elements( //dir=expr int id: @array_expr ref, int index: int ref, - int element: @expr ref + int element: @expr_or_none ref ); array_to_pointer_exprs( //dir=expr @@ -1146,7 +1165,7 @@ dictionary_exprs( //dir=expr dictionary_expr_elements( //dir=expr int id: @dictionary_expr ref, int index: int ref, - int element: @expr ref + int element: @expr_or_none ref ); differentiable_function_exprs( //dir=expr @@ -1201,25 +1220,25 @@ interpolated_string_literal_exprs( //dir=expr #keyset[id] interpolated_string_literal_expr_interpolation_exprs( //dir=expr int id: @interpolated_string_literal_expr ref, - int interpolation_expr: @opaque_value_expr ref + int interpolation_expr: @opaque_value_expr_or_none ref ); #keyset[id] interpolated_string_literal_expr_interpolation_count_exprs( //dir=expr int id: @interpolated_string_literal_expr ref, - int interpolation_count_expr: @expr ref + int interpolation_count_expr: @expr_or_none ref ); #keyset[id] interpolated_string_literal_expr_literal_capacity_exprs( //dir=expr int id: @interpolated_string_literal_expr ref, - int literal_capacity_expr: @expr ref + int literal_capacity_expr: @expr_or_none ref ); #keyset[id] interpolated_string_literal_expr_appending_exprs( //dir=expr int id: @interpolated_string_literal_expr ref, - int appending_expr: @tap_expr ref + int appending_expr: @tap_expr_or_none ref ); linear_function_exprs( //dir=expr @@ -1317,7 +1336,7 @@ reify_pack_exprs( //dir=expr #keyset[id] self_apply_exprs( //dir=expr int id: @self_apply_expr ref, - int base: @expr ref + int base: @expr_or_none ref ); string_to_pointer_exprs( //dir=expr @@ -1332,7 +1351,7 @@ subscript_exprs( //dir=expr subscript_expr_arguments( //dir=expr int id: @subscript_expr ref, int index: int ref, - int argument: @argument ref + int argument: @argument_or_none ref ); #keyset[id] @@ -1448,7 +1467,7 @@ any_patterns( //dir=pattern binding_patterns( //dir=pattern unique int id: @binding_pattern, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); bool_patterns( //dir=pattern @@ -1458,18 +1477,18 @@ bool_patterns( //dir=pattern enum_element_patterns( //dir=pattern unique int id: @enum_element_pattern, - int element: @enum_element_decl ref + int element: @enum_element_decl_or_none ref ); #keyset[id] enum_element_pattern_sub_patterns( //dir=pattern int id: @enum_element_pattern ref, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); expr_patterns( //dir=pattern unique int id: @expr_pattern, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); is_patterns( //dir=pattern @@ -1479,13 +1498,13 @@ is_patterns( //dir=pattern #keyset[id] is_pattern_cast_type_reprs( //dir=pattern int id: @is_pattern ref, - int cast_type_repr: @type_repr ref + int cast_type_repr: @type_repr_or_none ref ); #keyset[id] is_pattern_sub_patterns( //dir=pattern int id: @is_pattern ref, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); named_patterns( //dir=pattern @@ -1495,12 +1514,12 @@ named_patterns( //dir=pattern optional_some_patterns( //dir=pattern unique int id: @optional_some_pattern, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); paren_patterns( //dir=pattern unique int id: @paren_pattern, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); tuple_patterns( //dir=pattern @@ -1511,29 +1530,29 @@ tuple_patterns( //dir=pattern tuple_pattern_elements( //dir=pattern int id: @tuple_pattern ref, int index: int ref, - int element: @pattern ref + int element: @pattern_or_none ref ); typed_patterns( //dir=pattern unique int id: @typed_pattern, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); #keyset[id] typed_pattern_type_reprs( //dir=pattern int id: @typed_pattern ref, - int type_repr: @type_repr ref + int type_repr: @type_repr_or_none ref ); case_label_items( //dir=stmt unique int id: @case_label_item, - int pattern: @pattern ref + int pattern: @pattern_or_none ref ); #keyset[id] case_label_item_guards( //dir=stmt int id: @case_label_item ref, - int guard: @expr ref + int guard: @expr_or_none ref ); condition_elements( //dir=stmt @@ -1543,19 +1562,19 @@ condition_elements( //dir=stmt #keyset[id] condition_element_booleans( //dir=stmt int id: @condition_element ref, - int boolean_: @expr ref + int boolean_: @expr_or_none ref ); #keyset[id] condition_element_patterns( //dir=stmt int id: @condition_element ref, - int pattern: @pattern ref + int pattern: @pattern_or_none ref ); #keyset[id] condition_element_initializers( //dir=stmt int id: @condition_element ref, - int initializer: @expr ref + int initializer: @expr_or_none ref ); @stmt = @@ -1581,7 +1600,7 @@ stmt_conditions( //dir=stmt stmt_condition_elements( //dir=stmt int id: @stmt_condition ref, int index: int ref, - int element: @condition_element ref + int element: @condition_element_or_none ref ); brace_stmts( //dir=stmt @@ -1592,7 +1611,7 @@ brace_stmts( //dir=stmt brace_stmt_elements( //dir=stmt int id: @brace_stmt ref, int index: int ref, - int element: @ast_node ref + int element: @ast_node_or_none ref ); break_stmts( //dir=stmt @@ -1608,26 +1627,26 @@ break_stmt_target_names( //dir=stmt #keyset[id] break_stmt_targets( //dir=stmt int id: @break_stmt ref, - int target: @stmt ref + int target: @stmt_or_none ref ); case_stmts( //dir=stmt unique int id: @case_stmt, - int body: @stmt ref + int body: @stmt_or_none ref ); #keyset[id, index] case_stmt_labels( //dir=stmt int id: @case_stmt ref, int index: int ref, - int label: @case_label_item ref + int label: @case_label_item_or_none ref ); #keyset[id, index] case_stmt_variables( //dir=stmt int id: @case_stmt ref, int index: int ref, - int variable: @var_decl ref + int variable: @var_decl_or_none ref ); continue_stmts( //dir=stmt @@ -1643,12 +1662,12 @@ continue_stmt_target_names( //dir=stmt #keyset[id] continue_stmt_targets( //dir=stmt int id: @continue_stmt ref, - int target: @stmt ref + int target: @stmt_or_none ref ); defer_stmts( //dir=stmt unique int id: @defer_stmt, - int body: @brace_stmt ref + int body: @brace_stmt_or_none ref ); fail_stmts( //dir=stmt @@ -1657,8 +1676,8 @@ fail_stmts( //dir=stmt fallthrough_stmts( //dir=stmt unique int id: @fallthrough_stmt, - int fallthrough_source: @case_stmt ref, - int fallthrough_dest: @case_stmt ref + int fallthrough_source: @case_stmt_or_none ref, + int fallthrough_dest: @case_stmt_or_none ref ); @labeled_stmt = @@ -1687,12 +1706,12 @@ return_stmts( //dir=stmt #keyset[id] return_stmt_results( //dir=stmt int id: @return_stmt ref, - int result: @expr ref + int result: @expr_or_none ref ); throw_stmts( //dir=stmt unique int id: @throw_stmt, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); yield_stmts( //dir=stmt @@ -1703,37 +1722,37 @@ yield_stmts( //dir=stmt yield_stmt_results( //dir=stmt int id: @yield_stmt ref, int index: int ref, - int result: @expr ref + int result: @expr_or_none ref ); do_catch_stmts( //dir=stmt unique int id: @do_catch_stmt, - int body: @stmt ref + int body: @stmt_or_none ref ); #keyset[id, index] do_catch_stmt_catches( //dir=stmt int id: @do_catch_stmt ref, int index: int ref, - int catch: @case_stmt ref + int catch: @case_stmt_or_none ref ); do_stmts( //dir=stmt unique int id: @do_stmt, - int body: @brace_stmt ref + int body: @brace_stmt_or_none ref ); for_each_stmts( //dir=stmt unique int id: @for_each_stmt, - int pattern: @pattern ref, - int sequence: @expr ref, - int body: @brace_stmt ref + int pattern: @pattern_or_none ref, + int sequence: @expr_or_none ref, + int body: @brace_stmt_or_none ref ); #keyset[id] for_each_stmt_wheres( //dir=stmt int id: @for_each_stmt ref, - int where: @expr ref + int where: @expr_or_none ref ); @labeled_conditional_stmt = @@ -1745,46 +1764,46 @@ for_each_stmt_wheres( //dir=stmt #keyset[id] labeled_conditional_stmts( //dir=stmt int id: @labeled_conditional_stmt ref, - int condition: @stmt_condition ref + int condition: @stmt_condition_or_none ref ); repeat_while_stmts( //dir=stmt unique int id: @repeat_while_stmt, - int condition: @expr ref, - int body: @stmt ref + int condition: @expr_or_none ref, + int body: @stmt_or_none ref ); switch_stmts( //dir=stmt unique int id: @switch_stmt, - int expr: @expr ref + int expr: @expr_or_none ref ); #keyset[id, index] switch_stmt_cases( //dir=stmt int id: @switch_stmt ref, int index: int ref, - int case_: @case_stmt ref + int case_: @case_stmt_or_none ref ); guard_stmts( //dir=stmt unique int id: @guard_stmt, - int body: @brace_stmt ref + int body: @brace_stmt_or_none ref ); if_stmts( //dir=stmt unique int id: @if_stmt, - int then: @stmt ref + int then: @stmt_or_none ref ); #keyset[id] if_stmt_elses( //dir=stmt int id: @if_stmt ref, - int else: @stmt ref + int else: @stmt_or_none ref ); while_stmts( //dir=stmt unique int id: @while_stmt, - int body: @stmt ref + int body: @stmt_or_none ref ); @type = @@ -1820,12 +1839,12 @@ while_stmts( //dir=stmt types( //dir=type int id: @type ref, string name: string ref, - int canonical_type: @type ref + int canonical_type: @type_or_none ref ); type_reprs( //dir=type unique int id: @type_repr, - int type_: @type ref + int type_: @type_or_none ref ); @any_function_type = @@ -1836,14 +1855,14 @@ type_reprs( //dir=type #keyset[id] any_function_types( //dir=type int id: @any_function_type ref, - int result: @type ref + int result: @type_or_none ref ); #keyset[id, index] any_function_type_param_types( //dir=type int id: @any_function_type ref, int index: int ref, - int param_type: @type ref + int param_type: @type_or_none ref ); #keyset[id, index] @@ -1871,13 +1890,13 @@ any_function_type_is_async( //dir=type #keyset[id] any_generic_types( //dir=type int id: @any_generic_type ref, - int declaration: @decl ref + int declaration: @decl_or_none ref ); #keyset[id] any_generic_type_parents( //dir=type int id: @any_generic_type ref, - int parent: @type ref + int parent: @type_or_none ref ); @any_metatype_type = @@ -1901,13 +1920,13 @@ any_generic_type_parents( //dir=type dependent_member_types( //dir=type unique int id: @dependent_member_type, - int base_type: @type ref, - int associated_type_decl: @associated_type_decl ref + int base_type: @type_or_none ref, + int associated_type_decl: @associated_type_decl_or_none ref ); dynamic_self_types( //dir=type unique int id: @dynamic_self_type, - int static_self_type: @type ref + int static_self_type: @type_or_none ref ); error_types( //dir=type @@ -1916,22 +1935,22 @@ error_types( //dir=type existential_types( //dir=type unique int id: @existential_type, - int constraint: @type ref + int constraint: @type_or_none ref ); in_out_types( //dir=type unique int id: @in_out_type, - int object_type: @type ref + int object_type: @type_or_none ref ); l_value_types( //dir=type unique int id: @l_value_type, - int object_type: @type ref + int object_type: @type_or_none ref ); module_types( //dir=type unique int id: @module_type, - int module: @module_decl ref + int module: @module_decl_or_none ref ); pack_expansion_types( //dir=type @@ -1958,7 +1977,7 @@ protocol_composition_types( //dir=type protocol_composition_type_members( //dir=type int id: @protocol_composition_type ref, int index: int ref, - int member: @type ref + int member: @type_or_none ref ); @reference_storage_type = @@ -1970,7 +1989,7 @@ protocol_composition_type_members( //dir=type #keyset[id] reference_storage_types( //dir=type int id: @reference_storage_type ref, - int referent_type: @type ref + int referent_type: @type_or_none ref ); sil_block_storage_types( //dir=type @@ -2008,7 +2027,7 @@ tuple_types( //dir=type tuple_type_types( //dir=type int id: @tuple_type ref, int index: int ref, - int type_: @type ref + int type_: @type_or_none ref ); #keyset[id, index] @@ -2041,20 +2060,20 @@ unresolved_types( //dir=type #keyset[id] archetype_types( //dir=type int id: @archetype_type ref, - int interface_type: @type ref + int interface_type: @type_or_none ref ); #keyset[id] archetype_type_superclasses( //dir=type int id: @archetype_type ref, - int superclass: @type ref + int superclass: @type_or_none ref ); #keyset[id, index] archetype_type_protocols( //dir=type int id: @archetype_type ref, int index: int ref, - int protocol: @protocol_decl ref + int protocol: @protocol_decl_or_none ref ); builtin_bridge_object_types( //dir=type @@ -2113,7 +2132,7 @@ generic_function_types( //dir=type generic_function_type_generic_params( //dir=type int id: @generic_function_type ref, int index: int ref, - int generic_param: @generic_type_param_type ref + int generic_param: @generic_type_param_type_or_none ref ); generic_type_param_types( //dir=type @@ -2131,7 +2150,7 @@ metatype_types( //dir=type paren_types( //dir=type unique int id: @paren_type, - int type_: @type ref + int type_: @type_or_none ref ); @syntax_sugar_type = @@ -2141,7 +2160,7 @@ paren_types( //dir=type type_alias_types( //dir=type unique int id: @type_alias_type, - int decl: @type_alias_decl ref + int decl: @type_alias_decl_or_none ref ); unbound_generic_types( //dir=type @@ -2170,7 +2189,7 @@ weak_storage_types( //dir=type bound_generic_type_arg_types( //dir=type int id: @bound_generic_type ref, int index: int ref, - int arg_type: @type ref + int arg_type: @type_or_none ref ); builtin_integer_literal_types( //dir=type @@ -2189,8 +2208,8 @@ builtin_integer_type_widths( //dir=type dictionary_types( //dir=type unique int id: @dictionary_type, - int key_type: @type ref, - int value_type: @type ref + int key_type: @type_or_none ref, + int value_type: @type_or_none ref ); @nominal_type = @@ -2225,7 +2244,7 @@ sequence_archetype_types( //dir=type #keyset[id] unary_syntax_sugar_types( //dir=type int id: @unary_syntax_sugar_type ref, - int base_type: @type ref + int base_type: @type_or_none ref ); array_slice_types( //dir=type @@ -2267,3 +2286,173 @@ struct_types( //dir=type variadic_sequence_types( //dir=type unique int id: @variadic_sequence_type ); + +@abstract_function_decl_or_none = + @abstract_function_decl +| @unspecified_element +; + +@accessor_decl_or_none = + @accessor_decl +| @unspecified_element +; + +@argument_or_none = + @argument +| @unspecified_element +; + +@associated_type_decl_or_none = + @associated_type_decl +| @unspecified_element +; + +@ast_node_or_none = + @ast_node +| @unspecified_element +; + +@brace_stmt_or_none = + @brace_stmt +| @unspecified_element +; + +@case_label_item_or_none = + @case_label_item +| @unspecified_element +; + +@case_stmt_or_none = + @case_stmt +| @unspecified_element +; + +@closure_expr_or_none = + @closure_expr +| @unspecified_element +; + +@condition_element_or_none = + @condition_element +| @unspecified_element +; + +@constructor_decl_or_none = + @constructor_decl +| @unspecified_element +; + +@decl_or_none = + @decl +| @unspecified_element +; + +@enum_element_decl_or_none = + @enum_element_decl +| @unspecified_element +; + +@expr_or_none = + @expr +| @unspecified_element +; + +@file_or_none = + @file +| @unspecified_element +; + +@generic_type_param_decl_or_none = + @generic_type_param_decl +| @unspecified_element +; + +@generic_type_param_type_or_none = + @generic_type_param_type +| @unspecified_element +; + +@location_or_none = + @location +| @unspecified_element +; + +@module_decl_or_none = + @module_decl +| @unspecified_element +; + +@nominal_type_decl_or_none = + @nominal_type_decl +| @unspecified_element +; + +@opaque_value_expr_or_none = + @opaque_value_expr +| @unspecified_element +; + +@param_decl_or_none = + @param_decl +| @unspecified_element +; + +@pattern_or_none = + @pattern +| @unspecified_element +; + +@pattern_binding_decl_or_none = + @pattern_binding_decl +| @unspecified_element +; + +@precedence_group_decl_or_none = + @precedence_group_decl +| @unspecified_element +; + +@protocol_decl_or_none = + @protocol_decl +| @unspecified_element +; + +@stmt_or_none = + @stmt +| @unspecified_element +; + +@stmt_condition_or_none = + @stmt_condition +| @unspecified_element +; + +@tap_expr_or_none = + @tap_expr +| @unspecified_element +; + +@type_or_none = + @type +| @unspecified_element +; + +@type_alias_decl_or_none = + @type_alias_decl +| @unspecified_element +; + +@type_repr_or_none = + @type_repr +| @unspecified_element +; + +@value_decl_or_none = + @unspecified_element +| @value_decl +; + +@var_decl_or_none = + @unspecified_element +| @var_decl +; diff --git a/swift/ql/lib/swift.qll b/swift/ql/lib/swift.qll index 51d3ee15c83..f890aca45e5 100644 --- a/swift/ql/lib/swift.qll +++ b/swift/ql/lib/swift.qll @@ -2,6 +2,7 @@ import codeql.swift.elements import codeql.swift.elements.expr.ArithmeticOperation +import codeql.swift.elements.expr.BitwiseOperation import codeql.swift.elements.expr.LogicalOperation import codeql.swift.elements.decl.MethodDecl import codeql.swift.elements.decl.ClassOrStructDecl diff --git a/swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelp b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelp new file mode 100644 index 00000000000..99fd0168225 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelp @@ -0,0 +1,21 @@ + + + +

    Constant salts should not be used for password hashing. Data hashed using constant salts are vulnerable to dictionary attacks, enabling attackers to recover the original input.

    +     + + +

    Use randomly generated salts to securely hash input data.

    +     + + +

    The following example shows a few cases of hashing input data. In the 'BAD' cases, the salt is constant, making the generated hashes vulnerable to dictionary attacks. In the 'GOOD' cases, the salt is randomly generated, which protects the hashed data against recovery.

    + +     + + +     + + diff --git a/swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql new file mode 100644 index 00000000000..9cd6dbd5ace --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql @@ -0,0 +1,63 @@ +/** + * @name Use of constant salts + * @description Using constant salts for password hashing is not secure because potential attackers can precompute the hash value via dictionary attacks. + * @kind path-problem + * @problem.severity error + * @security-severity 7.5 + * @precision high + * @id swift/constant-salt + * @tags security + * external/cwe/cwe-760 + */ + +import swift +import codeql.swift.dataflow.DataFlow +import codeql.swift.dataflow.TaintTracking +import codeql.swift.dataflow.FlowSteps +import DataFlow::PathGraph + +/** + * A constant salt is created through either a byte array or string literals. + */ +class ConstantSaltSource extends Expr { + ConstantSaltSource() { + this = any(ArrayExpr arr | arr.getType().getName() = "Array") or + this instanceof StringLiteralExpr + } +} + +/** + * A class for all ways to use a constant salt. + */ +class ConstantSaltSink extends Expr { + ConstantSaltSink() { + // `salt` arg in `init` is a sink + exists(ClassOrStructDecl c, AbstractFunctionDecl f, CallExpr call, int arg | + c.getFullName() = ["HKDF", "PBKDF1", "PBKDF2", "Scrypt"] and + c.getAMember() = f and + f.getName().matches("%init(%salt:%") and + call.getStaticTarget() = f and + f.getParam(pragma[only_bind_into](arg)).getName() = "salt" and + call.getArgument(pragma[only_bind_into](arg)).getExpr() = this + ) + } +} + +/** + * A taint configuration from the source of constants salts to expressions that use + * them to initialize password-based enecryption keys. + */ +class ConstantSaltConfig extends TaintTracking::Configuration { + ConstantSaltConfig() { this = "ConstantSaltConfig" } + + override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof ConstantSaltSource } + + override predicate isSink(DataFlow::Node node) { node.asExpr() instanceof ConstantSaltSink } +} + +// The query itself +from ConstantSaltConfig config, DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode +where config.hasFlowPath(sourceNode, sinkNode) +select sinkNode.getNode(), sourceNode, sinkNode, + "The value '" + sourceNode.getNode().toString() + + "' is used as a constant salt, which is insecure for hashing passwords." diff --git a/swift/ql/src/queries/Security/CWE-760/ConstantSalt.swift b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.swift new file mode 100644 index 00000000000..a3bc11c826b --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.swift @@ -0,0 +1,22 @@ + +func encrypt(padding : Padding) { + // ... + + // BAD: Using constant salts for hashing + let salt: Array = [0x2a, 0x3a, 0x80, 0x05] + let randomArray = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) }) + _ = try HKDF(password: randomArray, salt: salt, info: randomArray, keyLength: 0, variant: Variant.sha2) + _ = try PKCS5.PBKDF1(password: randomArray, salt: salt, iterations: 120120, keyLength: 0) + _ = try PKCS5.PBKDF2(password: randomArray, salt: salt, iterations: 120120, keyLength: 0) + _ = try Scrypt(password: randomArray, salt: salt, dkLen: 64, N: 16384, r: 8, p: 1) + + // GOOD: Using randomly generated salts for hashing + let salt = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) }) + let randomArray = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) }) + _ = try HKDF(password: randomArray, salt: salt, info: randomArray, keyLength: 0, variant: Variant.sha2) + _ = try PKCS5.PBKDF1(password: randomArray, salt: salt, iterations: 120120, keyLength: 0) + _ = try PKCS5.PBKDF2(password: randomArray, salt: salt, iterations: 120120, keyLength: 0) + _ = try Scrypt(password: randomArray, salt: salt, dkLen: 64, N: 16384, r: 8, p: 1) + + // ... +} diff --git a/swift/ql/test/TestUtils.qll b/swift/ql/test/TestUtils.qll index 265513b4f4a..da21ff87778 100644 --- a/swift/ql/test/TestUtils.qll +++ b/swift/ql/test/TestUtils.qll @@ -29,4 +29,6 @@ predicate toBeTested(Element e) { ) ) ) + or + toBeTested(e.(UnspecifiedElement).getParent()) } diff --git a/swift/ql/test/extractor-tests/expressions/all.expected b/swift/ql/test/extractor-tests/expressions/all.expected index edb8c16c545..12c3ec02368 100644 --- a/swift/ql/test/extractor-tests/expressions/all.expected +++ b/swift/ql/test/extractor-tests/expressions/all.expected @@ -229,3 +229,44 @@ | expressions.swift:154:22:154:56 | \\...[...] | KeyPathApplicationExpr | | expressions.swift:154:33:154:33 | keyPathB | DeclRefExpr | | expressions.swift:154:52:154:55 | #keyPath(...) | KeyPathExpr | +| expressions.swift:158:3:158:3 | _ | DiscardAssignmentExpr | +| expressions.swift:158:3:158:8 | ... = ... | AssignExpr | +| expressions.swift:158:7:158:7 | .~(_:) | MethodRefExpr | +| expressions.swift:158:7:158:7 | Int.Type | TypeExpr | +| expressions.swift:158:7:158:8 | call to ~(_:) | PrefixUnaryExpr | +| expressions.swift:158:8:158:8 | 1 | IntegerLiteralExpr | +| expressions.swift:159:3:159:3 | _ | DiscardAssignmentExpr | +| expressions.swift:159:3:159:11 | ... = ... | AssignExpr | +| expressions.swift:159:7:159:7 | 1 | IntegerLiteralExpr | +| expressions.swift:159:7:159:11 | ... .&(_:_:) ... | BinaryExpr | +| expressions.swift:159:9:159:9 | .&(_:_:) | MethodRefExpr | +| expressions.swift:159:9:159:9 | Int.Type | TypeExpr | +| expressions.swift:159:11:159:11 | 2 | IntegerLiteralExpr | +| expressions.swift:160:3:160:3 | _ | DiscardAssignmentExpr | +| expressions.swift:160:3:160:11 | ... = ... | AssignExpr | +| expressions.swift:160:7:160:7 | 1 | IntegerLiteralExpr | +| expressions.swift:160:7:160:11 | ... .\|(_:_:) ... | BinaryExpr | +| expressions.swift:160:9:160:9 | .\|(_:_:) | MethodRefExpr | +| expressions.swift:160:9:160:9 | Int.Type | TypeExpr | +| expressions.swift:160:11:160:11 | 2 | IntegerLiteralExpr | +| expressions.swift:161:3:161:3 | _ | DiscardAssignmentExpr | +| expressions.swift:161:3:161:11 | ... = ... | AssignExpr | +| expressions.swift:161:7:161:7 | 1 | IntegerLiteralExpr | +| expressions.swift:161:7:161:11 | ... .^(_:_:) ... | BinaryExpr | +| expressions.swift:161:9:161:9 | .^(_:_:) | MethodRefExpr | +| expressions.swift:161:9:161:9 | Int.Type | TypeExpr | +| expressions.swift:161:11:161:11 | 2 | IntegerLiteralExpr | +| expressions.swift:162:3:162:3 | _ | DiscardAssignmentExpr | +| expressions.swift:162:3:162:12 | ... = ... | AssignExpr | +| expressions.swift:162:7:162:7 | 1 | IntegerLiteralExpr | +| expressions.swift:162:7:162:12 | ... .<<(_:_:) ... | BinaryExpr | +| expressions.swift:162:9:162:9 | .<<(_:_:) | MethodRefExpr | +| expressions.swift:162:9:162:9 | Int.Type | TypeExpr | +| expressions.swift:162:12:162:12 | 0 | IntegerLiteralExpr | +| expressions.swift:163:3:163:3 | _ | DiscardAssignmentExpr | +| expressions.swift:163:3:163:12 | ... = ... | AssignExpr | +| expressions.swift:163:7:163:7 | 1 | IntegerLiteralExpr | +| expressions.swift:163:7:163:12 | ... .>>(_:_:) ... | BinaryExpr | +| expressions.swift:163:9:163:9 | .>>(_:_:) | MethodRefExpr | +| expressions.swift:163:9:163:9 | Int.Type | TypeExpr | +| expressions.swift:163:12:163:12 | 0 | IntegerLiteralExpr | diff --git a/swift/ql/test/extractor-tests/expressions/expressions.swift b/swift/ql/test/extractor-tests/expressions/expressions.swift index bc720abe70f..3e16c0be0ac 100644 --- a/swift/ql/test/extractor-tests/expressions/expressions.swift +++ b/swift/ql/test/extractor-tests/expressions/expressions.swift @@ -152,4 +152,13 @@ func test(a : A, keyPathInt : WritableKeyPath, keyPathB : WritableKeyPat var apply_keyPathInt = a[keyPath: keyPathInt] var apply_keyPathB = a[keyPath: keyPathB] var nested_apply = a[keyPath: keyPathB][keyPath: \B.x] -} \ No newline at end of file +} + +func bitwise() { + _ = ~1 + _ = 1 & 2 + _ = 1 | 2 + _ = 1 ^ 2 + _ = 1 << 0 + _ = 1 >> 0 +} diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.expected b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.expected new file mode 100644 index 00000000000..25fac0d1236 --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.expected @@ -0,0 +1,9 @@ +| wrong.swift:3:1:3:23 | missing extended_type_decl from ExtensionDecl | getProperty: | extended_type_decl | getError: | element was unspecified by the extractor | +| wrong.swift:9:9:9:9 | missing fallthrough_dest from FallthroughStmt | getProperty: | fallthrough_dest | getError: | element was unspecified by the extractor | +| wrong.swift:9:9:9:9 | missing fallthrough_source from FallthroughStmt | getProperty: | fallthrough_source | getError: | element was unspecified by the extractor | +| wrong.swift:12:18:12:21 | missing element from EnumElementPattern | getProperty: | element | getError: | element was unspecified by the extractor | +| wrong.swift:14:18:14:26 | missing element from EnumElementPattern | getProperty: | element | getError: | element was unspecified by the extractor | +| wrong.swift:19:18:19:19 | missing element from EnumElementPattern | getProperty: | element | getError: | element was unspecified by the extractor | +| wrong.swift:22:13:22:13 | missing fallthrough_dest from FallthroughStmt | getProperty: | fallthrough_dest | getError: | element was unspecified by the extractor | +| wrong.swift:22:13:22:13 | missing fallthrough_source from FallthroughStmt | getProperty: | fallthrough_source | getError: | element was unspecified by the extractor | +| wrong.swift:26:18:26:19 | missing element from EnumElementPattern | getProperty: | element | getError: | element was unspecified by the extractor | diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.ql b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.ql new file mode 100644 index 00000000000..dc762fbbeb0 --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.ql @@ -0,0 +1,11 @@ +// generated by codegen/codegen.py +import codeql.swift.elements +import TestUtils + +from UnspecifiedElement x, string getProperty, string getError +where + toBeTested(x) and + not x.isUnknown() and + getProperty = x.getProperty() and + getError = x.getError() +select x, "getProperty:", getProperty, "getError:", getError diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getIndex.expected b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getIndex.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getIndex.ql b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getIndex.ql new file mode 100644 index 00000000000..d8d2a7fab69 --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getIndex.ql @@ -0,0 +1,7 @@ +// generated by codegen/codegen.py +import codeql.swift.elements +import TestUtils + +from UnspecifiedElement x +where toBeTested(x) and not x.isUnknown() +select x, x.getIndex() diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.expected b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.expected new file mode 100644 index 00000000000..870a99df44f --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.expected @@ -0,0 +1,9 @@ +| wrong.swift:3:1:3:23 | missing extended_type_decl from ExtensionDecl | extension | +| wrong.swift:9:9:9:9 | missing fallthrough_dest from FallthroughStmt | fallthrough | +| wrong.swift:9:9:9:9 | missing fallthrough_source from FallthroughStmt | fallthrough | +| wrong.swift:12:18:12:21 | missing element from EnumElementPattern | (no string representation) | +| wrong.swift:14:18:14:26 | missing element from EnumElementPattern | (no string representation) | +| wrong.swift:19:18:19:19 | missing element from EnumElementPattern | (no string representation) | +| wrong.swift:22:13:22:13 | missing fallthrough_dest from FallthroughStmt | fallthrough | +| wrong.swift:22:13:22:13 | missing fallthrough_source from FallthroughStmt | fallthrough | +| wrong.swift:26:18:26:19 | missing element from EnumElementPattern | (no string representation) | diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.ql b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.ql new file mode 100644 index 00000000000..7cfdb4bb609 --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.ql @@ -0,0 +1,7 @@ +// generated by codegen/codegen.py +import codeql.swift.elements +import TestUtils + +from UnspecifiedElement x +where toBeTested(x) and not x.isUnknown() +select x, x.getParent() diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/wrong.swift b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/wrong.swift new file mode 100644 index 00000000000..30c61f6b33b --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/wrong.swift @@ -0,0 +1,30 @@ +//codeql-extractor-expected-status: 1 + +extension Undefined { } + +enum Enum { + case A, B + + func test(e: Enum) { + fallthrough + + switch e { + case .A(): + break + case .B(let x): + let _ = x + break + case Int: + break + case .C: + break + default: + fallthrough + } + + switch undefined { + case .Whatever: + break + } + } +} diff --git a/swift/ql/test/library-tests/ast/Missing.expected b/swift/ql/test/library-tests/ast/Missing.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/swift/ql/test/library-tests/ast/Missing.qlref b/swift/ql/test/library-tests/ast/Missing.qlref new file mode 100644 index 00000000000..e9db12f988f --- /dev/null +++ b/swift/ql/test/library-tests/ast/Missing.qlref @@ -0,0 +1 @@ +extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.ql diff --git a/swift/ql/test/library-tests/ast/PrintAst.expected b/swift/ql/test/library-tests/ast/PrintAst.expected index 54af4da242c..8dbfecabeee 100644 --- a/swift/ql/test/library-tests/ast/PrintAst.expected +++ b/swift/ql/test/library-tests/ast/PrintAst.expected @@ -4338,6 +4338,67 @@ expressions.swift: # 154| getPattern(0): [NamedPattern] nested_apply # 154| getElement(5): [ConcreteVarDecl] nested_apply # 154| Type = Int +# 157| [ConcreteFuncDecl] bitwise() +# 157| InterfaceType = () -> () +# 157| getBody(): [BraceStmt] { ... } +# 158| getElement(0): [AssignExpr] ... = ... +# 158| getDest(): [DiscardAssignmentExpr] _ +# 158| getSource(): [PrefixUnaryExpr] call to ~(_:) +# 158| getFunction(): [MethodRefExpr] .~(_:) +# 158| getBase(): [TypeExpr] Int.Type +# 158| getTypeRepr(): [TypeRepr] Int +# 158| getArgument(0): [Argument] : 1 +# 158| getExpr(): [IntegerLiteralExpr] 1 +# 159| getElement(1): [AssignExpr] ... = ... +# 159| getDest(): [DiscardAssignmentExpr] _ +# 159| getSource(): [BinaryExpr] ... .&(_:_:) ... +# 159| getFunction(): [MethodRefExpr] .&(_:_:) +# 159| getBase(): [TypeExpr] Int.Type +# 159| getTypeRepr(): [TypeRepr] Int +# 159| getArgument(0): [Argument] : 1 +# 159| getExpr(): [IntegerLiteralExpr] 1 +# 159| getArgument(1): [Argument] : 2 +# 159| getExpr(): [IntegerLiteralExpr] 2 +# 160| getElement(2): [AssignExpr] ... = ... +# 160| getDest(): [DiscardAssignmentExpr] _ +# 160| getSource(): [BinaryExpr] ... .|(_:_:) ... +# 160| getFunction(): [MethodRefExpr] .|(_:_:) +# 160| getBase(): [TypeExpr] Int.Type +# 160| getTypeRepr(): [TypeRepr] Int +# 160| getArgument(0): [Argument] : 1 +# 160| getExpr(): [IntegerLiteralExpr] 1 +# 160| getArgument(1): [Argument] : 2 +# 160| getExpr(): [IntegerLiteralExpr] 2 +# 161| getElement(3): [AssignExpr] ... = ... +# 161| getDest(): [DiscardAssignmentExpr] _ +# 161| getSource(): [BinaryExpr] ... .^(_:_:) ... +# 161| getFunction(): [MethodRefExpr] .^(_:_:) +# 161| getBase(): [TypeExpr] Int.Type +# 161| getTypeRepr(): [TypeRepr] Int +# 161| getArgument(0): [Argument] : 1 +# 161| getExpr(): [IntegerLiteralExpr] 1 +# 161| getArgument(1): [Argument] : 2 +# 161| getExpr(): [IntegerLiteralExpr] 2 +# 162| getElement(4): [AssignExpr] ... = ... +# 162| getDest(): [DiscardAssignmentExpr] _ +# 162| getSource(): [BinaryExpr] ... .<<(_:_:) ... +# 162| getFunction(): [MethodRefExpr] .<<(_:_:) +# 162| getBase(): [TypeExpr] Int.Type +# 162| getTypeRepr(): [TypeRepr] Int +# 162| getArgument(0): [Argument] : 1 +# 162| getExpr(): [IntegerLiteralExpr] 1 +# 162| getArgument(1): [Argument] : 0 +# 162| getExpr(): [IntegerLiteralExpr] 0 +# 163| getElement(5): [AssignExpr] ... = ... +# 163| getDest(): [DiscardAssignmentExpr] _ +# 163| getSource(): [BinaryExpr] ... .>>(_:_:) ... +# 163| getFunction(): [MethodRefExpr] .>>(_:_:) +# 163| getBase(): [TypeExpr] Int.Type +# 163| getTypeRepr(): [TypeRepr] Int +# 163| getArgument(0): [Argument] : 1 +# 163| getExpr(): [IntegerLiteralExpr] 1 +# 163| getArgument(1): [Argument] : 0 +# 163| getExpr(): [IntegerLiteralExpr] 0 patterns.swift: # 1| [ConcreteFuncDecl] basic_patterns() # 1| InterfaceType = () -> () diff --git a/swift/ql/test/library-tests/ast/expressions.swift b/swift/ql/test/library-tests/ast/expressions.swift index bc720abe70f..3e16c0be0ac 100644 --- a/swift/ql/test/library-tests/ast/expressions.swift +++ b/swift/ql/test/library-tests/ast/expressions.swift @@ -152,4 +152,13 @@ func test(a : A, keyPathInt : WritableKeyPath, keyPathB : WritableKeyPat var apply_keyPathInt = a[keyPath: keyPathInt] var apply_keyPathB = a[keyPath: keyPathB] var nested_apply = a[keyPath: keyPathB][keyPath: \B.x] -} \ No newline at end of file +} + +func bitwise() { + _ = ~1 + _ = 1 & 2 + _ = 1 | 2 + _ = 1 ^ 2 + _ = 1 << 0 + _ = 1 >> 0 +} diff --git a/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.expected b/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.expected new file mode 100644 index 00000000000..8c8c5fe47e2 --- /dev/null +++ b/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.expected @@ -0,0 +1,6 @@ +| bitwiseoperation.swift:2:7:2:8 | call to ~(_:) | NotBitwiseExpr, UnaryBitwiseOperation | +| bitwiseoperation.swift:3:7:3:11 | ... .&(_:_:) ... | AndBitwiseExpr, BinaryBitwiseOperation | +| bitwiseoperation.swift:4:7:4:11 | ... .\|(_:_:) ... | BinaryBitwiseOperation, OrBitwiseExpr | +| bitwiseoperation.swift:5:7:5:11 | ... .^(_:_:) ... | BinaryBitwiseOperation, XorBitwiseExpr | +| bitwiseoperation.swift:6:7:6:12 | ... .<<(_:_:) ... | BinaryBitwiseOperation, ShiftLeftBitwiseExpr | +| bitwiseoperation.swift:7:7:7:12 | ... .>>(_:_:) ... | BinaryBitwiseOperation, ShiftRightBitwiseExpr | diff --git a/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.ql b/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.ql new file mode 100644 index 00000000000..6e52b7d8e4a --- /dev/null +++ b/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.ql @@ -0,0 +1,22 @@ +import swift + +string describe(BitwiseOperation e) { + e instanceof BinaryBitwiseOperation and result = "BinaryBitwiseOperation" + or + e instanceof AndBitwiseExpr and result = "AndBitwiseExpr" + or + e instanceof OrBitwiseExpr and result = "OrBitwiseExpr" + or + e instanceof XorBitwiseExpr and result = "XorBitwiseExpr" + or + e instanceof ShiftLeftBitwiseExpr and result = "ShiftLeftBitwiseExpr" + or + e instanceof ShiftRightBitwiseExpr and result = "ShiftRightBitwiseExpr" + or + e instanceof UnaryBitwiseOperation and result = "UnaryBitwiseOperation" + or + e instanceof NotBitwiseExpr and result = "NotBitwiseExpr" +} + +from BitwiseOperation e +select e, concat(describe(e), ", ") diff --git a/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.swift b/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.swift new file mode 100644 index 00000000000..2c6ae67eb84 --- /dev/null +++ b/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.swift @@ -0,0 +1,8 @@ +func bitwise() { + _ = ~1 + _ = 1 & 2 + _ = 1 | 2 + _ = 1 ^ 2 + _ = 1 << 0 + _ = 1 >> 0 +} diff --git a/swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.expected b/swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.expected new file mode 100644 index 00000000000..6c02fd2c9b4 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.expected @@ -0,0 +1,17 @@ +edges +| test.swift:43:35:43:130 | [...] : | test.swift:51:49:51:49 | constantSalt | +| test.swift:43:35:43:130 | [...] : | test.swift:56:59:56:59 | constantSalt | +| test.swift:43:35:43:130 | [...] : | test.swift:62:59:62:59 | constantSalt | +| test.swift:43:35:43:130 | [...] : | test.swift:67:53:67:53 | constantSalt | +nodes +| test.swift:43:35:43:130 | [...] : | semmle.label | [...] : | +| test.swift:51:49:51:49 | constantSalt | semmle.label | constantSalt | +| test.swift:56:59:56:59 | constantSalt | semmle.label | constantSalt | +| test.swift:62:59:62:59 | constantSalt | semmle.label | constantSalt | +| test.swift:67:53:67:53 | constantSalt | semmle.label | constantSalt | +subpaths +#select +| test.swift:51:49:51:49 | constantSalt | test.swift:43:35:43:130 | [...] : | test.swift:51:49:51:49 | constantSalt | The value '[...]' is used as a constant salt, which is insecure for hashing passwords. | +| test.swift:56:59:56:59 | constantSalt | test.swift:43:35:43:130 | [...] : | test.swift:56:59:56:59 | constantSalt | The value '[...]' is used as a constant salt, which is insecure for hashing passwords. | +| test.swift:62:59:62:59 | constantSalt | test.swift:43:35:43:130 | [...] : | test.swift:62:59:62:59 | constantSalt | The value '[...]' is used as a constant salt, which is insecure for hashing passwords. | +| test.swift:67:53:67:53 | constantSalt | test.swift:43:35:43:130 | [...] : | test.swift:67:53:67:53 | constantSalt | The value '[...]' is used as a constant salt, which is insecure for hashing passwords. | \ No newline at end of file diff --git a/swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.qlref b/swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.qlref new file mode 100644 index 00000000000..04aadc2161f --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.qlref @@ -0,0 +1 @@ +queries/Security/CWE-760/ConstantSalt.ql diff --git a/swift/ql/test/query-tests/Security/CWE-760/test.swift b/swift/ql/test/query-tests/Security/CWE-760/test.swift new file mode 100644 index 00000000000..05f5396d244 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-760/test.swift @@ -0,0 +1,70 @@ + +// --- stubs --- + +// These stubs roughly follows the same structure as classes from CryptoSwift +enum PKCS5 { } + +enum Variant { case md5, sha1, sha2, sha3 } + +extension PKCS5 { + struct PBKDF1 { + init(password: Array, salt: Array, variant: Variant = .sha1, iterations: Int = 4096, keyLength: Int? = nil) { } + } + + struct PBKDF2 { + init(password: Array, salt: Array, iterations: Int = 4096, keyLength: Int? = nil, variant: Variant = .sha2) { } + } +} + +struct HKDF { + init(password: Array, salt: Array? = nil, info: Array? = nil, keyLength: Int? = nil, variant: Variant = .sha2) { } +} + +final class Scrypt { + init(password: Array, salt: Array, dkLen: Int, N: Int, r: Int, p: Int) { } +} + +// Helper functions +func getConstantString() -> String { + "this string is constant" +} + +func getConstantArray() -> Array { + [UInt8](getConstantString().utf8) +} + +func getRandomArray() -> Array { + (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) }) +} + +// --- tests --- + +func test() { + let constantSalt: Array = [0x2a, 0x3a, 0x80, 0x05, 0xaf, 0x46, 0x58, 0x2d, 0x66, 0x52, 0x10, 0xae, 0x86, 0xd3, 0x8e, 0x8f] + let constantStringSalt = getConstantArray() + let randomSalt = getRandomArray() + let randomArray = getRandomArray() + let variant = Variant.sha2 + let iterations = 120120 + + // HKDF test cases + let hkdfb1 = HKDF(password: randomArray, salt: constantSalt, info: randomArray, keyLength: 0, variant: variant) // BAD + let hkdfb2 = HKDF(password: randomArray, salt: constantStringSalt, info: randomArray, keyLength: 0, variant: variant) // BAD [NOT DETECTED] + let hkdfg1 = HKDF(password: randomArray, salt: randomSalt, info: randomArray, keyLength: 0, variant: variant) // GOOD + + // PBKDF1 test cases + let pbkdf1b1 = PKCS5.PBKDF1(password: randomArray, salt: constantSalt, iterations: iterations, keyLength: 0) // BAD + let pbkdf1b2 = PKCS5.PBKDF1(password: randomArray, salt: constantStringSalt, iterations: iterations, keyLength: 0) // BAD [NOT DETECTED] + let pbkdf1g1 = PKCS5.PBKDF1(password: randomArray, salt: randomSalt, iterations: iterations, keyLength: 0) // GOOD + + + // PBKDF2 test cases + let pbkdf2b1 = PKCS5.PBKDF2(password: randomArray, salt: constantSalt, iterations: iterations, keyLength: 0) // BAD + let pbkdf2b2 = PKCS5.PBKDF2(password: randomArray, salt: constantStringSalt, iterations: iterations, keyLength: 0) // BAD [NOT DETECTED] + let pbkdf2g1 = PKCS5.PBKDF2(password: randomArray, salt: randomSalt, iterations: iterations, keyLength: 0) // GOOD + + // Scrypt test cases + let scryptb1 = Scrypt(password: randomArray, salt: constantSalt, dkLen: 64, N: 16384, r: 8, p: 1) // BAD + let scryptb2 = Scrypt(password: randomArray, salt: constantStringSalt, dkLen: 64, N: 16384, r: 8, p: 1) // BAD [NOT DETECTED] + let scryptg1 = Scrypt(password: randomArray, salt: randomSalt, dkLen: 64, N: 16384, r: 8, p: 1) // GOOD +} \ No newline at end of file diff --git a/swift/schema.py b/swift/schema.py index 737dd82adeb..11be598a8c6 100644 --- a/swift/schema.py +++ b/swift/schema.py @@ -37,6 +37,13 @@ class Location(Element): class Locatable(Element): location: optional[Location] | cpp.skip | doc("location associated with this element in the code") +@use_for_null +class UnspecifiedElement(Locatable): + parent: optional[Element] + property: string + index: optional[int] + error: string + class Comment(Locatable): text: string diff --git a/swift/third_party/swift-llvm-support/patches/remove_getFallthrougDest_assert.patch b/swift/third_party/swift-llvm-support/patches/remove_getFallthrougDest_assert.patch new file mode 100644 index 00000000000..c16942a807c --- /dev/null +++ b/swift/third_party/swift-llvm-support/patches/remove_getFallthrougDest_assert.patch @@ -0,0 +1,11 @@ +diff -ru a/include/swift/AST/Stmt.h b/include/swift/AST/Stmt.h +--- a/include/swift/AST/Stmt.h 2022-09-21 12:56:54.000000000 +0200 ++++ b/include/swift/AST/Stmt.h 2022-11-04 14:39:18.407971007 +0100 +@@ -920,7 +920,6 @@ + /// Get the CaseStmt block to which the fallthrough transfers control. + /// Set during Sema. + CaseStmt *getFallthroughDest() const { +- assert(FallthroughDest && "fallthrough dest is not set until Sema"); + return FallthroughDest; + } + void setFallthroughDest(CaseStmt *C) {
  • What are Salted Passwords and Password Hashing?