From 8f15b0b6c1c8595f5eb94067bd029baba9080e01 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Wed, 15 May 2024 22:54:34 +0100 Subject: [PATCH 001/281] Swift: Remove beta label on documentation. --- .../codeql-language-guides/analyzing-data-flow-in-swift.rst | 2 -- .../codeql-language-guides/basic-query-for-swift-code.rst | 1 - docs/codeql/codeql-language-guides/codeql-for-swift.rst | 2 -- docs/codeql/query-help/codeql-cwe-coverage.rst | 1 - docs/codeql/query-help/index.rst | 1 - docs/codeql/reusables/supported-frameworks.rst | 2 -- docs/codeql/reusables/swift-beta-note.rst | 4 ---- 7 files changed, 13 deletions(-) delete mode 100644 docs/codeql/reusables/swift-beta-note.rst diff --git a/docs/codeql/codeql-language-guides/analyzing-data-flow-in-swift.rst b/docs/codeql/codeql-language-guides/analyzing-data-flow-in-swift.rst index 9de7d620abf4..61aa63e00be7 100644 --- a/docs/codeql/codeql-language-guides/analyzing-data-flow-in-swift.rst +++ b/docs/codeql/codeql-language-guides/analyzing-data-flow-in-swift.rst @@ -5,8 +5,6 @@ Analyzing data flow in Swift You can use CodeQL to track the flow of data through a Swift program to places where the data is used. -.. include:: ../reusables/swift-beta-note.rst - About this article ------------------ diff --git a/docs/codeql/codeql-language-guides/basic-query-for-swift-code.rst b/docs/codeql/codeql-language-guides/basic-query-for-swift-code.rst index fdaa1ec62908..9e146513a20c 100644 --- a/docs/codeql/codeql-language-guides/basic-query-for-swift-code.rst +++ b/docs/codeql/codeql-language-guides/basic-query-for-swift-code.rst @@ -5,7 +5,6 @@ Basic query for Swift code Learn to write and run a simple CodeQL query using Visual Studio Code with the CodeQL extension. -.. include:: ../reusables/swift-beta-note.rst .. include:: ../reusables/vs-code-basic-instructions/setup-to-run-queries.rst About the query diff --git a/docs/codeql/codeql-language-guides/codeql-for-swift.rst b/docs/codeql/codeql-language-guides/codeql-for-swift.rst index 132ab004d6f0..5d05739829f8 100644 --- a/docs/codeql/codeql-language-guides/codeql-for-swift.rst +++ b/docs/codeql/codeql-language-guides/codeql-for-swift.rst @@ -5,8 +5,6 @@ CodeQL for Swift Experiment and learn how to write effective and efficient queries for CodeQL databases generated from Swift codebases. -.. include:: ../reusables/swift-beta-note.rst - .. toctree:: :hidden: diff --git a/docs/codeql/query-help/codeql-cwe-coverage.rst b/docs/codeql/query-help/codeql-cwe-coverage.rst index 54219ea8f3b3..31ce5b7ec965 100644 --- a/docs/codeql/query-help/codeql-cwe-coverage.rst +++ b/docs/codeql/query-help/codeql-cwe-coverage.rst @@ -4,7 +4,6 @@ CodeQL CWE coverage You can view the full coverage of MITRE's Common Weakness Enumeration (CWE) or coverage by language for the latest release of CodeQL. .. include:: ../reusables/kotlin-beta-note.rst -.. include:: ../reusables/swift-beta-note.rst About CWEs ########## diff --git a/docs/codeql/query-help/index.rst b/docs/codeql/query-help/index.rst index 05d53a2cddf9..e15aee39b2ce 100644 --- a/docs/codeql/query-help/index.rst +++ b/docs/codeql/query-help/index.rst @@ -13,7 +13,6 @@ View the query help for the queries included in the ``default``, ``security-exte - :doc:`CodeQL query help for Swift ` .. include:: ../reusables/kotlin-beta-note.rst -.. include:: ../reusables/swift-beta-note.rst .. pull-quote:: Information diff --git a/docs/codeql/reusables/supported-frameworks.rst b/docs/codeql/reusables/supported-frameworks.rst index 5ab3b6058343..2e858578b002 100644 --- a/docs/codeql/reusables/supported-frameworks.rst +++ b/docs/codeql/reusables/supported-frameworks.rst @@ -286,8 +286,6 @@ and the CodeQL library pack ``codeql/ruby-all`` (`changelog `__, `source `__) and the CodeQL library pack ``codeql/swift-all`` (`changelog `__, `source `__). diff --git a/docs/codeql/reusables/swift-beta-note.rst b/docs/codeql/reusables/swift-beta-note.rst deleted file mode 100644 index 273366833409..000000000000 --- a/docs/codeql/reusables/swift-beta-note.rst +++ /dev/null @@ -1,4 +0,0 @@ - .. pull-quote:: Note - - CodeQL analysis for Swift is currently in beta. During the beta, analysis of Swift code, - and the accompanying documentation, will not be as comprehensive as for other languages. \ No newline at end of file From 533c5218dd04cec872fc28f20504e01ca0d67abc Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Wed, 15 May 2024 22:56:50 +0100 Subject: [PATCH 002/281] Swift: Remove more beta references. --- docs/codeql/reusables/supported-versions-compilers.rst | 2 +- swift/README.md | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/docs/codeql/reusables/supported-versions-compilers.rst b/docs/codeql/reusables/supported-versions-compilers.rst index 6f47503d102a..30c8985079d4 100644 --- a/docs/codeql/reusables/supported-versions-compilers.rst +++ b/docs/codeql/reusables/supported-versions-compilers.rst @@ -39,5 +39,5 @@ .. [8] JSX and Flow code, YAML, JSON, HTML, and XML files may also be analyzed with JavaScript files. .. [9] The extractor requires Python 3 to run. To analyze Python 2.7 you should install both versions of Python. .. [10] Requires glibc 2.17. - .. [11] Swift support is currently in beta. Support for the analysis of Swift requires macOS or Linux. + .. [11] Support for the analysis of Swift requires macOS or Linux. .. [12] TypeScript analysis is performed by running the JavaScript extractor with TypeScript enabled. This is the default. diff --git a/swift/README.md b/swift/README.md index a2ac9fca380e..3baab86c51db 100644 --- a/swift/README.md +++ b/swift/README.md @@ -1,8 +1,5 @@ # Swift on CodeQL -> [!NOTE] -> CodeQL analysis for Swift is currently in beta. During the beta, analysis of Swift code, and the accompanying documentation, will not be as comprehensive as for other languages. - ## Development ### Building the Swift extractor From 3a885eaf9f64b190e89d420f4443e23ccb115951 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Mon, 20 May 2024 11:58:55 +0100 Subject: [PATCH 003/281] Insecure Helmet middle configuration - frameguard or CSP to 'false' --- .../src/Security/CWE-693/InsecureHelmet.qhelp | 71 +++++++++++++++++++ .../ql/src/Security/CWE-693/InsecureHelmet.ql | 36 ++++++++++ 2 files changed, 107 insertions(+) create mode 100644 javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp create mode 100644 javascript/ql/src/Security/CWE-693/InsecureHelmet.ql diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp new file mode 100644 index 000000000000..f2b4deeefc1e --- /dev/null +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp @@ -0,0 +1,71 @@ + + + +

+ Helmet is a collection of middleware functions for securing Express apps. It sets various HTTP headers to guard against common web vulnerabilities. + + This query detects Helmet misconfigurations that can lead to security vulnerabilities, specifically: + +

    +
  • Disabling frame protection
    • +
  • Disabling Content Security Policy
    • +
+ + Content Security Policy (CSP) helps spot and prevent injection attacks such as Cross-Site Scripting (XSS). + + Removing frame protections exposes an application to attacks such as clickjacking, where an attacker can trick a user into clicking on a button or link on a targeted page when they intended to click on the page carrying out the attack. +

+ + +

+ To help mitigate these vulnerabilities, ensure that the following Helmet functions are not disabled, and are configured appropriately to your application: +

    +
  • frameguard
    • +
  • contentSecurityPolicy
    • +
+

+ + +

+ The following code snippet demonstrates Helmet configured in an insecure manner: + + const helmet = require('helmet'); + app.use(helmet({ + frameguard: false, + contentSecurityPolicy: false + })); + +

+

+ In this example, the defaults are used, which enables frame protection and a default Content Security Policy. + + + app.use(helmet()); + + + You can also enable a custom Content Security Policy by passing an object to the contentSecurityPolicy key. For example, taken from the Helmet docs: + + + app.use( + helmet({ + contentSecurityPolicy: { + directives: { + "script-src": ["'self'", "example.com"], + "style-src": null, + }, + }, + }) + ); + +

+ +

