diff --git a/.codeqlmanifest.json b/.codeqlmanifest.json new file mode 100644 index 00000000000..421284bc4b9 --- /dev/null +++ b/.codeqlmanifest.json @@ -0,0 +1,6 @@ +{ "provide": [ "*/ql/src/qlpack.yml", + "*/ql/test/qlpack.yml", + "*/upgrades/qlpack.yml", + "misc/legacy-support/*/qlpack.yml", + "misc/suite-helpers/qlpack.yml", + "codeql/.codeqlmanifest.json" ] } diff --git a/.github/ISSUE_TEMPLATE/ql---general.md b/.github/ISSUE_TEMPLATE/ql---general.md new file mode 100644 index 00000000000..1d5b7d244f6 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/ql---general.md @@ -0,0 +1,14 @@ +--- +name: General issue +about: Tell us if you think something is wrong or if you have a question +title: General issue +labels: question +assignees: '' + +--- + +**Description of the issue** + + + diff --git a/.gitignore b/.gitignore index b5c88ce04ab..459fe556c7f 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,6 @@ # Visual studio temporaries, except a file used by QL4VS .vs/* !.vs/VSWorkspaceSettings.json + +# It's useful (though not required) to be able to unpack codeql in the ql checkout itself +/codeql/ diff --git a/CODEOWNERS b/CODEOWNERS index 45bb2c05818..8d4e4fd9b06 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -1,10 +1,11 @@ +/cpp/ @Semmle/cpp-analysis /csharp/ @Semmle/cs /java/ @Semmle/java /javascript/ @Semmle/js -/cpp/ @Semmle/cpp-analysis -/cpp/**/*.qhelp @semmledocs-ac +/python/ @Semmle/python +/cpp/**/*.qhelp @hubwriter /csharp/**/*.qhelp @jf205 -/java/**/*.qhelp @felicity-semmle -/javascript/**/*.qhelp @mc-semmle -/python/**/*.qhelp @felicity-semmle -/docs/language/ @felicity-semmle @jf205 +/java/**/*.qhelp @felicitymay +/javascript/**/*.qhelp @mchammer01 +/python/**/*.qhelp @felicitymay +/docs/language/ @shati-patel @jf205 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6c0714571fc..e074b1c2079 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,4 +1,4 @@ -# Contributing to QL +# Contributing to CodeQL We welcome contributions to our standard library and standard checks. Got an idea for a new check, or how to improve an existing query? Then please go ahead and open a pull request! @@ -9,13 +9,13 @@ Before we accept your pull request, we require that you have agreed to our Contr If you have an idea for a query that you would like to share with other Semmle users, please open a pull request to add it to this repository. Follow the steps below to help other users understand what your query does, and to ensure that your query is consistent with the other Semmle queries. -1. **Consult the QL documentation for query writers** +1. **Consult the documentation for query writers** - There is lots of useful documentation to help you write QL, ranging from information about query file structure to language-specific tutorials. For more information on the documentation available, see [Writing QL queries](https://help.semmle.com/QL/learn-ql/writing-queries/writing-queries.html) on [help.semmle.com](https://help.semmle.com). + There is lots of useful documentation to help you write queries, ranging from information about query file structure to tutorials for specific target languages. For more information on the documentation available, see [Writing CodeQL queries](https://help.semmle.com/QL/learn-ql/writing-queries/writing-queries.html) on [help.semmle.com](https://help.semmle.com). -2. **Format your QL correctly** +2. **Format your code correctly** - All of Semmle's standard QL queries and libraries are uniformly formatted for clarity and consistency, so we strongly recommend that all QL contributions follow the same formatting guidelines. If you use QL for Eclipse, you can auto-format your query in the [QL editor](https://help.semmle.com/ql-for-eclipse/Content/WebHelp/ql-editor.html). For more information, see the [QL style guide](https://github.com/Semmle/ql/blob/master/docs/ql-style-guide.md). + All of Semmle's standard queries and libraries are uniformly formatted for clarity and consistency, so we strongly recommend that all contributions follow the same formatting guidelines. If you use QL for Eclipse, you can auto-format your query in the [QL editor](https://help.semmle.com/ql-for-eclipse/Content/WebHelp/ql-editor.html). For more information, see the [CodeQL style guide](https://github.com/Semmle/ql/blob/master/docs/ql-style-guide.md). 3. **Make sure your query has the correct metadata** @@ -29,7 +29,7 @@ Follow the steps below to help other users understand what your query does, and The `select` statement of your query must be compatible with the query type (determined by the `@kind` metadata property) for alert or path results to be displayed correctly in LGTM and QL for Eclipse. For more information on `select` statement format, see [Introduction to query files](https://help.semmle.com/QL/learn-ql/writing-queries/introduction-to-queries.html#select-clause) on help.semmle.com. -5. **Save your query in a `.ql` file in correct language directory in this repository** +5. **Save your query in a `.ql` file in the correct language directory in this repository** There are five language-specific directories in this repository: @@ -54,7 +54,7 @@ repositories, which might be made public. We might also use this information to contact you in relation to your contributions, as well as in the normal course of software development. We also store records of your CLA agreements. Under GDPR legislation, we do this -on the basis of our legitimate interest in creating the QL product. +on the basis of our legitimate interest in creating the CodeQL product. Please do get in touch (privacy@semmle.com) if you have any questions about this or our data protection policies. diff --git a/README.md b/README.md index b483d29cae2..c7cafeea95b 100644 --- a/README.md +++ b/README.md @@ -1,16 +1,16 @@ -# Semmle QL +# CodeQL -This open source repository contains the standard QL libraries and queries that power [LGTM](https://lgtm.com), and the other products that [Semmle](https://semmle.com) makes available to its customers worldwide. +This open source repository contains the standard CodeQL libraries and queries that power [LGTM](https://lgtm.com), and the other products that [Semmle](https://semmle.com) makes available to its customers worldwide. -## How do I learn QL and run queries? +## How do I learn CodeQL and run queries? -There is [extensive documentation](https://help.semmle.com/QL/learn-ql/) on getting started with writing QL. -You can use the [interactive query console](https://lgtm.com/help/lgtm/using-query-console) on LGTM.com or the [QL for Eclipse](https://lgtm.com/help/lgtm/running-queries-ide) plugin to try out your queries on any open-source project that's currently being analyzed. +There is [extensive documentation](https://help.semmle.com/QL/learn-ql/) on getting started with writing CodeQL. +You can use the [interactive query console](https://lgtm.com/help/lgtm/using-query-console) on LGTM.com or the [CodeQL for Visual Studio Code](https://help.semmle.com/codeql/codeql-for-vscode.html) extension to try out your queries on any open source project that's currently being analyzed. ## Contributing -We welcome contributions to our standard library and standard checks. Do you have an idea for a new check, or how to improve an existing query? Then please go ahead and open a pull request! Before you do, though, please take the time to read our [contributing guidelines](CONTRIBUTING.md). You can also consult our [style guides](https://github.com/Semmle/ql/tree/master/docs) to learn how to format your QL for consistency and clarity, how to write query metadata, and how to write query help documentation for your query. +We welcome contributions to our standard library and standard checks. Do you have an idea for a new check, or how to improve an existing query? Then please go ahead and open a pull request! Before you do, though, please take the time to read our [contributing guidelines](CONTRIBUTING.md). You can also consult our [style guides](https://github.com/Semmle/ql/tree/master/docs) to learn how to format your code for consistency and clarity, how to write query metadata, and how to write query help documentation for your query. ## License -The QL queries in this repository are licensed under [Apache License 2.0](LICENSE) by [Semmle](https://semmle.com). +The code in this repository is licensed under [Apache License 2.0](LICENSE) by [Semmle](https://semmle.com). diff --git a/change-notes/1.22/analysis-javascript.md b/change-notes/1.22/analysis-javascript.md index fd211197157..fd09522fc25 100644 --- a/change-notes/1.22/analysis-javascript.md +++ b/change-notes/1.22/analysis-javascript.md @@ -36,6 +36,7 @@ | Shift out of range (`js/shift-out-of-range`| Fewer false positive results | This rule now correctly handles BigInt shift operands. | | Superfluous trailing arguments (`js/superfluous-trailing-arguments`) | Fewer false-positive results. | This rule no longer flags calls to placeholder functions that trivially throw an exception. | | Undocumented parameter (`js/jsdoc/missing-parameter`) | No changes to results | This rule is now run on LGTM, although its results are still not shown by default. | +| Missing space in string concatenation (`js/missing-space-in-concatenation`) | Fewer false positive results | The rule now requires a word-like part exists in the string concatenation. | ## Changes to QL libraries diff --git a/change-notes/1.23/analysis-cpp.md b/change-notes/1.23/analysis-cpp.md index ce317236f93..4c7b1d0a26c 100644 --- a/change-notes/1.23/analysis-cpp.md +++ b/change-notes/1.23/analysis-cpp.md @@ -9,6 +9,8 @@ The following changes in version 1.23 affect C/C++ analysis in all applications. | **Query** | **Tags** | **Purpose** | |-----------------------------|-----------|--------------------------------------------------------------------| | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) | reliability, japanese-era | This query is a combination of two old queries that were identical in purpose but separate as an implementation detail. This new query replaces Hard-coded Japanese era start date in call (`cpp/japanese-era/constructor-or-method-with-exact-era-date`) and Hard-coded Japanese era start date in struct (`cpp/japanese-era/struct-with-exact-era-date`). | +| Signed overflow check (`cpp/signed-overflow-check`) | correctness, security | Finds overflow checks that rely on signed integer addition to overflow, which has undefined behavior. Example: `a + b < a`. | +| Pointer overflow check (`cpp/pointer-overflow-check`) | correctness, security | Finds overflow checks that rely on pointer addition to overflow, which has undefined behavior. Example: `ptr + a < ptr`. | ## Changes to existing queries @@ -18,8 +20,15 @@ The following changes in version 1.23 affect C/C++ analysis in all applications. | Hard-coded Japanese era start date in call (`cpp/japanese-era/constructor-or-method-with-exact-era-date`) | Deprecated | This query has been deprecated. Use the new combined query Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) instead. | | Hard-coded Japanese era start date in struct (`cpp/japanese-era/struct-with-exact-era-date`) | Deprecated | This query has been deprecated. Use the new combined query Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) instead. | | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) | More correct results | This query now checks for the beginning date of the Reiwa era (1st May 2019). | +| Sign check of bitwise operation (`cpp/bitwise-sign-check`) | Fewer false positive results | Results involving `>=` or `<=` are no longer reported. | +| Too few arguments to formatting function (`cpp/wrong-number-format-arguments`) | Fewer false positive results | Fixed false positives resulting from mistmatching declarations of a formatting function. | +| Too many arguments to formatting function (`cpp/too-many-format-arguments`) | Fewer false positive results | Fixed false positives resulting from mistmatching declarations of a formatting function. | +| Unclear comparison precedence (`cpp/comparison-precedence`) | Fewer false positive results | False positives involving template classes and functions have been fixed. | +| Comparison of narrow type with wide type in loop condition (`cpp/comparison-with-wider-type`) | Higher precision | The precision of this query has been increased to "high" as the alerts from this query have proved to be valuable on real-world projects. With this precision, results are now displayed by default in LGTM. | +| Non-constant format string (`cpp/non-constant-format`) | Fewer false positive results | Fixed false positives resulting from mistmatching declarations of a formatting function. | +| Wrong type of arguments to formatting function (`cpp/wrong-type-format-argument`) | More correct results and fewer false positive results | This query now understands explicitly specified argument numbers in format strings, such as the `1$` in `%1$s`. | -## Changes to QL libraries +## Changes to libraries * The data-flow library has been extended with a new feature to aid debugging. Instead of specifying `isSink(Node n) { any() }` on a configuration to @@ -28,10 +37,31 @@ The following changes in version 1.23 affect C/C++ analysis in all applications. picture of the partial flow paths from a given source. The feature is disabled by default and can be enabled for individual configurations by overriding `int explorationLimit()`. +* The data-flow library now supports flow out of C++ reference parameters. +* The data-flow library now allows flow through the address-of operator (`&`). * The `DataFlow::DefinitionByReferenceNode` class now considers `f(x)` to be a definition of `x` when `x` is a variable of pointer type. It no longer considers deep paths such as `f(&x.myField)` to be definitions of `x`. These changes are in line with the user expectations we've observed. +* The data-flow library now makes it easier to specify barriers/sanitizers + arising from guards by overriding the predicate + `isBarrierGuard`/`isSanitizerGuard` on data-flow and taint-tracking + configurations respectively. * There is now a `DataFlow::localExprFlow` predicate and a `TaintTracking::localExprTaint` predicate to make it easy to use the most common case of local data flow and taint: from one `Expr` to another. +* The member predicates of the `FunctionInput` and `FunctionOutput` classes have been renamed for + clarity (e.g. `isOutReturnPointer()` to `isReturnValueDeref()`). The existing member predicates + have been deprecated, and will be removed in a future release. Code that uses the old member + predicates should be updated to use the corresponding new member predicate. +* The predicates `Declaration.hasStdName()` and `Declaration.hasGlobalOrStdName` + have been added, simplifying handling of C++ standard library functions. +* The control-flow graph is now computed in QL, not in the extractor. This can + lead to regressions (or improvements) in how queries are optimized because + optimization in QL relies on static size estimates, and the control-flow edge + relations will now have different size estimates than before. +* Support has been added for non-type template arguments. This means that the + return type of `Declaration::getTemplateArgument()` and + `Declaration::getATemplateArgument` have changed to `Locatable`. See the + documentation for `Declaration::getTemplateArgument()` and + `Declaration::getTemplateArgumentKind()` for details. diff --git a/change-notes/1.23/analysis-csharp.md b/change-notes/1.23/analysis-csharp.md index d22f7ea567a..c5bdae38dcd 100644 --- a/change-notes/1.23/analysis-csharp.md +++ b/change-notes/1.23/analysis-csharp.md @@ -8,13 +8,18 @@ The following changes in version 1.23 affect C# analysis in all applications. | **Query** | **Tags** | **Purpose** | |-----------------------------|-----------|--------------------------------------------------------------------| +| Deserialized delegate (`cs/deserialized-delegate`) | security, external/cwe/cwe-502 | Finds unsafe deserialization of delegate types. | +| Deserialization of untrusted data (`cs/unsafe-deserialization-untrusted-input`) | security, external/cwe/cwe-502 | Finds flow of untrusted input to calls to unsafe deserializers. | | Unsafe year argument for 'DateTime' constructor (`cs/unsafe-year-construction`) | reliability, date-time | Finds incorrect manipulation of `DateTime` values, which could lead to invalid dates. | +| Unsafe deserializer (`cs/unsafe-deserialization`) | security, external/cwe/cwe-502 | Finds calls to unsafe deserializers. | | Mishandling the Japanese era start date (`cs/mishandling-japanese-era`) | reliability, date-time | Finds hard-coded Japanese era start dates that could be invalid. | ## Changes to existing queries | **Query** | **Expected impact** | **Change** | |------------------------------|------------------------|-----------------------------------| +| Dereferenced variable may be null (`cs/dereferenced-value-may-be-null`) | Fewer false positive results | More `null` checks are now taken into account, including `null` checks for `dynamic` expressions and `null` checks such as `object alwaysNull = null; if (x != alwaysNull) ...`. | +| Missing Dispose call on local IDisposable (`cs/local-not-disposed`) | Fewer false positive results | The query has been rewritten in order to identify more dispose patterns. For example, a local `IDisposable` that is disposed of by passing through a fluent API is no longer reported. | ## Removal of old queries @@ -22,7 +27,7 @@ The following changes in version 1.23 affect C# analysis in all applications. * `nameof` expressions are now extracted correctly when the name is a namespace. -## Changes to QL libraries +## Changes to libraries * The new class `NamespaceAccess` models accesses to namespaces, for example in `nameof` expressions. * The data-flow library now makes it easier to specify barriers/sanitizers @@ -37,5 +42,11 @@ The following changes in version 1.23 affect C# analysis in all applications. disabled by default and can be enabled for individual configurations by overriding `int explorationLimit()`. * `foreach` statements where the body is guaranteed to be executed at least once, such as `foreach (var x in new string[]{ "a", "b", "c" }) { ... }`, are now recognized by all analyses based on the control flow graph (such as SSA, data flow and taint tracking). +* Fixed the control flow graph for `switch` statements where the `default` case was not the last case. This had caused the remaining cases to be unreachable. `SwitchStmt.getCase(int i)` now puts the `default` case last. +* There is now a `DataFlow::localExprFlow` predicate and a + `TaintTracking::localExprTaint` predicate to make it easy to use the most + common case of local data flow and taint: from one `Expr` to another. +* Data is now tracked through null-coalescing expressions (`??`). +* A new library `semmle.code.csharp.Unification` has been added. This library exposes two predicates `unifiable` and `subsumes` for calculating type unification and type subsumption, respectively. ## Changes to autobuilder diff --git a/change-notes/1.23/analysis-java.md b/change-notes/1.23/analysis-java.md index 76085832c01..8c38f57e9d2 100644 --- a/change-notes/1.23/analysis-java.md +++ b/change-notes/1.23/analysis-java.md @@ -2,15 +2,24 @@ The following changes in version 1.23 affect Java analysis in all applications. +## New queries + +| **Query** | **Tags** | **Purpose** | +|-----------------------------|-----------|--------------------------------------------------------------------| +| Continue statement that does not continue (`java/continue-in-false-loop`) | correctness | Finds `continue` statements in `do { ... } while (false)` loops. | + ## Changes to existing queries | **Query** | **Expected impact** | **Change** | |------------------------------|------------------------|-----------------------------------| +| Dereferenced variable may be null (`java/dereferenced-value-may-be-null`) | Fewer false positives | Certain indirect null guards involving two auxiliary variables known to be equal can now be detected. | +| Non-synchronized override of synchronized method (`java/non-sync-override`) | Fewer false positives | Results are now only reported if the immediately overridden method is synchronized. | | Query built from user-controlled sources (`java/sql-injection`) | More results | The query now identifies arguments to `Statement.executeLargeUpdate` and `Connection.prepareCall` as SQL expressions sinks. | | Query built from local-user-controlled sources (`java/sql-injection-local`) | More results | The query now identifies arguments to `Statement.executeLargeUpdate` and `Connection.prepareCall` as SQL expressions sinks. | | Query built without neutralizing special characters (`java/concatenated-sql-query`) | More results | The query now identifies arguments to `Statement.executeLargeUpdate` and `Connection.prepareCall` as SQL expressions sinks. | +| Useless comparison test (`java/constant-comparison`) | Fewer false positives | Additional overflow check patterns are now recognized and no longer reported. | -## Changes to QL libraries +## Changes to libraries * The data-flow library has been extended with a new feature to aid debugging. Instead of specifying `isSink(Node n) { any() }` on a configuration to diff --git a/change-notes/1.23/analysis-javascript.md b/change-notes/1.23/analysis-javascript.md index 3b570c12270..e815f5e8ba0 100644 --- a/change-notes/1.23/analysis-javascript.md +++ b/change-notes/1.23/analysis-javascript.md @@ -2,30 +2,81 @@ ## General improvements +* Automatic classification of generated and minified files has been improved, in particular files generated by Doxygen are now recognized. + +* Support for `globalThis` has been added. + * Support for the following frameworks and libraries has been improved: - [firebase](https://www.npmjs.com/package/firebase) + - [get-them-args](https://www.npmjs.com/package/get-them-args) + - [minimist](https://www.npmjs.com/package/minimist) - [mongodb](https://www.npmjs.com/package/mongodb) - [mongoose](https://www.npmjs.com/package/mongoose) + - [optimist](https://www.npmjs.com/package/optimist) + - [parse-torrent](https://www.npmjs.com/package/parse-torrent) + - [rate-limiter-flexible](https://www.npmjs.com/package/rate-limiter-flexible) + - [yargs](https://www.npmjs.com/package/yargs) * The call graph has been improved to resolve method calls in more cases. This may produce more security alerts. +* TypeScript 3.6 and 3.7 features are now supported. + ## New queries | **Query** | **Tags** | **Purpose** | |---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| Unused index variable (`js/unused-index-variable`) | correctness | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. | - +| Ignoring result from pure array method (`js/ignore-array-result`) | maintainability, correctness | Highlights calls to array methods without side effects where the return value is ignored. Results are shown on LGTM by default. | +| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | security, correctness, external/cwe/cwe-020 | Highlights checks for `javascript:` URLs that do not take `data:` or `vbscript:` URLs into account. Results are shown on LGTM by default. | +| Loop bound injection (`js/loop-bound-injection`) | security, external/cwe/cwe-834 | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are shown on LGTM by default. | +| Shell command built from environment values (`js/shell-command-injection-from-environment`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights shell commands that may change behavior inadvertently depending on the execution environment, indicating a possible violation of [CWE-78](https://cwe.mitre.org/data/definitions/78.html). Results are shown on LGTM by default.| +| Suspicious method name (`js/suspicious-method-name-declaration`) | correctness, typescript, methods | Highlights suspiciously named methods where the developer likely meant to write a constructor or function. Results are shown on LGTM by default. | +| Unreachable method overloads (`js/unreachable-method-overloads`) | correctness, typescript | Highlights method overloads that are impossible to use from client code. Results are shown on LGTM by default. | +| Unused index variable (`js/unused-index-variable`) | correctness | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. Results are shown on LGTM by default. | +| Use of returnless function (`js/use-of-returnless-function`) | maintainability, correctness | Highlights calls where the return value is used, but the callee never returns a value. Results are shown on LGTM by default. | +| Useless regular expression character escape (`js/useless-regexp-character-escape`) | correctness, security, external/cwe/cwe-20 | Highlights regular expression strings with useless character escapes, indicating a possible violation of [CWE-20](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default. | ## Changes to existing queries | **Query** | **Expected impact** | **Change** | |--------------------------------|------------------------------|---------------------------------------------------------------------------| | Incomplete string escaping or encoding (`js/incomplete-sanitization`) | Fewer false-positive results | This rule now recognizes additional ways delimiters can be stripped away. | -| Client-side cross-site scripting (`js/xss`) | More results | More potential vulnerabilities involving functions that manipulate DOM attributes are now recognized. | +| Client-side cross-site scripting (`js/xss`) | More results, fewer false-positive results | More potential vulnerabilities involving functions that manipulate DOM attributes are now recognized, and more sanitizers are detected. | | Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving functions that manipulate DOM event handler attributes are now recognized. | -| Prototype pollution (`js/prototype-pollution`) | More results | The query now highlights vulnerable uses of jQuery and Angular, and the results are shown on LGTM by default. | +| Hard-coded credentials (`js/hardcoded-credentials`) | Fewer false-positive results | This rule now flags fewer password examples. | +| Illegal invocation (`js/illegal-invocation`) | Fewer false-positive results | This rule now correctly handles methods named `call` and `apply`. | | Incorrect suffix check (`js/incorrect-suffix-check`) | Fewer false-positive results | The query recognizes valid checks in more cases. | +| Network data written to file (`js/http-to-file-access`) | Fewer false-positive results | This query has been renamed to better match its intended purpose, and now only considers network data untrusted. | +| Password in configuration file (`js/password-in-configuration-file`) | Fewer false-positive results | This rule now flags fewer password examples. | +| Prototype pollution (`js/prototype-pollution`) | More results | The query now highlights vulnerable uses of jQuery and Angular, and the results are shown on LGTM by default. | +| Reflected cross-site scripting (`js/reflected-xss`) | Fewer false-positive results | The query now recognizes more sanitizers. | +| Stored cross-site scripting (`js/stored-xss`) | Fewer false-positive results | The query now recognizes more sanitizers. | +| Uncontrolled command line (`js/command-line-injection`) | More results | This query now treats responses from servers as untrusted. | +| Uncontrolled data used in path expression (`js/path-injection`) | Fewer false-positive results | This query now recognizes calls to Express `sendFile` as safe in some cases. | +| Unknown directive (`js/unknown-directive`) | Fewer false positive results | This query no longer flags uses of ":", which is sometimes used like a directive. | -## Changes to QL libraries +## Changes to libraries * `Expr.getDocumentation()` now handles chain assignments. +* String literals are now parsed as regular expressions. + Consequently, a `RegExpTerm` may occur as part of a string literal or + as a regular expression literal. Queries that search for regular expressions may need to + use `RegExpTerm.isPartOfRegExpLiteral` or `RegExpTerm.isUsedAsRegExp` to restrict the search. + A regular expression AST can be obtained from a string literal using `StringLiteral.asRegExp`. + +## Removal of deprecated queries + +The following queries (deprecated since 1.17) are no longer available in the distribution: + +* Builtin redefined (js/builtin-redefinition) +* Inefficient method definition (js/method-definition-in-constructor) +* Bad parity check (js/incomplete-parity-check) +* Potentially misspelled property or variable name (js/wrong-capitalization) +* Unknown JSDoc tag (js/jsdoc/unknown-tag-type) +* Invalid JSLint directive (js/jslint/invalid-directive) +* Malformed JSLint directive (js/jslint/malformed-directive) +* Use of HTML comments (js/html-comment) +* Multi-line string literal (js/multi-line-string) +* Octal literal (js/octal-literal) +* Reserved word used as variable name (js/use-of-reserved-word) +* Trailing comma in array or object expressions (js/trailing-comma-in-array-or-object) +* Call to parseInt without radix (js/parseint-without-radix) diff --git a/change-notes/1.23/analysis-python.md b/change-notes/1.23/analysis-python.md index 3319697fcfb..6cea1745284 100644 --- a/change-notes/1.23/analysis-python.md +++ b/change-notes/1.23/analysis-python.md @@ -11,4 +11,17 @@ |-----------|----------|-------------| | Clear-text logging of sensitive information (`py/clear-text-logging-sensitive-data`) | security, external/cwe/cwe-312 | Finds instances where sensitive information is logged without encryption or hashing. Results are shown on LGTM by default. | | Clear-text storage of sensitive information (`py/clear-text-storage-sensitive-data`) | security, external/cwe/cwe-312 | Finds instances where sensitive information is stored without encryption or hashing. Results are shown on LGTM by default. | +| Binding a socket to all network interfaces (`py/bind-socket-all-network-interfaces`) | security | Finds instances where a socket is bound to all network interfaces. Results are shown on LGTM by default. | + +## Changes to existing queries + +| **Query** | **Expected impact** | **Change** | +|----------------------------|------------------------|------------| +| Unreachable code | Fewer false positives | Analysis now accounts for uses of `contextlib.suppress` to suppress exceptions. | +| `__iter__` method returns a non-iterator | Better alert message | Alert now highlights which class is expected to be an iterator. | + + +## Changes to QL libraries + +* Django library now recognizes positional arguments from a `django.conf.urls.url` regex (Django version 1.x) diff --git a/change-notes/1.23/extractor-javascript.md b/change-notes/1.23/extractor-javascript.md index ed9664ce812..8b7a35f5b4f 100644 --- a/change-notes/1.23/extractor-javascript.md +++ b/change-notes/1.23/extractor-javascript.md @@ -5,3 +5,19 @@ ## Changes to code extraction * Asynchronous generator methods are now parsed correctly and no longer cause a spurious syntax error. +* Files in `node_modules` and `bower_components` folders are no longer extracted by default. If you still want to extract files from these folders, you can add the following filters to your `lgtm.yml` file (or add them to existing filters): + +```yaml +extraction: + javascript: + index: + filters: + - include: "**/node_modules" + - include: "**/bower_components" +``` + +* Recognition of CommonJS modules has improved. As a result, some files that were previously extracted as + global scripts are now extracted as modules. +* Top-level `await` is now supported. +* A bug was fixed in how the TypeScript extractor handles default-exported anonymous classes. +* A bug was fixed in how the TypeScript extractor handles computed instance field names. diff --git a/change-notes/support/README.md b/change-notes/support/README.md index 181d5152126..f111a437183 100644 --- a/change-notes/support/README.md +++ b/change-notes/support/README.md @@ -1,6 +1,6 @@ # Files moved to ``docs`` directory -Now that all of the QL documentation is in this repository, +Now that all of the CodeQL documentation is in this repository, notes on the languages, compilers, and frameworks supported have moved. They're now stored as part of the Sphinx ``support`` project with the other documentation: -``docs/ql-documentation/support``. +``docs/language/support``. diff --git a/config/identical-files.json b/config/identical-files.json index 6a216ac256d..4fdac5c7fea 100644 --- a/config/identical-files.json +++ b/config/identical-files.json @@ -47,31 +47,40 @@ "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll", - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Instruction.qll" ], "IR IRBlock": [ "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRBlock.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRBlock.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll", - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRBlock.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRBlock.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/IRBlock.qll" ], "IR IRVariable": [ "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRVariable.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRVariable.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRVariable.qll", - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRVariable.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRVariable.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/IRVariable.qll" ], "IR IRFunction": [ "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRFunction.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRFunction.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRFunction.qll", - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRFunction.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRFunction.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/IRFunction.qll" ], "IR Operand": [ "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Operand.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll", - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Operand.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Operand.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Operand.qll" + ], + "IR IRType": [ + "cpp/ql/src/semmle/code/cpp/ir/implementation/IRType.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/IRType.qll" ], "IR Operand Tag": [ "cpp/ql/src/semmle/code/cpp/ir/implementation/internal/OperandTag.qll", @@ -85,19 +94,22 @@ "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IR.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IR.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IR.qll", - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IR.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IR.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/IR.qll" ], "IR IRSanity": [ "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRSanity.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRSanity.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRSanity.qll", - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRSanity.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRSanity.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/IRSanity.qll" ], "IR PrintIR": [ "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/PrintIR.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/PrintIR.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/PrintIR.qll", - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/PrintIR.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/PrintIR.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/PrintIR.qll" ], "IR IntegerConstant": [ "cpp/ql/src/semmle/code/cpp/ir/internal/IntegerConstant.qll", @@ -161,22 +173,39 @@ "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/PrintIRImports.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/PrintIRImports.qll" ], + "C++ SSA SSAConstructionImports": [ + "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstructionImports.qll", + "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstructionImports.qll" + ], "C++ SSA AliasAnalysis": [ "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll" ], - "C++ SSA SSAConstruction": [ + "C++ IR ValueNumberingImports": [ + "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/internal/ValueNumberingImports.qll", + "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/internal/ValueNumberingImports.qll", + "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/internal/ValueNumberingImports.qll" + ], + "IR SSA SimpleSSA": [ + "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll" + ], + "IR SSA SSAConstruction": [ "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll", - "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll" + "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll" ], - "C++ SSA PrintSSA": [ + "IR SSA PrintSSA": [ "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/PrintSSA.qll", - "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/PrintSSA.qll" + "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/PrintSSA.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/PrintSSA.qll" ], - "C++ IR ValueNumber": [ + "IR ValueNumber": [ "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll", "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll", - "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll" + "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/gvn/ValueNumbering.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll" ], "C++ IR ConstantAnalysis": [ "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/constant/ConstantAnalysis.qll", @@ -205,21 +234,31 @@ "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/reachability/PrintDominance.qll" ], "C# IR InstructionImports": [ - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/InstructionImports.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/InstructionImports.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/InstructionImports.qll" ], "C# IR IRImports": [ - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/IRImports.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/IRImports.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/IRImports.qll" ], "C# IR IRBlockImports": [ - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/IRBlockImports.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/IRBlockImports.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/IRBlockImports.qll" ], "C# IR IRVariableImports": [ - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/IRVariableImports.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/IRVariableImports.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/IRVariableImports.qll" ], "C# IR OperandImports": [ - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/OperandImports.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/OperandImports.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/OperandImports.qll" ], "C# IR PrintIRImports": [ - "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/PrintIRImports.qll" + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/PrintIRImports.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/PrintIRImports.qll" + ], + "C# IR ValueNumberingImports": [ + "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/gvn/internal/ValueNumberingImports.qll", + "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/gvn/internal/ValueNumberingImports.qll" ] } diff --git a/cpp/ql/src/Best Practices/Unused Entities/UnusedStaticVariables.ql b/cpp/ql/src/Best Practices/Unused Entities/UnusedStaticVariables.ql index 26cf42521e5..3ad43998d18 100644 --- a/cpp/ql/src/Best Practices/Unused Entities/UnusedStaticVariables.ql +++ b/cpp/ql/src/Best Practices/Unused Entities/UnusedStaticVariables.ql @@ -21,6 +21,7 @@ from Variable v where v.isStatic() and v.hasDefinition() and + not v.isConstexpr() and not exists(VariableAccess a | a.getTarget() = v) and not v instanceof MemberVariable and not declarationHasSideEffects(v) and diff --git a/cpp/ql/src/Critical/DescriptorMayNotBeClosed.ql b/cpp/ql/src/Critical/DescriptorMayNotBeClosed.ql index 9c93e066119..47401c6eea5 100644 --- a/cpp/ql/src/Critical/DescriptorMayNotBeClosed.ql +++ b/cpp/ql/src/Critical/DescriptorMayNotBeClosed.ql @@ -13,7 +13,7 @@ import semmle.code.cpp.pointsto.PointsTo import Negativity predicate closeCall(FunctionCall fc, Variable v) { - fc.getTarget().hasGlobalName("close") and v.getAnAccess() = fc.getArgument(0) + fc.getTarget().hasGlobalOrStdName("close") and v.getAnAccess() = fc.getArgument(0) or exists(FunctionCall midcall, Function mid, int arg | fc.getArgument(arg) = v.getAnAccess() and diff --git a/cpp/ql/src/Critical/DescriptorNeverClosed.ql b/cpp/ql/src/Critical/DescriptorNeverClosed.ql index b52af1bf2a3..f06708f4ae3 100644 --- a/cpp/ql/src/Critical/DescriptorNeverClosed.ql +++ b/cpp/ql/src/Critical/DescriptorNeverClosed.ql @@ -13,7 +13,7 @@ import semmle.code.cpp.pointsto.PointsTo predicate closed(Expr e) { exists(FunctionCall fc | - fc.getTarget().hasGlobalName("close") and + fc.getTarget().hasGlobalOrStdName("close") and fc.getArgument(0) = e ) } diff --git a/cpp/ql/src/Critical/MemoryMayNotBeFreed.ql b/cpp/ql/src/Critical/MemoryMayNotBeFreed.ql index 9bba8e9896d..2bede681912 100644 --- a/cpp/ql/src/Critical/MemoryMayNotBeFreed.ql +++ b/cpp/ql/src/Critical/MemoryMayNotBeFreed.ql @@ -53,7 +53,7 @@ predicate allocCallOrIndirect(Expr e) { * can cause memory leaks. */ predicate verifiedRealloc(FunctionCall reallocCall, Variable v, ControlFlowNode verified) { - reallocCall.getTarget().hasGlobalName("realloc") and + reallocCall.getTarget().hasGlobalOrStdName("realloc") and reallocCall.getArgument(0) = v.getAnAccess() and ( exists(Variable newV, ControlFlowNode node | @@ -79,7 +79,7 @@ predicate verifiedRealloc(FunctionCall reallocCall, Variable v, ControlFlowNode predicate freeCallOrIndirect(ControlFlowNode n, Variable v) { // direct free call freeCall(n, v.getAnAccess()) and - not n.(FunctionCall).getTarget().hasGlobalName("realloc") + not n.(FunctionCall).getTarget().hasGlobalOrStdName("realloc") or // verified realloc call verifiedRealloc(_, v, n) diff --git a/cpp/ql/src/Critical/OverflowCalculated.ql b/cpp/ql/src/Critical/OverflowCalculated.ql index 36ee0140cf7..fbca4a20a8f 100644 --- a/cpp/ql/src/Critical/OverflowCalculated.ql +++ b/cpp/ql/src/Critical/OverflowCalculated.ql @@ -13,10 +13,7 @@ import cpp class MallocCall extends FunctionCall { - MallocCall() { - this.getTarget().hasGlobalName("malloc") or - this.getTarget().hasQualifiedName("std", "malloc") - } + MallocCall() { this.getTarget().hasGlobalOrStdName("malloc") } Expr getAllocatedSize() { if this.getArgument(0) instanceof VariableAccess @@ -36,12 +33,12 @@ predicate spaceProblem(FunctionCall append, string msg) { malloc.getAllocatedSize() = add and buffer.getAnAccess() = strlen.getStringExpr() and ( - insert.getTarget().hasGlobalName("strcpy") or - insert.getTarget().hasGlobalName("strncpy") + insert.getTarget().hasGlobalOrStdName("strcpy") or + insert.getTarget().hasGlobalOrStdName("strncpy") ) and ( - append.getTarget().hasGlobalName("strcat") or - append.getTarget().hasGlobalName("strncat") + append.getTarget().hasGlobalOrStdName("strcat") or + append.getTarget().hasGlobalOrStdName("strncat") ) and malloc.getASuccessor+() = insert and insert.getArgument(1) = buffer.getAnAccess() and diff --git a/cpp/ql/src/Critical/OverflowDestination.ql b/cpp/ql/src/Critical/OverflowDestination.ql index d7b02b5d8d5..ad925daed62 100644 --- a/cpp/ql/src/Critical/OverflowDestination.ql +++ b/cpp/ql/src/Critical/OverflowDestination.ql @@ -25,7 +25,7 @@ import semmle.code.cpp.security.TaintTracking predicate sourceSized(FunctionCall fc, Expr src) { exists(string name | (name = "strncpy" or name = "strncat" or name = "memcpy" or name = "memmove") and - fc.getTarget().hasGlobalName(name) + fc.getTarget().hasGlobalOrStdName(name) ) and exists(Expr dest, Expr size, Variable v | fc.getArgument(0) = dest and diff --git a/cpp/ql/src/Critical/OverflowStatic.ql b/cpp/ql/src/Critical/OverflowStatic.ql index 1892d5acff1..82ffc879331 100644 --- a/cpp/ql/src/Critical/OverflowStatic.ql +++ b/cpp/ql/src/Critical/OverflowStatic.ql @@ -60,19 +60,19 @@ predicate overflowOffsetInLoop(BufferAccess bufaccess, string msg) { predicate bufferAndSizeFunction(Function f, int buf, int size) { f.hasGlobalName("read") and buf = 1 and size = 2 or - f.hasGlobalName("fgets") and buf = 0 and size = 1 + f.hasGlobalOrStdName("fgets") and buf = 0 and size = 1 or - f.hasGlobalName("strncpy") and buf = 0 and size = 2 + f.hasGlobalOrStdName("strncpy") and buf = 0 and size = 2 or - f.hasGlobalName("strncat") and buf = 0 and size = 2 + f.hasGlobalOrStdName("strncat") and buf = 0 and size = 2 or - f.hasGlobalName("memcpy") and buf = 0 and size = 2 + f.hasGlobalOrStdName("memcpy") and buf = 0 and size = 2 or - f.hasGlobalName("memmove") and buf = 0 and size = 2 + f.hasGlobalOrStdName("memmove") and buf = 0 and size = 2 or - f.hasGlobalName("snprintf") and buf = 0 and size = 1 + f.hasGlobalOrStdName("snprintf") and buf = 0 and size = 1 or - f.hasGlobalName("vsnprintf") and buf = 0 and size = 1 + f.hasGlobalOrStdName("vsnprintf") and buf = 0 and size = 1 } class CallWithBufferSize extends FunctionCall { diff --git a/cpp/ql/src/Critical/SizeCheck.ql b/cpp/ql/src/Critical/SizeCheck.ql index da841b73c9b..313763ba56c 100644 --- a/cpp/ql/src/Critical/SizeCheck.ql +++ b/cpp/ql/src/Critical/SizeCheck.ql @@ -17,12 +17,12 @@ import cpp class Allocation extends FunctionCall { Allocation() { exists(string name | - this.getTarget().hasGlobalName(name) and + this.getTarget().hasGlobalOrStdName(name) and (name = "malloc" or name = "calloc" or name = "realloc") ) } - private string getName() { this.getTarget().hasGlobalName(result) } + private string getName() { this.getTarget().hasGlobalOrStdName(result) } int getSize() { this.getName() = "malloc" and diff --git a/cpp/ql/src/Critical/SizeCheck2.ql b/cpp/ql/src/Critical/SizeCheck2.ql index 3cb5d1d28b0..1b716d79d49 100644 --- a/cpp/ql/src/Critical/SizeCheck2.ql +++ b/cpp/ql/src/Critical/SizeCheck2.ql @@ -17,12 +17,12 @@ import cpp class Allocation extends FunctionCall { Allocation() { exists(string name | - this.getTarget().hasGlobalName(name) and + this.getTarget().hasGlobalOrStdName(name) and (name = "malloc" or name = "calloc" or name = "realloc") ) } - private string getName() { this.getTarget().hasGlobalName(result) } + private string getName() { this.getTarget().hasGlobalOrStdName(result) } int getSize() { this.getName() = "malloc" and diff --git a/cpp/ql/src/Critical/UseAfterFree.ql b/cpp/ql/src/Critical/UseAfterFree.ql index db78c206ea1..9efbb6c3b44 100644 --- a/cpp/ql/src/Critical/UseAfterFree.ql +++ b/cpp/ql/src/Critical/UseAfterFree.ql @@ -16,7 +16,7 @@ import semmle.code.cpp.controlflow.LocalScopeVariableReachability predicate isFreeExpr(Expr e, LocalScopeVariable v) { exists(VariableAccess va | va.getTarget() = v | exists(FunctionCall fc | fc = e | - fc.getTarget().hasGlobalName("free") and + fc.getTarget().hasGlobalOrStdName("free") and va = fc.getArgument(0) ) or diff --git a/cpp/ql/src/DefaultOptions.qll b/cpp/ql/src/DefaultOptions.qll index 27e5584369e..3e03ec9ee65 100644 --- a/cpp/ql/src/DefaultOptions.qll +++ b/cpp/ql/src/DefaultOptions.qll @@ -59,7 +59,7 @@ class Options extends string { predicate exits(Function f) { f.getAnAttribute().hasName("noreturn") or - exists(string name | f.hasGlobalName(name) | + exists(string name | f.hasGlobalOrStdName(name) | name = "exit" or name = "_exit" or name = "abort" or @@ -91,7 +91,7 @@ class Options extends string { * By default holds only for `fgets`. */ predicate alwaysCheckReturnValue(Function f) { - f.hasGlobalName("fgets") or + f.hasGlobalOrStdName("fgets") or CustomOptions::alwaysCheckReturnValue(f) // old Options.qll } diff --git a/cpp/ql/src/Likely Bugs/AmbiguouslySignedBitField.ql b/cpp/ql/src/Likely Bugs/AmbiguouslySignedBitField.ql index a2f3ec1d119..7a7f328bd4e 100644 --- a/cpp/ql/src/Likely Bugs/AmbiguouslySignedBitField.ql +++ b/cpp/ql/src/Likely Bugs/AmbiguouslySignedBitField.ql @@ -28,7 +28,8 @@ where not bf.getType().hasName("BOOL") and // If this is true, then there cannot be unsigned sign extension or overflow. not bf.getDeclaredNumBits() = bf.getType().getSize() * 8 and - not bf.isAnonymous() + not bf.isAnonymous() and + not bf.isFromUninstantiatedTemplate(_) select bf, "Bit field " + bf.getName() + " of type " + bf.getUnderlyingType().getName() + " should have explicitly unsigned integral, explicitly signed integral, or enumeration type." diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/BadAdditionOverflowCheck.qhelp b/cpp/ql/src/Likely Bugs/Arithmetic/BadAdditionOverflowCheck.qhelp index a8c2b1567a7..0af74559dff 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/BadAdditionOverflowCheck.qhelp +++ b/cpp/ql/src/Likely Bugs/Arithmetic/BadAdditionOverflowCheck.qhelp @@ -2,36 +2,39 @@ "-//Semmle//qhelp//EN" "qhelp.dtd"> - -