+ + + + + \ No newline at end of file diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql new file mode 100644 index 000000000000..f059b37e783d --- /dev/null +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -0,0 +1,36 @@ +/** + * @name Insecure configuration of Helmet security middleware + * @description The Helmet middleware is used to set security-related HTTP headers in Express applications. This query finds instances where the middleware is configured with important security features disabled. + * @kind problem + * @problem.severity error + * @security-severity 5.0 + * @precision high + * @id javascript/insecure-helmet-configuration + * @tags security + * cwe-693 + * cwe-1021 + */ + +import semmle.javascript.frameworks.ExpressModules + +class HelmetProperty extends Property { + HelmetProperty() { + exists(ExpressLibraries::HelmetRouteHandler helmet | + helmet.(DataFlow::CallNode).getAnArgument().asExpr().(ObjectExpr).getAProperty() = this + ) + } + + predicate isFalse() { this.getInit().(BooleanLiteral).getBoolValue() = false } + + predicate isImportantSecuritySetting() { + this.getName() in ["frameguard", "contentSecurityPolicy"] + // read from data extensions to allow enforcing other settings + // TODO + } +} + +from HelmetProperty helmetSetting +where + helmetSetting.isFalse() and + helmetSetting.isImportantSecuritySetting() +select helmetSetting, "Helmet route handler, called with $@ set to 'false'", helmetSetting, helmetSetting.getName() From 8300aeb0a0a9189f717ac2bdee4ae1374c3b4ef0 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Mon, 20 May 2024 12:05:42 +0100 Subject: [PATCH 004/281] Tests for InsecureHelmet --- .../Security/CWE-693/InsecureHelment.expected | 2 ++ .../Security/CWE-693/InsecureHelment.qlref | 1 + .../Security/CWE-693/InsecureHelmetBad.js | 17 +++++++++++++++++ .../Security/CWE-693/InsecureHelmetGood.js | 14 ++++++++++++++ 4 files changed, 34 insertions(+) create mode 100644 javascript/ql/test/query-tests/Security/CWE-693/InsecureHelment.expected create mode 100644 javascript/ql/test/query-tests/Security/CWE-693/InsecureHelment.qlref create mode 100644 javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmetBad.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmetGood.js diff --git a/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelment.expected b/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelment.expected new file mode 100644 index 000000000000..7368d96f3d43 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelment.expected @@ -0,0 +1,2 @@ +| InsecureHelmetBad.js:7:5:7:32 | content ... : false | Helmet route handler, called with $@ set to 'false' | InsecureHelmetBad.js:7:5:7:32 | content ... : false | contentSecurityPolicy | +| InsecureHelmetBad.js:8:5:8:21 | frameguard: false | Helmet route handler, called with $@ set to 'false' | InsecureHelmetBad.js:8:5:8:21 | frameguard: false | frameguard | diff --git a/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelment.qlref b/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelment.qlref new file mode 100644 index 000000000000..9212b2674fcf --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelment.qlref @@ -0,0 +1 @@ +Security/CWE-693/InsecureHelmet.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmetBad.js b/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmetBad.js new file mode 100644 index 000000000000..d9257999ef0b --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmetBad.js @@ -0,0 +1,17 @@ +const express = require("express"); +const helmet = require("helmet"); + +const app = express(); + +app.use(helmet({ + contentSecurityPolicy: false, // BAD: switch off default CSP + frameguard: false // BAD: switch off default frameguard +})); + +app.get("/", (req, res) => { + res.send("Hello, world!"); +}); + +app.listen(3000, () => { + console.log("App is listening on port 3000"); +}); diff --git a/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmetGood.js b/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmetGood.js new file mode 100644 index 000000000000..609c1fc2763c --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmetGood.js @@ -0,0 +1,14 @@ +const express = require("express"); +const helmet = require("helmet"); + +const app = express(); + +app.use(helmet()); // GOOD: use the defaults + +app.get("/", (req, res) => { + res.send("Hello, world!"); +}); + +app.listen(3000, () => { + console.log("App is listening on port 3000"); +}); From 83037b11951d65294f07934ad509fadfd8874456 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Tue, 21 May 2024 13:51:13 +0100 Subject: [PATCH 005/281] Adjust structure to avoid warnings about message --- .../ql/src/Security/CWE-693/InsecureHelmet.ql | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql index f059b37e783d..ca27b717f186 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -14,12 +14,14 @@ import semmle.javascript.frameworks.ExpressModules class HelmetProperty extends Property { + ExpressLibraries::HelmetRouteHandler helmet; + HelmetProperty() { - exists(ExpressLibraries::HelmetRouteHandler helmet | - helmet.(DataFlow::CallNode).getAnArgument().asExpr().(ObjectExpr).getAProperty() = this - ) + helmet.(DataFlow::CallNode).getAnArgument().asExpr().(ObjectExpr).getAProperty() = this } + ExpressLibraries::HelmetRouteHandler getHelmet() { result = helmet } + predicate isFalse() { this.getInit().(BooleanLiteral).getBoolValue() = false } predicate isImportantSecuritySetting() { @@ -29,8 +31,10 @@ class HelmetProperty extends Property { } } -from HelmetProperty helmetSetting +from HelmetProperty helmetSetting, ExpressLibraries::HelmetRouteHandler helmet where helmetSetting.isFalse() and - helmetSetting.isImportantSecuritySetting() -select helmetSetting, "Helmet route handler, called with $@ set to 'false'", helmetSetting, helmetSetting.getName() + helmetSetting.isImportantSecuritySetting() and + helmetSetting.getHelmet() = helmet +select helmet, "Helmet route handler, called with $@ set to 'false'.", helmetSetting, + helmetSetting.getName() From bda794fde762e8e915b0baa6e4887e149c99fe8e Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Tue, 21 May 2024 14:34:58 +0100 Subject: [PATCH 006/281] Fixed wrong filenames in the InsecureHelmet tests --- .../CWE-693/{InsecureHelment.expected => InsecureHelmet.expected} | 0 .../CWE-693/{InsecureHelment.qlref => InsecureHelmet.qlref} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename javascript/ql/test/query-tests/Security/CWE-693/{InsecureHelment.expected => InsecureHelmet.expected} (100%) rename javascript/ql/test/query-tests/Security/CWE-693/{InsecureHelment.qlref => InsecureHelmet.qlref} (100%) diff --git a/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelment.expected b/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmet.expected similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-693/InsecureHelment.expected rename to javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmet.expected diff --git a/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelment.qlref b/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmet.qlref similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-693/InsecureHelment.qlref rename to javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmet.qlref From 68e21a594aad539c936ed19bdbecdafc9cc84bdb Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Tue, 21 May 2024 14:35:18 +0100 Subject: [PATCH 007/281] Fixed query help formatting issues --- .../src/Security/CWE-693/InsecureHelmet.qhelp | 80 ++++++++++--------- 1 file changed, 42 insertions(+), 38 deletions(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp index f2b4deeefc1e..b54047ba4ae4 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp @@ -5,12 +5,14 @@ Helmet is a collection of middleware functions for securing Express apps. It sets various HTTP headers to guard against common web vulnerabilities. This query detects Helmet misconfigurations that can lead to security vulnerabilities, specifically: +

-
    -
  • Disabling frame protection
    • -
  • Disabling Content Security Policy
    • -
+
    +
  • Disabling frame protection
    • +
  • Disabling Content Security Policy
    • +
+

Content Security Policy (CSP) helps spot and prevent injection attacks such as Cross-Site Scripting (XSS). Removing frame protections exposes an application to attacks such as clickjacking, where an attacker can trick a user into clicking on a button or link on a targeted page when they intended to click on the page carrying out the attack. @@ -19,53 +21,55 @@

To help mitigate these vulnerabilities, ensure that the following Helmet functions are not disabled, and are configured appropriately to your application: -

    -
  • frameguard
    • -
  • contentSecurityPolicy
    • -

+ +
    +
  • frameguard
    • +
  • contentSecurityPolicy
    • +

The following code snippet demonstrates Helmet configured in an insecure manner: - - const helmet = require('helmet'); - app.use(helmet({ - frameguard: false, - contentSecurityPolicy: false - })); -

+ + 
+            const helmet = require('helmet');
+            app.use(helmet({
+                frameguard: false,
+                contentSecurityPolicy: false
+            }));
+
+

In this example, the defaults are used, which enables frame protection and a default Content Security Policy. +

- - app.use(helmet()); - - - You can also enable a custom Content Security Policy by passing an object to the contentSecurityPolicy key. For example, taken from the Helmet docs: + 
+            app.use(helmet());
+
- - app.use( - helmet({ - contentSecurityPolicy: { - directives: { - "script-src": ["'self'", "example.com"], - "style-src": null, - }, - }, - }) - ); -

- + You can also enable a custom Content Security Policy by passing an object to the contentSecurityPolicy key. For example, taken from the Helmet docs:

+ + 
+            app.use(
+                helmet({
+                    contentSecurityPolicy: {
+                        directives: {
+                            "script-src": ["'self'", "example.com"],
+                            "style-src": null,
+                        },
+                    },
+                })
+            );
+
+ - +
  • + helmet.js website +
    • \ No newline at end of file From 0d4cd3e103d6613b26e3e4eea9927123d75cff8c Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 23 May 2024 10:03:27 +0100 Subject: [PATCH 008/281] Swift: Add more sensitive data test cases. --- .../CWE-311/CleartextStorageDatabase.expected | 16 ++++++++++++++++ .../Security/CWE-311/SensitiveExprs.expected | 2 ++ .../Security/CWE-311/testRealm2.swift | 18 +++++++++++++++++- 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/swift/ql/test/query-tests/Security/CWE-311/CleartextStorageDatabase.expected b/swift/ql/test/query-tests/Security/CWE-311/CleartextStorageDatabase.expected index 051f846c5b1d..e609a3211549 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/CleartextStorageDatabase.expected +++ b/swift/ql/test/query-tests/Security/CWE-311/CleartextStorageDatabase.expected @@ -297,6 +297,12 @@ edges | testRealm2.swift:18:2:18:2 | [post] o [data] | testRealm2.swift:18:2:18:2 | [post] o | provenance | | | testRealm2.swift:18:11:18:11 | myPassword | testRealm2.swift:13:6:13:6 | value | provenance | | | testRealm2.swift:18:11:18:11 | myPassword | testRealm2.swift:18:2:18:2 | [post] o [data] | provenance | | +| testRealm2.swift:24:2:24:2 | [post] o [data] | testRealm2.swift:24:2:24:2 | [post] o | provenance | | +| testRealm2.swift:24:11:24:11 | socialSecurityNumber | testRealm2.swift:13:6:13:6 | value | provenance | | +| testRealm2.swift:24:11:24:11 | socialSecurityNumber | testRealm2.swift:24:2:24:2 | [post] o [data] | provenance | | +| testRealm2.swift:32:2:32:2 | [post] o [data] | testRealm2.swift:32:2:32:2 | [post] o | provenance | | +| testRealm2.swift:32:11:32:11 | creditCardNumber | testRealm2.swift:13:6:13:6 | value | provenance | | +| testRealm2.swift:32:11:32:11 | creditCardNumber | testRealm2.swift:32:2:32:2 | [post] o [data] | provenance | | | testRealm.swift:27:6:27:6 | value | file://:0:0:0:0 | value | provenance | | | testRealm.swift:34:6:34:6 | value | file://:0:0:0:0 | value | provenance | | | testRealm.swift:41:2:41:2 | [post] a [data] | testRealm.swift:41:2:41:2 | [post] a | provenance | | @@ -721,6 +727,12 @@ nodes | testRealm2.swift:18:2:18:2 | [post] o | semmle.label | [post] o | | testRealm2.swift:18:2:18:2 | [post] o [data] | semmle.label | [post] o [data] | | testRealm2.swift:18:11:18:11 | myPassword | semmle.label | myPassword | +| testRealm2.swift:24:2:24:2 | [post] o | semmle.label | [post] o | +| testRealm2.swift:24:2:24:2 | [post] o [data] | semmle.label | [post] o [data] | +| testRealm2.swift:24:11:24:11 | socialSecurityNumber | semmle.label | socialSecurityNumber | +| testRealm2.swift:32:2:32:2 | [post] o | semmle.label | [post] o | +| testRealm2.swift:32:2:32:2 | [post] o [data] | semmle.label | [post] o [data] | +| testRealm2.swift:32:11:32:11 | creditCardNumber | semmle.label | creditCardNumber | | testRealm.swift:27:6:27:6 | self [Return] [data] | semmle.label | self [Return] [data] | | testRealm.swift:27:6:27:6 | value | semmle.label | value | | testRealm.swift:34:6:34:6 | self [Return] [password] | semmle.label | self [Return] [password] | @@ -756,6 +768,8 @@ subpaths | testCoreData2.swift:104:18:104:18 | e | testCoreData2.swift:70:9:70:9 | self | file://:0:0:0:0 | .value | testCoreData2.swift:104:18:104:20 | .value | | testCoreData2.swift:105:18:105:18 | e | testCoreData2.swift:71:9:71:9 | self | file://:0:0:0:0 | .value2 | testCoreData2.swift:105:18:105:20 | .value2 | | testRealm2.swift:18:11:18:11 | myPassword | testRealm2.swift:13:6:13:6 | value | testRealm2.swift:13:6:13:6 | self [Return] [data] | testRealm2.swift:18:2:18:2 | [post] o [data] | +| testRealm2.swift:24:11:24:11 | socialSecurityNumber | testRealm2.swift:13:6:13:6 | value | testRealm2.swift:13:6:13:6 | self [Return] [data] | testRealm2.swift:24:2:24:2 | [post] o [data] | +| testRealm2.swift:32:11:32:11 | creditCardNumber | testRealm2.swift:13:6:13:6 | value | testRealm2.swift:13:6:13:6 | self [Return] [data] | testRealm2.swift:32:2:32:2 | [post] o [data] | | testRealm.swift:41:11:41:11 | myPassword | testRealm.swift:27:6:27:6 | value | testRealm.swift:27:6:27:6 | self [Return] [data] | testRealm.swift:41:2:41:2 | [post] a [data] | | testRealm.swift:49:11:49:11 | myPassword | testRealm.swift:27:6:27:6 | value | testRealm.swift:27:6:27:6 | self [Return] [data] | testRealm.swift:49:2:49:2 | [post] c [data] | | testRealm.swift:59:12:59:12 | myPassword | testRealm.swift:27:6:27:6 | value | testRealm.swift:27:6:27:6 | self [Return] [data] | testRealm.swift:59:2:59:3 | [post] ...! [data] | @@ -890,6 +904,8 @@ subpaths | testGRDB.swift:210:84:210:93 | [...] | testGRDB.swift:210:85:210:85 | password | testGRDB.swift:210:84:210:93 | [...] | This operation stores '[...]' in a database. It may contain unencrypted sensitive data from $@. | testGRDB.swift:210:85:210:85 | password | password | | testGRDB.swift:212:98:212:107 | [...] | testGRDB.swift:212:99:212:99 | password | testGRDB.swift:212:98:212:107 | [...] | This operation stores '[...]' in a database. It may contain unencrypted sensitive data from $@. | testGRDB.swift:212:99:212:99 | password | password | | testRealm2.swift:18:2:18:2 | o | testRealm2.swift:18:11:18:11 | myPassword | testRealm2.swift:18:2:18:2 | [post] o | This operation stores 'o' in a database. It may contain unencrypted sensitive data from $@. | testRealm2.swift:18:11:18:11 | myPassword | myPassword | +| testRealm2.swift:24:2:24:2 | o | testRealm2.swift:24:11:24:11 | socialSecurityNumber | testRealm2.swift:24:2:24:2 | [post] o | This operation stores 'o' in a database. It may contain unencrypted sensitive data from $@. | testRealm2.swift:24:11:24:11 | socialSecurityNumber | socialSecurityNumber | +| testRealm2.swift:32:2:32:2 | o | testRealm2.swift:32:11:32:11 | creditCardNumber | testRealm2.swift:32:2:32:2 | [post] o | This operation stores 'o' in a database. It may contain unencrypted sensitive data from $@. | testRealm2.swift:32:11:32:11 | creditCardNumber | creditCardNumber | | testRealm.swift:41:2:41:2 | a | testRealm.swift:41:11:41:11 | myPassword | testRealm.swift:41:2:41:2 | [post] a | This operation stores 'a' in a database. It may contain unencrypted sensitive data from $@. | testRealm.swift:41:11:41:11 | myPassword | myPassword | | testRealm.swift:49:2:49:2 | c | testRealm.swift:49:11:49:11 | myPassword | testRealm.swift:49:2:49:2 | [post] c | This operation stores 'c' in a database. It may contain unencrypted sensitive data from $@. | testRealm.swift:49:11:49:11 | myPassword | myPassword | | testRealm.swift:59:2:59:3 | ...! | testRealm.swift:59:12:59:12 | myPassword | testRealm.swift:59:2:59:3 | [post] ...! | This operation stores '...!' in a database. It may contain unencrypted sensitive data from $@. | testRealm.swift:59:12:59:12 | myPassword | myPassword | diff --git a/swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected b/swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected index 57e4267d9e43..b5f6abb01a40 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected +++ b/swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected @@ -141,6 +141,8 @@ | testGRDB.swift:210:85:210:85 | password | label:password, type:password | | testGRDB.swift:212:99:212:99 | password | label:password, type:password | | testRealm2.swift:18:11:18:11 | myPassword | label:myPassword, type:password | +| testRealm2.swift:24:11:24:11 | socialSecurityNumber | label:socialSecurityNumber, type:private information | +| testRealm2.swift:32:11:32:11 | creditCardNumber | label:creditCardNumber, type:private information | | testRealm.swift:31:20:31:20 | .password | label:password, type:password | | testRealm.swift:41:11:41:11 | myPassword | label:myPassword, type:password | | testRealm.swift:49:11:49:11 | myPassword | label:myPassword, type:password | diff --git a/swift/ql/test/query-tests/Security/CWE-311/testRealm2.swift b/swift/ql/test/query-tests/Security/CWE-311/testRealm2.swift index 2c1850ee6209..8fcfcb002114 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/testRealm2.swift +++ b/swift/ql/test/query-tests/Security/CWE-311/testRealm2.swift @@ -13,9 +13,25 @@ class MyRealmSwiftObject3 : Object { var data: String } -func test1(o: MyRealmSwiftObject3, myHarmless: String, myPassword : String) { +func test1(o: MyRealmSwiftObject3, myHarmless: String, myPassword: String) { // ... o.data = myPassword // BAD o.data = myHarmless // ... } + +func test2(o: MyRealmSwiftObject3, ccn: String, socialSecurityNumber: String, ssn: String, ssn_int: Int, userSSN: String, classno: String) { + o.data = socialSecurityNumber // BAD + o.data = ssn // BAD [NOT DETECTED] + o.data = String(ssn_int) // BAD [NOT DETECTED] + o.data = userSSN // BAD [NOT DETECTED] + o.data = classno // GOOD +} + +func test3(o: MyRealmSwiftObject3, ccn: String, creditCardNumber: String, CCN: String, int_ccn: Int, userCcn: String, succnode: String) { + o.data = creditCardNumber // BAD + o.data = CCN // BAD [NOT DETECTED] + o.data = String(int_ccn) // BAD [NOT DETECTED] + o.data = userCcn // BAD [NOT DETECTED] + o.data = succnode // GOOD +} From 06dea2d27ff3884109b85b8e7293db65e20c6a45 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 9 May 2024 18:09:55 +0100 Subject: [PATCH 009/281] Swift: Use sensitive private info regex from the shared library, now that it has that. --- .../codeql/swift/security/SensitiveExprs.qll | 30 +--------- .../CWE-311/CleartextStorageDatabase.expected | 57 +++++++++++++++++++ .../Security/CWE-311/SensitiveExprs.expected | 4 ++ .../Security/CWE-311/testRealm2.swift | 8 +-- 4 files changed, 68 insertions(+), 31 deletions(-) diff --git a/swift/ql/lib/codeql/swift/security/SensitiveExprs.qll b/swift/ql/lib/codeql/swift/security/SensitiveExprs.qll index 0c712b4fbfdf..a00d73c0a9f5 100644 --- a/swift/ql/lib/codeql/swift/security/SensitiveExprs.qll +++ b/swift/ql/lib/codeql/swift/security/SensitiveExprs.qll @@ -64,33 +64,9 @@ class SensitivePrivateInfo extends SensitiveDataType, TPrivateInfo { override string toString() { result = "private information" } override string getRegexp() { - result = - "(?is).*(" + - // Inspired by the list on https://cwe.mitre.org/data/definitions/359.html - // Government identifiers, such as Social Security Numbers - "social.?security|employer.?identification|national.?insurance|resident.?id|" + - "passport.?(num|no)|" + - // Contact information, such as home addresses - "post.?code|zip.?code|home.?addr|" + - // and telephone numbers - "(mob(ile)?|home).?(num|no|tel|phone)|(tel|fax|phone).?(num|no)|telephone|" + - "emergency.?contact|" + - // Geographic location - where the user is (or was) - "l(atitude|ongitude)|nationality|" + - // Financial data - such as credit card numbers, salary, bank accounts, and debts - "(credit|debit|bank|visa).?(card|num|no|acc(ou?)nt)|acc(ou)?nt.?(no|num|credit)|" + - "salary|billing|credit.?(rating|score)|" + - // Communications - e-mail addresses, private e-mail messages, SMS text messages, chat logs, etc. - "e(mail|_mail)|" + - // Health - medical conditions, insurance status, prescription records - "birth.?da(te|y)|da(te|y).?(of.?)?birth|" + - "medical|(health|care).?plan|healthkit|appointment|prescription|" + - "blood.?(type|alcohol|glucose|pressure)|heart.?(rate|rhythm)|body.?(mass|fat)|" + - "menstrua|pregnan|insulin|inhaler|" + - // Relationships - work and family - "employ(er|ee)|spouse|maiden.?name" + - // --- - ").*" + result = HeuristicNames::maybeSensitiveRegexp(SensitiveDataClassification::private()) + or + result = "(?is).*e(mail|_mail).*" } } diff --git a/swift/ql/test/query-tests/Security/CWE-311/CleartextStorageDatabase.expected b/swift/ql/test/query-tests/Security/CWE-311/CleartextStorageDatabase.expected index e609a3211549..83fb7d9c1fe8 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/CleartextStorageDatabase.expected +++ b/swift/ql/test/query-tests/Security/CWE-311/CleartextStorageDatabase.expected @@ -69,6 +69,7 @@ edges | SQLite.swift:197:17:197:49 | [...] [Collection element] | SQLite.swift:197:16:197:50 | [...] [Collection element, Collection element] | provenance | | | SQLite.swift:197:18:197:32 | ... <-(_:_:) ... | SQLite.swift:197:17:197:49 | [...] [Collection element] | provenance | | | SQLite.swift:197:32:197:32 | mobilePhoneNumber | SQLite.swift:197:18:197:32 | ... <-(_:_:) ... | provenance | | +| file://:0:0:0:0 | [post] self [data, Collection element] | testRealm2.swift:13:6:13:6 | self [Return] [data, Collection element] | provenance | | | file://:0:0:0:0 | [post] self [data] | testRealm2.swift:13:6:13:6 | self [Return] [data] | provenance | | | file://:0:0:0:0 | [post] self [data] | testRealm.swift:27:6:27:6 | self [Return] [data] | provenance | | | file://:0:0:0:0 | [post] self [notStoredBankAccountNumber] | testCoreData2.swift:23:13:23:13 | self [Return] [notStoredBankAccountNumber] | provenance | | @@ -82,6 +83,7 @@ edges | file://:0:0:0:0 | value | file://:0:0:0:0 | [post] self [notStoredBankAccountNumber] | provenance | | | file://:0:0:0:0 | value | file://:0:0:0:0 | [post] self [password] | provenance | | | file://:0:0:0:0 | value | file://:0:0:0:0 | [post] self [value] | provenance | | +| file://:0:0:0:0 | value [Collection element] | file://:0:0:0:0 | [post] self [data, Collection element] | provenance | | | sqlite3_c_api.swift:42:69:42:69 | medicalNotes | sqlite3_c_api.swift:46:27:46:27 | insertQuery | provenance | | | sqlite3_c_api.swift:43:49:43:49 | medicalNotes | sqlite3_c_api.swift:47:27:47:27 | updateQuery | provenance | | | testCoreData2.swift:23:13:23:13 | value | file://:0:0:0:0 | value | provenance | | @@ -294,15 +296,38 @@ edges | testGRDB.swift:212:98:212:107 | [...] [Collection element] | testGRDB.swift:212:98:212:107 | [...] | provenance | | | testGRDB.swift:212:99:212:99 | password | testGRDB.swift:212:98:212:107 | [...] [Collection element] | provenance | | | testRealm2.swift:13:6:13:6 | value | file://:0:0:0:0 | value | provenance | | +| testRealm2.swift:13:6:13:6 | value [Collection element] | file://:0:0:0:0 | value [Collection element] | provenance | | | testRealm2.swift:18:2:18:2 | [post] o [data] | testRealm2.swift:18:2:18:2 | [post] o | provenance | | | testRealm2.swift:18:11:18:11 | myPassword | testRealm2.swift:13:6:13:6 | value | provenance | | | testRealm2.swift:18:11:18:11 | myPassword | testRealm2.swift:18:2:18:2 | [post] o [data] | provenance | | | testRealm2.swift:24:2:24:2 | [post] o [data] | testRealm2.swift:24:2:24:2 | [post] o | provenance | | | testRealm2.swift:24:11:24:11 | socialSecurityNumber | testRealm2.swift:13:6:13:6 | value | provenance | | | testRealm2.swift:24:11:24:11 | socialSecurityNumber | testRealm2.swift:24:2:24:2 | [post] o [data] | provenance | | +| testRealm2.swift:25:2:25:2 | [post] o [data] | testRealm2.swift:25:2:25:2 | [post] o | provenance | | +| testRealm2.swift:25:11:25:11 | ssn | testRealm2.swift:13:6:13:6 | value | provenance | | +| testRealm2.swift:25:11:25:11 | ssn | testRealm2.swift:25:2:25:2 | [post] o [data] | provenance | | +| testRealm2.swift:26:2:26:2 | [post] o [data, Collection element] | testRealm2.swift:26:2:26:2 | [post] o | provenance | | +| testRealm2.swift:26:2:26:2 | [post] o [data] | testRealm2.swift:26:2:26:2 | [post] o | provenance | | +| testRealm2.swift:26:11:26:25 | call to String.init(_:) | testRealm2.swift:13:6:13:6 | value | provenance | | +| testRealm2.swift:26:11:26:25 | call to String.init(_:) | testRealm2.swift:26:2:26:2 | [post] o [data] | provenance | | +| testRealm2.swift:26:11:26:25 | call to String.init(_:) [Collection element] | testRealm2.swift:13:6:13:6 | value [Collection element] | provenance | | +| testRealm2.swift:26:11:26:25 | call to String.init(_:) [Collection element] | testRealm2.swift:26:2:26:2 | [post] o [data, Collection element] | provenance | | +| testRealm2.swift:26:18:26:18 | ssn_int | testRealm2.swift:26:11:26:25 | call to String.init(_:) | provenance | | +| testRealm2.swift:26:18:26:18 | ssn_int | testRealm2.swift:26:11:26:25 | call to String.init(_:) [Collection element] | provenance | | | testRealm2.swift:32:2:32:2 | [post] o [data] | testRealm2.swift:32:2:32:2 | [post] o | provenance | | | testRealm2.swift:32:11:32:11 | creditCardNumber | testRealm2.swift:13:6:13:6 | value | provenance | | | testRealm2.swift:32:11:32:11 | creditCardNumber | testRealm2.swift:32:2:32:2 | [post] o [data] | provenance | | +| testRealm2.swift:33:2:33:2 | [post] o [data] | testRealm2.swift:33:2:33:2 | [post] o | provenance | | +| testRealm2.swift:33:11:33:11 | CCN | testRealm2.swift:13:6:13:6 | value | provenance | | +| testRealm2.swift:33:11:33:11 | CCN | testRealm2.swift:33:2:33:2 | [post] o [data] | provenance | | +| testRealm2.swift:34:2:34:2 | [post] o [data, Collection element] | testRealm2.swift:34:2:34:2 | [post] o | provenance | | +| testRealm2.swift:34:2:34:2 | [post] o [data] | testRealm2.swift:34:2:34:2 | [post] o | provenance | | +| testRealm2.swift:34:11:34:25 | call to String.init(_:) | testRealm2.swift:13:6:13:6 | value | provenance | | +| testRealm2.swift:34:11:34:25 | call to String.init(_:) | testRealm2.swift:34:2:34:2 | [post] o [data] | provenance | | +| testRealm2.swift:34:11:34:25 | call to String.init(_:) [Collection element] | testRealm2.swift:13:6:13:6 | value [Collection element] | provenance | | +| testRealm2.swift:34:11:34:25 | call to String.init(_:) [Collection element] | testRealm2.swift:34:2:34:2 | [post] o [data, Collection element] | provenance | | +| testRealm2.swift:34:18:34:18 | int_ccn | testRealm2.swift:34:11:34:25 | call to String.init(_:) | provenance | | +| testRealm2.swift:34:18:34:18 | int_ccn | testRealm2.swift:34:11:34:25 | call to String.init(_:) [Collection element] | provenance | | | testRealm.swift:27:6:27:6 | value | file://:0:0:0:0 | value | provenance | | | testRealm.swift:34:6:34:6 | value | file://:0:0:0:0 | value | provenance | | | testRealm.swift:41:2:41:2 | [post] a [data] | testRealm.swift:41:2:41:2 | [post] a | provenance | | @@ -419,6 +444,7 @@ nodes | file://:0:0:0:0 | .value | semmle.label | .value | | file://:0:0:0:0 | .value | semmle.label | .value | | file://:0:0:0:0 | .value2 | semmle.label | .value2 | +| file://:0:0:0:0 | [post] self [data, Collection element] | semmle.label | [post] self [data, Collection element] | | file://:0:0:0:0 | [post] self [data] | semmle.label | [post] self [data] | | file://:0:0:0:0 | [post] self [data] | semmle.label | [post] self [data] | | file://:0:0:0:0 | [post] self [notStoredBankAccountNumber] | semmle.label | [post] self [notStoredBankAccountNumber] | @@ -432,6 +458,7 @@ nodes | file://:0:0:0:0 | value | semmle.label | value | | file://:0:0:0:0 | value | semmle.label | value | | file://:0:0:0:0 | value | semmle.label | value | +| file://:0:0:0:0 | value [Collection element] | semmle.label | value [Collection element] | | sqlite3_c_api.swift:42:69:42:69 | medicalNotes | semmle.label | medicalNotes | | sqlite3_c_api.swift:43:49:43:49 | medicalNotes | semmle.label | medicalNotes | | sqlite3_c_api.swift:46:27:46:27 | insertQuery | semmle.label | insertQuery | @@ -722,17 +749,37 @@ nodes | testGRDB.swift:212:98:212:107 | [...] | semmle.label | [...] | | testGRDB.swift:212:98:212:107 | [...] [Collection element] | semmle.label | [...] [Collection element] | | testGRDB.swift:212:99:212:99 | password | semmle.label | password | +| testRealm2.swift:13:6:13:6 | self [Return] [data, Collection element] | semmle.label | self [Return] [data, Collection element] | | testRealm2.swift:13:6:13:6 | self [Return] [data] | semmle.label | self [Return] [data] | | testRealm2.swift:13:6:13:6 | value | semmle.label | value | +| testRealm2.swift:13:6:13:6 | value [Collection element] | semmle.label | value [Collection element] | | testRealm2.swift:18:2:18:2 | [post] o | semmle.label | [post] o | | testRealm2.swift:18:2:18:2 | [post] o [data] | semmle.label | [post] o [data] | | testRealm2.swift:18:11:18:11 | myPassword | semmle.label | myPassword | | testRealm2.swift:24:2:24:2 | [post] o | semmle.label | [post] o | | testRealm2.swift:24:2:24:2 | [post] o [data] | semmle.label | [post] o [data] | | testRealm2.swift:24:11:24:11 | socialSecurityNumber | semmle.label | socialSecurityNumber | +| testRealm2.swift:25:2:25:2 | [post] o | semmle.label | [post] o | +| testRealm2.swift:25:2:25:2 | [post] o [data] | semmle.label | [post] o [data] | +| testRealm2.swift:25:11:25:11 | ssn | semmle.label | ssn | +| testRealm2.swift:26:2:26:2 | [post] o | semmle.label | [post] o | +| testRealm2.swift:26:2:26:2 | [post] o [data, Collection element] | semmle.label | [post] o [data, Collection element] | +| testRealm2.swift:26:2:26:2 | [post] o [data] | semmle.label | [post] o [data] | +| testRealm2.swift:26:11:26:25 | call to String.init(_:) | semmle.label | call to String.init(_:) | +| testRealm2.swift:26:11:26:25 | call to String.init(_:) [Collection element] | semmle.label | call to String.init(_:) [Collection element] | +| testRealm2.swift:26:18:26:18 | ssn_int | semmle.label | ssn_int | | testRealm2.swift:32:2:32:2 | [post] o | semmle.label | [post] o | | testRealm2.swift:32:2:32:2 | [post] o [data] | semmle.label | [post] o [data] | | testRealm2.swift:32:11:32:11 | creditCardNumber | semmle.label | creditCardNumber | +| testRealm2.swift:33:2:33:2 | [post] o | semmle.label | [post] o | +| testRealm2.swift:33:2:33:2 | [post] o [data] | semmle.label | [post] o [data] | +| testRealm2.swift:33:11:33:11 | CCN | semmle.label | CCN | +| testRealm2.swift:34:2:34:2 | [post] o | semmle.label | [post] o | +| testRealm2.swift:34:2:34:2 | [post] o [data, Collection element] | semmle.label | [post] o [data, Collection element] | +| testRealm2.swift:34:2:34:2 | [post] o [data] | semmle.label | [post] o [data] | +| testRealm2.swift:34:11:34:25 | call to String.init(_:) | semmle.label | call to String.init(_:) | +| testRealm2.swift:34:11:34:25 | call to String.init(_:) [Collection element] | semmle.label | call to String.init(_:) [Collection element] | +| testRealm2.swift:34:18:34:18 | int_ccn | semmle.label | int_ccn | | testRealm.swift:27:6:27:6 | self [Return] [data] | semmle.label | self [Return] [data] | | testRealm.swift:27:6:27:6 | value | semmle.label | value | | testRealm.swift:34:6:34:6 | self [Return] [password] | semmle.label | self [Return] [password] | @@ -769,7 +816,13 @@ subpaths | testCoreData2.swift:105:18:105:18 | e | testCoreData2.swift:71:9:71:9 | self | file://:0:0:0:0 | .value2 | testCoreData2.swift:105:18:105:20 | .value2 | | testRealm2.swift:18:11:18:11 | myPassword | testRealm2.swift:13:6:13:6 | value | testRealm2.swift:13:6:13:6 | self [Return] [data] | testRealm2.swift:18:2:18:2 | [post] o [data] | | testRealm2.swift:24:11:24:11 | socialSecurityNumber | testRealm2.swift:13:6:13:6 | value | testRealm2.swift:13:6:13:6 | self [Return] [data] | testRealm2.swift:24:2:24:2 | [post] o [data] | +| testRealm2.swift:25:11:25:11 | ssn | testRealm2.swift:13:6:13:6 | value | testRealm2.swift:13:6:13:6 | self [Return] [data] | testRealm2.swift:25:2:25:2 | [post] o [data] | +| testRealm2.swift:26:11:26:25 | call to String.init(_:) | testRealm2.swift:13:6:13:6 | value | testRealm2.swift:13:6:13:6 | self [Return] [data] | testRealm2.swift:26:2:26:2 | [post] o [data] | +| testRealm2.swift:26:11:26:25 | call to String.init(_:) [Collection element] | testRealm2.swift:13:6:13:6 | value [Collection element] | testRealm2.swift:13:6:13:6 | self [Return] [data, Collection element] | testRealm2.swift:26:2:26:2 | [post] o [data, Collection element] | | testRealm2.swift:32:11:32:11 | creditCardNumber | testRealm2.swift:13:6:13:6 | value | testRealm2.swift:13:6:13:6 | self [Return] [data] | testRealm2.swift:32:2:32:2 | [post] o [data] | +| testRealm2.swift:33:11:33:11 | CCN | testRealm2.swift:13:6:13:6 | value | testRealm2.swift:13:6:13:6 | self [Return] [data] | testRealm2.swift:33:2:33:2 | [post] o [data] | +| testRealm2.swift:34:11:34:25 | call to String.init(_:) | testRealm2.swift:13:6:13:6 | value | testRealm2.swift:13:6:13:6 | self [Return] [data] | testRealm2.swift:34:2:34:2 | [post] o [data] | +| testRealm2.swift:34:11:34:25 | call to String.init(_:) [Collection element] | testRealm2.swift:13:6:13:6 | value [Collection element] | testRealm2.swift:13:6:13:6 | self [Return] [data, Collection element] | testRealm2.swift:34:2:34:2 | [post] o [data, Collection element] | | testRealm.swift:41:11:41:11 | myPassword | testRealm.swift:27:6:27:6 | value | testRealm.swift:27:6:27:6 | self [Return] [data] | testRealm.swift:41:2:41:2 | [post] a [data] | | testRealm.swift:49:11:49:11 | myPassword | testRealm.swift:27:6:27:6 | value | testRealm.swift:27:6:27:6 | self [Return] [data] | testRealm.swift:49:2:49:2 | [post] c [data] | | testRealm.swift:59:12:59:12 | myPassword | testRealm.swift:27:6:27:6 | value | testRealm.swift:27:6:27:6 | self [Return] [data] | testRealm.swift:59:2:59:3 | [post] ...! [data] | @@ -905,7 +958,11 @@ subpaths | testGRDB.swift:212:98:212:107 | [...] | testGRDB.swift:212:99:212:99 | password | testGRDB.swift:212:98:212:107 | [...] | This operation stores '[...]' in a database. It may contain unencrypted sensitive data from $@. | testGRDB.swift:212:99:212:99 | password | password | | testRealm2.swift:18:2:18:2 | o | testRealm2.swift:18:11:18:11 | myPassword | testRealm2.swift:18:2:18:2 | [post] o | This operation stores 'o' in a database. It may contain unencrypted sensitive data from $@. | testRealm2.swift:18:11:18:11 | myPassword | myPassword | | testRealm2.swift:24:2:24:2 | o | testRealm2.swift:24:11:24:11 | socialSecurityNumber | testRealm2.swift:24:2:24:2 | [post] o | This operation stores 'o' in a database. It may contain unencrypted sensitive data from $@. | testRealm2.swift:24:11:24:11 | socialSecurityNumber | socialSecurityNumber | +| testRealm2.swift:25:2:25:2 | o | testRealm2.swift:25:11:25:11 | ssn | testRealm2.swift:25:2:25:2 | [post] o | This operation stores 'o' in a database. It may contain unencrypted sensitive data from $@. | testRealm2.swift:25:11:25:11 | ssn | ssn | +| testRealm2.swift:26:2:26:2 | o | testRealm2.swift:26:18:26:18 | ssn_int | testRealm2.swift:26:2:26:2 | [post] o | This operation stores 'o' in a database. It may contain unencrypted sensitive data from $@. | testRealm2.swift:26:18:26:18 | ssn_int | ssn_int | | testRealm2.swift:32:2:32:2 | o | testRealm2.swift:32:11:32:11 | creditCardNumber | testRealm2.swift:32:2:32:2 | [post] o | This operation stores 'o' in a database. It may contain unencrypted sensitive data from $@. | testRealm2.swift:32:11:32:11 | creditCardNumber | creditCardNumber | +| testRealm2.swift:33:2:33:2 | o | testRealm2.swift:33:11:33:11 | CCN | testRealm2.swift:33:2:33:2 | [post] o | This operation stores 'o' in a database. It may contain unencrypted sensitive data from $@. | testRealm2.swift:33:11:33:11 | CCN | CCN | +| testRealm2.swift:34:2:34:2 | o | testRealm2.swift:34:18:34:18 | int_ccn | testRealm2.swift:34:2:34:2 | [post] o | This operation stores 'o' in a database. It may contain unencrypted sensitive data from $@. | testRealm2.swift:34:18:34:18 | int_ccn | int_ccn | | testRealm.swift:41:2:41:2 | a | testRealm.swift:41:11:41:11 | myPassword | testRealm.swift:41:2:41:2 | [post] a | This operation stores 'a' in a database. It may contain unencrypted sensitive data from $@. | testRealm.swift:41:11:41:11 | myPassword | myPassword | | testRealm.swift:49:2:49:2 | c | testRealm.swift:49:11:49:11 | myPassword | testRealm.swift:49:2:49:2 | [post] c | This operation stores 'c' in a database. It may contain unencrypted sensitive data from $@. | testRealm.swift:49:11:49:11 | myPassword | myPassword | | testRealm.swift:59:2:59:3 | ...! | testRealm.swift:59:12:59:12 | myPassword | testRealm.swift:59:2:59:3 | [post] ...! | This operation stores '...!' in a database. It may contain unencrypted sensitive data from $@. | testRealm.swift:59:12:59:12 | myPassword | myPassword | diff --git a/swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected b/swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected index b5f6abb01a40..353d6d0a6315 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected +++ b/swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected @@ -142,7 +142,11 @@ | testGRDB.swift:212:99:212:99 | password | label:password, type:password | | testRealm2.swift:18:11:18:11 | myPassword | label:myPassword, type:password | | testRealm2.swift:24:11:24:11 | socialSecurityNumber | label:socialSecurityNumber, type:private information | +| testRealm2.swift:25:11:25:11 | ssn | label:ssn, type:private information | +| testRealm2.swift:26:18:26:18 | ssn_int | label:ssn_int, type:private information | | testRealm2.swift:32:11:32:11 | creditCardNumber | label:creditCardNumber, type:private information | +| testRealm2.swift:33:11:33:11 | CCN | label:CCN, type:private information | +| testRealm2.swift:34:18:34:18 | int_ccn | label:int_ccn, type:private information | | testRealm.swift:31:20:31:20 | .password | label:password, type:password | | testRealm.swift:41:11:41:11 | myPassword | label:myPassword, type:password | | testRealm.swift:49:11:49:11 | myPassword | label:myPassword, type:password | diff --git a/swift/ql/test/query-tests/Security/CWE-311/testRealm2.swift b/swift/ql/test/query-tests/Security/CWE-311/testRealm2.swift index 8fcfcb002114..f9a325c42bbd 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/testRealm2.swift +++ b/swift/ql/test/query-tests/Security/CWE-311/testRealm2.swift @@ -22,16 +22,16 @@ func test1(o: MyRealmSwiftObject3, myHarmless: String, myPassword: String) { func test2(o: MyRealmSwiftObject3, ccn: String, socialSecurityNumber: String, ssn: String, ssn_int: Int, userSSN: String, classno: String) { o.data = socialSecurityNumber // BAD - o.data = ssn // BAD [NOT DETECTED] - o.data = String(ssn_int) // BAD [NOT DETECTED] + o.data = ssn // BAD + o.data = String(ssn_int) // BAD o.data = userSSN // BAD [NOT DETECTED] o.data = classno // GOOD } func test3(o: MyRealmSwiftObject3, ccn: String, creditCardNumber: String, CCN: String, int_ccn: Int, userCcn: String, succnode: String) { o.data = creditCardNumber // BAD - o.data = CCN // BAD [NOT DETECTED] - o.data = String(int_ccn) // BAD [NOT DETECTED] + o.data = CCN // BAD + o.data = String(int_ccn) // BAD o.data = userCcn // BAD [NOT DETECTED] o.data = succnode // GOOD } From 1f13e462b15ae420e1bc6bfd3db2019f22273631 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 23 May 2024 10:14:59 +0100 Subject: [PATCH 010/281] Swift: Change note. --- swift/ql/lib/change-notes/2024-05-23-sensitive-data.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 swift/ql/lib/change-notes/2024-05-23-sensitive-data.md diff --git a/swift/ql/lib/change-notes/2024-05-23-sensitive-data.md b/swift/ql/lib/change-notes/2024-05-23-sensitive-data.md new file mode 100644 index 000000000000..f42901c45895 --- /dev/null +++ b/swift/ql/lib/change-notes/2024-05-23-sensitive-data.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Additional heuristics for sensitive private information have been added to the `SensitiveExprs.qll` library, improving coverage for credit card and social security numbers. This may result in additional results for queries that use sensitive data such as `swift/cleartext-transmission`. From f5d465f08adb6077abf5508838f2025d5623eebb Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Fri, 7 Jun 2024 15:32:11 +0100 Subject: [PATCH 011/281] Added data extension to allow setting extra required Helmet features --- .../src/Security/CWE-693/InsecureHelmet.qhelp | 22 +++++++++++++++++-- .../ql/src/Security/CWE-693/InsecureHelmet.ql | 19 +++++++++++++++- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp index b54047ba4ae4..d0550823ab6e 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp @@ -2,7 +2,7 @@

    - Helmet is a collection of middleware functions for securing Express apps. It sets various HTTP headers to guard against common web vulnerabilities. + Helmet is a collection of middleware functions for securing Express apps. It sets various HTTP headers to guard against common web vulnerabilities.
    This query detects Helmet misconfigurations that can lead to security vulnerabilities, specifically:

    @@ -13,10 +13,28 @@

    - Content Security Policy (CSP) helps spot and prevent injection attacks such as Cross-Site Scripting (XSS). + Content Security Policy (CSP) helps spot and prevent injection attacks such as Cross-Site Scripting (XSS).
    Removing frame protections exposes an application to attacks such as clickjacking, where an attacker can trick a user into clicking on a button or link on a targeted page when they intended to click on the page carrying out the attack.

    + +

    + Users of the query can extend the set of required Helmet features by adding additional checks for them, using CodeQL data extensions. +

    + + 
    +        extensions:
+          - addsTo:
+              pack: codeql/javascript-all
+              extensible: requiredHelmetSecuritySetting
+            data:
+              - name: "frameguard"
+
    + +

    + Note: frameguard is an example: the query already enforces this setting, so it is not necessary to add it to the data extension. +

    +

    diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql index ca27b717f186..b8e3bb08131b 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -27,10 +27,27 @@ class HelmetProperty extends Property { predicate isImportantSecuritySetting() { this.getName() in ["frameguard", "contentSecurityPolicy"] // read from data extensions to allow enforcing other settings - // TODO + or requiredHelmetSecuritySetting(this.getName()) } } +/* + * Extend the required Helmet security settings using data extensions. + * Docs: https://codeql.github.com/docs/codeql-language-guides/customizing-library-models-for-javascript/ + * For example: + +extensions: + - addsTo: + pack: codeql/javascript-all + extensible: requiredHelmetSecuritySetting + data: + - name: "frameguard" + + * Note: `frameguard` is an example: the query already enforces this setting, so it is not necessary to add it to the data extension. + + */ +extensible predicate requiredHelmetSecuritySetting(string name); + from HelmetProperty helmetSetting, ExpressLibraries::HelmetRouteHandler helmet where helmetSetting.isFalse() and From 465d64a810a1d77cccfd6f1d63335530e9967959 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Fri, 7 Jun 2024 15:34:45 +0100 Subject: [PATCH 012/281] Removed br tags --- javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp index d0550823ab6e..8c0484d8a9e9 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp @@ -2,7 +2,7 @@

    - Helmet is a collection of middleware functions for securing Express apps. It sets various HTTP headers to guard against common web vulnerabilities.
    + Helmet is a collection of middleware functions for securing Express apps. It sets various HTTP headers to guard against common web vulnerabilities. This query detects Helmet misconfigurations that can lead to security vulnerabilities, specifically:

    @@ -13,7 +13,7 @@

    - Content Security Policy (CSP) helps spot and prevent injection attacks such as Cross-Site Scripting (XSS).
    + Content Security Policy (CSP) helps spot and prevent injection attacks such as Cross-Site Scripting (XSS). Removing frame protections exposes an application to attacks such as clickjacking, where an attacker can trick a user into clicking on a button or link on a targeted page when they intended to click on the page carrying out the attack.

    From 7136763c37c4f993fde037df7c44bc9c1133fec5 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Fri, 7 Jun 2024 15:36:39 +0100 Subject: [PATCH 013/281] Formatting --- .../ql/src/Security/CWE-693/InsecureHelmet.ql | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql index b8e3bb08131b..70d4b41e096c 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -26,8 +26,9 @@ class HelmetProperty extends Property { predicate isImportantSecuritySetting() { this.getName() in ["frameguard", "contentSecurityPolicy"] + or // read from data extensions to allow enforcing other settings - or requiredHelmetSecuritySetting(this.getName()) + requiredHelmetSecuritySetting(this.getName()) } } @@ -35,17 +36,17 @@ class HelmetProperty extends Property { * Extend the required Helmet security settings using data extensions. * Docs: https://codeql.github.com/docs/codeql-language-guides/customizing-library-models-for-javascript/ * For example: - -extensions: - - addsTo: - pack: codeql/javascript-all - extensible: requiredHelmetSecuritySetting - data: - - name: "frameguard" - - * Note: `frameguard` is an example: the query already enforces this setting, so it is not necessary to add it to the data extension. - + * + * extensions: + * - addsTo: + * pack: codeql/javascript-all + * extensible: requiredHelmetSecuritySetting + * data: + * - name: "frameguard" + * + * Note: `frameguard` is an example: the query already enforces this setting, so it is not necessary to add it to the data extension. */ + extensible predicate requiredHelmetSecuritySetting(string name); from HelmetProperty helmetSetting, ExpressLibraries::HelmetRouteHandler helmet From 975811ae5973f537ca785ef0c453fb4b166b1205 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Fri, 7 Jun 2024 15:50:06 +0100 Subject: [PATCH 014/281] Change layout of qhelp example code --- .../src/Security/CWE-693/InsecureHelmet.qhelp | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp index 8c0484d8a9e9..b2f87f9aeacd 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp @@ -23,12 +23,12 @@ 

    -        extensions:
-          - addsTo:
-              pack: codeql/javascript-all
-              extensible: requiredHelmetSecuritySetting
-            data:
-              - name: "frameguard"
+extensions:
+- addsTo:
+    pack: codeql/javascript-all
+    extensible: requiredHelmetSecuritySetting
+data:
+    - name: "frameguard"

    @@ -52,11 +52,11 @@ 

    -            const helmet = require('helmet');
-            app.use(helmet({
-                frameguard: false,
-                contentSecurityPolicy: false
-            }));
+const helmet = require('helmet');
+app.use(helmet({
+    frameguard: false,
+    contentSecurityPolicy: false
+}));

    @@ -64,7 +64,7 @@ 

    -            app.use(helmet());
+app.use(helmet());

    @@ -72,16 +72,16 @@ 

    -            app.use(
-                helmet({
-                    contentSecurityPolicy: {
-                        directives: {
-                            "script-src": ["'self'", "example.com"],
-                            "style-src": null,
-                        },
-                    },
-                })
-            );
+app.use(
+    helmet({
+        contentSecurityPolicy: {
+            directives: {
+                "script-src": ["'self'", "example.com"],
+                "style-src": null,
+            },
+        },
+    })
+);
    From da9e1e61a4efa16001e6bc57faaf318bbf595f94 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Tue, 18 Jun 2024 19:50:06 +0100 Subject: [PATCH 015/281] Moved examples into separate files --- .../src/Security/CWE-693/InsecureHelmet.qhelp | 25 +++---------------- .../CWE-693/examples/helmet_custom.js | 10 ++++++++ .../CWE-693/examples/helmet_default.js | 1 + .../CWE-693/examples/helmet_insecure.js | 6 +++++ 4 files changed, 20 insertions(+), 22 deletions(-) create mode 100644 javascript/ql/src/Security/CWE-693/examples/helmet_custom.js create mode 100644 javascript/ql/src/Security/CWE-693/examples/helmet_default.js create mode 100644 javascript/ql/src/Security/CWE-693/examples/helmet_insecure.js diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp index b2f87f9aeacd..f0813ffecd71 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp @@ -51,38 +51,19 @@ data: The following code snippet demonstrates Helmet configured in an insecure manner:

    - 
    -const helmet = require('helmet');
-app.use(helmet({
-    frameguard: false,
-    contentSecurityPolicy: false
-}));
-
    +

    In this example, the defaults are used, which enables frame protection and a default Content Security Policy.

    - 
    -app.use(helmet());
-
    +

    You can also enable a custom Content Security Policy by passing an object to the contentSecurityPolicy key. For example, taken from the Helmet docs:

    - 
    -app.use(
-    helmet({
-        contentSecurityPolicy: {
-            directives: {
-                "script-src": ["'self'", "example.com"],
-                "style-src": null,
-            },
-        },
-    })
-);
-
    + diff --git a/javascript/ql/src/Security/CWE-693/examples/helmet_custom.js b/javascript/ql/src/Security/CWE-693/examples/helmet_custom.js new file mode 100644 index 000000000000..5b9e25033f22 --- /dev/null +++ b/javascript/ql/src/Security/CWE-693/examples/helmet_custom.js @@ -0,0 +1,10 @@ +app.use( + helmet({ + contentSecurityPolicy: { + directives: { + "script-src": ["'self'", "example.com"], + "style-src": null, + }, + }, + }) +); \ No newline at end of file diff --git a/javascript/ql/src/Security/CWE-693/examples/helmet_default.js b/javascript/ql/src/Security/CWE-693/examples/helmet_default.js new file mode 100644 index 000000000000..98936520dcb7 --- /dev/null +++ b/javascript/ql/src/Security/CWE-693/examples/helmet_default.js @@ -0,0 +1 @@ +app.use(helmet()); \ No newline at end of file diff --git a/javascript/ql/src/Security/CWE-693/examples/helmet_insecure.js b/javascript/ql/src/Security/CWE-693/examples/helmet_insecure.js new file mode 100644 index 000000000000..62852b9f4823 --- /dev/null +++ b/javascript/ql/src/Security/CWE-693/examples/helmet_insecure.js @@ -0,0 +1,6 @@ +const helmet = require('helmet'); + +app.use(helmet({ + frameguard: false, + contentSecurityPolicy: false +})); \ No newline at end of file From 81ef255a87553ef361cd8b6bca8e64262a3cde76 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Wed, 19 Jun 2024 10:09:50 +0100 Subject: [PATCH 016/281] Change to helmetProperty from helmetSetting variable name --- javascript/ql/src/Security/CWE-693/InsecureHelmet.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql index 70d4b41e096c..c4bb5b5ab520 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -49,7 +49,7 @@ class HelmetProperty extends Property { extensible predicate requiredHelmetSecuritySetting(string name); -from HelmetProperty helmetSetting, ExpressLibraries::HelmetRouteHandler helmet +from HelmetProperty helmetProperty, ExpressLibraries::HelmetRouteHandler helmet where helmetSetting.isFalse() and helmetSetting.isImportantSecuritySetting() and From f4691b191934b853eddafb000a76087ac8d30367 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Wed, 19 Jun 2024 10:11:06 +0100 Subject: [PATCH 017/281] Changed to more-modern Dataflow libraries --- javascript/ql/src/Security/CWE-693/InsecureHelmet.ql | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql index c4bb5b5ab520..90ac834575db 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -13,16 +13,18 @@ import semmle.javascript.frameworks.ExpressModules -class HelmetProperty extends Property { +class HelmetProperty extends DataFlow::Node instanceof DataFlow::PropWrite { ExpressLibraries::HelmetRouteHandler helmet; HelmetProperty() { - helmet.(DataFlow::CallNode).getAnArgument().asExpr().(ObjectExpr).getAProperty() = this + this = helmet.(DataFlow::CallNode).getAnArgument().getALocalSource().getAPropertyWrite() } ExpressLibraries::HelmetRouteHandler getHelmet() { result = helmet } - predicate isFalse() { this.getInit().(BooleanLiteral).getBoolValue() = false } + predicate isFalse() { DataFlow::PropWrite.super.getRhs().mayHaveBooleanValue(true) } + + string getName() { result = DataFlow::PropWrite.super.getPropertyName() } predicate isImportantSecuritySetting() { this.getName() in ["frameguard", "contentSecurityPolicy"] From de96d3951d288788519afff210ad183cf860574b Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Wed, 19 Jun 2024 10:15:06 +0100 Subject: [PATCH 018/281] Renamed to helmetProperty everywhere --- javascript/ql/src/Security/CWE-693/InsecureHelmet.ql | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql index 90ac834575db..debcd9c3ddd5 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -53,8 +53,8 @@ extensible predicate requiredHelmetSecuritySetting(string name); from HelmetProperty helmetProperty, ExpressLibraries::HelmetRouteHandler helmet where - helmetSetting.isFalse() and - helmetSetting.isImportantSecuritySetting() and - helmetSetting.getHelmet() = helmet -select helmet, "Helmet route handler, called with $@ set to 'false'.", helmetSetting, - helmetSetting.getName() + helmetProperty.isFalse() and + helmetProperty.isImportantSecuritySetting() and + helmetProperty.getHelmet() = helmet +select helmet, "Helmet route handler, called with $@ set to 'false'.", helmetProperty, +helmetProperty.getName() From 8a3cec49778fa97d572c2e0c46fe3867783efd43 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Wed, 19 Jun 2024 11:38:20 +0100 Subject: [PATCH 019/281] Fix formatting for check --- javascript/ql/src/Security/CWE-693/InsecureHelmet.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql index debcd9c3ddd5..c1ff6ca3e395 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -57,4 +57,4 @@ where helmetProperty.isImportantSecuritySetting() and helmetProperty.getHelmet() = helmet select helmet, "Helmet route handler, called with $@ set to 'false'.", helmetProperty, -helmetProperty.getName() + helmetProperty.getName() From d142f830da8097c28722b79ef2920b8a2e2545dc Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Wed, 19 Jun 2024 12:04:32 +0100 Subject: [PATCH 020/281] Change note and changed name of query in `.ql` file --- javascript/ql/src/Security/CWE-693/InsecureHelmet.ql | 2 +- .../ql/src/change-notes/2024-06-19-insecure-helmet-config.md | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 javascript/ql/src/change-notes/2024-06-19-insecure-helmet-config.md diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql index c1ff6ca3e395..3a2643d603ee 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -5,7 +5,7 @@ * @problem.severity error * @security-severity 5.0 * @precision high - * @id javascript/insecure-helmet-configuration + * @id js/insecure-helmet-configuration * @tags security * cwe-693 * cwe-1021 diff --git a/javascript/ql/src/change-notes/2024-06-19-insecure-helmet-config.md b/javascript/ql/src/change-notes/2024-06-19-insecure-helmet-config.md new file mode 100644 index 000000000000..bee7ccb8fb94 --- /dev/null +++ b/javascript/ql/src/change-notes/2024-06-19-insecure-helmet-config.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* Added a new query, `js/insecure-helmet-configuration`, to detect instances where Helmet middleware is configured with important security features disabled. From 252c9e9416c9b923ac98bdf0a53c09ce32e9c6a9 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Wed, 19 Jun 2024 17:27:17 +0100 Subject: [PATCH 021/281] Added data extension to set defaults, updated help, added README to explain customization --- .../helmet/Helmet.Required.Setting.model.yml | 7 ++++ .../src/Security/CWE-693/InsecureHelmet.qhelp | 12 +++---- .../ql/src/Security/CWE-693/InsecureHelmet.ql | 26 ++++---------- javascript/ql/src/Security/CWE-693/README.md | 36 +++++++++++++++++++ 4 files changed, 54 insertions(+), 27 deletions(-) create mode 100644 javascript/ql/lib/semmle/javascript/frameworks/helmet/Helmet.Required.Setting.model.yml create mode 100644 javascript/ql/src/Security/CWE-693/README.md diff --git a/javascript/ql/lib/semmle/javascript/frameworks/helmet/Helmet.Required.Setting.model.yml b/javascript/ql/lib/semmle/javascript/frameworks/helmet/Helmet.Required.Setting.model.yml new file mode 100644 index 000000000000..ab01ec5206df --- /dev/null +++ b/javascript/ql/lib/semmle/javascript/frameworks/helmet/Helmet.Required.Setting.model.yml @@ -0,0 +1,7 @@ +extensions: + - addsTo: + pack: codeql/javascript-queries + extensible: requiredHelmetSecuritySetting + data: + - ["frameguard"] + - ["contentSecurityPolicy"] diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp index f0813ffecd71..c09978e13d15 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp @@ -22,14 +22,12 @@ Users of the query can extend the set of required Helmet features by adding additional checks for them, using CodeQL data extensions.

    - 
    -extensions:
-- addsTo:
-    pack: codeql/javascript-all
-    extensible: requiredHelmetSecuritySetting
+        
    extensions:
+  - addsTo:
+      pack: codeql/javascript-all
+      extensible: requiredHelmetSecuritySetting
 data:
-    - name: "frameguard"
-        
    
+    - ["frameguard"]

    Note: frameguard is an example: the query already enforces this setting, so it is not necessary to add it to the data extension. diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql index 3a2643d603ee..b5495714ac72 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -11,6 +11,8 @@ * cwe-1021 */ +import javascript +import DataFlow import semmle.javascript.frameworks.ExpressModules class HelmetProperty extends DataFlow::Node instanceof DataFlow::PropWrite { @@ -22,33 +24,17 @@ class HelmetProperty extends DataFlow::Node instanceof DataFlow::PropWrite { ExpressLibraries::HelmetRouteHandler getHelmet() { result = helmet } - predicate isFalse() { DataFlow::PropWrite.super.getRhs().mayHaveBooleanValue(true) } + predicate isFalse() { DataFlow::PropWrite.super.getRhs().mayHaveBooleanValue(false) } string getName() { result = DataFlow::PropWrite.super.getPropertyName() } predicate isImportantSecuritySetting() { - this.getName() in ["frameguard", "contentSecurityPolicy"] - or - // read from data extensions to allow enforcing other settings + // read from data extensions to allow enforcing custom settings + // defaults are located in javascript/ql/lib/semmle/frameworks/helmet/Helmet.Required.Setting.model.yml requiredHelmetSecuritySetting(this.getName()) } } -/* - * Extend the required Helmet security settings using data extensions. - * Docs: https://codeql.github.com/docs/codeql-language-guides/customizing-library-models-for-javascript/ - * For example: - * - * extensions: - * - addsTo: - * pack: codeql/javascript-all - * extensible: requiredHelmetSecuritySetting - * data: - * - name: "frameguard" - * - * Note: `frameguard` is an example: the query already enforces this setting, so it is not necessary to add it to the data extension. - */ - extensible predicate requiredHelmetSecuritySetting(string name); from HelmetProperty helmetProperty, ExpressLibraries::HelmetRouteHandler helmet @@ -56,5 +42,5 @@ where helmetProperty.isFalse() and helmetProperty.isImportantSecuritySetting() and helmetProperty.getHelmet() = helmet -select helmet, "Helmet route handler, called with $@ set to 'false'.", helmetProperty, +select helmet, "Helmet security middleware, configured with security setting $@ set to 'false', which disables enforcing that feature.", helmetProperty, helmetProperty.getName() diff --git a/javascript/ql/src/Security/CWE-693/README.md b/javascript/ql/src/Security/CWE-693/README.md new file mode 100644 index 000000000000..0ca0afd74bb0 --- /dev/null +++ b/javascript/ql/src/Security/CWE-693/README.md @@ -0,0 +1,36 @@ +# Insecure Helmet Configuration - customizations + +You can extend the required [Helmet security settings](https://helmetjs.github.io/) using [data extensions](https://codeql.github.com/docs/codeql-language-guides/customizing-library-models-for-javascript/). + +They are defaulted to just `frameguard` and `contentSecurityPolicy`, but you can add more using this method, to require them not to be set to `false` (which explicitly disables them) in the Helmet configuration. + +For example, this YAML model can be used inside a CodeQL model pack to require `frameguard` and `contentSecurityPolicy`: + +```yaml +extensions: + - addsTo: + pack: codeql/javascript-all + extensible: requiredHelmetSecuritySetting + data: + - ["frameguard"] + - ["contentSecurityPolicy"] +``` + +Note: Using `frameguard` and `contentSecurityPolicy` is an example: the query already enforces these, so it is not necessary to add it with your own data extension. + +A suitable model pack might be: + +```yaml +name: my-org/javascript-helmet-insecure-config-model-pack +version: 1.0.0 +extensionTargets: + codeql/java-all: '*' +dataExtensions: + - models/**/*.yml +``` + +## References + +- [Helmet security settings](https://helmetjs.github.io/) +- [Customizing library models for javascript](https://codeql.github.com/docs/codeql-language-guides/customizing-library-models-for-javascript/) +- [Creating and working with CodeQL packs](https://docs.github.com/en/code-security/codeql-cli/using-the-advanced-functionality-of-the-codeql-cli/creating-and-working-with-codeql-packs#creating-a-codeql-model-pack) From 26f1b367363b6fc1f6abc2b051bb0b22c2cf033d Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Wed, 19 Jun 2024 17:41:58 +0100 Subject: [PATCH 022/281] Fixed formatting --- javascript/ql/src/Security/CWE-693/InsecureHelmet.ql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql index b5495714ac72..39159b72406d 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -42,5 +42,6 @@ where helmetProperty.isFalse() and helmetProperty.isImportantSecuritySetting() and helmetProperty.getHelmet() = helmet -select helmet, "Helmet security middleware, configured with security setting $@ set to 'false', which disables enforcing that feature.", helmetProperty, - helmetProperty.getName() +select helmet, + "Helmet security middleware, configured with security setting $@ set to 'false', which disables enforcing that feature.", + helmetProperty, helmetProperty.getName() From a07639f4f6750ddf4be63f6ced724d36b9ae7c66 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Wed, 19 Jun 2024 17:43:41 +0100 Subject: [PATCH 023/281] Set severity to 7.0, in line with other configuration queries --- javascript/ql/src/Security/CWE-693/InsecureHelmet.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql index 39159b72406d..c4437d4913d4 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -3,7 +3,7 @@ * @description The Helmet middleware is used to set security-related HTTP headers in Express applications. This query finds instances where the middleware is configured with important security features disabled. * @kind problem * @problem.severity error - * @security-severity 5.0 + * @security-severity 7.0 * @precision high * @id js/insecure-helmet-configuration * @tags security From 1ecd72727ddacb1380cb7c7698bfbaac398728de Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Wed, 19 Jun 2024 17:59:43 +0100 Subject: [PATCH 024/281] Renamed README to CUSTOMIZING, removed details from qhelp and referenced md doc instead --- .../Security/CWE-693/{README.md => CUSTOMIZING.md} | 4 ++-- .../ql/src/Security/CWE-693/InsecureHelmet.qhelp | 13 +------------ 2 files changed, 3 insertions(+), 14 deletions(-) rename javascript/ql/src/Security/CWE-693/{README.md => CUSTOMIZING.md} (78%) diff --git a/javascript/ql/src/Security/CWE-693/README.md b/javascript/ql/src/Security/CWE-693/CUSTOMIZING.md similarity index 78% rename from javascript/ql/src/Security/CWE-693/README.md rename to javascript/ql/src/Security/CWE-693/CUSTOMIZING.md index 0ca0afd74bb0..34ae2851a855 100644 --- a/javascript/ql/src/Security/CWE-693/README.md +++ b/javascript/ql/src/Security/CWE-693/CUSTOMIZING.md @@ -1,6 +1,6 @@ # Insecure Helmet Configuration - customizations -You can extend the required [Helmet security settings](https://helmetjs.github.io/) using [data extensions](https://codeql.github.com/docs/codeql-language-guides/customizing-library-models-for-javascript/). +You can extend the required [Helmet security settings](https://helmetjs.github.io/) using [data extensions](https://codeql.github.com/docs/codeql-language-guides/customizing-library-models-for-javascript/) in a [CodeQL model pack](https://docs.github.com/en/code-security/codeql-cli/using-the-advanced-functionality-of-the-codeql-cli/creating-and-working-with-codeql-packs#creating-a-codeql-model-pack). They are defaulted to just `frameguard` and `contentSecurityPolicy`, but you can add more using this method, to require them not to be set to `false` (which explicitly disables them) in the Helmet configuration. @@ -18,7 +18,7 @@ extensions: Note: Using `frameguard` and `contentSecurityPolicy` is an example: the query already enforces these, so it is not necessary to add it with your own data extension. -A suitable model pack might be: +A suitable [model pack](https://docs.github.com/en/code-security/codeql-cli/using-the-advanced-functionality-of-the-codeql-cli/creating-and-working-with-codeql-packs#creating-a-codeql-model-pack) might be: ```yaml name: my-org/javascript-helmet-insecure-config-model-pack diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp index c09978e13d15..e294779d6b85 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp @@ -19,18 +19,7 @@

    - Users of the query can extend the set of required Helmet features by adding additional checks for them, using CodeQL data extensions. -

    - - 
    extensions:
-  - addsTo:
-      pack: codeql/javascript-all
-      extensible: requiredHelmetSecuritySetting
-data:
-    - ["frameguard"]
    - -

    - Note: frameguard is an example: the query already enforces this setting, so it is not necessary to add it to the data extension. + Users of the query can extend the set of required Helmet features by adding additional checks for them, using CodeQL data extensions in a CodeQL model pack. See `CUSTOMIZING.md` in the query source for more information.

    From b71ba7c30f47d7fa7901c71ee8a481b94236333b Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 28 May 2024 10:40:15 +0100 Subject: [PATCH 025/281] Move Header Write derrived concepts to Concepts --- python/ql/lib/semmle/python/Concepts.qll | 48 ++++++++++++++++++ .../HttpHeaderInjectionCustomizations.qll | 50 ------------------- 2 files changed, 48 insertions(+), 50 deletions(-) diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll index 029f13ee0c2a..8e64deddb4e5 100644 --- a/python/ql/lib/semmle/python/Concepts.qll +++ b/python/ql/lib/semmle/python/Concepts.qll @@ -1134,6 +1134,54 @@ module Http { } } + /** A key-value pair in a literal for a bulk header update, considered as a single header update. */ + private class HeaderBulkWriteDictLiteral extends Http::Server::ResponseHeaderWrite::Range instanceof Http::Server::ResponseHeaderBulkWrite + { + KeyValuePair item; + + HeaderBulkWriteDictLiteral() { + exists(Dict dict | DataFlow::localFlow(DataFlow::exprNode(dict), super.getBulkArg()) | + item = dict.getAnItem() + ) + } + + override DataFlow::Node getNameArg() { result.asExpr() = item.getKey() } + + override DataFlow::Node getValueArg() { result.asExpr() = item.getValue() } + + override predicate nameAllowsNewline() { + Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline() + } + + override predicate valueAllowsNewline() { + Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline() + } + } + + /** A tuple in a list for a bulk header update, considered as a single header update. */ + private class HeaderBulkWriteListLiteral extends Http::Server::ResponseHeaderWrite::Range instanceof Http::Server::ResponseHeaderBulkWrite + { + Tuple item; + + HeaderBulkWriteListLiteral() { + exists(List list | DataFlow::localFlow(DataFlow::exprNode(list), super.getBulkArg()) | + item = list.getAnElt() + ) + } + + override DataFlow::Node getNameArg() { result.asExpr() = item.getElt(0) } + + override DataFlow::Node getValueArg() { result.asExpr() = item.getElt(1) } + + override predicate nameAllowsNewline() { + Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline() + } + + override predicate valueAllowsNewline() { + Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline() + } + } + /** * A data-flow node that sets a cookie in an HTTP response. * diff --git a/python/ql/lib/semmle/python/security/dataflow/HttpHeaderInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/HttpHeaderInjectionCustomizations.qll index b3fe233629e4..e529d3f29e0f 100644 --- a/python/ql/lib/semmle/python/security/dataflow/HttpHeaderInjectionCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/HttpHeaderInjectionCustomizations.qll @@ -51,56 +51,6 @@ module HttpHeaderInjection { } } - /** A key-value pair in a literal for a bulk header update, considered as a single header update. */ - // TODO: We could instead consider bulk writes as sinks with an implicit read step of DictionaryKey/DictionaryValue content as needed. - private class HeaderBulkWriteDictLiteral extends Http::Server::ResponseHeaderWrite::Range instanceof Http::Server::ResponseHeaderBulkWrite - { - KeyValuePair item; - - HeaderBulkWriteDictLiteral() { - exists(Dict dict | DataFlow::localFlow(DataFlow::exprNode(dict), super.getBulkArg()) | - item = dict.getAnItem() - ) - } - - override DataFlow::Node getNameArg() { result.asExpr() = item.getKey() } - - override DataFlow::Node getValueArg() { result.asExpr() = item.getValue() } - - override predicate nameAllowsNewline() { - Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline() - } - - override predicate valueAllowsNewline() { - Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline() - } - } - - /** A tuple in a list for a bulk header update, considered as a single header update. */ - // TODO: We could instead consider bulk writes as sinks with implicit read steps as needed. - private class HeaderBulkWriteListLiteral extends Http::Server::ResponseHeaderWrite::Range instanceof Http::Server::ResponseHeaderBulkWrite - { - Tuple item; - - HeaderBulkWriteListLiteral() { - exists(List list | DataFlow::localFlow(DataFlow::exprNode(list), super.getBulkArg()) | - item = list.getAnElt() - ) - } - - override DataFlow::Node getNameArg() { result.asExpr() = item.getElt(0) } - - override DataFlow::Node getValueArg() { result.asExpr() = item.getElt(1) } - - override predicate nameAllowsNewline() { - Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline() - } - - override predicate valueAllowsNewline() { - Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline() - } - } - /** * A call to replace line breaks, considered as a sanitizer. */ From d11f58f768db1bea06bec169d60144f5aa214ec3 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 28 May 2024 10:47:41 +0100 Subject: [PATCH 026/281] Add cookie header write concept from experimental. --- python/ql/lib/semmle/python/Concepts.qll | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll index 8e64deddb4e5..c351a7dceedb 100644 --- a/python/ql/lib/semmle/python/Concepts.qll +++ b/python/ql/lib/semmle/python/Concepts.qll @@ -1234,6 +1234,29 @@ module Http { } } + /** A write to a `Set-Cookie` header that sets a cookie directly. */ + private class CookieHeaderWrite extends CookieWrite::Range instanceof Http::Server::ResponseHeaderWrite + { + CookieHeaderWrite() { + exists(StringLiteral str | + str.getText() = "Set-Cookie" and + DataFlow::exprNode(str) + .(DataFlow::LocalSourceNode) + .flowsTo(this.(Http::Server::ResponseHeaderWrite).getNameArg()) + ) + } + + override DataFlow::Node getNameArg() { + result = this.(Http::Server::ResponseHeaderWrite).getValueArg() + } + + override DataFlow::Node getHeaderArg() { + result = this.(Http::Server::ResponseHeaderWrite).getValueArg() + } + + override DataFlow::Node getValueArg() { none() } + } + /** * A data-flow node that enables or disables Cross-site request forgery protection * in a global manner. From 6b8080a5b3957e88002ef66f7dd3033a57f437a6 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Thu, 6 Jun 2024 15:10:25 +0100 Subject: [PATCH 027/281] Update concept tests for header writes --- .../test/experimental/meta/ConceptsTest.qll | 33 ++++++++------- .../frameworks/flask/response_test.py | 42 +++++++++---------- .../stdlib/wsgiref_simple_server_test.py | 10 ++--- 3 files changed, 43 insertions(+), 42 deletions(-) diff --git a/python/ql/test/experimental/meta/ConceptsTest.qll b/python/ql/test/experimental/meta/ConceptsTest.qll index b552758582b3..473c7c177c7e 100644 --- a/python/ql/test/experimental/meta/ConceptsTest.qll +++ b/python/ql/test/experimental/meta/ConceptsTest.qll @@ -323,8 +323,8 @@ module HttpResponseHeaderWriteTest implements TestSig { string getARelevantTag() { result = [ - "headerWriteNameUnsanitized", "headerWriteNameSanitized", "headerWriteValueUnsanitized", - "headerWriteValueSanitized", "headerWriteBulk" + "headerWriteNameUnsanitized", "headerWriteName", "headerWriteValueUnsanitized", + "headerWriteValue", "headerWriteBulk", "headerWriteBulkUnsanitized" ] } @@ -339,7 +339,7 @@ module HttpResponseHeaderWriteTest implements TestSig { ( if write.nameAllowsNewline() then tag = "headerWriteNameUnsanitized" - else tag = "headerWriteNameSanitized" + else tag = "headerWriteName" ) and value = prettyNodeForInlineTest(node) or @@ -347,7 +347,7 @@ module HttpResponseHeaderWriteTest implements TestSig { ( if write.valueAllowsNewline() then tag = "headerWriteValueUnsanitized" - else tag = "headerWriteValueSanitized" + else tag = "headerWriteValue" ) and value = prettyNodeForInlineTest(node) ) @@ -360,19 +360,20 @@ module HttpResponseHeaderWriteTest implements TestSig { tag = "headerWriteBulk" and value = prettyNodeForInlineTest(node) or + tag = "headerWriteBulkUnsanitized" and ( - if write.nameAllowsNewline() - then tag = "headerWriteNameUnsanitized" - else tag = "headerWriteNameSanitized" - ) and - value = "" - or - ( - if write.valueAllowsNewline() - then tag = "headerWriteValueUnsanitized" - else tag = "headerWriteValueSanitized" - ) and - value = "" + write.nameAllowsNewline() and + not write.valueAllowsNewline() and + value = "name" + or + not write.nameAllowsNewline() and + write.valueAllowsNewline() and + value = "value" + or + write.nameAllowsNewline() and + write.valueAllowsNewline() and + value = "name,value" + ) ) ) ) diff --git a/python/ql/test/library-tests/frameworks/flask/response_test.py b/python/ql/test/library-tests/frameworks/flask/response_test.py index 1359d2f93810..bcfc36ff365e 100644 --- a/python/ql/test/library-tests/frameworks/flask/response_test.py +++ b/python/ql/test/library-tests/frameworks/flask/response_test.py @@ -118,7 +118,7 @@ def response_modification1(): # $requestHandler @app.route("/content-type/response-modification2") # $routeSetup="/content-type/response-modification2" def response_modification2(): # $requestHandler resp = make_response("

    hello

    ") # $HttpResponse mimetype=text/html responseBody="

    hello

    " - resp.headers["content-type"] = "text/plain" # $ headerWriteNameUnsanitized="content-type" headerWriteValueSanitized="text/plain" MISSING: HttpResponse mimetype=text/plain + resp.headers["content-type"] = "text/plain" # $ headerWriteNameUnsanitized="content-type" headerWriteValue="text/plain" MISSING: HttpResponse mimetype=text/plain return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @@ -148,7 +148,7 @@ def Response3(): # $requestHandler @app.route("/content-type/Response4") # $routeSetup="/content-type/Response4" def Response4(): # $requestHandler # note: capitalization of Content-Type does not matter - resp = Response("

    hello

    ", headers={"Content-TYPE": "text/plain"}) # $ headerWriteBulk=Dict headerWriteNameUnsanitized headerWriteValueSanitized HttpResponse responseBody="

    hello

    " SPURIOUS: mimetype=text/html MISSING: mimetype=text/plain + resp = Response("

    hello

    ", headers={"Content-TYPE": "text/plain"}) # $ headerWriteBulk=Dict headerWriteBulkUnsanitized=name headerWriteNameUnsanitized="Content-TYPE" headerWriteValue="text/plain" HttpResponse responseBody="

    hello

    " SPURIOUS: mimetype=text/html MISSING: mimetype=text/plain return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @@ -156,7 +156,7 @@ def Response4(): # $requestHandler def Response5(): # $requestHandler # content_type argument takes priority (and result is text/plain) # note: capitalization of Content-Type does not matter - resp = Response("

    hello

    ", headers={"Content-TYPE": "text/html"}, content_type="text/plain; charset=utf-8") # $ headerWriteBulk=Dict headerWriteNameUnsanitized headerWriteValueSanitized HttpResponse mimetype=text/plain responseBody="

    hello

    " + resp = Response("

    hello

    ", headers={"Content-TYPE": "text/html"}, content_type="text/plain; charset=utf-8") # $ headerWriteBulk=Dict headerWriteBulkUnsanitized=name headerWriteNameUnsanitized="Content-TYPE" headerWriteValue="text/html" HttpResponse mimetype=text/plain responseBody="

    hello

    " return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @@ -164,7 +164,7 @@ def Response5(): # $requestHandler def Response6(): # $requestHandler # mimetype argument takes priority over header (and result is text/plain) # note: capitalization of Content-Type does not matter - resp = Response("

    hello

    ", headers={"Content-TYPE": "text/html"}, mimetype="text/plain") # $ headerWriteBulk=Dict headerWriteNameUnsanitized headerWriteValueSanitized HttpResponse mimetype=text/plain responseBody="

    hello

    " + resp = Response("

    hello

    ", headers={"Content-TYPE": "text/html"}, mimetype="text/plain") # $ headerWriteBulk=Dict headerWriteBulkUnsanitized=name headerWriteNameUnsanitized="Content-TYPE" headerWriteValue="text/html" HttpResponse mimetype=text/plain responseBody="

    hello

    " return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @@ -208,7 +208,7 @@ def setting_cookie(): # $requestHandler resp = make_response() # $ HttpResponse mimetype=text/html resp.set_cookie("key", "value") # $ CookieWrite CookieName="key" CookieValue="value" resp.set_cookie(key="key", value="value") # $ CookieWrite CookieName="key" CookieValue="value" - resp.headers.add("Set-Cookie", "key2=value2") # $ headerWriteNameUnsanitized="Set-Cookie" headerWriteValueSanitized="key2=value2" MISSING: CookieWrite CookieRawHeader="key2=value2" + resp.headers.add("Set-Cookie", "key2=value2") # $ headerWriteNameUnsanitized="Set-Cookie" headerWriteValue="key2=value2" MISSING: CookieWrite CookieRawHeader="key2=value2" resp.delete_cookie("key3") # $ CookieWrite CookieName="key3" resp.delete_cookie(key="key3") # $ CookieWrite CookieName="key3" return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @@ -220,29 +220,29 @@ def setting_cookie(): # $requestHandler @app.route("/headers") # $routeSetup="/headers" def headers(): # $requestHandler resp1 = Response() # $ HttpResponse mimetype=text/html - resp1.headers["X-MyHeader"] = "a" # $ headerWriteNameUnsanitized="X-MyHeader" headerWriteValueSanitized="a" + resp1.headers["X-MyHeader"] = "a" # $ headerWriteNameUnsanitized="X-MyHeader" headerWriteValue="a" resp2 = make_response() # $ HttpResponse mimetype=text/html - resp2.headers["X-MyHeader"] = "aa" # $ headerWriteNameUnsanitized="X-MyHeader" headerWriteValueSanitized="aa" - resp2.headers.extend({"X-MyHeader2": "b"}) # $ headerWriteBulk=Dict headerWriteNameUnsanitized headerWriteValueSanitized - resp3 = make_response("hello", 200, {"X-MyHeader3": "c"}) # $ HttpResponse mimetype=text/html responseBody="hello" headerWriteBulk=Dict headerWriteNameUnsanitized headerWriteValueSanitized - resp4 = make_response("hello", {"X-MyHeader4": "d"}) # $ HttpResponse mimetype=text/html responseBody="hello" headerWriteBulk=Dict headerWriteNameUnsanitized headerWriteValueSanitized - resp5 = Response(headers={"X-MyHeader5":"e"}) # $ HttpResponse mimetype=text/html headerWriteBulk=Dict headerWriteNameUnsanitized headerWriteValueSanitized + resp2.headers["X-MyHeader"] = "aa" # $ headerWriteNameUnsanitized="X-MyHeader" headerWriteValue="aa" + resp2.headers.extend({"X-MyHeader2": "b"}) # $ headerWriteBulk=Dict headerWriteBulkUnsanitized=name headerWriteNameUnsanitized="X-MyHeader2" headerWriteValue="b" + resp3 = make_response("hello", 200, {"X-MyHeader3": "c"}) # $ HttpResponse mimetype=text/html responseBody="hello" headerWriteBulk=Dict headerWriteBulkUnsanitized=name headerWriteNameUnsanitized="X-MyHeader3" headerWriteValue="c" + resp4 = make_response("hello", {"X-MyHeader4": "d"}) # $ HttpResponse mimetype=text/html responseBody="hello" headerWriteBulk=Dict headerWriteBulkUnsanitized=name headerWriteNameUnsanitized="X-MyHeader4" headerWriteValue="d" + resp5 = Response(headers={"X-MyHeader5":"e"}) # $ HttpResponse mimetype=text/html headerWriteBulk=Dict headerWriteBulkUnsanitized=name headerWriteBulkUnsanitized=name headerWriteNameUnsanitized="X-MyHeader5" headerWriteValue="e" return resp5 # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp5 @app.route("/werkzeug-headers") # $routeSetup="/werkzeug-headers" def werkzeug_headers(): # $requestHandler response = Response() # $ HttpResponse mimetype=text/html headers = Headers() - headers.add("X-MyHeader1", "a") # $ headerWriteNameUnsanitized="X-MyHeader1" headerWriteValueSanitized="a" - headers.add_header("X-MyHeader2", "b") # $ headerWriteNameUnsanitized="X-MyHeader2" headerWriteValueSanitized="b" - headers.set("X-MyHeader3", "c") # $ headerWriteNameUnsanitized="X-MyHeader3" headerWriteValueSanitized="c" - headers.setdefault("X-MyHeader4", "d") # $ headerWriteNameUnsanitized="X-MyHeader4" headerWriteValueSanitized="d" - headers.__setitem__("X-MyHeader5", "e") # $ headerWriteNameUnsanitized="X-MyHeader5" headerWriteValueSanitized="e" - headers["X-MyHeader6"] = "f" # $ headerWriteNameUnsanitized="X-MyHeader6" headerWriteValueSanitized="f" - h1 = {"X-MyHeader7": "g"} - headers.extend(h1) # $ headerWriteBulk=h1 headerWriteNameUnsanitized headerWriteValueSanitized - h2 = [("X-MyHeader8", "h")] - headers.extend(h2) # $ headerWriteBulk=h2 headerWriteNameUnsanitized headerWriteValueSanitized + headers.add("X-MyHeader1", "a") # $ headerWriteNameUnsanitized="X-MyHeader1" headerWriteValue="a" + headers.add_header("X-MyHeader2", "b") # $ headerWriteNameUnsanitized="X-MyHeader2" headerWriteValue="b" + headers.set("X-MyHeader3", "c") # $ headerWriteNameUnsanitized="X-MyHeader3" headerWriteValue="c" + headers.setdefault("X-MyHeader4", "d") # $ headerWriteNameUnsanitized="X-MyHeader4" headerWriteValue="d" + headers.__setitem__("X-MyHeader5", "e") # $ headerWriteNameUnsanitized="X-MyHeader5" headerWriteValue="e" + headers["X-MyHeader6"] = "f" # $ headerWriteNameUnsanitized="X-MyHeader6" headerWriteValue="f" + h1 = {"X-MyHeader7": "g"} # $ headerWriteNameUnsanitized="X-MyHeader7" headerWriteValue="g" + headers.extend(h1) # $ headerWriteBulk=h1 headerWriteBulkUnsanitized=name + h2 = [("X-MyHeader8", "h")] # $ headerWriteNameUnsanitized="X-MyHeader8" headerWriteValue="h" + headers.extend(h2) # $ headerWriteBulk=h2 headerWriteBulkUnsanitized=name response.headers = headers return response # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=response diff --git a/python/ql/test/library-tests/frameworks/stdlib/wsgiref_simple_server_test.py b/python/ql/test/library-tests/frameworks/stdlib/wsgiref_simple_server_test.py index 6a2031699f42..7327385c0647 100644 --- a/python/ql/test/library-tests/frameworks/stdlib/wsgiref_simple_server_test.py +++ b/python/ql/test/library-tests/frameworks/stdlib/wsgiref_simple_server_test.py @@ -18,7 +18,7 @@ def func(environ, start_response): # $ requestHandler environ, # $ tainted environ["PATH_INFO"], # $ tainted ) - write = start_response("200 OK", [("Content-Type", "text/plain")]) # $ headerWriteBulk=List headerWriteNameUnsanitized headerWriteValueUnsanitized + write = start_response("200 OK", [("Content-Type", "text/plain")]) # $ headerWriteBulk=List headerWriteBulkUnsanitized=name,value headerWriteNameUnsanitized="Content-Type" headerWriteValueUnsanitized="text/plain" write(b"hello") # $ HttpResponse responseBody=b"hello" write(data=b" ") # $ HttpResponse responseBody=b" " @@ -33,16 +33,16 @@ def __init__(self): self.set_app(self.my_method) def my_method(self, _env, start_response): # $ requestHandler - start_response("200 OK", []) # $ headerWriteBulk=List headerWriteNameUnsanitized headerWriteValueUnsanitized + start_response("200 OK", []) # $ headerWriteBulk=List headerWriteBulkUnsanitized=name,value return [b"my_method"] # $ HttpResponse responseBody=List def func2(environ, start_response): # $ requestHandler - headers = wsgiref.headers.Headers([("Content-Type", "text/plain")]) # $ headerWriteBulk=List headerWriteNameUnsanitized headerWriteValueUnsanitized + headers = wsgiref.headers.Headers([("Content-Type", "text/plain")]) # $ headerWriteBulk=List headerWriteBulkUnsanitized=name,value headerWriteNameUnsanitized="Content-Type" headerWriteValueUnsanitized="text/plain" headers.add_header("X-MyHeader", "a") # $ headerWriteNameUnsanitized="X-MyHeader" headerWriteValueUnsanitized="a" headers.setdefault("X-MyHeader2", "b") # $ headerWriteNameUnsanitized="X-MyHeader2" headerWriteValueUnsanitized="b" headers.__setitem__("X-MyHeader3", "c") # $ headerWriteNameUnsanitized="X-MyHeader3" headerWriteValueUnsanitized="c" headers["X-MyHeader4"] = "d" # $ headerWriteNameUnsanitized="X-MyHeader4" headerWriteValueUnsanitized="d" - start_response(status, headers) # $ headerWriteBulk=headers headerWriteNameUnsanitized headerWriteValueUnsanitized + start_response(status, headers) # $ headerWriteBulk=headers headerWriteBulkUnsanitized=name,value return [b"Hello"] # $ HttpResponse responseBody=List case = sys.argv[1] @@ -54,7 +54,7 @@ def func2(environ, start_response): # $ requestHandler elif case == "3": server = MyServer() def func3(_env, start_response): # $ requestHandler - start_response("200 OK", []) # $ headerWriteBulk=List headerWriteNameUnsanitized headerWriteValueUnsanitized + start_response("200 OK", []) # $ headerWriteBulk=List headerWriteBulkUnsanitized=name,value return [b"foo"] # $ HttpResponse responseBody=List server.set_app(func3) elif case == "4": From a0201e9c4ff6e04fad195750d98dc8591ae1d5f6 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Thu, 6 Jun 2024 15:19:02 +0100 Subject: [PATCH 028/281] Update tests for new cookie write from headers --- python/ql/lib/semmle/python/Concepts.qll | 4 +--- .../ql/test/library-tests/frameworks/flask/response_test.py | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll index c351a7dceedb..74e06b54b0be 100644 --- a/python/ql/lib/semmle/python/Concepts.qll +++ b/python/ql/lib/semmle/python/Concepts.qll @@ -1246,9 +1246,7 @@ module Http { ) } - override DataFlow::Node getNameArg() { - result = this.(Http::Server::ResponseHeaderWrite).getValueArg() - } + override DataFlow::Node getNameArg() { none() } override DataFlow::Node getHeaderArg() { result = this.(Http::Server::ResponseHeaderWrite).getValueArg() diff --git a/python/ql/test/library-tests/frameworks/flask/response_test.py b/python/ql/test/library-tests/frameworks/flask/response_test.py index bcfc36ff365e..a1341022c4ec 100644 --- a/python/ql/test/library-tests/frameworks/flask/response_test.py +++ b/python/ql/test/library-tests/frameworks/flask/response_test.py @@ -208,7 +208,7 @@ def setting_cookie(): # $requestHandler resp = make_response() # $ HttpResponse mimetype=text/html resp.set_cookie("key", "value") # $ CookieWrite CookieName="key" CookieValue="value" resp.set_cookie(key="key", value="value") # $ CookieWrite CookieName="key" CookieValue="value" - resp.headers.add("Set-Cookie", "key2=value2") # $ headerWriteNameUnsanitized="Set-Cookie" headerWriteValue="key2=value2" MISSING: CookieWrite CookieRawHeader="key2=value2" + resp.headers.add("Set-Cookie", "key2=value2") # $ headerWriteNameUnsanitized="Set-Cookie" headerWriteValue="key2=value2" CookieWrite CookieRawHeader="key2=value2" resp.delete_cookie("key3") # $ CookieWrite CookieName="key3" resp.delete_cookie(key="key3") # $ CookieWrite CookieName="key3" return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp From 7704801e47a8c10a7597546b51f7e5f17bf1a93f Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 21 Jun 2024 09:09:14 +0100 Subject: [PATCH 029/281] Change fastapi raw cookie header models to header write models --- python/ql/lib/semmle/python/Concepts.qll | 2 +- .../lib/semmle/python/frameworks/FastApi.qll | 21 +++++++++---------- .../frameworks/fastapi/response_test.py | 8 +++---- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll index 74e06b54b0be..20578e26960f 100644 --- a/python/ql/lib/semmle/python/Concepts.qll +++ b/python/ql/lib/semmle/python/Concepts.qll @@ -1239,7 +1239,7 @@ module Http { { CookieHeaderWrite() { exists(StringLiteral str | - str.getText() = "Set-Cookie" and + str.getText().toLowerCase() = "set-cookie" and DataFlow::exprNode(str) .(DataFlow::LocalSourceNode) .flowsTo(this.(Http::Server::ResponseHeaderWrite).getNameArg()) diff --git a/python/ql/lib/semmle/python/frameworks/FastApi.qll b/python/ql/lib/semmle/python/frameworks/FastApi.qll index 8c958e9343db..423f8580a5b9 100644 --- a/python/ql/lib/semmle/python/frameworks/FastApi.qll +++ b/python/ql/lib/semmle/python/frameworks/FastApi.qll @@ -361,28 +361,27 @@ module FastApi { } /** - * A call to `append` on a `headers` of a FastAPI Response, with the `Set-Cookie` - * header-key. + * A call to `append` on a `headers` of a FastAPI Response. */ - private class HeadersAppendCookie extends Http::Server::CookieWrite::Range, + private class HeadersAppend extends Http::Server::ResponseHeaderWrite::Range, DataFlow::MethodCallNode { - HeadersAppendCookie() { - exists(DataFlow::AttrRead headers, DataFlow::Node keyArg | + HeadersAppend() { + exists(DataFlow::AttrRead headers | headers.accesses(instance(), "headers") and - this.calls(headers, "append") and - keyArg in [this.getArg(0), this.getArgByName("key")] and - keyArg.getALocalSource().asExpr().(StringLiteral).getText().toLowerCase() = "set-cookie" + this.calls(headers, "append") ) } - override DataFlow::Node getHeaderArg() { + override DataFlow::Node getNameArg() { result = [this.getArg(0), this.getArgByName("key")] } + + override DataFlow::Node getValueArg() { result in [this.getArg(1), this.getArgByName("value")] } - override DataFlow::Node getNameArg() { none() } + override predicate nameAllowsNewline() { none() } - override DataFlow::Node getValueArg() { none() } + override predicate valueAllowsNewline() { none() } } } } diff --git a/python/ql/test/library-tests/frameworks/fastapi/response_test.py b/python/ql/test/library-tests/frameworks/fastapi/response_test.py index 9f276338c8cc..44582d6cd6ef 100644 --- a/python/ql/test/library-tests/frameworks/fastapi/response_test.py +++ b/python/ql/test/library-tests/frameworks/fastapi/response_test.py @@ -11,9 +11,9 @@ async def response_parameter(response: Response): # $ requestHandler response.set_cookie("key", "value") # $ CookieWrite CookieName="key" CookieValue="value" response.set_cookie(key="key", value="value") # $ CookieWrite CookieName="key" CookieValue="value" - response.headers.append("Set-Cookie", "key2=value2") # $ CookieWrite CookieRawHeader="key2=value2" - response.headers.append(key="Set-Cookie", value="key2=value2") # $ CookieWrite CookieRawHeader="key2=value2" - response.headers["X-MyHeader"] = "header-value" + response.headers.append("Set-Cookie", "key2=value2") # $ headerWriteName="Set-Cookie" headerWriteValue="key2=value2" CookieWrite CookieRawHeader="key2=value2" + response.headers.append(key="Set-Cookie", value="key2=value2") # $ headerWriteName="Set-Cookie" headerWriteValue="key2=value2" CookieWrite CookieRawHeader="key2=value2" + response.headers["X-MyHeader"] = "header-value" # $ MISSING: headerWriteName="X-MyHeader" headerWriteValue="header-value" response.status_code = 418 return {"message": "response as parameter"} # $ HttpResponse mimetype=application/json responseBody=Dict @@ -45,7 +45,7 @@ async def response_parameter_custom_type(response: MyXmlResponse): # $ requestHa print(type(response)) assert type(response) == fastapi.responses.Response response.set_cookie("key", "value") # $ CookieWrite CookieName="key" CookieValue="value" - response.headers["Custom-Response-Type"] = "yes, but only after function has run" + response.headers["Custom-Response-Type"] = "yes, but only after function has run" # $ MISSING: headerWriteName="Custom-Response-Typer" headerWriteValue="yes, but only after function has run" xml_data = "FOO" return xml_data # $ HttpResponse responseBody=xml_data mimetype=application/xml From 5ced5c010c9450a6d393652e60c3e7ffe97abddd Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 21 Jun 2024 11:29:20 +0100 Subject: [PATCH 030/281] Add django header writes --- .../lib/semmle/python/frameworks/Django.qll | 31 +++++++++++++++++++ .../frameworks/django-v2-v3/response_test.py | 4 +-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/Django.qll b/python/ql/lib/semmle/python/frameworks/Django.qll index 064dba57f92a..7c0befa6349d 100644 --- a/python/ql/lib/semmle/python/frameworks/Django.qll +++ b/python/ql/lib/semmle/python/frameworks/Django.qll @@ -2239,6 +2239,37 @@ module PrivateDjango { override DataFlow::Node getValueArg() { result = value } } + + class DjangoResponseHeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range { + DataFlow::Node index; + DataFlow::Node value; + + DjangoResponseHeaderSubscriptWrite() { + exists(SubscriptNode subscript, DataFlow::AttrRead headerLookup | + // To give `this` a value, we need to choose between either LHS or RHS, + // and just go with the LHS + this.asCfgNode() = subscript + | + headerLookup + .accesses(DjangoImpl::DjangoHttp::Response::HttpResponse::instance(), "headers") and + exists(DataFlow::Node subscriptObj | + subscriptObj.asCfgNode() = subscript.getObject() + | + headerLookup.flowsTo(subscriptObj) + ) and + value.asCfgNode() = subscript.(DefinitionNode).getValue() and + index.asCfgNode() = subscript.getIndex() + ) + } + + override DataFlow::Node getNameArg() { result = index } + + override DataFlow::Node getValueArg() { result = value } + + override predicate nameAllowsNewline() { none() } + + override predicate valueAllowsNewline() { none() } + } } } diff --git a/python/ql/test/library-tests/frameworks/django-v2-v3/response_test.py b/python/ql/test/library-tests/frameworks/django-v2-v3/response_test.py index dd78cd510168..d4f17f7c3cf7 100644 --- a/python/ql/test/library-tests/frameworks/django-v2-v3/response_test.py +++ b/python/ql/test/library-tests/frameworks/django-v2-v3/response_test.py @@ -72,7 +72,7 @@ def redirect_through_normal_response_new_headers_attr(request): resp = HttpResponse() # $ HttpResponse mimetype=text/html resp.status_code = 302 - resp.headers['Location'] = next # $ MISSING: redirectLocation=next + resp.headers['Location'] = next # $ headerWriteName='Location' headerWriteValue=next MISSING: redirectLocation=next resp.content = private # $ MISSING: responseBody=private return resp @@ -130,7 +130,7 @@ def setting_cookie(request): resp = HttpResponse() # $ HttpResponse mimetype=text/html resp.set_cookie("key", "value") # $ CookieWrite CookieName="key" CookieValue="value" resp.set_cookie(key="key", value="value") # $ CookieWrite CookieName="key" CookieValue="value" - resp.headers["Set-Cookie"] = "key2=value2" # $ MISSING: CookieWrite CookieRawHeader="key2=value2" + resp.headers["Set-Cookie"] = "key2=value2" # $ headerWriteName="Set-Cookie" headerWriteValue="key2=value2" CookieWrite CookieRawHeader="key2=value2" resp.cookies["key3"] = "value3" # $ CookieWrite CookieName="key3" CookieValue="value3" resp.delete_cookie("key4") # $ CookieWrite CookieName="key4" resp.delete_cookie(key="key4") # $ CookieWrite CookieName="key4" From 79c0ed607487f7c59d903113f576dfdfde60bcca Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 21 Jun 2024 14:01:33 +0100 Subject: [PATCH 031/281] Add additional fastapi mheader write models --- .../lib/semmle/python/frameworks/FastApi.qll | 28 +++++++++++++++++++ .../frameworks/fastapi/response_test.py | 4 +-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/FastApi.qll b/python/ql/lib/semmle/python/frameworks/FastApi.qll index 423f8580a5b9..028c5f266010 100644 --- a/python/ql/lib/semmle/python/frameworks/FastApi.qll +++ b/python/ql/lib/semmle/python/frameworks/FastApi.qll @@ -383,5 +383,33 @@ module FastApi { override predicate valueAllowsNewline() { none() } } + + class HeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range { + DataFlow::Node index; + DataFlow::Node value; + + HeaderSubscriptWrite() { + exists(SubscriptNode subscript, DataFlow::AttrRead headerLookup | + // To give `this` a value, we need to choose between either LHS or RHS, + // and just go with the LHS + this.asCfgNode() = subscript + | + headerLookup.accesses(instance(), "headers") and + exists(DataFlow::Node subscriptObj | subscriptObj.asCfgNode() = subscript.getObject() | + headerLookup.flowsTo(subscriptObj) + ) and + value.asCfgNode() = subscript.(DefinitionNode).getValue() and + index.asCfgNode() = subscript.getIndex() + ) + } + + override DataFlow::Node getNameArg() { result = index } + + override DataFlow::Node getValueArg() { result = value } + + override predicate nameAllowsNewline() { none() } + + override predicate valueAllowsNewline() { none() } + } } } diff --git a/python/ql/test/library-tests/frameworks/fastapi/response_test.py b/python/ql/test/library-tests/frameworks/fastapi/response_test.py index 44582d6cd6ef..2bc26c15fdac 100644 --- a/python/ql/test/library-tests/frameworks/fastapi/response_test.py +++ b/python/ql/test/library-tests/frameworks/fastapi/response_test.py @@ -13,7 +13,7 @@ async def response_parameter(response: Response): # $ requestHandler response.set_cookie(key="key", value="value") # $ CookieWrite CookieName="key" CookieValue="value" response.headers.append("Set-Cookie", "key2=value2") # $ headerWriteName="Set-Cookie" headerWriteValue="key2=value2" CookieWrite CookieRawHeader="key2=value2" response.headers.append(key="Set-Cookie", value="key2=value2") # $ headerWriteName="Set-Cookie" headerWriteValue="key2=value2" CookieWrite CookieRawHeader="key2=value2" - response.headers["X-MyHeader"] = "header-value" # $ MISSING: headerWriteName="X-MyHeader" headerWriteValue="header-value" + response.headers["X-MyHeader"] = "header-value" # $ headerWriteName="X-MyHeader" headerWriteValue="header-value" response.status_code = 418 return {"message": "response as parameter"} # $ HttpResponse mimetype=application/json responseBody=Dict @@ -45,7 +45,7 @@ async def response_parameter_custom_type(response: MyXmlResponse): # $ requestHa print(type(response)) assert type(response) == fastapi.responses.Response response.set_cookie("key", "value") # $ CookieWrite CookieName="key" CookieValue="value" - response.headers["Custom-Response-Type"] = "yes, but only after function has run" # $ MISSING: headerWriteName="Custom-Response-Typer" headerWriteValue="yes, but only after function has run" + response.headers["Custom-Response-Type"] = "yes, but only after function has run" # $ headerWriteName="Custom-Response-Type" headerWriteValue="yes, but only after function has run" xml_data = "FOO" return xml_data # $ HttpResponse responseBody=xml_data mimetype=application/xml From c404f00a9b228366393a2bf15939b4cf76a10a7f Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 24 Jun 2024 10:41:07 +0100 Subject: [PATCH 032/281] Add additional header write models for aiohttp and tornado + added qldoc --- .../lib/semmle/python/frameworks/Aiohttp.qll | 27 ++++++++ .../lib/semmle/python/frameworks/Django.qll | 4 ++ .../lib/semmle/python/frameworks/FastApi.qll | 4 ++ .../lib/semmle/python/frameworks/Tornado.qll | 63 +++++++++++++++++++ .../frameworks/aiohttp/response_test.py | 2 +- .../frameworks/tornado/response_test.py | 9 ++- 6 files changed, 105 insertions(+), 4 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/Aiohttp.qll b/python/ql/lib/semmle/python/frameworks/Aiohttp.qll index 78d269c31d31..517b309594aa 100644 --- a/python/ql/lib/semmle/python/frameworks/Aiohttp.qll +++ b/python/ql/lib/semmle/python/frameworks/Aiohttp.qll @@ -706,6 +706,33 @@ module AiohttpWebModel { override DataFlow::Node getValueArg() { result = value } } + + /** + * A dict-like write to an item of the `headers` attribute on a HTTP response, such as + * `response.headers[name] = value`. + */ + class AiohttpResponseHeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range { + DataFlow::Node index; + DataFlow::Node value; + + AiohttpResponseHeaderSubscriptWrite() { + exists(API::Node i | + value = aiohttpResponseInstance().getMember("headers").getSubscriptAt(i).asSink() and + index = i.asSink() and + // To give `this` a value, we need to choose between either LHS or RHS, + // and just go with the RHS as it is readily available + this = value + ) + } + + override DataFlow::Node getNameArg() { result = index } + + override DataFlow::Node getValueArg() { result = value } + + override predicate nameAllowsNewline() { none() } + + override predicate valueAllowsNewline() { none() } + } } /** diff --git a/python/ql/lib/semmle/python/frameworks/Django.qll b/python/ql/lib/semmle/python/frameworks/Django.qll index 7c0befa6349d..69b0e8710daf 100644 --- a/python/ql/lib/semmle/python/frameworks/Django.qll +++ b/python/ql/lib/semmle/python/frameworks/Django.qll @@ -2240,6 +2240,10 @@ module PrivateDjango { override DataFlow::Node getValueArg() { result = value } } + /** + * A dict-like write to an item of the `headers` attribute on a HTTP response, such as + * `response.headers[name] = value`. + */ class DjangoResponseHeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range { DataFlow::Node index; DataFlow::Node value; diff --git a/python/ql/lib/semmle/python/frameworks/FastApi.qll b/python/ql/lib/semmle/python/frameworks/FastApi.qll index 028c5f266010..0793b4b7d6ab 100644 --- a/python/ql/lib/semmle/python/frameworks/FastApi.qll +++ b/python/ql/lib/semmle/python/frameworks/FastApi.qll @@ -384,6 +384,10 @@ module FastApi { override predicate valueAllowsNewline() { none() } } + /** + * A dict-like write to an item of the `headers` attribute on a HTTP response, such as + * `response.headers[name] = value`. + */ class HeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range { DataFlow::Node index; DataFlow::Node value; diff --git a/python/ql/lib/semmle/python/frameworks/Tornado.qll b/python/ql/lib/semmle/python/frameworks/Tornado.qll index 1bd406032962..7bc4cf25794d 100644 --- a/python/ql/lib/semmle/python/frameworks/Tornado.qll +++ b/python/ql/lib/semmle/python/frameworks/Tornado.qll @@ -63,6 +63,50 @@ module Tornado { override string getAsyncMethodName() { none() } } + + /** + * A dict-like write to an item of an `HTTPHeaders` object. + */ + private class TornadoHeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range { + DataFlow::Node index; + DataFlow::Node value; + + TornadoHeaderSubscriptWrite() { + exists(SubscriptNode subscript | + subscript.getObject() = instance().asCfgNode() and + value.asCfgNode() = subscript.(DefinitionNode).getValue() and + index.asCfgNode() = subscript.getIndex() and + this.asCfgNode() = subscript + ) + } + + override DataFlow::Node getNameArg() { result = index } + + override DataFlow::Node getValueArg() { result = value } + + override predicate nameAllowsNewline() { none() } + + override predicate valueAllowsNewline() { none() } + } + + /** + * A call to `HTTPHeaders.add`. + */ + private class TornadoHeadersAppendCall extends Http::Server::ResponseHeaderWrite::Range, + DataFlow::MethodCallNode + { + TornadoHeadersAppendCall() { this.calls(instance(), "append") } + + override DataFlow::Node getNameArg() { result = [this.getArg(0), this.getArgByName("name")] } + + override DataFlow::Node getValueArg() { + result in [this.getArg(1), this.getArgByName("value")] + } + + override predicate nameAllowsNewline() { none() } + + override predicate valueAllowsNewline() { none() } + } } // --------------------------------------------------------------------------- @@ -209,6 +253,25 @@ module Tornado { this.(DataFlow::AttrRead).getAttributeName() = "request" } } + + /** A call to `RequestHandler.set_header` or `RequestHandler.add_header` */ + private class TornadoSetHeaderCall extends Http::Server::ResponseHeaderWrite::Range, + DataFlow::MethodCallNode + { + TornadoSetHeaderCall() { this.calls(instance(), ["set_header", "add_header"]) } + + override DataFlow::Node getNameArg() { + result = [this.getArg(0), this.getArgByName("name")] + } + + override DataFlow::Node getValueArg() { + result in [this.getArg(1), this.getArgByName("value")] + } + + override predicate nameAllowsNewline() { none() } + + override predicate valueAllowsNewline() { none() } + } } /** diff --git a/python/ql/test/library-tests/frameworks/aiohttp/response_test.py b/python/ql/test/library-tests/frameworks/aiohttp/response_test.py index bc9bc8d3bda2..a40bf0bbc65e 100644 --- a/python/ql/test/library-tests/frameworks/aiohttp/response_test.py +++ b/python/ql/test/library-tests/frameworks/aiohttp/response_test.py @@ -96,7 +96,7 @@ async def streaming_response(request): # $ requestHandler async def setting_cookie(request): # $ requestHandler resp = web.Response(text="foo") # $ HttpResponse mimetype=text/plain responseBody="foo" resp.cookies["key"] = "value" # $ CookieWrite CookieName="key" CookieValue="value" - resp.headers["Set-Cookie"] = "key2=value2" # $ MISSING: CookieWrite CookieRawHeader="key2=value2" + resp.headers["Set-Cookie"] = "key2=value2" # $ headerWriteName="Set-Cookie" headerWriteValue="key2=value2" CookieWrite CookieRawHeader="key2=value2" resp.set_cookie("key3", "value3") # $ CookieWrite CookieName="key3" CookieValue="value3" resp.set_cookie(name="key3", value="value3") # $ CookieWrite CookieName="key3" CookieValue="value3" resp.del_cookie("key4") # $ CookieWrite CookieName="key4" diff --git a/python/ql/test/library-tests/frameworks/tornado/response_test.py b/python/ql/test/library-tests/frameworks/tornado/response_test.py index 1ca37ed773c8..1685ac4d158d 100644 --- a/python/ql/test/library-tests/frameworks/tornado/response_test.py +++ b/python/ql/test/library-tests/frameworks/tornado/response_test.py @@ -24,10 +24,10 @@ def get(self): # $ requestHandler # what matters. self.write("foo") # $ HttpResponse mimetype=text/html responseBody="foo" - self.set_header("Content-Type", "text/plain; charset=utf-8") + self.set_header("Content-Type", "text/plain; charset=utf-8") # $ headerWriteName="Content-Type" headerWriteValue="text/plain; charset=utf-8" def post(self): # $ requestHandler - self.set_header("Content-Type", "text/plain; charset=utf-8") + self.set_header("Content-Type", "text/plain; charset=utf-8") # $ headerWriteName="Content-Type" headerWriteValue="text/plain; charset=utf-8" self.write("foo") # $ HttpResponse responseBody="foo" MISSING: mimetype=text/plain SPURIOUS: mimetype=text/html @@ -67,7 +67,10 @@ def get(self): # $ requestHandler self.write("foo") # $ HttpResponse mimetype=text/html responseBody="foo" self.set_cookie("key", "value") # $ CookieWrite CookieName="key" CookieValue="value" self.set_cookie(name="key", value="value") # $ CookieWrite CookieName="key" CookieValue="value" - self.set_header("Set-Cookie", "key2=value2") # $ MISSING: CookieWrite CookieRawHeader="key2=value2" + self.set_header("Set-Cookie", "key2=value2") # $ headerWriteName="Set-Cookie" headerWriteValue="key2=value2" CookieWrite CookieRawHeader="key2=value2" + self.add_header("Set-Cookie", "key3=value3") # $ headerWriteName="Set-Cookie" headerWriteValue="key3=value3" CookieWrite CookieRawHeader="key3=value3" + self.request.headers.append("Set-Cookie", "key4=value4") # $ headerWriteName="Set-Cookie" headerWriteValue="key4=value4" CookieWrite CookieRawHeader="key4=value4" + self.request.headers["Set-Cookie"] = "key5=value5" # $ headerWriteName="Set-Cookie" headerWriteValue="key5=value5" CookieWrite CookieRawHeader="key5=value5" def make_app(): From d0f735ac28c9747ff2bc7a6f20a5092daf667da4 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 24 Jun 2024 20:52:09 +0100 Subject: [PATCH 033/281] Update tests for restframework --- .../library-tests/frameworks/rest_framework/response_test.py | 2 +- .../library-tests/frameworks/rest_framework/testapp/views.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/test/library-tests/frameworks/rest_framework/response_test.py b/python/ql/test/library-tests/frameworks/rest_framework/response_test.py index ec093499df63..3e4f821693bf 100644 --- a/python/ql/test/library-tests/frameworks/rest_framework/response_test.py +++ b/python/ql/test/library-tests/frameworks/rest_framework/response_test.py @@ -28,7 +28,7 @@ def setting_cookie(request): resp = Response() # $ HttpResponse resp.set_cookie("key", "value") # $ CookieWrite CookieName="key" CookieValue="value" resp.set_cookie(key="key4", value="value") # $ CookieWrite CookieName="key4" CookieValue="value" - resp.headers["Set-Cookie"] = "key2=value2" # $ MISSING: CookieWrite CookieRawHeader="key2=value2" + resp.headers["Set-Cookie"] = "key2=value2" # $ headerWriteName="Set-Cookie" headerWriteValue="key2=value2" CookieWrite CookieRawHeader="key2=value2" resp.cookies["key3"] = "value3" # $ CookieWrite CookieName="key3" CookieValue="value3" resp.delete_cookie("key4") # $ CookieWrite CookieName="key4" resp.delete_cookie(key="key4") # $ CookieWrite CookieName="key4" diff --git a/python/ql/test/library-tests/frameworks/rest_framework/testapp/views.py b/python/ql/test/library-tests/frameworks/rest_framework/testapp/views.py index 6affb5dac4b9..6ce06fdba31e 100644 --- a/python/ql/test/library-tests/frameworks/rest_framework/testapp/views.py +++ b/python/ql/test/library-tests/frameworks/rest_framework/testapp/views.py @@ -70,7 +70,7 @@ def cookie_test(request: Request): # $ requestHandler resp = Response("wat") # $ HttpResponse resp.set_cookie("key", "value") # $ CookieWrite CookieName="key" CookieValue="value" resp.set_cookie(key="key4", value="value") # $ CookieWrite CookieName="key4" CookieValue="value" - resp.headers["Set-Cookie"] = "key2=value2" # $ MISSING: CookieWrite CookieRawHeader="key2=value2" + resp.headers["Set-Cookie"] = "key2=value2" # $ headerWriteName="Set-Cookie" headerWriteValue="key2=value2" CookieWrite CookieRawHeader="key2=value2" resp.cookies["key3"] = "value3" # $ CookieWrite CookieName="key3" CookieValue="value3" return resp From 0901b3d0a67aa4836161c9d188b706f38d5a1346 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 24 Jun 2024 21:43:09 +0100 Subject: [PATCH 034/281] Add change note --- python/ql/lib/change-notes/2024-06-24-cookie-header-writes.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 python/ql/lib/change-notes/2024-06-24-cookie-header-writes.md diff --git a/python/ql/lib/change-notes/2024-06-24-cookie-header-writes.md b/python/ql/lib/change-notes/2024-06-24-cookie-header-writes.md new file mode 100644 index 000000000000..583e0f44c059 --- /dev/null +++ b/python/ql/lib/change-notes/2024-06-24-cookie-header-writes.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Additional modelling has been added to detect cookie writes from direct writes to the `Set-Cookie` header have been added for several web frameworks. \ No newline at end of file From 6538d22d3f32dce3a8762bbe15b1621e3c34a556 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Wed, 26 Jun 2024 09:21:53 +0100 Subject: [PATCH 035/281] Fix tornado model of httheaders.add. --- python/ql/lib/semmle/python/frameworks/Tornado.qll | 2 +- .../ql/test/library-tests/frameworks/tornado/response_test.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/Tornado.qll b/python/ql/lib/semmle/python/frameworks/Tornado.qll index 7bc4cf25794d..214f2fb9bad3 100644 --- a/python/ql/lib/semmle/python/frameworks/Tornado.qll +++ b/python/ql/lib/semmle/python/frameworks/Tornado.qll @@ -95,7 +95,7 @@ module Tornado { private class TornadoHeadersAppendCall extends Http::Server::ResponseHeaderWrite::Range, DataFlow::MethodCallNode { - TornadoHeadersAppendCall() { this.calls(instance(), "append") } + TornadoHeadersAppendCall() { this.calls(instance(), "add") } override DataFlow::Node getNameArg() { result = [this.getArg(0), this.getArgByName("name")] } diff --git a/python/ql/test/library-tests/frameworks/tornado/response_test.py b/python/ql/test/library-tests/frameworks/tornado/response_test.py index 1685ac4d158d..a1054f28dc96 100644 --- a/python/ql/test/library-tests/frameworks/tornado/response_test.py +++ b/python/ql/test/library-tests/frameworks/tornado/response_test.py @@ -69,7 +69,7 @@ def get(self): # $ requestHandler self.set_cookie(name="key", value="value") # $ CookieWrite CookieName="key" CookieValue="value" self.set_header("Set-Cookie", "key2=value2") # $ headerWriteName="Set-Cookie" headerWriteValue="key2=value2" CookieWrite CookieRawHeader="key2=value2" self.add_header("Set-Cookie", "key3=value3") # $ headerWriteName="Set-Cookie" headerWriteValue="key3=value3" CookieWrite CookieRawHeader="key3=value3" - self.request.headers.append("Set-Cookie", "key4=value4") # $ headerWriteName="Set-Cookie" headerWriteValue="key4=value4" CookieWrite CookieRawHeader="key4=value4" + self.request.headers.add("Set-Cookie", "key4=value4") # $ headerWriteName="Set-Cookie" headerWriteValue="key4=value4" CookieWrite CookieRawHeader="key4=value4" self.request.headers["Set-Cookie"] = "key5=value5" # $ headerWriteName="Set-Cookie" headerWriteValue="key5=value5" CookieWrite CookieRawHeader="key5=value5" From f22778960bfdaa674adff0d1fac1392350048f93 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Wed, 26 Jun 2024 11:31:57 +0100 Subject: [PATCH 036/281] Fixed expected test results for Helmet query --- .../test/query-tests/Security/CWE-693/InsecureHelmet.expected | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmet.expected b/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmet.expected index 7368d96f3d43..2c9407136aa4 100644 --- a/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmet.expected +++ b/javascript/ql/test/query-tests/Security/CWE-693/InsecureHelmet.expected @@ -1,2 +1,2 @@ -| InsecureHelmetBad.js:7:5:7:32 | content ... : false | Helmet route handler, called with $@ set to 'false' | InsecureHelmetBad.js:7:5:7:32 | content ... : false | contentSecurityPolicy | -| InsecureHelmetBad.js:8:5:8:21 | frameguard: false | Helmet route handler, called with $@ set to 'false' | InsecureHelmetBad.js:8:5:8:21 | frameguard: false | frameguard | +| InsecureHelmetBad.js:6:9:9:2 | helmet( ... uard\\n}) | Helmet security middleware, configured with security setting $@ set to 'false', which disables enforcing that feature. | InsecureHelmetBad.js:7:5:7:32 | content ... : false | contentSecurityPolicy | +| InsecureHelmetBad.js:6:9:9:2 | helmet( ... uard\\n}) | Helmet security middleware, configured with security setting $@ set to 'false', which disables enforcing that feature. | InsecureHelmetBad.js:8:5:8:21 | frameguard: false | frameguard | From b81d41ba7b64c81717ff49ce51be293ff344a0e5 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 1 Jul 2024 11:26:54 +0100 Subject: [PATCH 037/281] Add django header write models for direct subscript write --- .../lib/semmle/python/frameworks/Django.qll | 30 +++++++++++++++++++ .../Security/CWE-614/CookieInjection.expected | 11 +++++++ .../Security/CWE-614/InsecureCookie.expected | 6 ++++ .../Security/CWE-614/django_bad.py | 4 +-- .../frameworks/django-v2-v3/response_test.py | 3 +- 5 files changed, 51 insertions(+), 3 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/Django.qll b/python/ql/lib/semmle/python/frameworks/Django.qll index 69b0e8710daf..b3b027e48ffe 100644 --- a/python/ql/lib/semmle/python/frameworks/Django.qll +++ b/python/ql/lib/semmle/python/frameworks/Django.qll @@ -2274,6 +2274,36 @@ module PrivateDjango { override predicate valueAllowsNewline() { none() } } + + /** + * A dict-like write to an item of an HTTP response, which is treated as a header write, + * such as `response[headerName] = value` + */ + class DjangoResponseSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range { + DataFlow::Node index; + DataFlow::Node value; + + DjangoResponseSubscriptWrite() { + exists(SubscriptNode subscript | + // To give `this` a value, we need to choose between either LHS or RHS, + // and just go with the LHS + this.asCfgNode() = subscript + | + subscript.getObject() = + DjangoImpl::DjangoHttp::Response::HttpResponse::instance().asCfgNode() and + value.asCfgNode() = subscript.(DefinitionNode).getValue() and + index.asCfgNode() = subscript.getIndex() + ) + } + + override DataFlow::Node getNameArg() { result = index } + + override DataFlow::Node getValueArg() { result = value } + + override predicate nameAllowsNewline() { none() } + + override predicate valueAllowsNewline() { none() } + } } } diff --git a/python/ql/test/experimental/query-tests/Security/CWE-614/CookieInjection.expected b/python/ql/test/experimental/query-tests/Security/CWE-614/CookieInjection.expected index 1b3120c8546c..80dcbae21773 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-614/CookieInjection.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-614/CookieInjection.expected @@ -1,4 +1,6 @@ edges +| django_bad.py:27:33:27:67 | ControlFlowNode for Attribute() | django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | provenance | | +| django_bad.py:27:71:27:106 | ControlFlowNode for Attribute() | django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | provenance | | | flask_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | flask_bad.py:1:26:1:32 | ControlFlowNode for request | provenance | | | flask_bad.py:1:26:1:32 | ControlFlowNode for request | flask_bad.py:24:21:24:27 | ControlFlowNode for request | provenance | | | flask_bad.py:1:26:1:32 | ControlFlowNode for request | flask_bad.py:24:49:24:55 | ControlFlowNode for request | provenance | | @@ -12,6 +14,9 @@ edges nodes | django_bad.py:19:21:19:55 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | semmle.label | ControlFlowNode for Fstring | +| django_bad.py:27:33:27:67 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| django_bad.py:27:71:27:106 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | flask_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember | | flask_bad.py:1:26:1:32 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | | flask_bad.py:24:21:24:27 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | @@ -29,6 +34,12 @@ subpaths | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | Cookie is constructed from a $@,and its httponly flag is not properly set. | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | user-supplied input | | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | Cookie is constructed from a $@,and its samesite flag is not properly set. | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | user-supplied input | | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | Cookie is constructed from a $@,and its secure flag is not properly set. | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | user-supplied input | +| django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | django_bad.py:27:33:27:67 | ControlFlowNode for Attribute() | django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | Cookie is constructed from a $@,and its httponly flag is not properly set. | django_bad.py:27:33:27:67 | ControlFlowNode for Attribute() | user-supplied input | +| django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | django_bad.py:27:33:27:67 | ControlFlowNode for Attribute() | django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | Cookie is constructed from a $@,and its samesite flag is not properly set. | django_bad.py:27:33:27:67 | ControlFlowNode for Attribute() | user-supplied input | +| django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | django_bad.py:27:33:27:67 | ControlFlowNode for Attribute() | django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | Cookie is constructed from a $@,and its secure flag is not properly set. | django_bad.py:27:33:27:67 | ControlFlowNode for Attribute() | user-supplied input | +| django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | django_bad.py:27:71:27:106 | ControlFlowNode for Attribute() | django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | Cookie is constructed from a $@,and its httponly flag is not properly set. | django_bad.py:27:71:27:106 | ControlFlowNode for Attribute() | user-supplied input | +| django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | django_bad.py:27:71:27:106 | ControlFlowNode for Attribute() | django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | Cookie is constructed from a $@,and its samesite flag is not properly set. | django_bad.py:27:71:27:106 | ControlFlowNode for Attribute() | user-supplied input | +| django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | django_bad.py:27:71:27:106 | ControlFlowNode for Attribute() | django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | Cookie is constructed from a $@,and its secure flag is not properly set. | django_bad.py:27:71:27:106 | ControlFlowNode for Attribute() | user-supplied input | | flask_bad.py:24:21:24:40 | ControlFlowNode for Subscript | flask_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | flask_bad.py:24:21:24:40 | ControlFlowNode for Subscript | Cookie is constructed from a $@,and its httponly flag is not properly set. | flask_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | user-supplied input | | flask_bad.py:24:21:24:40 | ControlFlowNode for Subscript | flask_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | flask_bad.py:24:21:24:40 | ControlFlowNode for Subscript | Cookie is constructed from a $@,and its samesite flag is not properly set. | flask_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | user-supplied input | | flask_bad.py:24:21:24:40 | ControlFlowNode for Subscript | flask_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | flask_bad.py:24:21:24:40 | ControlFlowNode for Subscript | Cookie is constructed from a $@,and its secure flag is not properly set. | flask_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | user-supplied input | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-614/InsecureCookie.expected b/python/ql/test/experimental/query-tests/Security/CWE-614/InsecureCookie.expected index 2743a8d21167..61f9b9b74692 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-614/InsecureCookie.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-614/InsecureCookie.expected @@ -1,9 +1,15 @@ | django_bad.py:6:5:7:52 | ControlFlowNode for Attribute() | Cookie is added without the 'httponly' flag properly set. | | django_bad.py:6:5:7:52 | ControlFlowNode for Attribute() | Cookie is added without the 'samesite' flag properly set. | | django_bad.py:6:5:7:52 | ControlFlowNode for Attribute() | Cookie is added without the 'secure' flag properly set. | +| django_bad.py:13:5:13:26 | ControlFlowNode for Subscript | Cookie is added without the 'httponly' flag properly set. | +| django_bad.py:13:5:13:26 | ControlFlowNode for Subscript | Cookie is added without the 'samesite' flag properly set. | +| django_bad.py:13:5:13:26 | ControlFlowNode for Subscript | Cookie is added without the 'secure' flag properly set. | | django_bad.py:19:5:21:66 | ControlFlowNode for Attribute() | Cookie is added without the 'httponly' flag properly set. | | django_bad.py:19:5:21:66 | ControlFlowNode for Attribute() | Cookie is added without the 'samesite' flag properly set. | | django_bad.py:19:5:21:66 | ControlFlowNode for Attribute() | Cookie is added without the 'secure' flag properly set. | +| django_bad.py:27:5:27:26 | ControlFlowNode for Subscript | Cookie is added without the 'httponly' flag properly set. | +| django_bad.py:27:5:27:26 | ControlFlowNode for Subscript | Cookie is added without the 'samesite' flag properly set. | +| django_bad.py:27:5:27:26 | ControlFlowNode for Subscript | Cookie is added without the 'secure' flag properly set. | | django_good.py:19:5:19:44 | ControlFlowNode for Attribute() | Cookie is added without the 'httponly' flag properly set. | | django_good.py:19:5:19:44 | ControlFlowNode for Attribute() | Cookie is added without the 'samesite' flag properly set. | | django_good.py:19:5:19:44 | ControlFlowNode for Attribute() | Cookie is added without the 'secure' flag properly set. | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-614/django_bad.py b/python/ql/test/experimental/query-tests/Security/CWE-614/django_bad.py index 45cece4390f1..6f1916930e53 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-614/django_bad.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-614/django_bad.py @@ -7,7 +7,7 @@ def django_response(request): httponly=False, samesite='None') return resp -# This test no longer produces an output due to django header setting methods not being modeled in the main query pack + def django_response(): response = django.http.HttpResponse() response['Set-Cookie'] = "name=value; SameSite=None;" @@ -21,7 +21,7 @@ def django_response(request): secure=False, httponly=False, samesite='None') return resp -# This test no longer produces an output due to django header setting methods not being modeled in the main query pack + def django_response(): response = django.http.HttpResponse() response['Set-Cookie'] = f"{django.http.request.GET.get('name')}={django.http.request.GET.get('value')}; SameSite=None;" diff --git a/python/ql/test/library-tests/frameworks/django-v2-v3/response_test.py b/python/ql/test/library-tests/frameworks/django-v2-v3/response_test.py index d4f17f7c3cf7..7722b4de8e18 100644 --- a/python/ql/test/library-tests/frameworks/django-v2-v3/response_test.py +++ b/python/ql/test/library-tests/frameworks/django-v2-v3/response_test.py @@ -62,7 +62,7 @@ def redirect_through_normal_response(request): resp = HttpResponse() # $ HttpResponse mimetype=text/html resp.status_code = 302 - resp['Location'] = next # $ MISSING: redirectLocation=next + resp['Location'] = next # $ headerWriteName='Location' headerWriteValue=next MISSING: redirectLocation=next resp.content = private # $ MISSING: responseBody=private return resp @@ -134,4 +134,5 @@ def setting_cookie(request): resp.cookies["key3"] = "value3" # $ CookieWrite CookieName="key3" CookieValue="value3" resp.delete_cookie("key4") # $ CookieWrite CookieName="key4" resp.delete_cookie(key="key4") # $ CookieWrite CookieName="key4" + resp["Set-Cookie"] = "key5=value5" # $ headerWriteName="Set-Cookie" headerWriteValue="key5=value5" CookieWrite CookieRawHeader="key5=value5" return resp From d1d082982ac5f63ed0a60f4de41ec748dd38dfe8 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Mon, 1 Jul 2024 14:25:29 +0100 Subject: [PATCH 038/281] More external references --- javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp index e294779d6b85..30fb2f891790 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp @@ -57,5 +57,14 @@
  • helmet.js website
    • +
  • + Content Security Policy (CSP) | MDN +
    • +
  • + Mozilla Web Security Guidelines +
    • +
  • + Protect against clickjacking | MDN + \ No newline at end of file From fc6fba8d06f04fa2b6af60b91a3fa6519445174a Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Mon, 1 Jul 2024 14:25:47 +0100 Subject: [PATCH 039/281] Fixed CWE tags --- javascript/ql/src/Security/CWE-693/InsecureHelmet.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql index c4437d4913d4..8f837669ffc3 100644 --- a/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql +++ b/javascript/ql/src/Security/CWE-693/InsecureHelmet.ql @@ -7,8 +7,8 @@ * @precision high * @id js/insecure-helmet-configuration * @tags security - * cwe-693 - * cwe-1021 + * external/cwe/cwe-693 + * external/cwe/cwe-1021 */ import javascript From a1b0703690239fb16686e6e4210193b7513d84a8 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Mon, 1 Jul 2024 16:21:34 +0100 Subject: [PATCH 040/281] Added detection for specific Polyfill.io CDN compromise - edited existing library and added new query and tests --- .../FunctionalityFromUntrustedSource.qll | 171 ++++++++++++++++++ .../FunctionalityFromUntrustedSource.ql | 153 +--------------- .../CWE-830/PolyfillIOCompromisedScript.qhelp | 77 ++++++++ .../CWE-830/PolyfillIOCompromisedScript.ql | 18 ++ .../src/Security/CWE-830/polyfill-check.html | 9 + .../Security/CWE-830/polyfill-nocheck.html | 9 + .../FunctionalityFromUntrustedSource.expected | 1 + .../PolyfillIOCompromisedScript.expected | 1 + .../CWE-830/PolyfillIOCompromisedScript.qlref | 1 + .../Security/CWE-830/polyfill-check.html | 9 + .../Security/CWE-830/polyfill-nocheck.html | 9 + 11 files changed, 306 insertions(+), 152 deletions(-) create mode 100644 javascript/ql/lib/semmle/javascript/security/FunctionalityFromUntrustedSource.qll create mode 100644 javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.qhelp create mode 100644 javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.ql create mode 100644 javascript/ql/src/Security/CWE-830/polyfill-check.html create mode 100644 javascript/ql/src/Security/CWE-830/polyfill-nocheck.html create mode 100644 javascript/ql/test/query-tests/Security/CWE-830/PolyfillIOCompromisedScript.expected create mode 100644 javascript/ql/test/query-tests/Security/CWE-830/PolyfillIOCompromisedScript.qlref create mode 100644 javascript/ql/test/query-tests/Security/CWE-830/polyfill-check.html create mode 100644 javascript/ql/test/query-tests/Security/CWE-830/polyfill-nocheck.html diff --git a/javascript/ql/lib/semmle/javascript/security/FunctionalityFromUntrustedSource.qll b/javascript/ql/lib/semmle/javascript/security/FunctionalityFromUntrustedSource.qll new file mode 100644 index 000000000000..5c145f072436 --- /dev/null +++ b/javascript/ql/lib/semmle/javascript/security/FunctionalityFromUntrustedSource.qll @@ -0,0 +1,171 @@ +import javascript + +/** A location that adds a reference to an untrusted source. */ +abstract class AddsUntrustedUrl extends Locatable { + /** Gets an explanation why this source is untrusted. */ + abstract string getProblem(); + + /** Gets the URL of the untrusted source. */ + abstract string getUrl(); +} + +module StaticCreation { + /** Holds if `host` is an alias of localhost. */ + bindingset[host] + predicate isLocalhostPrefix(string host) { + host.toLowerCase() + .regexpMatch([ + "(?i)localhost(:[0-9]+)?/.*", "127.0.0.1(:[0-9]+)?/.*", "::1/.*", "\\[::1\\]:[0-9]+/.*" + ]) + } + + /** Holds if `url` is a url that is vulnerable to a MITM attack. */ + bindingset[url] + predicate isUntrustedSourceUrl(string url) { + exists(string hostPath | hostPath = url.regexpCapture("(?i)http://(.*)", 1) | + not isLocalhostPrefix(hostPath) + ) + } + + /** Holds if `url` refers to a CDN that needs an integrity check - even with https. */ + bindingset[url] + predicate isCdnUrlWithCheckingRequired(string url) { + // Some CDN URLs are required to have an integrity attribute. We only add CDNs to that list + // that recommend integrity-checking. + url.regexpMatch("(?i)^https?://" + + [ + "code\\.jquery\\.com", // + "cdnjs\\.cloudflare\\.com", // + "cdnjs\\.com", // + "cdn\\.polyfill\\.io", // compromised + "polyfill\\.io", // compromised + ] + "/.*\\.js$") + } + + /** A script element that refers to untrusted content. */ + class ScriptElementWithUntrustedContent extends AddsUntrustedUrl instanceof HTML::ScriptElement { + ScriptElementWithUntrustedContent() { + not exists(string digest | not digest = "" | super.getIntegrityDigest() = digest) and + isUntrustedSourceUrl(super.getSourcePath()) + } + + override string getUrl() { result = super.getSourcePath() } + + override string getProblem() { result = "Script loaded using unencrypted connection." } + } + + /** A script element that refers to untrusted content. */ + class CdnScriptElementWithUntrustedContent extends AddsUntrustedUrl, HTML::ScriptElement { + CdnScriptElementWithUntrustedContent() { + not exists(string digest | not digest = "" | this.getIntegrityDigest() = digest) and + isCdnUrlWithCheckingRequired(this.getSourcePath()) + } + + override string getUrl() { result = this.getSourcePath() } + + override string getProblem() { + result = "Script loaded from content delivery network with no integrity check." + } + } + + /** An iframe element that includes untrusted content. */ + class IframeElementWithUntrustedContent extends AddsUntrustedUrl instanceof HTML::IframeElement { + IframeElementWithUntrustedContent() { isUntrustedSourceUrl(super.getSourcePath()) } + + override string getUrl() { result = super.getSourcePath() } + + override string getProblem() { result = "Iframe loaded using unencrypted connection." } + } +} + +module DynamicCreation { + /** Holds if `call` creates a tag of kind `name`. */ + predicate isCreateElementNode(DataFlow::CallNode call, string name) { + call = DataFlow::globalVarRef("document").getAMethodCall("createElement") and + call.getArgument(0).getStringValue().toLowerCase() = name + } + + DataFlow::Node getAttributeAssignmentRhs(DataFlow::CallNode createCall, string name) { + result = createCall.getAPropertyWrite(name).getRhs() + or + exists(DataFlow::InvokeNode inv | inv = createCall.getAMemberInvocation("setAttribute") | + inv.getArgument(0).getStringValue() = name and + result = inv.getArgument(1) + ) + } + + /** + * Holds if `createCall` creates a ` + + + ... + + \ No newline at end of file diff --git a/javascript/ql/src/Security/CWE-830/polyfill-nocheck.html b/javascript/ql/src/Security/CWE-830/polyfill-nocheck.html new file mode 100644 index 000000000000..6b9fbfe65c8a --- /dev/null +++ b/javascript/ql/src/Security/CWE-830/polyfill-nocheck.html @@ -0,0 +1,9 @@ + + + Polyfill.io demo + + + + ... + + \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-830/FunctionalityFromUntrustedSource.expected b/javascript/ql/test/query-tests/Security/CWE-830/FunctionalityFromUntrustedSource.expected index 53ed4f16aba8..cbca22218323 100644 --- a/javascript/ql/test/query-tests/Security/CWE-830/FunctionalityFromUntrustedSource.expected +++ b/javascript/ql/test/query-tests/Security/CWE-830/FunctionalityFromUntrustedSource.expected @@ -5,3 +5,4 @@ | StaticCreationOfUntrustedSourceUse.html:6:9:6:56 | + + + ... + + \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-830/polyfill-nocheck.html b/javascript/ql/test/query-tests/Security/CWE-830/polyfill-nocheck.html new file mode 100644 index 000000000000..6b9fbfe65c8a --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-830/polyfill-nocheck.html @@ -0,0 +1,9 @@ + + + Polyfill.io demo + + + + ... + + \ No newline at end of file From ceda46e31730ce2996c79dd706d74f698f5908e4 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Mon, 1 Jul 2024 16:52:28 +0100 Subject: [PATCH 041/281] Fixed ending

    tags --- .../ql/src/Security/CWE-830/PolyfillIOCompromisedScript.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.qhelp b/javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.qhelp index 7f61a6353057..8fa713481a0a 100644 --- a/javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.qhelp +++ b/javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.qhelp @@ -6,13 +6,13 @@

    Polyfill.io was a popular JavaScript polyflll Content Delivery Network (CDN), used to support new web browser standards on older browsers. -

    +

    In February 2024 the domain was sold, and in June 2024 it was widely publicised that the domain had been used to serve malicious scripts. It was taken down later in that month, leaving a window where sites that used the service could have been compromised. -

    +

    Including a resource from an untrusted source or using an untrusted channel may From 1744a9801750d1dfa30a9aec0ca432e281457485 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Mon, 1 Jul 2024 16:53:22 +0100 Subject: [PATCH 042/281] Added full stop to end of message --- .../ql/src/Security/CWE-830/PolyfillIOCompromisedScript.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.ql b/javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.ql index 18c9ef0468d0..358fcbdee4b6 100644 --- a/javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.ql +++ b/javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.ql @@ -15,4 +15,4 @@ import semmle.javascript.security.FunctionalityFromUntrustedSource from AddsUntrustedUrl s where s.getUrl().regexpMatch("^(?i)https?://(cdn\\.)?polyfill\\.io/.*") -select s, "Script loaded from known-compromised content delivery network with no integrity check" +select s, "Script loaded from known-compromised content delivery network with no integrity check." From c985c9adb3e37792a1761e0fa041e35a868078b3 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Mon, 1 Jul 2024 16:56:07 +0100 Subject: [PATCH 043/281] Added change note for polyfill.io query --- .../change-notes/2024-07-01-polyfill-io-compromised-script.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 javascript/ql/src/change-notes/2024-07-01-polyfill-io-compromised-script.md diff --git a/javascript/ql/src/change-notes/2024-07-01-polyfill-io-compromised-script.md b/javascript/ql/src/change-notes/2024-07-01-polyfill-io-compromised-script.md new file mode 100644 index 000000000000..b10ca07d1fa7 --- /dev/null +++ b/javascript/ql/src/change-notes/2024-07-01-polyfill-io-compromised-script.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added a new query, `js/polyfill-io-compromised-script`, which detects uses in HTML and JavaScript of the compromised `polyfill.io` content delivery network. \ No newline at end of file From b4d8c4889aee5e94410c81cae7f81d99dc475ec7 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Mon, 1 Jul 2024 16:58:03 +0100 Subject: [PATCH 044/281] Fixed wrong name for example HTML --- .../ql/src/Security/CWE-830/PolyfillIOCompromisedScript.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.qhelp b/javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.qhelp index 8fa713481a0a..6b63d295ee60 100644 --- a/javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.qhelp +++ b/javascript/ql/src/Security/CWE-830/PolyfillIOCompromisedScript.qhelp @@ -65,7 +65,7 @@ subresource integrity is recommended, as in the next example.

    - + From 73fc6bcdb1de53ceccb7642387cb7b3ecbf87b4a Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Mon, 1 Jul 2024 17:10:24 +0100 Subject: [PATCH 045/281] Added some missing QLDoc --- .../security/FunctionalityFromUntrustedSource.qll | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/security/FunctionalityFromUntrustedSource.qll b/javascript/ql/lib/semmle/javascript/security/FunctionalityFromUntrustedSource.qll index 5c145f072436..3ee72b20f595 100644 --- a/javascript/ql/lib/semmle/javascript/security/FunctionalityFromUntrustedSource.qll +++ b/javascript/ql/lib/semmle/javascript/security/FunctionalityFromUntrustedSource.qll @@ -1,3 +1,7 @@ +/** + * Provides classes for finding functionality that is loaded from untrusted sources and used in script or frame elements. + */ + import javascript /** A location that adds a reference to an untrusted source. */ @@ -9,6 +13,7 @@ abstract class AddsUntrustedUrl extends Locatable { abstract string getUrl(); } +/** Looks for static creation of an element and source. */ module StaticCreation { /** Holds if `host` is an alias of localhost. */ bindingset[host] @@ -78,6 +83,7 @@ module StaticCreation { } } +/** Looks for dyanmic creation of an element and source. */ module DynamicCreation { /** Holds if `call` creates a tag of kind `name`. */ predicate isCreateElementNode(DataFlow::CallNode call, string name) { @@ -85,6 +91,7 @@ module DynamicCreation { call.getArgument(0).getStringValue().toLowerCase() = name } + /** Get the right-hand side of an assignment to a named attribute. */ DataFlow::Node getAttributeAssignmentRhs(DataFlow::CallNode createCall, string name) { result = createCall.getAPropertyWrite(name).getRhs() or @@ -103,6 +110,7 @@ module DynamicCreation { not exists(getAttributeAssignmentRhs(createCall, "integrity")) } + /** Holds if `t` tracks a URL that is loaded from an untrusted source. */ DataFlow::Node urlTrackedFromUnsafeSourceLiteral(DataFlow::TypeTracker t) { t.start() and result.getStringValue().regexpMatch("(?i)http:.*") or @@ -126,6 +134,7 @@ module DynamicCreation { ) } + /** Holds a dataflow node is traked from an untrusted source. */ DataFlow::Node urlTrackedFromUnsafeSourceLiteral() { result = urlTrackedFromUnsafeSourceLiteral(DataFlow::TypeTracker::end()) } @@ -144,6 +153,7 @@ module DynamicCreation { ) } + /** A script or iframe element that refers to untrusted content. */ class IframeOrScriptSrcAssignment extends AddsUntrustedUrl { string name; From e2b37f97b0dcaee73dafa5925cc3a71e7629c532 Mon Sep 17 00:00:00 2001 From: aegilops <41705651+aegilops@users.noreply.github.com> Date: Mon, 1 Jul 2024 17:41:26 +0100 Subject: [PATCH 046/281] Added dot to end of test message --- .../Security/CWE-830/PolyfillIOCompromisedScript.expected | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/test/query-tests/Security/CWE-830/PolyfillIOCompromisedScript.expected b/javascript/ql/test/query-tests/Security/CWE-830/PolyfillIOCompromisedScript.expected index dca655504a9c..32cba6bf3da4 100644 --- a/javascript/ql/test/query-tests/Security/CWE-830/PolyfillIOCompromisedScript.expected +++ b/javascript/ql/test/query-tests/Security/CWE-830/PolyfillIOCompromisedScript.expected @@ -1 +1 @@ -| polyfill-nocheck.html:4:9:4:98 | + + + ... + + \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-830/polyfill-check.html b/javascript/ql/src/experimental/Security/CWE-830/polyfill-trusted.html similarity index 52% rename from javascript/ql/test/query-tests/Security/CWE-830/polyfill-check.html rename to javascript/ql/src/experimental/Security/CWE-830/polyfill-trusted.html index 800105064ae3..7b216314780c 100644 --- a/javascript/ql/test/query-tests/Security/CWE-830/polyfill-check.html +++ b/javascript/ql/src/experimental/Security/CWE-830/polyfill-trusted.html @@ -1,7 +1,7 @@ - Polyfill demo - Cloudflare hosted with pinned version and integrity checking - + Polyfill demo - Cloudflare hosted with pinned version (but no integrity checking, since it is dynamically generated) + ... diff --git a/javascript/ql/test/experimental/Security/CWE-830/FunctionalityFromUntrustedDomain.expected b/javascript/ql/test/experimental/Security/CWE-830/FunctionalityFromUntrustedDomain.expected new file mode 100644 index 000000000000..fa0ba043f2be --- /dev/null +++ b/javascript/ql/test/experimental/Security/CWE-830/FunctionalityFromUntrustedDomain.expected @@ -0,0 +1,6 @@ +WARNING: Unused predicate isCdnDomainWithCheckingRequiredTest (FunctionalityFromUntrustedDomain.ql:34,11-46) +WARNING: Unused predicate isUntrustedDomainTest (FunctionalityFromUntrustedDomain.ql:26,11-32) +WARNING: Unused predicate isUntrustedDomainTest2 (FunctionalityFromUntrustedDomain.ql:30,11-33) +WARNING: Unused predicate isUntrustedHostnameTest (FunctionalityFromUntrustedDomain.ql:21,11-34) +WARNING: Unused predicate isUntrustedTest (FunctionalityFromUntrustedDomain.ql:16,11-26) +| polyfill-nocheck.html:4:9:4:98 |