- Checking for overflow of integer addition needs to be done with - care, because automatic type promotion can prevent the check - from working correctly. -

-
- -

- Use an explicit cast to make sure that the result of the addition is - not implicitly converted to a larger type. -

-
- - -

- On a typical architecture where short is 16 bits - and int is 32 bits, the operands of the addition are - automatically promoted to int, so it cannot overflow - and the result of the comparison is always false. -

-

- The code below implements the check correctly, by using an - explicit cast to make sure that the result of the addition - is unsigned short. -

- -
- -
  • Preserving Rules
  • -
  • Understand integer conversion rules
  • -
    + + +

    +Checking for overflow of integer addition needs to be done with +care, because automatic type promotion can prevent the check +from working as intended, with the same value (true +or false) always being returned. +

    +
    + +

    +Use an explicit cast to make sure that the result of the addition is +not implicitly converted to a larger type. +

    +
    + + +

    +On a typical architecture where short is 16 bits +and int is 32 bits, the operands of the addition are +automatically promoted to int, so it cannot overflow +and the result of the comparison is always false. +

    +

    +The code below implements the check correctly, by using an +explicit cast to make sure that the result of the addition +is unsigned short (which may overflow, in which case +the comparison would evaluate to true). +

    + +
    + +
  • Preserving Rules
  • +
  • Understand integer conversion rules
  • +
    diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/BadAdditionOverflowCheckExample1.cpp b/cpp/ql/src/Likely Bugs/Arithmetic/BadAdditionOverflowCheckExample1.cpp index 4da403cdf51..0e1e539ccb9 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/BadAdditionOverflowCheckExample1.cpp +++ b/cpp/ql/src/Likely Bugs/Arithmetic/BadAdditionOverflowCheckExample1.cpp @@ -1,3 +1,4 @@ bool checkOverflow(unsigned short x, unsigned short y) { - return (x + y < x); // BAD: x and y are automatically promoted to int. + // BAD: comparison is always false due to type promotion + return (x + y < x); } diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/BitwiseSignCheck.ql b/cpp/ql/src/Likely Bugs/Arithmetic/BitwiseSignCheck.ql index 1e29e77742f..fe1b2640058 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/BitwiseSignCheck.ql +++ b/cpp/ql/src/Likely Bugs/Arithmetic/BitwiseSignCheck.ql @@ -14,9 +14,16 @@ import cpp from RelationalOperation e, BinaryBitwiseOperation lhs where - lhs = e.getGreaterOperand() and - lhs.getActualType().(IntegralType).isSigned() and - forall(int op | op = lhs.(BitwiseAndExpr).getAnOperand().getValue().toInt() | op < 0) and + // `lhs > 0` (or `0 < lhs`) + // (note that `lhs < 0`, `lhs >= 0` or `lhs <= 0` all imply that the signedness of + // `lhs` is understood, so should not be flagged). + (e instanceof GTExpr or e instanceof LTExpr) and + e.getGreaterOperand() = lhs and e.getLesserOperand().getValue() = "0" and + // lhs is signed + lhs.getActualType().(IntegralType).isSigned() and + // if `lhs` has the form `x & c`, with constant `c`, `c` is negative + forall(int op | op = lhs.(BitwiseAndExpr).getAnOperand().getValue().toInt() | op < 0) and + // exception for cases involving macros not e.isAffectedByMacro() select e, "Potential unsafe sign check of a bitwise operation." diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/ComparisonPrecedence.ql b/cpp/ql/src/Likely Bugs/Arithmetic/ComparisonPrecedence.ql index 16d7925c53f..d4b23265eed 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/ComparisonPrecedence.ql +++ b/cpp/ql/src/Likely Bugs/Arithmetic/ComparisonPrecedence.ql @@ -14,5 +14,8 @@ import cpp from ComparisonOperation co, ComparisonOperation chco -where co.getAChild() = chco and not chco.isParenthesised() -select co, "Check the comparison operator precedence." +where + co.getAChild() = chco and + not chco.isParenthesised() and + not co.isFromUninstantiatedTemplate(_) +select co, "Comparison as an operand to another comparison." diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/PointlessSelfComparison.ql b/cpp/ql/src/Likely Bugs/Arithmetic/PointlessSelfComparison.ql index afe49a1053b..04ef1af44ae 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/PointlessSelfComparison.ql +++ b/cpp/ql/src/Likely Bugs/Arithmetic/PointlessSelfComparison.ql @@ -13,17 +13,13 @@ import cpp import PointlessSelfComparison +import semmle.code.cpp.commons.Exclusions from ComparisonOperation cmp where pointlessSelfComparison(cmp) and not nanTest(cmp) and not overflowTest(cmp) and - not exists(MacroInvocation mi | - // cmp is in mi - mi.getAnExpandedElement() = cmp and - // and cmp was apparently not passed in as a macro parameter - cmp.getLocation().getStartLine() = mi.getLocation().getStartLine() and - cmp.getLocation().getStartColumn() = mi.getLocation().getStartColumn() - ) + not cmp.isFromTemplateInstantiation(_) and + not isFromMacroDefinition(cmp) select cmp, "Self comparison." diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-bad1.cpp b/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-bad1.cpp new file mode 100644 index 00000000000..e50273aacde --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-bad1.cpp @@ -0,0 +1,3 @@ +bool foo(int n1, unsigned short delta) { + return n1 + delta < n1; // BAD +} diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-bad2.cpp b/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-bad2.cpp new file mode 100644 index 00000000000..7f69e374ed1 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-bad2.cpp @@ -0,0 +1,4 @@ +bool bar(unsigned short n1, unsigned short delta) { + // NB: Comparison is always false + return n1 + delta < n1; // GOOD (but misleading) +} diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-good1.cpp b/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-good1.cpp new file mode 100644 index 00000000000..424684ee2ec --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-good1.cpp @@ -0,0 +1,4 @@ +#include +bool foo(int n1, unsigned short delta) { + return n1 > INT_MAX - delta; // GOOD +} diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-good2.cpp b/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-good2.cpp new file mode 100644 index 00000000000..de8de2b9847 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-good2.cpp @@ -0,0 +1,3 @@ +bool bar(unsigned short n1, unsigned short delta) { + return (unsigned short)(n1 + delta) < n1; // GOOD +} diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck.qhelp b/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck.qhelp new file mode 100644 index 00000000000..621ae3273fd --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck.qhelp @@ -0,0 +1,115 @@ + + + +

    +When checking for integer overflow, you may often write tests like +a + b < a. This works fine if a or +b are unsigned integers, since any overflow in the addition +will cause the value to simply "wrap around." However, using +signed integers is problematic because signed overflow has undefined +behavior according to the C and C++ standards. If the addition overflows +and has an undefined result, the comparison will likewise be undefined; +it may produce an unintended result, or may be deleted entirely by an +optimizing compiler. +

    +
    + +

    +Solutions to this problem can be thought of as falling into one of two +categories: (1) rewrite the signed expression so that overflow cannot occur +but the signedness remains, or (2) rewrite (or cast) the signed expression +into unsigned form. +

    + +

    +Below we list examples of expressions where signed overflow may +occur, along with proposed solutions. The list should not be +considered exhaustive. +

    + +

    +Given unsigned short i, delta and i + delta < i, +it is possible to rewrite it as (unsigned short)(i + delta) < i. +Note that i + deltadoes not actually overflow, due to int promotion +

    + +

    +Given unsigned short i, delta and i + delta < i, +it is also possible to rewrite it as USHORT_MAX - delta. It must be true +that delta > 0 and the limits.h or climits +header has been included. +

    + +

    +Given int i, delta and i + delta < i, +it is possible to rewrite it as INT_MAX - delta. It must be true +that delta > 0 and the limits.h or climits +header has been included. +

    + +

    +Given int i, delta and i + delta < i, +it is also possible to rewrite it as (unsigned)i + delta < i. +Note that program semantics are affected by this change. +

    + +

    +Given int i, delta and i + delta < i, +it is also possible to rewrite it as unsigned int i, delta and +i + delta < i. Note that program semantics are +affected by this change. +

    +
    + + +

    +In the following example, even though delta has been declared +unsigned short, C/C++ type promotion rules require that its +type is promoted to the larger type used in the addition and comparison, +namely a signed int. Addition is performed on +signed integers, and may have undefined behavior if an overflow occurs. +As a result, the entire (comparison) expression may also have an undefined +result. +

    + +

    +The following example builds upon the previous one. Instead of +performing an addition (which could overflow), we have re-framed the +solution so that a subtraction is used instead. Since delta +is promoted to a signed int and INT_MAX denotes +the largest possible positive value for an signed int, +the expression INT_MAX - delta can never be less than zero +or more than INT_MAX. Hence, any overflow and underflow +are avoided. +

    + +

    +In the following example, even though both n and delta +have been declared unsigned short, both are promoted to +signed int prior to addition. Because we started out with the +narrower short type, the addition is guaranteed not to overflow +and is therefore defined. But the fact that n1 + delta never +overflows means that the condition n1 + delta < n1 will never +hold true, which likely is not what the programmer intended. (see also the +cpp/bad-addition-overflow-check query). +

    + +

    +The next example provides a solution to the previous one. Even though +i + delta does not overflow, casting it to an +unsigned short truncates the addition modulo 2^16, +so that unsigned short "wrap around" may now be observed. +Furthermore, since the left-hand side is now of type unsigned short, +the right-hand side does not need to be promoted to a signed int. +

    + + +
    + +
  • comp.lang.c FAQ list · Question 3.19 (Preserving rules)
  • +
  • INT31-C. Ensure that integer conversions do not result in lost or misinterpreted data
  • +
  • W. Dietz, P. Li, J. Regehr, V. Adve. Understanding Integer Overflow in C/C++
  • +
    +
    diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck.ql b/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck.ql new file mode 100644 index 00000000000..ce1a8149c40 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck.ql @@ -0,0 +1,31 @@ +/** + * @name Signed overflow check + * @description Testing for overflow by adding a value to a variable + * to see if it "wraps around" works only for + * unsigned integer values. + * @kind problem + * @problem.severity warning + * @precision high + * @id cpp/signed-overflow-check + * @tags correctness + * security + */ + +import cpp +private import semmle.code.cpp.valuenumbering.GlobalValueNumbering +private import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis + +from RelationalOperation ro, AddExpr add, Expr expr1, Expr expr2 +where + ro.getAnOperand() = add and + add.getAnOperand() = expr1 and + ro.getAnOperand() = expr2 and + globalValueNumber(expr1) = globalValueNumber(expr2) and + add.getUnspecifiedType().(IntegralType).isSigned() and + not exists(MacroInvocation mi | mi.getAnAffectedElement() = add) and + exprMightOverflowPositively(add) and + exists(Compilation c | c.getAFileCompiled() = ro.getFile() | + not c.getAnArgument() = "-fwrapv" and + not c.getAnArgument() = "-fno-strict-overflow" + ) +select ro, "Testing for signed overflow may produce undefined results." diff --git a/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql b/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql index 75c5bc73029..6f44f3d4d08 100644 --- a/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql +++ b/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql @@ -157,7 +157,8 @@ where formatOtherArgType(ffc, n, expected, arg, actual) and not actual.getUnspecifiedType().(IntegralType).getSize() = sizeof_IntType() ) and - not arg.isAffectedByMacro() + not arg.isAffectedByMacro() and + not arg.isFromUninstantiatedTemplate(_) select arg, "This argument should be of type '" + expected.getName() + "' but is of type '" + actual.getUnspecifiedType().getName() + "'" diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll index f75305dbf2a..23b66bd94a6 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll +++ b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll @@ -30,7 +30,7 @@ private predicate additionalLogicalCheck(Expr e, string operation, int valueToCh /** * An `Operation` that seems to be checking for leap year. */ -class CheckForLeapYearOperation extends Operation { +class CheckForLeapYearOperation extends Expr { CheckForLeapYearOperation() { exists(BinaryArithmeticOperation bo | bo = this | bo.getAnOperand().getValue().toInt() = 4 and @@ -39,8 +39,6 @@ class CheckForLeapYearOperation extends Operation { additionalLogicalCheck(this.getEnclosingElement(), "%", 400) ) } - - override string getOperator() { result = "LeapYearCheck" } } /** diff --git a/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound-bad.cpp b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound-bad.cpp new file mode 100644 index 00000000000..0185bbb9da6 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound-bad.cpp @@ -0,0 +1,6 @@ +int get_number_from_network(); + +int process_network(int[] buff, int buffSize) { + int i = ntohl(get_number_from_network()); + return buff[i]; +} \ No newline at end of file diff --git a/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound-good.cpp b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound-good.cpp new file mode 100644 index 00000000000..2d5ec265261 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound-good.cpp @@ -0,0 +1,10 @@ +uint32_t get_number_from_network(); + +int process_network(int[] buff, uint32_t buffSize) { + uint32_t i = ntohl(get_number_from_network()); + if (i < buffSize) { + return buff[i]; + } else { + return -1; + } +} \ No newline at end of file diff --git a/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.qhelp b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.qhelp new file mode 100644 index 00000000000..3556df38c58 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.qhelp @@ -0,0 +1,54 @@ + + + + +

    +Data received over a network connection may be received in a different byte order than +the byte order used by the local host, making the data difficult to process. To address this, +data received over the wire is usually converted to host byte order by a call to a network-to-host +byte order function, such as ntohl. +

    +

    +The use of a network-to-host byte order function is therefore a good indicator that the returned +value is unvalidated data retrieved from the network, and should not be used without further +validation. In particular, the returned value should not be used as an array index or array length +value without validation, as this could result in a buffer overflow vulnerability. +

    +
    + + +

    +Validate data returned by network-to-host byte order functions before use and especially before +using the value as an array index or bound. +

    +
    + + +

    In the example below, network data is retrieved and passed to ntohl to convert +it to host byte order. The data is then used as an index in an array access expression. However, +there is no validation that the data returned by ntohl is within the bounds of the array, +which could lead to reading outside the bounds of the buffer. +

    + +

    In the corrected example, the returned data is validated against the known size of the buffer, +before being used as an array index.

    + +
    + + +
  • + + ntohl - winsock reference + +
  • +
  • + + ntohl - Linux man page + +
  • + +
    + +
    diff --git a/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.ql b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.ql new file mode 100644 index 00000000000..d6d0a55d148 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.ql @@ -0,0 +1,17 @@ +/** + * @id cpp/network-to-host-function-as-array-bound + * @name Untrusted network-to-host usage + * @description Using the result of a network-to-host byte order function, such as ntohl, as an + * array bound or length value without checking it may result in buffer overflows or + * other vulnerabilties. + * @kind problem + * @problem.severity error + */ + +import cpp +import NtohlArrayNoBound +import semmle.code.cpp.dataflow.DataFlow + +from NetworkToBufferSizeConfiguration bufConfig, DataFlow::Node source, DataFlow::Node sink +where bufConfig.hasFlow(source, sink) +select sink, "Unchecked use of data from network function $@", source, source.toString() diff --git a/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.qll b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.qll new file mode 100644 index 00000000000..0871da2f92e --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.qll @@ -0,0 +1,154 @@ +import cpp +import semmle.code.cpp.dataflow.DataFlow +import semmle.code.cpp.controlflow.Guards +import semmle.code.cpp.valuenumbering.GlobalValueNumbering + +/** + * An access (read or write) to a buffer, provided as a pair of + * a pointer to the buffer and the length of data to be read or written. + * Extend this class to support different kinds of buffer access. + */ +abstract class BufferAccess extends Locatable { + /** Gets the pointer to the buffer being accessed. */ + abstract Expr getPointer(); + + /** Gets the length of the data being read or written by this buffer access. */ + abstract Expr getAccessedLength(); +} + +/** + * A buffer access through an array expression. + */ +class ArrayBufferAccess extends BufferAccess, ArrayExpr { + override Expr getPointer() { result = this.getArrayBase() } + + override Expr getAccessedLength() { result = this.getArrayOffset() } +} + +/** + * A buffer access through an overloaded array expression. + */ +class OverloadedArrayBufferAccess extends BufferAccess, OverloadedArrayExpr { + override Expr getPointer() { result = this.getQualifier() } + + override Expr getAccessedLength() { result = this.getAnArgument() } +} + +/** + * A buffer access through pointer arithmetic. + */ +class PointerArithmeticAccess extends BufferAccess, Expr { + PointerArithmeticOperation p; + + PointerArithmeticAccess() { + this = p and + p.getAnOperand().getType().getUnspecifiedType() instanceof IntegralType and + not p.getParent() instanceof ComparisonOperation + } + + override Expr getPointer() { + result = p.getAnOperand() and + result.getType().getUnspecifiedType() instanceof PointerType + } + + override Expr getAccessedLength() { + result = p.getAnOperand() and + result.getType().getUnspecifiedType() instanceof IntegralType + } +} + +/** + * A pair of buffer accesses through a call to memcpy. + */ +class MemCpy extends BufferAccess, FunctionCall { + MemCpy() { getTarget().hasName("memcpy") } + + override Expr getPointer() { + result = getArgument(0) or + result = getArgument(1) + } + + override Expr getAccessedLength() { result = getArgument(2) } +} + +class StrncpySizeExpr extends BufferAccess, FunctionCall { + StrncpySizeExpr() { getTarget().hasName("strncpy") } + + override Expr getPointer() { + result = getArgument(0) or + result = getArgument(1) + } + + override Expr getAccessedLength() { result = getArgument(2) } +} + +class RecvSizeExpr extends BufferAccess, FunctionCall { + RecvSizeExpr() { getTarget().hasName("recv") } + + override Expr getPointer() { result = getArgument(1) } + + override Expr getAccessedLength() { result = getArgument(2) } +} + +class SendSizeExpr extends BufferAccess, FunctionCall { + SendSizeExpr() { getTarget().hasName("send") } + + override Expr getPointer() { result = getArgument(1) } + + override Expr getAccessedLength() { result = getArgument(2) } +} + +class SnprintfSizeExpr extends BufferAccess, FunctionCall { + SnprintfSizeExpr() { getTarget().hasName("snprintf") } + + override Expr getPointer() { result = getArgument(0) } + + override Expr getAccessedLength() { result = getArgument(1) } +} + +class MemcmpSizeExpr extends BufferAccess, FunctionCall { + MemcmpSizeExpr() { getTarget().hasName("Memcmp") } + + override Expr getPointer() { + result = getArgument(0) or + result = getArgument(1) + } + + override Expr getAccessedLength() { result = getArgument(2) } +} + +class MallocSizeExpr extends BufferAccess, FunctionCall { + MallocSizeExpr() { getTarget().hasName("malloc") } + + override Expr getPointer() { none() } + + override Expr getAccessedLength() { result = getArgument(1) } +} + +class NetworkFunctionCall extends FunctionCall { + NetworkFunctionCall() { + getTarget().hasName("ntohd") or + getTarget().hasName("ntohf") or + getTarget().hasName("ntohl") or + getTarget().hasName("ntohll") or + getTarget().hasName("ntohs") + } +} + +class NetworkToBufferSizeConfiguration extends DataFlow::Configuration { + NetworkToBufferSizeConfiguration() { this = "NetworkToBufferSizeConfiguration" } + + override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof NetworkFunctionCall } + + override predicate isSink(DataFlow::Node node) { + node.asExpr() = any(BufferAccess ba).getAccessedLength() + } + + override predicate isBarrier(DataFlow::Node node) { + exists(GuardCondition gc, GVN gvn | + gc.getAChild*() = gvn.getAnExpr() and + globalValueNumber(node.asExpr()) = gvn and + gc.controls(node.asExpr().getBasicBlock(), _) + ) + } +} diff --git a/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow-bad.cpp b/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow-bad.cpp new file mode 100644 index 00000000000..4ab13e20912 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow-bad.cpp @@ -0,0 +1,3 @@ +bool not_in_range(T *ptr, T *ptr_end, size_t i) { + return ptr + i >= ptr_end || ptr + i < ptr; // BAD +} diff --git a/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow-good.cpp b/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow-good.cpp new file mode 100644 index 00000000000..33c619cc806 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow-good.cpp @@ -0,0 +1,3 @@ +bool not_in_range(T *ptr, T *ptr_end, size_t i) { + return i >= ptr_end - ptr; // GOOD +} \ No newline at end of file diff --git a/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow.qhelp b/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow.qhelp new file mode 100644 index 00000000000..5cc0ae21af9 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow.qhelp @@ -0,0 +1,67 @@ + + + +

    +When checking for integer overflow, you may often write tests like +p + i < p. This works fine if p and +i are unsigned integers, since any overflow in the addition +will cause the value to simply "wrap around." However, using this pattern when +p is a pointer is problematic because pointer overflow has +undefined behavior according to the C and C++ standards. If the addition +overflows and has an undefined result, the comparison will likewise be +undefined; it may produce an unintended result, or may be deleted entirely by an +optimizing compiler. +

    + +
    + +

    +To check whether an index i is less than the length of an array, +simply compare these two numbers as unsigned integers: i < ARRAY_LENGTH. +If the length of the array is defined as the difference between two pointers +ptr and p_end, write i < p_end - ptr. +If i is signed, cast it to unsigned +in order to guard against negative i. For example, write +(size_t)i < p_end - ptr. +

    +
    + +

    +An invalid check for pointer overflow is most often seen as part of checking +whether a number a is too large by checking first if adding the +number to ptr goes past the end of an allocation and then +checking if adding it to ptr creates a pointer so large that it +overflows and wraps around. +

    + + + +

    +In both of these checks, the operations are performed in the wrong order. +First, an expression that may cause undefined behavior is evaluated +(ptr + i), and then the result is checked for being in range. +But once undefined behavior has happened in the pointer addition, it cannot +be recovered from: it's too late to perform the range check after a possible +pointer overflow. +

    + +

    +While it's not the subject of this query, the expression ptr + i < +ptr_end is also an invalid range check. It's undefined behavor in +C/C++ to create a pointer that points more than one past the end of an +allocation. +

    + +

    +The next example shows how to portably check whether an unsigned number is outside the +range of an allocation between ptr and ptr_end. +

    + +
    + +
  • Embedded in Academia: Pointer Overflow Checking.
  • +
  • LWN: GCC and pointer overflows.
  • +
    +
    diff --git a/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow.ql b/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow.ql new file mode 100644 index 00000000000..6344addd547 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow.ql @@ -0,0 +1,31 @@ +/** + * @name Pointer overflow check + * @description Adding a value to a pointer to check if it overflows relies + * on undefined behavior and may lead to memory corruption. + * @kind problem + * @problem.severity error + * @precision high + * @id cpp/pointer-overflow-check + * @tags reliability + * security + */ + +import cpp +private import semmle.code.cpp.valuenumbering.GlobalValueNumbering +private import semmle.code.cpp.commons.Exclusions + +from RelationalOperation ro, PointerAddExpr add, Expr expr1, Expr expr2 +where + ro.getAnOperand() = add and + add.getAnOperand() = expr1 and + ro.getAnOperand() = expr2 and + globalValueNumber(expr1) = globalValueNumber(expr2) and + // Exclude macros but not their arguments + not isFromMacroDefinition(ro) and + // There must be a compilation of this file without a flag that makes pointer + // overflow well defined. + exists(Compilation c | c.getAFileCompiled() = ro.getFile() | + not c.getAnArgument() = "-fwrapv-pointer" and + not c.getAnArgument() = "-fno-strict-overflow" + ) +select ro, "Range check relying on pointer overflow." diff --git a/cpp/ql/src/Likely Bugs/Protocols/TlsSettingsMisconfiguration.qhelp b/cpp/ql/src/Likely Bugs/Protocols/TlsSettingsMisconfiguration.qhelp new file mode 100644 index 00000000000..03288c39a71 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Protocols/TlsSettingsMisconfiguration.qhelp @@ -0,0 +1,15 @@ + + + +

    Using the TLS or SSLv23 protocol from the boost::asio library, but not disabling deprecated protocols may expose the software to known vulnerabilities or permit weak encryption algorithms to be used. Disabling the minimum-recommended protocols is also flagged.

    +
    + + +
  • + Boost.Asio documentation. +
  • +
    +
    + diff --git a/cpp/ql/src/Likely Bugs/Protocols/TlsSettingsMisconfiguration.ql b/cpp/ql/src/Likely Bugs/Protocols/TlsSettingsMisconfiguration.ql new file mode 100644 index 00000000000..fb7edce038c --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Protocols/TlsSettingsMisconfiguration.ql @@ -0,0 +1,94 @@ +/** + * @name Boost_asio TLS Settings Misconfiguration + * @description Using the TLS or SSLv23 protocol from the boost::asio library, but not disabling deprecated protocols, or disabling minimum-recommended protocols. + * @kind problem + * @problem.severity error + * @id cpp/boost/tls_settings_misconfiguration + * @tags security + */ + +import cpp +import semmle.code.cpp.security.boostorg.asio.protocols + +class ExistsAnyFlowConfig extends DataFlow::Configuration { + ExistsAnyFlowConfig() { this = "ExistsAnyFlowConfig" } + + override predicate isSource(DataFlow::Node source) { + exists(BoostorgAsio::SslContextClass c | c.getAContructorCall() = source.asExpr()) + } + + override predicate isSink(DataFlow::Node sink) { + exists(BoostorgAsio::SslSetOptionsFunction f, FunctionCall fcSetOptions | + f.getACallToThisFunction() = fcSetOptions and + fcSetOptions.getQualifier() = sink.asExpr() + ) + } +} + +bindingset[flag] +predicate isOptionSet(ConstructorCall cc, int flag, FunctionCall fcSetOptions) { + exists(ExistsAnyFlowConfig anyFlowConfig, VariableAccess contextSetOptions | + anyFlowConfig.hasFlow(DataFlow::exprNode(cc), DataFlow::exprNode(contextSetOptions)) and + exists(BoostorgAsio::SslSetOptionsFunction f | f.getACallToThisFunction() = fcSetOptions | + contextSetOptions = fcSetOptions.getQualifier() and + forall( + Expr optionArgument, BoostorgAsio::SslOptionConfig optionArgConfig, + Expr optionArgumentSource + | + optionArgument = fcSetOptions.getArgument(0) and + optionArgConfig + .hasFlow(DataFlow::exprNode(optionArgumentSource), DataFlow::exprNode(optionArgument)) + | + optionArgument.getValue().toInt().bitShiftRight(16).bitAnd(flag) = flag + ) + ) + ) +} + +bindingset[flag] +predicate isOptionNotSet(ConstructorCall cc, int flag) { + not exists(FunctionCall fcSetOptions | isOptionSet(cc, flag, fcSetOptions)) +} + +from + BoostorgAsio::SslContextCallTlsProtocolConfig configConstructor, Expr protocolSource, + Expr protocolSink, ConstructorCall cc, Expr e, string msg +where + configConstructor.hasFlow(DataFlow::exprNode(protocolSource), DataFlow::exprNode(protocolSink)) and + cc.getArgument(0) = protocolSink and + ( + BoostorgAsio::isExprSslV23BoostProtocol(protocolSource) and + not ( + isOptionSet(cc, BoostorgAsio::getShiftedSslOptionsNoSsl3(), _) and + isOptionSet(cc, BoostorgAsio::getShiftedSslOptionsNoTls1(), _) and + isOptionSet(cc, BoostorgAsio::getShiftedSslOptionsNoTls1_1(), _) and + isOptionNotSet(cc, BoostorgAsio::getShiftedSslOptionsNoTls1_2()) + ) + or + BoostorgAsio::isExprTlsBoostProtocol(protocolSource) and + not BoostorgAsio::isExprSslV23BoostProtocol(protocolSource) and + not ( + isOptionSet(cc, BoostorgAsio::getShiftedSslOptionsNoTls1(), _) and + isOptionSet(cc, BoostorgAsio::getShiftedSslOptionsNoTls1_1(), _) and + isOptionNotSet(cc, BoostorgAsio::getShiftedSslOptionsNoTls1_2()) + ) + ) and + ( + BoostorgAsio::isExprSslV23BoostProtocol(protocolSource) and + isOptionNotSet(cc, BoostorgAsio::getShiftedSslOptionsNoSsl3()) and + e = cc and + msg = "no_sslv3 has not been set" + or + isOptionNotSet(cc, BoostorgAsio::getShiftedSslOptionsNoTls1()) and + e = cc and + msg = "no_tlsv1 has not been set" + or + isOptionNotSet(cc, BoostorgAsio::getShiftedSslOptionsNoTls1_1()) and + e = cc and + msg = "no_tlsv1_1 has not been set" + or + isOptionSet(cc, BoostorgAsio::getShiftedSslOptionsNoTls1_2(), e) and + msg = "no_tlsv1_2 was set" + ) +select cc, "Usage of $@ with protocol $@ is not configured correctly: The option $@.", cc, + "boost::asio::ssl::context::context", protocolSource, protocolSource.toString(), e, msg diff --git a/cpp/ql/src/Likely Bugs/Protocols/UseOfDeprecatedHardcodedProtocol.qhelp b/cpp/ql/src/Likely Bugs/Protocols/UseOfDeprecatedHardcodedProtocol.qhelp new file mode 100644 index 00000000000..6f4d2acedd6 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Protocols/UseOfDeprecatedHardcodedProtocol.qhelp @@ -0,0 +1,16 @@ + + + +

    Using boost::asio library but specifying a deprecated hardcoded protocol.

    +

    Using a deprecated hardcoded protocol instead of negotiting would lock your application to a protocol that has known vulnerabilities or weaknesses.

    +
    + + +
  • + Boost.Asio documentation. +
  • +
    +
    + diff --git a/cpp/ql/src/Likely Bugs/Protocols/UseOfDeprecatedHardcodedProtocol.ql b/cpp/ql/src/Likely Bugs/Protocols/UseOfDeprecatedHardcodedProtocol.ql new file mode 100644 index 00000000000..b3693ead656 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Protocols/UseOfDeprecatedHardcodedProtocol.ql @@ -0,0 +1,27 @@ +/** + * @name boost::asio Use of deprecated hardcoded Protocol + * @description Using a deprecated hard-coded protocol using the boost::asio library. + * @kind problem + * @problem.severity error + * @id cpp/boost/use-of-deprecated-hardcoded-security-protocol + * @tags security + */ + +import cpp +import semmle.code.cpp.security.boostorg.asio.protocols + +from + BoostorgAsio::SslContextCallConfig config, Expr protocolSource, Expr protocolSink, + ConstructorCall cc +where + config.hasFlow(DataFlow::exprNode(protocolSource), DataFlow::exprNode(protocolSink)) and + not exists(BoostorgAsio::SslContextCallTlsProtocolConfig tlsConfig | + tlsConfig.hasFlow(DataFlow::exprNode(protocolSource), DataFlow::exprNode(protocolSink)) + ) and + cc.getArgument(0) = protocolSink and + exists(BoostorgAsio::SslContextCallBannedProtocolConfig bannedConfig | + bannedConfig.hasFlow(DataFlow::exprNode(protocolSource), DataFlow::exprNode(protocolSink)) + ) +select protocolSink, "Usage of $@ specifying a deprecated hardcoded protocol $@ in function $@.", + cc, "boost::asio::ssl::context::context", protocolSource, protocolSource.toString(), + cc.getEnclosingFunction(), cc.getEnclosingFunction().toString() diff --git a/cpp/ql/src/Microsoft/IgnoreReturnValueSAL.ql b/cpp/ql/src/Microsoft/IgnoreReturnValueSAL.ql index 64f398fa855..fb25a93963d 100644 --- a/cpp/ql/src/Microsoft/IgnoreReturnValueSAL.ql +++ b/cpp/ql/src/Microsoft/IgnoreReturnValueSAL.ql @@ -7,6 +7,10 @@ * @id cpp/ignore-return-value-sal * @problem.severity warning * @tags reliability + * external/cwe/cwe-573 + * external/cwe/cwe-252 + * @opaque-id SM02344 + * @microsoft.severity Important */ import SAL diff --git a/cpp/ql/src/Microsoft/SAL.qll b/cpp/ql/src/Microsoft/SAL.qll index b9f6c2d037d..963a726ae54 100644 --- a/cpp/ql/src/Microsoft/SAL.qll +++ b/cpp/ql/src/Microsoft/SAL.qll @@ -2,29 +2,45 @@ import cpp class SALMacro extends Macro { SALMacro() { - this.getFile().getBaseName() = "sal.h" or - this.getFile().getBaseName() = "specstrings_strict.h" or - this.getFile().getBaseName() = "specstrings.h" + exists(string filename | filename = this.getFile().getBaseName() | + filename = "sal.h" or + filename = "specstrings_strict.h" or + filename = "specstrings.h" or + filename = "w32p.h" or + filename = "minwindef.h" + ) and + ( + // Dialect for Windows 8 and above + this.getName().matches("\\_%\\_") + or + // Dialect for Windows 7 + this.getName().matches("\\_\\_%") + ) } } +pragma[noinline] +predicate isTopLevelMacroAccess(MacroAccess ma) { not exists(ma.getParentInvocation()) } + class SALAnnotation extends MacroInvocation { SALAnnotation() { this.getMacro() instanceof SALMacro and - not exists(this.getParentInvocation()) + isTopLevelMacroAccess(this) } /** Returns the `Declaration` annotated by `this`. */ - Declaration getDeclaration() { annotatesAt(this, result.getADeclarationEntry(), _, _) } + Declaration getDeclaration() { + annotatesAt(this, result.getADeclarationEntry(), _, _) and + not result instanceof Type // exclude typedefs + } /** Returns the `DeclarationEntry` annotated by `this`. */ - DeclarationEntry getDeclarationEntry() { annotatesAt(this, result, _, _) } + DeclarationEntry getDeclarationEntry() { + annotatesAt(this, result, _, _) and + not result instanceof TypeDeclarationEntry // exclude typedefs + } } -/* - * Particular SAL annotations of interest - */ - class SALCheckReturn extends SALAnnotation { SALCheckReturn() { exists(SALMacro m | m = this.getMacro() | @@ -39,8 +55,8 @@ class SALNotNull extends SALAnnotation { exists(SALMacro m | m = this.getMacro() | not m.getName().matches("%\\_opt\\_%") and ( - m.getName().matches("\\_In%") or - m.getName().matches("\\_Out%") or + m.getName().matches("_In%") or + m.getName().matches("_Out%") or m.getName() = "_Ret_notnull_" ) ) and @@ -63,42 +79,124 @@ class SALMaybeNull extends SALAnnotation { } } -/* - * Implementation details +/////////////////////////////////////////////////////////////////////////////// +// Implementation details +/** + * Holds if `a` annotates the declaration entry `d` and + * its start position is the `idx`th position in `file` that holds a SAL element. */ - -private predicate annotatesAt(SALAnnotation a, DeclarationEntry e, File file, int idx) { - a = salElementAt(file, idx) and - ( - // Base case: `a` right before `e` - e = salElementAt(file, idx + 1) - or - // Recursive case: `a` right before some annotation on `e` - annotatesAt(_, e, file, idx + 1) - ) -} - -library class SALElement extends Element { - SALElement() { - this instanceof DeclarationEntry or - this instanceof SALAnnotation - } -} - -/** Gets the `idx`th `SALElement` in `file`. */ -private SALElement salElementAt(File file, int idx) { - interestingLoc(file, result, interestingStartPos(file, idx)) +predicate annotatesAt(SALAnnotation a, DeclarationEntry d, File file, int idx) { + annotatesAtPosition(a.(SALElement).getStartPosition(), d, file, idx) } /** - * Holds if an SALElement element at character `result` comes at - * position `idx` in `file`. + * Holds if `pos` is the `idx`th position in `file` that holds a SAL element, + * which annotates the declaration entry `d` (by occurring before it without + * any other declaration entries in between). */ -private int interestingStartPos(File file, int idx) { - result = rank[idx](int otherStart | interestingLoc(file, _, otherStart)) +// For performance reasons, do not mention the annotation itself here, +// but compute with positions instead. This performs better on databases +// with many annotations at the same position. +private predicate annotatesAtPosition(SALPosition pos, DeclarationEntry d, File file, int idx) { + pos = salRelevantPositionAt(file, idx) and + salAnnotationPos(pos) and + ( + // Base case: `pos` right before `d` + d.(SALElement).getStartPosition() = salRelevantPositionAt(file, idx + 1) + or + // Recursive case: `pos` right before some annotation on `d` + annotatesAtPosition(_, d, file, idx + 1) + ) } -/** Holds if `element` in `file` is at character `startPos`. */ -private predicate interestingLoc(File file, SALElement element, int startPos) { - element.getLocation().charLoc(file, startPos, _) +/** + * A parameter annotated by one or more SAL annotations. + */ +class SALParameter extends Parameter { + /** One of this parameter's annotations. */ + SALAnnotation a; + + SALParameter() { annotatesAt(a, this.getADeclarationEntry(), _, _) } + + predicate isIn() { a.getMacroName().toLowerCase().matches("%\\_in%") } + + predicate isOut() { a.getMacroName().toLowerCase().matches("%\\_out%") } + + predicate isInOut() { a.getMacroName().toLowerCase().matches("%\\_inout%") } +} + +/** + * A SAL element, that is, a SAL annotation or a declaration entry + * that may have SAL annotations. + */ +library class SALElement extends Element { + SALElement() { + containsSALAnnotation(this.(DeclarationEntry).getFile()) or + this instanceof SALAnnotation + } + + predicate hasStartPosition(File file, int line, int col) { + exists(Location loc | loc = this.getLocation() | + file = loc.getFile() and + line = loc.getStartLine() and + col = loc.getStartColumn() + ) + } + + predicate hasEndPosition(File file, int line, int col) { + exists(Location loc | + loc = this.(FunctionDeclarationEntry).getBlock().getLocation() + or + this = any(VariableDeclarationEntry vde | + vde.isDefinition() and + loc = vde.getVariable().getInitializer().getLocation() + ) + | + file = loc.getFile() and + line = loc.getEndLine() and + col = loc.getEndColumn() + ) + } + + SALPosition getStartPosition() { + exists(File file, int line, int col | + this.hasStartPosition(file, line, col) and + result = MkSALPosition(file, line, col) + ) + } +} + +/** Holds if `file` contains a SAL annotation. */ +pragma[noinline] +private predicate containsSALAnnotation(File file) { any(SALAnnotation a).getFile() = file } + +/** + * A source-file position of a `SALElement`. Unlike location, this denotes a + * point in the file rather than a range. + */ +private newtype SALPosition = + MkSALPosition(File file, int line, int col) { + exists(SALElement e | + e.hasStartPosition(file, line, col) + or + e.hasEndPosition(file, line, col) + ) + } + +/** Holds if `pos` is the start position of a SAL annotation. */ +pragma[noinline] +private predicate salAnnotationPos(SALPosition pos) { + any(SALAnnotation a).(SALElement).getStartPosition() = pos +} + +/** + * Gets the `idx`th position in `file` that holds a SAL element, + * ordering positions lexicographically by their start line and start column. + */ +private SALPosition salRelevantPositionAt(File file, int idx) { + result = rank[idx](SALPosition pos, int line, int col | + pos = MkSALPosition(file, line, col) + | + pos order by line, col + ) } diff --git a/cpp/ql/src/Security/CWE/CWE-022/TaintedPath.ql b/cpp/ql/src/Security/CWE/CWE-022/TaintedPath.ql index 096b1468bb9..42a29b96268 100644 --- a/cpp/ql/src/Security/CWE/CWE-022/TaintedPath.ql +++ b/cpp/ql/src/Security/CWE/CWE-022/TaintedPath.ql @@ -34,8 +34,10 @@ class FileFunction extends FunctionWithWrappers { nme.matches("CreateFile%") ) or + this.hasQualifiedName("std", "fopen") + or // on any of the fstream classes, or filebuf - exists(string nme | this.getDeclaringType().getSimpleName() = nme | + exists(string nme | this.getDeclaringType().hasQualifiedName("std", nme) | nme = "basic_fstream" or nme = "basic_ifstream" or nme = "basic_ofstream" or diff --git a/cpp/ql/src/Security/CWE/CWE-079/CgiXss.qhelp b/cpp/ql/src/Security/CWE/CWE-079/CgiXss.qhelp index ccd297c3b36..4ad7a40fed6 100644 --- a/cpp/ql/src/Security/CWE/CWE-079/CgiXss.qhelp +++ b/cpp/ql/src/Security/CWE/CWE-079/CgiXss.qhelp @@ -34,7 +34,7 @@ characters before writing to the HTML page.

  • OWASP: -XSS +XSS (Cross Site Scripting) Prevention Cheat Sheet.
  • diff --git a/cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql b/cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql index 4469517c369..8b7fb83df81 100644 --- a/cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql +++ b/cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql @@ -17,8 +17,8 @@ import semmle.code.cpp.security.TaintTracking /** A call that prints its arguments to `stdout`. */ class PrintStdoutCall extends FunctionCall { PrintStdoutCall() { - getTarget().hasGlobalName("puts") or - getTarget().hasGlobalName("printf") + getTarget().hasGlobalOrStdName("puts") or + getTarget().hasGlobalOrStdName("printf") } } diff --git a/cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql b/cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql index 575752f0b74..ff17daca05c 100644 --- a/cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql +++ b/cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql @@ -19,10 +19,7 @@ import semmle.code.cpp.dataflow.DataFlow import semmle.code.cpp.models.implementations.Memcpy class MallocCall extends FunctionCall { - MallocCall() { - this.getTarget().hasGlobalName("malloc") or - this.getTarget().hasQualifiedName("std", "malloc") - } + MallocCall() { this.getTarget().hasGlobalOrStdName("malloc") } Expr getAllocatedSize() { if this.getArgument(0) instanceof VariableAccess diff --git a/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql b/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql index c704506a4fa..92f679a0f1f 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql +++ b/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql @@ -5,7 +5,7 @@ * @id cpp/comparison-with-wider-type * @kind problem * @problem.severity warning - * @precision medium + * @precision high * @tags reliability * security * external/cwe/cwe-190 diff --git a/cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariable.qhelp b/cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariable.qhelp new file mode 100644 index 00000000000..8e6a8903483 --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariable.qhelp @@ -0,0 +1,59 @@ + + + + +

    A common pattern is to initialize a local variable by calling another function (an +"initialization" function) with the address of the local variable as a pointer argument. That +function is then responsible for writing to the memory location referenced by the pointer.

    + +

    In some cases, the called function may not always write to the memory pointed to by the +pointer argument. In such cases, the function will typically return a "status" code, informing the +caller as to whether the initialization succeeded or not. If the caller does not check the status +code before reading the local variable, it may read unitialized memory, which can result in +unexpected behavior.

    + +
    + + +

    When using a initialization function that does not guarantee to initialize the memory pointed to +by the passed pointer, and returns a status code to indicate whether such initialization occurred, +the status code should be checked before reading from the local variable.

    + +
    + + +

    In this hypothetical example we have code for managing a series of devices. The code +includes a DeviceConfig struct that can represent properties about each device. +The initDeviceConfig function can be called to initialize one of these structures, by +providing a "device number", which can be used to look up the appropriate properties in some data +store. If an invalid device number is provided, the function returns a status code of +-1, and does not initialize the provided pointer.

    + +

    In the first code sample below, the notify function calls the +initDeviceConfig function with a pointer to the local variable config, +which is then subsequently accessed to fetch properties of the device. However, the code does not +check the return value from the function call to initDeviceConfig. If the +device number passed to the notify function was invalid, the +initDeviceConfig function will leave the config variable uninitialized, +which will result in the notify function accessing uninitialized memory.

    + + + +

    To fix this, the code needs to check that the return value of the call to +initDeviceConfig is zero. If that is true, then the calling code can safely assume +that the local variable has been initialized.

    + + + +
    + + +
  • + Wikipedia: + Uninitialized variable. +
  • + + + \ No newline at end of file diff --git a/cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariable.ql b/cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariable.ql new file mode 100644 index 00000000000..f9eb2fe5400 --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariable.ql @@ -0,0 +1,33 @@ +/** + * @name Conditionally uninitialized variable + * @description When an initialization function is used to initialize a local variable, but the + * returned status code is not checked, the variable may be left in an uninitialized + * state, and reading the variable may result in undefined behavior. + * @kind problem + * @problem.severity warning + * @opaque-id SM02313 + * @id cpp/conditionally-uninitialized-variable + * @tags security + * external/cwe/cwe-457 + */ + +import cpp +import semmle.code.cpp.controlflow.SSA +private import UninitializedVariables + +from + ConditionallyInitializedVariable v, ConditionalInitializationFunction f, + ConditionalInitializationCall call, string defined, Evidence e +where + exists(v.getARiskyAccess(f, call, e)) and + ( + if e = DefinitionInSnapshot() + then defined = "" + else + if e = SuggestiveSALAnnotation() + then defined = "externally defined (SAL) " + else defined = "externally defined (CSV) " + ) +select call, + "The status of this call to " + defined + + "$@ is not checked, potentially leaving $@ uninitialized.", f, f.getName(), v, v.getName() diff --git a/cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariableBad.c b/cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariableBad.c new file mode 100644 index 00000000000..73a01c2d900 --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariableBad.c @@ -0,0 +1,25 @@ +struct DeviceConfig { + bool isEnabled; + int channel; +}; + +int initDeviceConfig(DeviceConfig *ref, int deviceNumber) { + if (deviceNumber >= getMaxDevices()) { + // No device with that number, return -1 to indicate failure + return -1; + } + // Device with that number, fetch parameters and initialize struct + ref->isEnabled = fetchIsDeviceEnabled(deviceNumber); + ref->channel = fetchDeviceChannel(deviceNumber); + // Return 0 to indicate success + return 0; +} + +int notify(int deviceNumber) { + DeviceConfig config; + initDeviceConfig(&config, deviceNumber); + // BAD: Using config without checking the status code that is returned + if (config.isEnabled) { + notifyChannel(config.channel); + } +} diff --git a/cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariableGood.c b/cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariableGood.c new file mode 100644 index 00000000000..ced43a66cfc --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-457/ConditionallyUninitializedVariableGood.c @@ -0,0 +1,27 @@ +struct DeviceConfig { + bool isEnabled; + int channel; +}; + +int initDeviceConfig(DeviceConfig *ref, int deviceNumber) { + if (deviceNumber >= getMaxDevices()) { + // No device with that number, return -1 to indicate failure + return -1; + } + // Device with that number, fetch parameters and initialize struct + ref->isEnabled = fetchIsDeviceEnabled(deviceNumber); + ref->channel = fetchDeviceChannel(deviceNumber); + // Return 0 to indicate success + return 0; +} + +void notify(int deviceNumber) { + DeviceConfig config; + int statusCode = initDeviceConfig(&config, deviceNumber); + if (statusCode == 0) { + // GOOD: Status code returned by initialization function is checked, so this is safe + if (config.isEnabled) { + notifyChannel(config.channel); + } + } +} diff --git a/cpp/ql/src/Security/CWE/CWE-457/InitializationFunctions.qll b/cpp/ql/src/Security/CWE/CWE-457/InitializationFunctions.qll new file mode 100644 index 00000000000..8f2950c9123 --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-457/InitializationFunctions.qll @@ -0,0 +1,705 @@ +/** + * Provides classes and predicates for identifying functions that initialize their arguments. + */ + +import cpp +import external.ExternalArtifact +private import semmle.code.cpp.dispatch.VirtualDispatchPrototype +import semmle.code.cpp.NestedFields +import Microsoft.SAL +import semmle.code.cpp.controlflow.Guards + +/** A context under which a function may be called. */ +private newtype TContext = + /** No specific call context. */ + NoContext() or + /** + * The call context is that the given other parameter is null. + * + * This context is created for all parameters that are null checked in the body of the function. + */ + ParamNull(Parameter p) { p = any(ParameterNullCheck pnc).getParameter() } or + /** + * The call context is that the given other parameter is not null. + * + * This context is created for all parameters that are null checked in the body of the function. + */ + ParamNotNull(Parameter p) { p = any(ParameterNullCheck pnc).getParameter() } + +/** + * A context under which a function may be called. + * + * Some functions may conditionally initialize a parameter depending on the value of another + * parameter. Consider: + * ``` + * int MyInitFunction(int* paramToBeInitialized, int* paramToCheck) { + * if (!paramToCheck) { + * // fail! + * return -1; + * } + * paramToBeInitialized = 0; + * } + * ``` + * In this case, whether `paramToBeInitialized` is initialized when this function call completes + * depends on whether `paramToCheck` is or is not null. A call-context insensitive analysis will + * determine that any call to this function may leave the parameter uninitialized, even if the + * argument to paramToCheck is guaranteed to be non-null (`&foo`, for example). + * + * This class models call contexts that can be considered when calculating whether a given parameter + * initializes or not. The supported contexts are: + * - `ParamNull(otherParam)` - the given `otherParam` is considered to be null. Applies when + * exactly one parameter other than this one is null checked. + * - `ParamNotNull(otherParam)` - the given `otherParam` is considered to be not null. Applies when + * exactly one parameter other than this one is null checked. + * - `NoContext()` - applies in all other circumstances. + */ +class Context extends TContext { + string toString() { + this = NoContext() and result = "NoContext" + or + this = ParamNull(any(Parameter p | result = "ParamNull(" + p.getName() + ")")) + or + this = ParamNotNull(any(Parameter p | result = "ParamNotNull(" + p.getName() + ")")) + } +} + +/** + * A check against a parameter. + */ +abstract class ParameterCheck extends Expr { + /** + * Gets a successor of this check that should be ignored for the given context. + */ + abstract ControlFlowNode getIgnoredSuccessorForContext(Context c); +} + +/** A null-check expression on a parameter. */ +class ParameterNullCheck extends ParameterCheck { + Parameter p; + ControlFlowNode nullSuccessor; + ControlFlowNode notNullSuccessor; + + ParameterNullCheck() { + this.isCondition() and + p.getFunction() instanceof InitializationFunction and + p.getType().getUnspecifiedType() instanceof PointerType and + exists(VariableAccess va | va = p.getAnAccess() | + nullSuccessor = getATrueSuccessor() and + notNullSuccessor = getAFalseSuccessor() and + ( + va = this.(NotExpr).getOperand() or + va = any(EQExpr eq | eq = this and eq.getAnOperand().getValue() = "0").getAnOperand() or + va = getCheckedFalseCondition(this) or + va = any(NEExpr eq | + eq = getCheckedFalseCondition(this) and eq.getAnOperand().getValue() = "0" + ).getAnOperand() + ) + or + nullSuccessor = getAFalseSuccessor() and + notNullSuccessor = getATrueSuccessor() and + ( + va = this or + va = any(NEExpr eq | eq = this and eq.getAnOperand().getValue() = "0").getAnOperand() or + va = any(EQExpr eq | + eq = getCheckedFalseCondition(this) and eq.getAnOperand().getValue() = "0" + ).getAnOperand() + ) + ) + } + + /** The parameter being null-checked. */ + Parameter getParameter() { result = p } + + override ControlFlowNode getIgnoredSuccessorForContext(Context c) { + c = ParamNull(p) and result = notNullSuccessor + or + c = ParamNotNull(p) and result = nullSuccessor + } + + /** The successor at which the parameter is confirmed to be null. */ + ControlFlowNode getNullSuccessor() { result = nullSuccessor } + + /** The successor at which the parameter is confirmed to be not-null. */ + ControlFlowNode getNotNullSuccessor() { result = notNullSuccessor } +} + +/** + * An entry in a CSV file in cond-init that contains externally defined functions that are + * conditional initializers. These files are typically produced by running the + * ConditionallyInitializedFunction companion query. + */ +class ValidatedExternalCondInitFunction extends ExternalData { + ValidatedExternalCondInitFunction() { this.getDataPath().matches("%cond-init%.csv") } + + predicate isExternallyVerified(Function f, int param) { + functionSignature(f, getField(1), getField(2)) and param = getFieldAsInt(3) + } +} + +/** + * The type of evidence used to determine whether a function initializes a parameter. + */ +newtype Evidence = + /** + * The function is defined in the snapshot, and the CFG has been analyzed to determine that the + * parameter is not initialized on at least one path to the exit. + */ + DefinitionInSnapshot() or + /** + * The function is externally defined, but the parameter has an `_out` SAL annotation which + * suggests that it is initialized in the function. + */ + SuggestiveSALAnnotation() or + /** + * We have been given a CSV file which indicates this parameter is conditionally initialized. + */ + ExternalEvidence() + +/** + * A call to an function which initializes one or more of its parameters. + */ +class InitializationFunctionCall extends FunctionCall { + Expr initializedArgument; + + InitializationFunctionCall() { initializedArgument = getAnInitializedArgument(this) } + + /** Gets a parameter that is initialized by this call. */ + Parameter getAnInitParameter() { result.getAnAccess() = initializedArgument } +} + +/** + * A variable access which is dereferenced then assigned to. + */ +private predicate isPointerDereferenceAssignmentTarget(VariableAccess target) { + target.getParent().(PointerDereferenceExpr) = any(Assignment e).getLValue() +} + +/** + * A function which initializes one or more of its parameters. + */ +class InitializationFunction extends Function { + int i; + Evidence evidence; + + InitializationFunction() { + evidence = DefinitionInSnapshot() and + ( + // Assignment by pointer dereferencing the parameter + isPointerDereferenceAssignmentTarget(this.getParameter(i).getAnAccess()) or + // Field wise assignment to the parameter + any(Assignment e).getLValue() = getAFieldAccess(this.getParameter(i)) or + i = this + .(MemberFunction) + .getAnOverridingFunction+() + .(InitializationFunction) + .initializedParameter() or + getParameter(i) = any(InitializationFunctionCall c).getAnInitParameter() + ) + or + // If we have no definition, we look at SAL annotations + not this.isDefined() and + this.getParameter(i).(SALParameter).isOut() and + evidence = SuggestiveSALAnnotation() + or + // We have some external information that this function conditionally initializes + not this.isDefined() and + any(ValidatedExternalCondInitFunction vc).isExternallyVerified(this, i) and + evidence = ExternalEvidence() + } + + /** Gets a parameter index which is initialized by this function. */ + int initializedParameter() { result = i } + + /** Gets a `ControlFlowNode` which assigns a new value to the parameter with the given index. */ + ControlFlowNode paramReassignment(int index) { + index = i and + ( + result = this.getParameter(i).getAnAccess() and + ( + result = any(Assignment a).getLValue().(PointerDereferenceExpr).getOperand() + or + // Field wise assignment to the parameter + result = any(Assignment a).getLValue().(FieldAccess).getQualifier() + or + // Assignment to a nested field of the parameter + result = any(Assignment a).getLValue().(NestedFieldAccess).getUltimateQualifier() + or + result = getAnInitializedArgument(any(Call c)) + or + exists(IfStmt check | result = check.getCondition().getAChild*() | + paramReassignmentCondition(check) + ) + ) + or + result = any(AssumeExpr e | + e.getEnclosingFunction() = this and e.getAChild().(Literal).getValue() = "0" + ) + ) + } + + /** + * Helper predicate: holds if the `if` statement `check` contains a + * reassignment to the `i`th parameter within its `then` statement. + */ + pragma[noinline] + private predicate paramReassignmentCondition(IfStmt check) { + this.paramReassignment(i).getEnclosingStmt().getParentStmt*() = check.getThen() + } + + /** Holds if `n` can be reached without the parameter at `index` being reassigned. */ + predicate paramNotReassignedAt(ControlFlowNode n, int index, Context c) { + c = getAContext(index) and + ( + not exists(this.getEntryPoint()) and index = i and n = this + or + n = this.getEntryPoint() and index = i + or + exists(ControlFlowNode mid | paramNotReassignedAt(mid, index, c) | + n = mid.getASuccessor() and + not n = paramReassignment(index) and + /* + * Ignore successor edges where the parameter is null, because it is then confirmed to be + * initialized. + */ + + not exists(ParameterNullCheck nullCheck | + nullCheck = mid and + nullCheck = getANullCheck(index) and + n = nullCheck.getNullSuccessor() + ) and + /* + * Ignore successor edges which are excluded by the given context + */ + + not exists(ParameterCheck paramCheck | paramCheck = mid | + n = paramCheck.getIgnoredSuccessorForContext(c) + ) + ) + ) + } + + /** Gets a null-check on the parameter at `index`. */ + private ParameterNullCheck getANullCheck(int index) { + getParameter(index) = result.getParameter() + } + + /** Gets a parameter which is not at the given index. */ + private Parameter getOtherParameter(int index) { + index = i and + result = getAParameter() and + not result.getIndex() = index + } + + /** + * Gets a call `Context` that is applicable when considering whether parameter at the `index` can + * be conditionally initialized. + */ + Context getAContext(int index) { + index = i and + /* + * If there is one and only one other parameter which is null checked in the body of the method, + * then we have two contexts to consider - that the other param is null, or that the other param + * is not null. + */ + + if + strictcount(Parameter p | + exists(Context c | c = ParamNull(p) or c = ParamNotNull(p)) and + p = getOtherParameter(index) + ) = 1 + then + exists(Parameter p | p = getOtherParameter(index) | + result = ParamNull(p) or result = ParamNotNull(p) + ) + else + // Otherwise, only consider NoContext. + result = NoContext() + } + + /** + * Holds if this function should be whitelisted - that is, not considered as conditionally + * initializing its parameters. + */ + predicate whitelisted() { + exists(string name | this.hasName(name) | + // Return value is not a success code but the output functions never fail. + name.matches("_Interlocked%") + or + // Functions that never fail, according to MSDN. + name = "QueryPerformanceCounter" + or + name = "QueryPerformanceFrequency" + or + // Functions that never fail post-Vista, according to MSDN. + name = "InitializeCriticalSectionAndSpinCount" + or + // `rand_s` writes 0 to a non-null argument if it fails, according to MSDN. + name = "rand_s" + or + // IntersectRect initializes the argument regardless of whether the input intersects + name = "IntersectRect" + or + name = "SetRect" + or + name = "UnionRect" + or + // These functions appears to have an incorrect CFG, which leads to false positives + name = "PhysicalToLogicalDPIPoint" + or + name = "LogicalToPhysicalDPIPoint" + or + // Sets NtProductType to default on error + name = "RtlGetNtProductType" + or + // Our CFG is not sophisticated enough to detect that the argument is always initialized + name = "StringCchLengthA" + or + // All paths init the argument, and always returns SUCCESS. + name = "RtlUnicodeToMultiByteSize" + or + // All paths init the argument, and always returns SUCCESS. + name = "RtlMultiByteToUnicodeSize" + or + // All paths init the argument, and always returns SUCCESS. + name = "RtlUnicodeToMultiByteN" + or + // Always initializes argument + name = "RtlGetFirstRange" + or + // Destination range is zeroed out on failure, assuming first two parameters are valid + name = "memcpy_s" + or + // This zeroes the memory unconditionally + name = "SeCreateAccessState" + ) + } +} + +/** + * A function which initializes one or more of its parameters, but not on all paths. + */ +class ConditionalInitializationFunction extends InitializationFunction { + Context c; + + ConditionalInitializationFunction() { + c = this.getAContext(i) and + not this.whitelisted() and + exists(Type status | status = this.getType().getUnspecifiedType() | + status instanceof IntegralType or + status instanceof Enum + ) and + not this.getType().getName().toLowerCase() = "size_t" and + ( + /* + * If there is no definition, consider this to be conditionally initializing (based on either + * SAL or external data). + */ + + not evidence = DefinitionInSnapshot() + or + /* + * If this function is defined in this snapshot, then it conditionally initializes if there + * is at least one path through the function which doesn't initialize the parameter. + * + * Explicitly ignore pure virtual functions. + */ + + this.isDefined() and + this.paramNotReassignedAt(this, i, c) and + not this instanceof PureVirtualFunction + ) + } + + /** Gets the evidence associated with the given parameter. */ + Evidence getEvidence(int param) { + /* + * Note: due to the way the predicate dispatch interacts with fields, this needs to be + * implemented on this class, not `InitializationFunction`. If implemented on the latter it + * can return evidence that does not result in conditional initialization. + */ + + param = i and evidence = result + } + + /** Gets the index of a parameter which is conditionally initialized. */ + int conditionallyInitializedParameter(Context context) { result = i and context = c } +} + +/** + * More elaborate tracking, flagging cases where the status is checked after + * the potentially uninitialized variable has been used, and ignoring cases + * where the status is not checked but there is no use of the potentially + * uninitialized variable, may be obtained via `getARiskyAccess`. + */ +class ConditionalInitializationCall extends FunctionCall { + ConditionalInitializationFunction target; + + ConditionalInitializationCall() { target = getTarget(this) } + + /** Gets the argument passed for the given parameter to this call. */ + Expr getArgumentForParameter(Parameter p) { + p = getTarget().getAParameter() and + result = getArgument(p.getIndex()) + } + + /** + * Gets an argument conditionally initialized by this call. + */ + Expr getAConditionallyInitializedArgument(ConditionalInitializationFunction condTarget, Evidence e) { + condTarget = target and + exists(Context context | + result = getAConditionallyInitializedArgument(this, condTarget, context, e) + | + context = NoContext() + or + exists(Parameter otherP, Expr otherArg | + context = ParamNotNull(otherP) or + context = ParamNull(otherP) + | + otherArg = getArgumentForParameter(otherP) and + (otherArg instanceof AddressOfExpr implies context = ParamNotNull(otherP)) and + (otherArg.getType() instanceof ArrayType implies context = ParamNotNull(otherP)) and + (otherArg.getValue() = "0" implies context = ParamNull(otherP)) + ) + ) + } + + VariableAccess getAConditionallyInitializedVariable() { + not result.getTarget().getAnAssignedValue().getASuccessor+() = result and + // Should not be assigned field-wise prior to the call. + not exists(Assignment a, FieldAccess fa | + fa.getQualifier() = result.getTarget().getAnAccess() and + a.getLValue() = fa and + fa.getASuccessor+() = result + ) and + result = this + .getArgument(getTarget(this) + .(ConditionalInitializationFunction) + .conditionallyInitializedParameter(_)) + .(AddressOfExpr) + .getOperand() + } + + Variable getStatusVariable() { + exists(AssignExpr a | a.getLValue() = result.getAnAccess() | a.getRValue() = this) + or + result.getInitializer().getExpr() = this + } + + Expr getSuccessCheck() { + exists(this.getAFalseSuccessor()) and result = this + or + result = this.getParent() and + ( + result instanceof NotExpr or + result.(EQExpr).getAnOperand().getValue() = "0" or + result.(GEExpr).getLesserOperand().getValue() = "0" + ) + } + + Expr getFailureCheck() { + result = this.getParent() and + ( + result instanceof NotExpr or + result.(NEExpr).getAnOperand().getValue() = "0" or + result.(LTExpr).getLesserOperand().getValue() = "0" + ) + } + + private predicate inCheckedContext() { + exists(Call parent | this = parent.getAnArgument() | + parent.getTarget() instanceof Operator or + parent.getTarget().hasName("VerifyOkCatastrophic") + ) + } + + ControlFlowNode uncheckedReaches(LocalVariable var) { + ( + not exists(var.getInitializer()) and + var = this.getAConditionallyInitializedVariable().getTarget() and + if exists(this.getFailureCheck()) + then result = this.getFailureCheck().getATrueSuccessor() + else + if exists(this.getSuccessCheck()) + then result = this.getSuccessCheck().getAFalseSuccessor() + else ( + result = this.getASuccessor() and not this.inCheckedContext() + ) + ) + or + exists(ControlFlowNode mid | mid = uncheckedReaches(var) | + not mid = getStatusVariable().getAnAccess() and + not mid = var.getAnAccess() and + not exists(VariableAccess write | result = write and write = var.getAnAccess() | + write = any(AssignExpr a).getLValue() or + write = any(AddressOfExpr a).getOperand() + ) and + result = mid.getASuccessor() + ) + } + + VariableAccess getARiskyRead(Function f) { + f = this.getTarget() and + exists(this.getFile().getRelativePath()) and + result = this.uncheckedReaches(result.getTarget()) and + not this.(GuardCondition).controls(result.getBasicBlock(), _) + } +} + +/** + * Gets the position of an argument to the call which is initialized by the call. + */ +pragma[nomagic] +int initializedArgument(Call call) { + exists(InitializationFunction target | + target = getTarget(call) and + result = target.initializedParameter() + ) +} + +/** + * Gets an argument which is initialized by the call. + */ +Expr getAnInitializedArgument(Call call) { result = call.getArgument(initializedArgument(call)) } + +/** + * Gets the position of an argument to the call to the target which is conditionally initialized by + * the call, under the given context and evidence. + */ +pragma[nomagic] +private int conditionallyInitializedArgument( + Call call, ConditionalInitializationFunction target, Context c, Evidence e +) { + target = getTarget(call) and + c = target.getAContext(result) and + e = target.getEvidence(result) and + result = target.conditionallyInitializedParameter(c) +} + +/** + * Gets an argument which is conditionally initialized by the call to the given target under the given context and evidence. + */ +Expr getAConditionallyInitializedArgument( + Call call, ConditionalInitializationFunction target, Context c, Evidence e +) { + result = call.getArgument(conditionallyInitializedArgument(call, target, c, e)) +} + +/** + * Gets the type signature for the functions parameters. + */ +private string typeSig(Function f) { + result = concat(int i, Type pt | + pt = f.getParameter(i).getType() + | + pt.getUnspecifiedType().toString(), "," order by i + ) +} + +/** + * Holds where qualifiedName and typeSig make up the signature for the function. + */ +private predicate functionSignature(Function f, string qualifiedName, string typeSig) { + qualifiedName = f.getQualifiedName() and + typeSig = typeSig(f) +} + +/** + * Gets a possible definition for the undefined function by matching the undefined function name + * and parameter arity with a defined function. + * + * This is useful for identifying call to target dependencies across libraries, where the libraries + * are never statically linked together. + */ +private Function getAPossibleDefinition(Function undefinedFunction) { + not undefinedFunction.isDefined() and + exists(string qn, string typeSig | + functionSignature(undefinedFunction, qn, typeSig) and functionSignature(result, qn, typeSig) + ) and + result.isDefined() +} + +/** + * Helper predicate for `getTarget`, that computes possible targets of a `Call`. + * + * If there is at least one defined target after performing some simple virtual dispatch + * resolution, then the result is all the defined targets. + */ +private Function getTarget1(Call c) { + result = VirtualDispatch::getAViableTarget(c) and + result.isDefined() +} + +/** + * Helper predicate for `getTarget`, that computes possible targets of a `Call`. + * + * If we can use the heuristic matching of functions to find definitions for some of the viable + * targets, return those. + */ +private Function getTarget2(Call c) { + not exists(getTarget1(c)) and + result = getAPossibleDefinition(VirtualDispatch::getAViableTarget(c)) +} + +/** + * Helper predicate for `getTarget`, that computes possible targets of a `Call`. + * + * Otherwise, the result is the undefined `Function` instances. + */ +private Function getTarget3(Call c) { + not exists(getTarget1(c)) and + not exists(getTarget2(c)) and + result = VirtualDispatch::getAViableTarget(c) +} + +/** + * Gets a possible target for the `Call`, using the name and parameter matching if we did not associate + * this call with a specific definition at link or compile time, and performing simple virtual + * dispatch resolution. + */ +Function getTarget(Call c) { + result = getTarget1(c) or + result = getTarget2(c) or + result = getTarget3(c) +} + +/** + * Get an access of a field on `Variable` v. + */ +FieldAccess getAFieldAccess(Variable v) { + exists(VariableAccess va, Expr qualifierExpr | + // Find an access of the variable, or an AddressOfExpr that has the access + va = v.getAnAccess() and + ( + qualifierExpr = va or + qualifierExpr.(AddressOfExpr).getOperand() = va + ) + | + // Direct field access + qualifierExpr = result.getQualifier() + or + // Nested field access + qualifierExpr = result.(NestedFieldAccess).getUltimateQualifier() + ) +} + +/** + * Gets a condition which is checked to be false by the given `ne` expression, according to this pattern: + * ``` + * int a = !!result; + * if (!a) { // <- ne + * .... + * } + * ``` + */ +private Expr getCheckedFalseCondition(NotExpr ne) { + exists(LocalVariable v | + result = v.getInitializer().getExpr().(NotExpr).getOperand().(NotExpr).getOperand() and + ne.getOperand() = v.getAnAccess() and + nonAssignedVariable(v) + // and not passed by val? + ) +} + +pragma[noinline] +private predicate nonAssignedVariable(Variable v) { not exists(v.getAnAssignment()) } diff --git a/cpp/ql/src/Security/CWE/CWE-457/UninitializedVariables.qll b/cpp/ql/src/Security/CWE/CWE-457/UninitializedVariables.qll new file mode 100644 index 00000000000..4289f66e21d --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-457/UninitializedVariables.qll @@ -0,0 +1,190 @@ +/** + * A module for identifying conditionally initialized variables. + */ + +import cpp +import InitializationFunctions + +// Optimised reachability predicates +private predicate reaches(ControlFlowNode a, ControlFlowNode b) = fastTC(successor/2)(a, b) + +private predicate successor(ControlFlowNode a, ControlFlowNode b) { b = a.getASuccessor() } + +class WhitelistedCallsConfig extends string { + WhitelistedCallsConfig() { this = "config" } + + abstract predicate isWhitelisted(Call c); +} + +abstract class WhitelistedCall extends Call { + override Function getTarget() { none() } +} + +private predicate hasConditionalInitialization( + ConditionalInitializationFunction f, ConditionalInitializationCall call, LocalVariable v, + VariableAccess initAccess, Evidence e +) { + // Ignore whitelisted calls + not call instanceof WhitelistedCall and + f = getTarget(call) and + initAccess = v.getAnAccess() and + initAccess = call.getAConditionallyInitializedArgument(f, e).(AddressOfExpr).getOperand() +} + +/** + * A variable that can be conditionally initialized by a call. + */ +class ConditionallyInitializedVariable extends LocalVariable { + ConditionalInitializationCall call; + ConditionalInitializationFunction f; + VariableAccess initAccess; + Evidence e; + + ConditionallyInitializedVariable() { + // Find a call that conditionally initializes this variable + hasConditionalInitialization(f, call, this, initAccess, e) and + // Ignore cases where the variable is assigned prior to the call + not reaches(getAnAssignedValue(), initAccess) and + // Ignore cases where the variable is assigned field-wise prior to the call. + not exists(FieldAccess fa | + exists(Assignment a | + fa = getAFieldAccess(this) and + a.getLValue() = fa + ) + | + reaches(fa, initAccess) + ) and + // Ignore cases where the variable is assigned by a prior call to an initialization function + not exists(Call c | + getAnAccess() = getAnInitializedArgument(c).(AddressOfExpr).getOperand() and + reaches(c, initAccess) + ) and + /* + * Static local variables with constant initializers do not have the initializer expr as part of + * the CFG, but should always be considered as initialized, so exclude them. + */ + + not exists(getInitializer().getExpr()) + } + + /** + * Gets an access of the variable `v` which is not used as an lvalue, and not used as an argument + * to an initialization function. + */ + private VariableAccess getAReadAccess() { + result = this.getAnAccess() and + // Not used as an lvalue + not result = any(AssignExpr a).getLValue() and + // Not passed to another initialization function + not exists(Call c, int j | j = c.getTarget().(InitializationFunction).initializedParameter() | + result = c.getArgument(j).(AddressOfExpr).getOperand() + ) and + // Not a pointless read + not result = any(ExprStmt es).getExpr() + } + + /** + * Gets a read access of variable `v` that occurs after the `initializingCall`. + */ + private VariableAccess getAReadAccessAfterCall(ConditionalInitializationCall initializingCall) { + // Variable associated with this particular call + call = initializingCall and + // Access is a meaningful read access + result = getAReadAccess() and + // Which occurs after the call + reaches(call, result) and + /* + * Ignore risky accesses which are arguments to calls which also include another parameter to + * the original call. This is an attempt to eliminate results where the "status" can be checked + * through another parameter that assigned as part of the original call. + */ + + not exists(Call c | + c.getAnArgument() = result or + c.getAnArgument().(AddressOfExpr).getOperand() = result + | + exists(LocalVariable lv | + call.getAnArgument().(AddressOfExpr).getOperand() = lv.getAnAccess() and + not lv = this + | + c.getAnArgument() = lv.getAnAccess() + ) + ) + } + + /** + * Gets an access to the variable that is risky because the variable may not be initialized after + * the `call`, and the status of the call is never checked. + */ + VariableAccess getARiskyAccessWithNoStatusCheck( + ConditionalInitializationFunction initializingFunction, + ConditionalInitializationCall initializingCall, Evidence evidence + ) { + // Variable associated with this particular call + call = initializingCall and + initializingFunction = f and + e = evidence and + result = getAReadAccessAfterCall(initializingCall) and + ( + // Access is risky because status return code ignored completely + call instanceof ExprInVoidContext + or + // Access is risky because status return code ignored completely + exists(LocalVariable status | call = status.getAnAssignedValue() | + not exists(status.getAnAccess()) + ) + ) + } + + /** + * Gets an access to the variable that is risky because the variable may not be initialized after + * the `call`, and the status of the call is only checked after the risky access. + */ + VariableAccess getARiskyAccessBeforeStatusCheck( + ConditionalInitializationFunction initializingFunction, + ConditionalInitializationCall initializingCall, Evidence evidence + ) { + // Variable associated with this particular call + call = initializingCall and + initializingFunction = f and + e = evidence and + result = getAReadAccessAfterCall(initializingCall) and + exists(LocalVariable status, Assignment a | + a.getRValue() = call and + call = status.getAnAssignedValue() and + // There exists a check of the status code + definitionUsePair(status, a, _) and + // And the check of the status code does not occur before the risky access + not exists(VariableAccess statusAccess | + definitionUsePair(status, a, statusAccess) and + reaches(statusAccess, result) + ) and + // Ignore cases where the assignment to the status code is used directly + a instanceof ExprInVoidContext and + /* + * Ignore risky accesses which are arguments to calls which also include the status code. + * If both the risky value and status code are passed to a different function, that + * function is responsible for checking the status code. + */ + + not exists(Call c | + c.getAnArgument() = result or + c.getAnArgument().(AddressOfExpr).getOperand() = result + | + definitionUsePair(status, a, c.getAnArgument()) + ) + ) + } + + /** + * Gets an access to the variable that is risky because the variable may not be initialized after + * the `call`. + */ + VariableAccess getARiskyAccess( + ConditionalInitializationFunction initializingFunction, + ConditionalInitializationCall initializingCall, Evidence evidence + ) { + result = getARiskyAccessBeforeStatusCheck(initializingFunction, initializingCall, evidence) or + result = getARiskyAccessWithNoStatusCheck(initializingFunction, initializingCall, evidence) + } +} diff --git a/cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql b/cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql index fa74c85555d..63eca292297 100644 --- a/cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql +++ b/cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql @@ -190,11 +190,11 @@ private predicate windowsSystemInfo(FunctionCall source, Element use) { // void WINAPI GetSystemInfo(_Out_ LPSYSTEM_INFO lpSystemInfo); // void WINAPI GetNativeSystemInfo(_Out_ LPSYSTEM_INFO lpSystemInfo); ( - source.getTarget().hasName("GetVersionEx") or - source.getTarget().hasName("GetVersionExA") or - source.getTarget().hasName("GetVersionExW") or - source.getTarget().hasName("GetSystemInfo") or - source.getTarget().hasName("GetNativeSystemInfo") + source.getTarget().hasGlobalName("GetVersionEx") or + source.getTarget().hasGlobalName("GetVersionExA") or + source.getTarget().hasGlobalName("GetVersionExW") or + source.getTarget().hasGlobalName("GetSystemInfo") or + source.getTarget().hasGlobalName("GetNativeSystemInfo") ) and use = source.getArgument(0) } @@ -216,9 +216,9 @@ private predicate windowsFolderPath(FunctionCall source, Element use) { // _In_ BOOL fCreate // ); ( - source.getTarget().hasName("SHGetSpecialFolderPath") or - source.getTarget().hasName("SHGetSpecialFolderPathA") or - source.getTarget().hasName("SHGetSpecialFolderPathW") + source.getTarget().hasGlobalName("SHGetSpecialFolderPath") or + source.getTarget().hasGlobalName("SHGetSpecialFolderPathA") or + source.getTarget().hasGlobalName("SHGetSpecialFolderPathW") ) and use = source.getArgument(1) or @@ -228,7 +228,7 @@ private predicate windowsFolderPath(FunctionCall source, Element use) { // _In_opt_ HANDLE hToken, // _Out_ PWSTR *ppszPath // ); - source.getTarget().hasName("SHGetKnownFolderPath") and + source.getTarget().hasGlobalName("SHGetKnownFolderPath") and use = source.getArgument(3) or // HRESULT SHGetFolderPath( @@ -239,9 +239,9 @@ private predicate windowsFolderPath(FunctionCall source, Element use) { // _Out_ LPTSTR pszPath // ); ( - source.getTarget().hasName("SHGetFolderPath") or - source.getTarget().hasName("SHGetFolderPathA") or - source.getTarget().hasName("SHGetFolderPathW") + source.getTarget().hasGlobalName("SHGetFolderPath") or + source.getTarget().hasGlobalName("SHGetFolderPathA") or + source.getTarget().hasGlobalName("SHGetFolderPathW") ) and use = source.getArgument(4) or @@ -254,9 +254,9 @@ private predicate windowsFolderPath(FunctionCall source, Element use) { // _Out_ LPTSTR pszPath // ); ( - source.getTarget().hasName("SHGetFolderPathAndSubDir") or - source.getTarget().hasName("SHGetFolderPathAndSubDirA") or - source.getTarget().hasName("SHGetFolderPathAndSubDirW") + source.getTarget().hasGlobalName("SHGetFolderPathAndSubDir") or + source.getTarget().hasGlobalName("SHGetFolderPathAndSubDirA") or + source.getTarget().hasGlobalName("SHGetFolderPathAndSubDirW") ) and use = source.getArgument(5) } @@ -273,9 +273,9 @@ class WindowsFolderPath extends SystemData { private predicate logonUser(FunctionCall source, VariableAccess use) { ( - source.getTarget().hasName("LogonUser") or - source.getTarget().hasName("LogonUserW") or - source.getTarget().hasName("LogonUserA") + source.getTarget().hasGlobalName("LogonUser") or + source.getTarget().hasGlobalName("LogonUserW") or + source.getTarget().hasGlobalName("LogonUserA") ) and use = source.getAnArgument() } @@ -297,9 +297,9 @@ private predicate regQuery(FunctionCall source, VariableAccess use) { // _Inout_opt_ PLONG lpcbValue // ); ( - source.getTarget().hasName("RegQueryValue") or - source.getTarget().hasName("RegQueryValueA") or - source.getTarget().hasName("RegQueryValueW") + source.getTarget().hasGlobalName("RegQueryValue") or + source.getTarget().hasGlobalName("RegQueryValueA") or + source.getTarget().hasGlobalName("RegQueryValueW") ) and use = source.getArgument(2) or @@ -311,9 +311,9 @@ private predicate regQuery(FunctionCall source, VariableAccess use) { // _Inout_opt_ LPDWORD ldwTotsize // ); ( - source.getTarget().hasName("RegQueryMultipleValues") or - source.getTarget().hasName("RegQueryMultipleValuesA") or - source.getTarget().hasName("RegQueryMultipleValuesW") + source.getTarget().hasGlobalName("RegQueryMultipleValues") or + source.getTarget().hasGlobalName("RegQueryMultipleValuesA") or + source.getTarget().hasGlobalName("RegQueryMultipleValuesW") ) and use = source.getArgument(3) or @@ -326,9 +326,9 @@ private predicate regQuery(FunctionCall source, VariableAccess use) { // _Inout_opt_ LPDWORD lpcbData // ); ( - source.getTarget().hasName("RegQueryValueEx") or - source.getTarget().hasName("RegQueryValueExA") or - source.getTarget().hasName("RegQueryValueExW") + source.getTarget().hasGlobalName("RegQueryValueEx") or + source.getTarget().hasGlobalName("RegQueryValueExA") or + source.getTarget().hasGlobalName("RegQueryValueExW") ) and use = source.getArgument(4) or @@ -342,9 +342,9 @@ private predicate regQuery(FunctionCall source, VariableAccess use) { // _Inout_opt_ LPDWORD pcbData // ); ( - source.getTarget().hasName("RegGetValue") or - source.getTarget().hasName("RegGetValueA") or - source.getTarget().hasName("RegGetValueW") + source.getTarget().hasGlobalName("RegGetValue") or + source.getTarget().hasGlobalName("RegGetValueA") or + source.getTarget().hasGlobalName("RegGetValueW") ) and use = source.getArgument(5) } diff --git a/cpp/ql/src/Security/CWE/CWE-676/DangerousFunctionOverflow.ql b/cpp/ql/src/Security/CWE/CWE-676/DangerousFunctionOverflow.ql index 84d7b48e265..fe9b56bd521 100644 --- a/cpp/ql/src/Security/CWE/CWE-676/DangerousFunctionOverflow.ql +++ b/cpp/ql/src/Security/CWE/CWE-676/DangerousFunctionOverflow.ql @@ -15,5 +15,5 @@ import cpp from FunctionCall call, Function target where call.getTarget() = target and - target.hasGlobalName("gets") + target.hasGlobalOrStdName("gets") select call, "gets does not guard against buffer overflow" diff --git a/cpp/ql/src/codeql-suites/cpp-lgtm-full.qls b/cpp/ql/src/codeql-suites/cpp-lgtm-full.qls new file mode 100644 index 00000000000..2036584e44c --- /dev/null +++ b/cpp/ql/src/codeql-suites/cpp-lgtm-full.qls @@ -0,0 +1,14 @@ +- description: Standard LGTM queries for C/C++, including ones not displayed by default +- qlpack: codeql-cpp +- apply: lgtm-selectors.yml + from: codeql-suite-helpers +# These queries are infeasible to compute on large projects: +- exclude: + query path: + - Security/CWE/CWE-497/ExposedSystemData.ql + - Critical/DescriptorMayNotBeClosed.ql + - Critical/DescriptorNeverClosed.ql + - Critical/FileMayNotBeClosed.ql + - Critical/FileNeverClosed.ql + - Critical/MemoryMayNotBeFreed.ql + - Critical/MemoryNeverFreed.ql diff --git a/cpp/ql/src/codeql-suites/cpp-lgtm.qls b/cpp/ql/src/codeql-suites/cpp-lgtm.qls new file mode 100644 index 00000000000..fe06e19b7fa --- /dev/null +++ b/cpp/ql/src/codeql-suites/cpp-lgtm.qls @@ -0,0 +1,4 @@ +- description: Standard LGTM queries for C/C++ +- apply: codeql-suites/cpp-lgtm-full.qls +- apply: lgtm-displayed-only.yml + from: codeql-suite-helpers diff --git a/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql b/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql index 4b1f45185d1..f2512b93003 100644 --- a/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql +++ b/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql @@ -22,7 +22,7 @@ predicate acquireExpr(Expr acquire, string kind) { exists(FunctionCall fc, Function f, string name | fc = acquire and f = fc.getTarget() and - f.hasGlobalName(name) and + f.hasGlobalOrStdName(name) and ( name = "fopen" and kind = "file" @@ -46,7 +46,7 @@ predicate releaseExpr(Expr release, Expr resource, string kind) { exists(FunctionCall fc, Function f, string name | fc = release and f = fc.getTarget() and - f.hasGlobalName(name) and + f.hasGlobalOrStdName(name) and ( name = "fclose" and resource = fc.getArgument(0) and diff --git a/cpp/ql/src/jsf/4.10 Classes/AV Rule 81.ql b/cpp/ql/src/jsf/4.10 Classes/AV Rule 81.ql index 5a8df7fc625..2a3a70bd016 100644 --- a/cpp/ql/src/jsf/4.10 Classes/AV Rule 81.ql +++ b/cpp/ql/src/jsf/4.10 Classes/AV Rule 81.ql @@ -51,7 +51,7 @@ class ReferenceCopyAssignmentOperator extends MemberFunction { /** * A call to a function called swap. Note: could be a member, - * std::swap or a function overloading std::swap (not in std::) + * `std::swap` or a function overloading `std::swap` (not in `std::`) * so keep it simple */ FunctionCall getASwapCall() { diff --git a/cpp/ql/src/jsf/4.10 Classes/AV Rule 97.ql b/cpp/ql/src/jsf/4.10 Classes/AV Rule 97.ql index 501101f706c..872a7443e6e 100644 --- a/cpp/ql/src/jsf/4.10 Classes/AV Rule 97.ql +++ b/cpp/ql/src/jsf/4.10 Classes/AV Rule 97.ql @@ -22,8 +22,8 @@ predicate containsArray(Type t) { or containsArray(t.getUnderlyingType()) and not exists(TypedefType allowed | allowed = t | - allowed.hasGlobalName("jmp_buf") or - allowed.hasGlobalName("va_list") + allowed.hasGlobalOrStdName("jmp_buf") or + allowed.hasGlobalOrStdName("va_list") ) } diff --git a/cpp/ql/src/jsf/4.21 Operators/AV Rule 159.ql b/cpp/ql/src/jsf/4.21 Operators/AV Rule 159.ql index 99d60f7290d..5542dad405b 100644 --- a/cpp/ql/src/jsf/4.21 Operators/AV Rule 159.ql +++ b/cpp/ql/src/jsf/4.21 Operators/AV Rule 159.ql @@ -10,7 +10,7 @@ /* * See More Effective C++ item 7. - * Note: Meyers allows unary & to be overloaded but not comma + * Note: Meyers allows unary `&` to be overloaded but not comma. */ import cpp diff --git a/cpp/ql/src/jsf/4.28 Portable Code/AV Rule 213.ql b/cpp/ql/src/jsf/4.28 Portable Code/AV Rule 213.ql index fbfe211cb19..992351e4c82 100644 --- a/cpp/ql/src/jsf/4.28 Portable Code/AV Rule 213.ql +++ b/cpp/ql/src/jsf/4.28 Portable Code/AV Rule 213.ql @@ -15,11 +15,11 @@ import cpp /* * Interpretation and deviations: * - if the higher operator has precedence > arithmetic then it is fine - * RATIONALE: exprs like f(), *x, &x are easily understood to bind tightly + * RATIONALE: exprs like `f()`, `*x`, `&x` are easily understood to bind tightly * - if the higher operator is the RHS of an assign then it is fine * RATIONALE: cf. MISRA, too many cases excluded otherwise * - comparison operators can be mixed with arithmetic - * RATIONALE: x==y+z is common and unambiguous + * RATIONALE: `x==y+z` is common and unambiguous */ predicate arithmeticPrecedence(int p) { p = 12 or p = 13 } diff --git a/cpp/ql/src/qlpack.yml b/cpp/ql/src/qlpack.yml new file mode 100644 index 00000000000..a1c7df902de --- /dev/null +++ b/cpp/ql/src/qlpack.yml @@ -0,0 +1,4 @@ +name: codeql-cpp +version: 0.0.0 +dbscheme: semmlecode.cpp.dbscheme +suites: codeql-suites diff --git a/cpp/ql/src/semmle/code/cpp/Class.qll b/cpp/ql/src/semmle/code/cpp/Class.qll index 5ac4f52392e..297654a1afa 100644 --- a/cpp/ql/src/semmle/code/cpp/Class.qll +++ b/cpp/ql/src/semmle/code/cpp/Class.qll @@ -605,15 +605,6 @@ class Class extends UserType { class_instantiation(underlyingElement(this), unresolveElement(c)) } - /** - * Gets the `i`th template argument used to instantiate this class from a - * class template. When called on a class template, this will return the - * `i`th template parameter. - */ - override Type getTemplateArgument(int i) { - class_template_argument(underlyingElement(this), i, unresolveElement(result)) - } - /** * Holds if this class/struct is polymorphic (has a virtual function, or * inherits one). @@ -623,7 +614,7 @@ class Class extends UserType { } override predicate involvesTemplateParameter() { - getATemplateArgument().involvesTemplateParameter() + getATemplateArgument().(Type).involvesTemplateParameter() } /** Holds if this class, struct or union was declared 'final'. */ diff --git a/cpp/ql/src/semmle/code/cpp/Declaration.qll b/cpp/ql/src/semmle/code/cpp/Declaration.qll index fa061b239cc..1d5603fe4f4 100644 --- a/cpp/ql/src/semmle/code/cpp/Declaration.qll +++ b/cpp/ql/src/semmle/code/cpp/Declaration.qll @@ -14,8 +14,12 @@ private import semmle.code.cpp.internal.QualifiedName as Q * ``` * extern int myglobal; * ``` - * Each of these declarations is given its own distinct `DeclarationEntry`, - * but they all share the same `Declaration`. + * and defined in one: + * ``` + * int myglobal; + * ``` + * Each of these declarations (including the definition) is given its own + * distinct `DeclarationEntry`, but they all share the same `Declaration`. * * Some derived class of `Declaration` do not have a corresponding * `DeclarationEntry`, because they always have a unique source location. @@ -119,6 +123,13 @@ abstract class Declaration extends Locatable, @declaration { /** Holds if this declaration has the given name in the global namespace. */ predicate hasGlobalName(string name) { this.hasQualifiedName("", "", name) } + /** Holds if this declaration has the given name in the global namespace or the `std` namespace. */ + predicate hasGlobalOrStdName(string name) { + this.hasGlobalName(name) + or + this.hasQualifiedName("std", "", name) + } + /** Gets a specifier of this declaration. */ abstract Specifier getASpecifier(); @@ -189,26 +200,99 @@ abstract class Declaration extends Locatable, @declaration { /** * Gets a template argument used to instantiate this declaration from a template. - * When called on a template, this will return a template parameter. + * When called on a template, this will return a template parameter type for + * both typed and non-typed parameters. */ - final Type getATemplateArgument() { result = getTemplateArgument(_) } + final Locatable getATemplateArgument() { result = getTemplateArgument(_) } + + /** + * Gets a template argument used to instantiate this declaration from a template. + * When called on a template, this will return a non-typed template + * parameter value. + */ + final Locatable getATemplateArgumentKind() { result = getTemplateArgumentKind(_) } /** * Gets the `i`th template argument used to instantiate this declaration from a - * template. When called on a template, this will return the `i`th template parameter. + * template. + * + * For example: + * + * `template class Foo;` + * + * Will have `getTemplateArgument(0)` return `T`, and + * `getTemplateArgument(1)` return `X`. + * + * `Foo bar;` + * + * Will have `getTemplateArgument())` return `int`, and + * `getTemplateArgument(1)` return `1`. */ - Type getTemplateArgument(int index) { none() } + final Locatable getTemplateArgument(int index) { + if exists(getTemplateArgumentValue(index)) + then result = getTemplateArgumentValue(index) + else result = getTemplateArgumentType(index) + } + + /** + * Gets the `i`th template argument value used to instantiate this declaration + * from a template. When called on a template, this will return the `i`th template + * parameter value if it exists. + * + * For example: + * + * `template class Foo;` + * + * Will have `getTemplateArgumentKind(1)` return `T`, and no result for + * `getTemplateArgumentKind(0)`. + * + * `Foo bar; + * + * Will have `getTemplateArgumentKind(1)` return `int`, and no result for + * `getTemplateArgumentKind(0)`. + */ + final Locatable getTemplateArgumentKind(int index) { + if exists(getTemplateArgumentValue(index)) + then result = getTemplateArgumentType(index) + else none() + } /** Gets the number of template arguments for this declaration. */ final int getNumberOfTemplateArguments() { result = count(int i | exists(getTemplateArgument(i))) } + + private Type getTemplateArgumentType(int index) { + class_template_argument(underlyingElement(this), index, unresolveElement(result)) + or + function_template_argument(underlyingElement(this), index, unresolveElement(result)) + or + variable_template_argument(underlyingElement(this), index, unresolveElement(result)) + } + + private Expr getTemplateArgumentValue(int index) { + class_template_argument_value(underlyingElement(this), index, unresolveElement(result)) + or + function_template_argument_value(underlyingElement(this), index, unresolveElement(result)) + or + variable_template_argument_value(underlyingElement(this), index, unresolveElement(result)) + } } /** - * A C/C++ declaration entry. See the comment above `Declaration` for an - * explanation of the relationship between `Declaration` and - * `DeclarationEntry`. + * A C/C++ declaration entry. For example the following code contains five + * declaration entries: + * ``` + * extern int myGlobal; + * int myVariable; + * typedef char MyChar; + * void myFunction(); + * void myFunction() { + * // ... + * } + * ``` + * See the comment above `Declaration` for an explanation of the relationship + * between `Declaration` and `DeclarationEntry`. */ abstract class DeclarationEntry extends Locatable { /** Gets a specifier associated with this declaration entry. */ @@ -281,8 +365,19 @@ abstract class DeclarationEntry extends Locatable { * A declaration that can potentially have more C++ access rights than its * enclosing element. This comprises `Class` (they have access to their own * private members) along with other `UserType`s and `Function` (they can be - * the target of `friend` declarations). + * the target of `friend` declarations). For example `MyClass` and + * `myFunction` in the following code: + * ``` + * class MyClass + * { + * public: + * ... + * }; * + * void myFunction() { + * // ... + * } + * ``` * In the C++ standard (N4140 11.2), rules for access control revolve around * the informal phrase "_R_ occurs in a member or friend of class C", where * `AccessHolder` corresponds to this _R_. @@ -416,8 +511,19 @@ abstract class AccessHolder extends Declaration { /** * A declaration that very likely has more C++ access rights than its * enclosing element. This comprises `Class` (they have access to their own - * private members) along with any target of a `friend` declaration. + * private members) along with any target of a `friend` declaration. For + * example `MyClass` and `friendFunction` in the following code: + * ``` + * class MyClass + * { + * public: + * friend void friendFunction(); + * }; * + * void friendFunction() { + * // ... + * } + * ``` * Most access rights are computed for `DirectAccessHolder` instead of * `AccessHolder` -- that's more efficient because there are fewer * `DirectAccessHolder`s. If a `DirectAccessHolder` contains an `AccessHolder`, diff --git a/cpp/ql/src/semmle/code/cpp/Function.qll b/cpp/ql/src/semmle/code/cpp/Function.qll index 94d11d63575..85c277f945b 100644 --- a/cpp/ql/src/semmle/code/cpp/Function.qll +++ b/cpp/ql/src/semmle/code/cpp/Function.qll @@ -343,15 +343,6 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function { function_instantiation(underlyingElement(this), unresolveElement(f)) } - /** - * Gets the `i`th template argument used to instantiate this function from a - * function template. When called on a function template, this will return the - * `i`th template parameter. - */ - override Type getTemplateArgument(int index) { - function_template_argument(underlyingElement(this), index, unresolveElement(result)) - } - /** * Holds if this function is defined in several files. This is illegal in * C (though possible in some C++ compilers), and likely indicates that @@ -434,7 +425,7 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function { // ... and likewise for destructors. this.(Destructor).getADestruction().mayBeGloballyImpure() else - not exists(string name | this.hasGlobalName(name) | + not exists(string name | this.hasGlobalOrStdName(name) | // Unless it's a function that we know is side-effect-free, it may // have side-effects. name = "strcmp" or diff --git a/cpp/ql/src/semmle/code/cpp/NestedFields.qll b/cpp/ql/src/semmle/code/cpp/NestedFields.qll new file mode 100644 index 00000000000..c4be8b8b9ff --- /dev/null +++ b/cpp/ql/src/semmle/code/cpp/NestedFields.qll @@ -0,0 +1,40 @@ +import cpp + +/** + * Gets a `Field` that is within the given `Struct`, either directly or nested + * inside one or more levels of member structs. + */ +private Field getANestedField(Struct s) { + result = s.getAField() + or + exists(NestedStruct ns | + s = ns.getDeclaringType() and + result = getANestedField(ns) + ) +} + +/** + * Unwraps a series of field accesses to determine the outer-most qualifier. + */ +private Expr getUltimateQualifier(FieldAccess fa) { + exists(Expr qualifier | qualifier = fa.getQualifier() | + result = getUltimateQualifier(qualifier) + or + not qualifier instanceof FieldAccess and result = qualifier + ) +} + +/** + * Accesses to nested fields. + */ +class NestedFieldAccess extends FieldAccess { + Expr ultimateQualifier; + + NestedFieldAccess() { + ultimateQualifier = getUltimateQualifier(this) and + getTarget() = getANestedField(ultimateQualifier.getType().stripType()) + } + + /** Gets the ultimate qualifier of this nested field access. */ + Expr getUltimateQualifier() { result = ultimateQualifier } +} diff --git a/cpp/ql/src/semmle/code/cpp/Parameter.qll b/cpp/ql/src/semmle/code/cpp/Parameter.qll index 74326ada99c..8b391101c6c 100644 --- a/cpp/ql/src/semmle/code/cpp/Parameter.qll +++ b/cpp/ql/src/semmle/code/cpp/Parameter.qll @@ -158,3 +158,10 @@ class Parameter extends LocalScopeVariable, @parameter { ) } } + +/** + * An `int` that is a parameter index for some function. This is needed for binding in certain cases. + */ +class ParameterIndex extends int { + ParameterIndex() { exists(Parameter p | this = p.getIndex()) } +} diff --git a/cpp/ql/src/semmle/code/cpp/Print.qll b/cpp/ql/src/semmle/code/cpp/Print.qll index ab979d89b03..e3ad19f6028 100644 --- a/cpp/ql/src/semmle/code/cpp/Print.qll +++ b/cpp/ql/src/semmle/code/cpp/Print.qll @@ -35,6 +35,14 @@ private string getParameterTypeString(Type parameterType) { else result = parameterType.(DumpType).getTypeIdentityString() } +private string getTemplateArgumentString(Declaration d, int i) { + if exists(d.getTemplateArgumentKind(i)) + then + result = d.getTemplateArgumentKind(i).(DumpType).getTypeIdentityString() + " " + + d.getTemplateArgument(i) + else result = d.getTemplateArgument(i).(DumpType).getTypeIdentityString() +} + /** * A `Declaration` extended to add methods for generating strings useful only for dumps and debugging. */ @@ -56,7 +64,7 @@ abstract private class DumpDeclaration extends Declaration { strictconcat(int i | exists(this.getTemplateArgument(i)) | - this.getTemplateArgument(i).(DumpType).getTypeIdentityString(), ", " order by i + getTemplateArgumentString(this, i), ", " order by i ) + ">" else result = "" } diff --git a/cpp/ql/src/semmle/code/cpp/PrintAST.ql b/cpp/ql/src/semmle/code/cpp/PrintAST.ql index 6fc40dd0525..e4c53030da5 100644 --- a/cpp/ql/src/semmle/code/cpp/PrintAST.ql +++ b/cpp/ql/src/semmle/code/cpp/PrintAST.ql @@ -7,3 +7,15 @@ import cpp import PrintAST + +/** + * Temporarily tweak this class or make a copy to control which functions are + * printed. + */ +class Cfg extends PrintASTConfiguration { + /** + * TWEAK THIS PREDICATE AS NEEDED. + * Holds if the AST for `func` should be printed. + */ + override predicate shouldPrintFunction(Function func) { any() } +} diff --git a/cpp/ql/src/semmle/code/cpp/Type.qll b/cpp/ql/src/semmle/code/cpp/Type.qll index c65c4941355..ae42165bab8 100644 --- a/cpp/ql/src/semmle/code/cpp/Type.qll +++ b/cpp/ql/src/semmle/code/cpp/Type.qll @@ -5,6 +5,8 @@ private import semmle.code.cpp.internal.ResolveClass /** * A C/C++ type. + * + * This QL class represents the root of the C/C++ type hierarchy. */ class Type extends Locatable, @type { Type() { isType(underlyingElement(this)) } @@ -210,7 +212,7 @@ class Type extends Locatable, @type { // A function call that provides an explicit template argument that refers to T uses T. // We exclude calls within instantiations, since they do not appear directly in the source. exists(FunctionCall c | - c.getAnExplicitTemplateArgument().refersTo(this) and + c.getAnExplicitTemplateArgument().(Type).refersTo(this) and result = c and not c.getEnclosingFunction().isConstructedFrom(_) ) @@ -289,6 +291,13 @@ class Type extends Locatable, @type { /** * A C/C++ built-in primitive type (int, float, void, and so on). See 4.1.1. + * In the following example, `unsigned int` and `double` denote primitive + * built-in types: + * ``` + * double a; + * unsigned int ua[40]; + * typedef double LargeFloat; + * ``` */ class BuiltInType extends Type, @builtintype { override string toString() { result = this.getName() } @@ -301,7 +310,14 @@ class BuiltInType extends Type, @builtintype { } /** - * An erroneous type. + * An erroneous type. This type has no corresponding C/C++ syntax. + * + * `ErroneousType` is the type of `ErrorExpr`, which in turn refers to an illegal + * language construct. In the example below, a temporary (`0`) cannot be bound + * to an lvalue reference (`int &`): + * ``` + * int &intref = 0; + * ``` */ class ErroneousType extends BuiltInType { ErroneousType() { builtintypes(underlyingElement(this), _, 1, _, _, _) } @@ -310,7 +326,18 @@ class ErroneousType extends BuiltInType { } /** - * The unknown type. + * The unknown type. This type has no corresponding C/C++ syntax. + * + * Unknown types usually occur inside _uninstantiated_ template functions. + * In the example below, the expressions `x.a` and `x.b` have unknown type + * in the _uninstantiated_ template. + * ``` + * template + * bool check(T x) { + * if (x.a == x.b) + * abort(); + * } + * ``` */ class UnknownType extends BuiltInType { UnknownType() { builtintypes(underlyingElement(this), _, 2, _, _, _) } @@ -326,6 +353,10 @@ private predicate isArithmeticType(@builtintype type, int kind) { /** * The C/C++ arithmetic types. See 4.1.1. + * + * This includes primitive types on which arithmetic, bitwise or logical + * operations may be performed. Examples of arithmetic types include + * `char`, `int`, `float`, and `bool`. */ class ArithmeticType extends BuiltInType { ArithmeticType() { isArithmeticType(underlyingElement(this), _) } @@ -349,11 +380,20 @@ private predicate isIntegralType(@builtintype type, int kind) { } /** - * A C/C++ integral or enum type. - * The definition of "integral type" in the C++ Standard excludes enum types, - * but because an enum type holds a value of its underlying integral type, + * A C/C++ integral or `enum` type. + * + * The definition of "integral type" in the C++ standard excludes `enum` types, + * but because an `enum` type holds a value of its underlying integral type, * it is often useful to have a common category that includes both integral - * and enum types. + * and `enum` types. + * + * In the following example, `a`, `b` and `c` are all declared with an + * integral or `enum` type: + * ``` + * unsigned long a; + * enum e1 { val1, val2 } b; + * enum class e2: short { val3, val4 } c; + * ``` */ class IntegralOrEnumType extends Type { IntegralOrEnumType() { @@ -426,7 +466,17 @@ private predicate integralTypeMapping(int original, int canonical, int unsigned, } /** - * The C/C++ integral types. See 4.1.1. + * The C/C++ integral types. See 4.1.1. These are types that are represented + * as integers of varying sizes. Both `enum` types and floating-point types + * are excluded. + * + * In the following examples, `a`, `b` and `c` are declared using integral + * types: + * ``` + * unsigned int a; + * long long b; + * char c; + * ``` */ class IntegralType extends ArithmeticType, IntegralOrEnumType { int kind; @@ -497,7 +547,12 @@ class IntegralType extends ArithmeticType, IntegralOrEnumType { } /** - * The C/C++ boolean type. See 4.2. + * The C/C++ boolean type. See 4.2. This is the C `_Bool` type + * or the C++ `bool` type. For example: + * ``` + * extern bool a, b; // C++ + * _Bool c, d; // C + * ``` */ class BoolType extends IntegralType { BoolType() { builtintypes(underlyingElement(this), _, 4, _, _, _) } @@ -506,12 +561,23 @@ class BoolType extends IntegralType { } /** - * The C/C++ character types. See 4.3. + * The C/C++ character types. See 4.3. This includes the `char`, + * `signed char` and `unsigned char` types, all of which are + * distinct from one another. For example: + * ``` + * char a, b; + * signed char c, d; + * unsigned char e, f; + * ``` */ abstract class CharType extends IntegralType { } /** - * The C/C++ char type (which is different to signed char and unsigned char). + * The C/C++ `char` type (which is distinct from `signed char` and + * `unsigned char`). For example: + * ``` + * char a, b; + * ``` */ class PlainCharType extends CharType { PlainCharType() { builtintypes(underlyingElement(this), _, 5, _, _, _) } @@ -520,7 +586,11 @@ class PlainCharType extends CharType { } /** - * The C/C++ unsigned char type (which is different to plain char, even when chars are unsigned by default). + * The C/C++ `unsigned char` type (which is distinct from plain `char` + * even when `char` is `unsigned` by default). + * ``` + * unsigned char e, f; + * ``` */ class UnsignedCharType extends CharType { UnsignedCharType() { builtintypes(underlyingElement(this), _, 6, _, _, _) } @@ -529,7 +599,11 @@ class UnsignedCharType extends CharType { } /** - * The C/C++ signed char type (which is different to plain char, even when chars are signed by default). + * The C/C++ `signed char` type (which is distinct from plain `char` + * even when `char` is `signed` by default). + * ``` + * signed char c, d; + * ``` */ class SignedCharType extends CharType { SignedCharType() { builtintypes(underlyingElement(this), _, 7, _, _, _) } @@ -538,7 +612,11 @@ class SignedCharType extends CharType { } /** - * The C/C++ short types. See 4.3. + * The C/C++ short types. See 4.3. This includes `short`, `signed short` + * and `unsigned short`. + * ``` + * signed short ss; + * ``` */ class ShortType extends IntegralType { ShortType() { @@ -551,7 +629,11 @@ class ShortType extends IntegralType { } /** - * The C/C++ integer types. See 4.4. + * The C/C++ integer types. See 4.4. This includes `int`, `signed int` + * and `unsigned int`. + * ``` + * unsigned int ui; + * ``` */ class IntType extends IntegralType { IntType() { @@ -564,7 +646,11 @@ class IntType extends IntegralType { } /** - * The C/C++ long types. See 4.4. + * The C/C++ long types. See 4.4. This includes `long`, `signed long` + * and `unsigned long`. + * ``` + * long l; + * ``` */ class LongType extends IntegralType { LongType() { @@ -577,7 +663,11 @@ class LongType extends IntegralType { } /** - * The C/C++ long long types. See 4.4. + * The C/C++ long long types. See 4.4. This includes `long long`, `signed long long` + * and `unsigned long long`. + * ``` + * signed long long sll; + * ``` */ class LongLongType extends IntegralType { LongLongType() { @@ -590,7 +680,12 @@ class LongLongType extends IntegralType { } /** - * The GNU C __int128 types. + * The GNU C __int128 primitive types. They are not part of standard C/C++. + * + * This includes `__int128`, `signed __int128` and `unsigned __int128`. + * ``` + * unsigned __int128 ui128; + * ``` */ class Int128Type extends IntegralType { Int128Type() { @@ -598,10 +693,18 @@ class Int128Type extends IntegralType { builtintypes(underlyingElement(this), _, 36, _, _, _) or builtintypes(underlyingElement(this), _, 37, _, _, _) } + + override string getCanonicalQLClass() { result = "Int128Type" } } /** - * The C/C++ floating point types. See 4.5. + * The C/C++ floating point types. See 4.5. This includes `float`, + * `double` and `long double` types. + * ``` + * float f; + * double d; + * long double ld; + * ``` */ class FloatingPointType extends ArithmeticType { FloatingPointType() { @@ -610,14 +713,19 @@ class FloatingPointType extends ArithmeticType { ( kind >= 24 and kind <= 32 or - kind = 38 + kind >= 38 and kind <= 42 + or + kind >= 45 and kind <= 50 ) ) } } /** - * The C/C++ float type. + * The C/C++ `float` type. + * ``` + * float f; + * ``` */ class FloatType extends FloatingPointType { FloatType() { builtintypes(underlyingElement(this), _, 24, _, _, _) } @@ -626,7 +734,10 @@ class FloatType extends FloatingPointType { } /** - * The C/C++ double type. + * The C/C++ `double` type. + * ``` + * double d; + * ``` */ class DoubleType extends FloatingPointType { DoubleType() { builtintypes(underlyingElement(this), _, 25, _, _, _) } @@ -635,7 +746,10 @@ class DoubleType extends FloatingPointType { } /** - * The C/C++ long double type. + * The C/C++ `long double` type. + * ``` + * long double ld; + * ``` */ class LongDoubleType extends FloatingPointType { LongDoubleType() { builtintypes(underlyingElement(this), _, 26, _, _, _) } @@ -644,35 +758,58 @@ class LongDoubleType extends FloatingPointType { } /** - * The GNU C __float128 type. + * The GNU C `__float128` primitive type. This is not standard C/C++. + * ``` + * __float128 f128; + * ``` */ class Float128Type extends FloatingPointType { Float128Type() { builtintypes(underlyingElement(this), _, 38, _, _, _) } + + override string getCanonicalQLClass() { result = "Float128Type" } } /** - * The GNU C _Decimal32 type. + * The GNU C `_Decimal32` primitive type. This is not standard C/C++. + * ``` + * _Decimal32 d32; + * ``` */ class Decimal32Type extends FloatingPointType { Decimal32Type() { builtintypes(underlyingElement(this), _, 40, _, _, _) } + + override string getCanonicalQLClass() { result = "Decimal32Type" } } /** - * The GNU C _Decimal64 type. + * The GNU C `_Decimal64` primitive type. This is not standard C/C++. + * ``` + * _Decimal64 d64; + * ``` */ class Decimal64Type extends FloatingPointType { Decimal64Type() { builtintypes(underlyingElement(this), _, 41, _, _, _) } + + override string getCanonicalQLClass() { result = "Decimal64Type" } } /** - * The GNU C _Decimal128 type. + * The GNU C `_Decimal128` primitive type. This is not standard C/C++. + * ``` + * _Decimal128 d128; + * ``` */ class Decimal128Type extends FloatingPointType { Decimal128Type() { builtintypes(underlyingElement(this), _, 42, _, _, _) } + + override string getCanonicalQLClass() { result = "Decimal128Type" } } /** - * The C/C++ void type. See 4.7. + * The C/C++ `void` type. See 4.7. + * ``` + * void foo(); + * ``` */ class VoidType extends BuiltInType { VoidType() { builtintypes(underlyingElement(this), _, 3, _, _, _) } @@ -686,6 +823,9 @@ class VoidType extends BuiltInType { * Note that on some platforms `wchar_t` doesn't exist as a built-in * type but a typedef is provided. Consider using the `Wchar_t` QL * class to include these types. + * ``` + * wchar_t wc; + * ``` */ class WideCharType extends IntegralType { WideCharType() { builtintypes(underlyingElement(this), _, 33, _, _, _) } @@ -694,7 +834,10 @@ class WideCharType extends IntegralType { } /** - * The C/C++ `char16_t` type. + * The C/C++ `char16_t` type. This is available starting with C11 and C++11. + * ``` + * char16_t c16; + * ``` */ class Char16Type extends IntegralType { Char16Type() { builtintypes(underlyingElement(this), _, 43, _, _, _) } @@ -703,7 +846,10 @@ class Char16Type extends IntegralType { } /** - * The C/C++ `char32_t` type. + * The C/C++ `char32_t` type. This is available starting with C11 and C++11. + * ``` + * char32_t c32; + * ``` */ class Char32Type extends IntegralType { Char32Type() { builtintypes(underlyingElement(this), _, 44, _, _, _) } @@ -712,13 +858,13 @@ class Char32Type extends IntegralType { } /** - * The type of the C++11 nullptr constant. - * - * Note that this is not `nullptr_t`, as `nullptr_t` is defined as: + * The (primitive) type of the C++11 `nullptr` constant. It is a + * distinct type, denoted by `decltype(nullptr)`, that is not itself a pointer + * type or a pointer to member type. The `` header usually defines + * the `std::nullptr_t` type as follows: * ``` - * typedef decltype(nullptr) nullptr_t; + * typedef decltype(nullptr) nullptr_t; * ``` - * Instead, this is the unspeakable type given by `decltype(nullptr)`. */ class NullPointerType extends BuiltInType { NullPointerType() { builtintypes(underlyingElement(this), _, 34, _, _, _) } @@ -729,8 +875,13 @@ class NullPointerType extends BuiltInType { /** * A C/C++ derived type. * - * These are pointer and reference types, array and vector types, and const and volatile types. - * In all cases, the type is formed from a single base type. + * These are pointer and reference types, array and GNU vector types, and `const` and `volatile` types. + * In all cases, the type is formed from a single base type. For example: + * ``` + * int *pi; + * int &ri = *pi; + * const float fa[40]; + * ``` */ class DerivedType extends Type, @derivedtype { override string toString() { result = this.getName() } @@ -775,9 +926,15 @@ class DerivedType extends Type, @derivedtype { } /** - * An instance of the C++11 decltype operator. + * An instance of the C++11 `decltype` operator. For example: + * ``` + * int a; + * decltype(a) b; + * ``` */ class Decltype extends Type, @decltype { + override string getCanonicalQLClass() { result = "Decltype" } + /** * The expression whose type is being obtained by this decltype. */ @@ -788,17 +945,17 @@ class Decltype extends Type, @decltype { */ Type getBaseType() { decltypes(underlyingElement(this), _, unresolveElement(result), _) } - override string getCanonicalQLClass() { result = "Decltype" } - /** * Whether an extra pair of parentheses around the expression would change the semantics of this decltype. * * The following example shows the effect of an extra pair of parentheses: - * struct A { double x; }; - * const A* a = new A(); - * decltype( a->x ); // type is double - * decltype((a->x)); // type is const double& - * Consult the C++11 standard for more details. + * ``` + * struct A { double x; }; + * const A* a = new A(); + * decltype( a->x ); // type is double + * decltype((a->x)); // type is const double& + * ``` + * Please consult the C++11 standard for more details. */ predicate parenthesesWouldChangeMeaning() { decltypes(underlyingElement(this), _, _, true) } @@ -841,6 +998,10 @@ class Decltype extends Type, @decltype { /** * A C/C++ pointer type. See 4.9.1. + * ``` + * void *ptr; + * void **ptr2 = &ptr; + * ``` */ class PointerType extends DerivedType { PointerType() { derivedtypes(underlyingElement(this), _, 1, _) } @@ -863,8 +1024,8 @@ class PointerType extends DerivedType { /** * A C++ reference type. See 4.9.1. * - * For C++11 code bases, this includes both lvalue references (&) and rvalue references (&&). - * To distinguish between them, use the LValueReferenceType and RValueReferenceType classes. + * For C++11 code bases, this includes both _lvalue_ references (`&`) and _rvalue_ references (`&&`). + * To distinguish between them, use the LValueReferenceType and RValueReferenceType QL classes. */ class ReferenceType extends DerivedType { ReferenceType() { @@ -889,7 +1050,11 @@ class ReferenceType extends DerivedType { } /** - * A C++11 lvalue reference type (e.g. int&). + * A C++11 lvalue reference type (e.g. `int &`). + * ``` + * int a; + * int& b = a; + * ``` */ class LValueReferenceType extends ReferenceType { LValueReferenceType() { derivedtypes(underlyingElement(this), _, 2, _) } @@ -898,7 +1063,14 @@ class LValueReferenceType extends ReferenceType { } /** - * A C++11 rvalue reference type (e.g. int&&). + * A C++11 rvalue reference type (e.g., `int &&`). It is used to + * implement "move" semantics for object construction and assignment. + * ``` + * class C { + * E e; + * C(C&& from): e(std::move(from.e)) { } + * }; + * ``` */ class RValueReferenceType extends ReferenceType { RValueReferenceType() { derivedtypes(underlyingElement(this), _, 8, _) } @@ -910,6 +1082,10 @@ class RValueReferenceType extends ReferenceType { /** * A type with specifiers. + * ``` + * const int a; + * volatile char v; + * ``` */ class SpecifiedType extends DerivedType { SpecifiedType() { derivedtypes(underlyingElement(this), _, 3, _) } @@ -955,6 +1131,9 @@ class SpecifiedType extends DerivedType { /** * A C/C++ array type. See 4.9.1. + * ``` + * char table[32]; + * ``` */ class ArrayType extends DerivedType { ArrayType() { derivedtypes(underlyingElement(this), _, 4, _) } @@ -1001,10 +1180,16 @@ class ArrayType extends DerivedType { * A GNU/Clang vector type. * * In both Clang and GNU compilers, vector types can be introduced using the - * __attribute__((vector_size(byte_size))) syntax. The Clang compiler also - * allows vector types to be introduced using the ext_vector_type, - * neon_vector_type, and neon_polyvector_type attributes (all of which take - * an element type rather than a byte size). + * `__attribute__((vector_size(byte_size)))` syntax. The Clang compiler also + * allows vector types to be introduced using the `ext_vector_type`, + * `neon_vector_type`, and `neon_polyvector_type` attributes (all of which take + * an element count rather than a byte size). + * + * In the example below, both `v4si` and `float4` are GNU vector types: + * ``` + * typedef int v4si __attribute__ (( vector_size(4*sizeof(int)) )); + * typedef float float4 __attribute__((ext_vector_type(4))); + * ``` */ class GNUVectorType extends DerivedType { GNUVectorType() { derivedtypes(underlyingElement(this), _, 5, _) } @@ -1043,7 +1228,10 @@ class GNUVectorType extends DerivedType { } /** - * A C/C++ pointer to function. See 7.7. + * A C/C++ pointer to a function. See 7.7. + * ``` + * int(* pointer)(const void *element1, const void *element2); + * ``` */ class FunctionPointerType extends FunctionPointerIshType { FunctionPointerType() { derivedtypes(underlyingElement(this), _, 6, _) } @@ -1058,7 +1246,10 @@ class FunctionPointerType extends FunctionPointerIshType { } /** - * A C/C++ reference to function. + * A C++ reference to a function. + * ``` + * int(& reference)(const void *element1, const void *element2); + * ``` */ class FunctionReferenceType extends FunctionPointerIshType { FunctionReferenceType() { derivedtypes(underlyingElement(this), _, 7, _) } @@ -1073,10 +1264,14 @@ class FunctionReferenceType extends FunctionPointerIshType { } /** - * A block type, for example int(^)(char, float). + * A block type, for example, `int(^)(char, float)`. * * Block types (along with blocks themselves) are a language extension * supported by Clang, and by Apple's branch of GCC. + * ``` + * int(^ block)(const char *element1, const char *element2) + * = ^int (const char *element1, const char *element2) { return element1 - element 2; } + * ``` */ class BlockType extends FunctionPointerIshType { BlockType() { derivedtypes(underlyingElement(this), _, 10, _) } @@ -1089,7 +1284,9 @@ class BlockType extends FunctionPointerIshType { } /** - * A C/C++ pointer to function, or a block. + * A C/C++ pointer to a function, a C++ function reference, or a clang/Apple block. + * + * See `FunctionPointerType`, `FunctionReferenceType` and `BlockType` for more information. */ class FunctionPointerIshType extends DerivedType { FunctionPointerIshType() { @@ -1134,7 +1331,13 @@ class FunctionPointerIshType extends DerivedType { } /** - * A C++ pointer to member. See 15.5. + * A C++ pointer to data member. See 15.5. + * ``` + * class C { int m; }; + * int C::* p = &C::m; // pointer to data member m of class C + * class C *; + * int val = c.*p; // access data member + * ``` */ class PointerToMemberType extends Type, @ptrtomember { /** a printable representation of this named element */ @@ -1171,7 +1374,14 @@ class PointerToMemberType extends Type, @ptrtomember { } /** - * A C/C++ routine type. This is what results from stripping away the pointer from a function pointer type. + * A C/C++ routine type. Conceptually, this is what results from stripping + * away the pointer from a function pointer type. It can also occur in C++ + * code, for example the base type of `myRoutineType` in the following code: + * ``` + * using myRoutineType = int(int); + * + * myRoutineType *fp = 0; + * ``` */ class RoutineType extends Type, @routinetype { /** a printable representation of this named element */ @@ -1231,7 +1441,13 @@ class RoutineType extends Type, @routinetype { } /** - * A C++ typename template parameter. + * A C++ `typename` (or `class`) template parameter. + * + * In the example below, `T` is a template parameter: + * ``` + * template + * class C { }; + * ``` */ class TemplateParameter extends UserType { TemplateParameter() { @@ -1243,7 +1459,16 @@ class TemplateParameter extends UserType { override predicate involvesTemplateParameter() { any() } } -/** A C++ template template parameter, e.g. template <template <typename,typename> class T>. */ +/** + * A C++ template template parameter. + * + * In the example below, `T` is a template template parameter (although its name + * may be omitted): + * ``` + * template