From c172ce8dbe40408459e218f153458937c08e6c4a Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Fri, 3 Jan 2025 14:23:31 +0100 Subject: [PATCH] fix(csp): allow duplicate report-* directives (#151) * Added duplicate report-* directives detection * addressed some nits --- scan | 8 -------- src/analyzer/cspParser.js | 25 ++++++++++++++++++++----- src/analyzer/tests/csp.js | 15 ++++++++++++++- src/grader/charts.js | 10 ++++++++++ src/types.js | 2 ++ test/csp-parser.test.js | 19 ++++++++++++++++++- 6 files changed, 64 insertions(+), 15 deletions(-) delete mode 100755 scan diff --git a/scan b/scan deleted file mode 100755 index 26e3494..0000000 --- a/scan +++ /dev/null @@ -1,8 +0,0 @@ -#!/usr/bin/env bash - -if [ -z "$1" ]; then - echo "Usage: $0 " - exit 1 -fi - -node src/cli/index.js scan $1 \ No newline at end of file diff --git a/src/analyzer/cspParser.js b/src/analyzer/cspParser.js index 8272d33..2aa4378 100644 --- a/src/analyzer/cspParser.js +++ b/src/analyzer/cspParser.js @@ -7,6 +7,8 @@ const DIRECTIVES_DISALLOWED_IN_META = [ "report-uri", "sandbox", ]; +const ALLOWED_DUPLICATE_KEYS = new Set(["report-uri", "report-to"]); +export const DUPLICATE_WARNINGS_KEY = "_observatory_duplicate_key_warnings"; /** * Parse CSP from meta tags, weeding out directives @@ -24,6 +26,10 @@ export function parseCspMeta(cspList) { } /** + * The returned Map has the directive as the key and a Set of sources as the value. + * If there are allowed duplicates detected, the first one is kept and the rest are discarded, + * and an entry in the final Map is added with the key "_observatory_duplicate_key_warnings" + * and the directive's name as the value. * * @param {string[]} cspList * @returns {Map>} @@ -44,6 +50,8 @@ export function parseCsp(cspList) { /** @type {Map} */ const csp = new Map(); + /** @type {Set} */ + const duplicate_warnings = new Set(); for (const [policyIndex, policy] of cleanCspList.entries()) { const directiveSeenBeforeThisPolicy = new Set(); @@ -59,11 +67,16 @@ export function parseCsp(cspList) { const directive = directiveEntry.toLowerCase(); // While technically valid in that you just use the first entry, we are saying that repeated - // directives are invalid so that people notice it + // directives are invalid so that people notice it. The exception are duplicate report-uri + // and report-to directives, which we allow. if (directiveSeenBeforeThisPolicy.has(directive)) { - throw new Error( - `Duplicate directive ${directive} in policy ${policyIndex}` - ); + if (ALLOWED_DUPLICATE_KEYS.has(directive)) { + duplicate_warnings.add(directive); + } else { + throw new Error( + `Duplicate directive ${directive} in policy ${policyIndex}` + ); + } } else { directiveSeenBeforeThisPolicy.add(directive); } @@ -146,7 +159,9 @@ export function parseCsp(cspList) { : new Set(["'none'"]), ]) ); - + if (duplicate_warnings.size) { + finalCsp.set(DUPLICATE_WARNINGS_KEY, duplicate_warnings); + } return finalCsp; } diff --git a/src/analyzer/tests/csp.js b/src/analyzer/tests/csp.js index 619f91a..e49e81a 100644 --- a/src/analyzer/tests/csp.js +++ b/src/analyzer/tests/csp.js @@ -4,7 +4,11 @@ import { } from "../../headers.js"; import { Requests, Policy, BaseOutput } from "../../types.js"; import { Expectation } from "../../types.js"; -import { parseCsp, parseCspMeta } from "../cspParser.js"; +import { + DUPLICATE_WARNINGS_KEY, + parseCsp, + parseCspMeta, +} from "../cspParser.js"; import { getHttpHeaders } from "../utils.js"; const DANGEROUSLY_BROAD = new Set([ @@ -47,6 +51,7 @@ export class CspOutput extends BaseOutput { Expectation.CspImplementedWithUnsafeEval, Expectation.CspImplementedWithUnsafeInline, Expectation.CspImplementedWithInsecureScheme, + Expectation.CspImplementedButDuplicateDirectives, Expectation.CspHeaderInvalid, Expectation.CspNotImplemented, Expectation.CspNotImplementedButReportingEnabled, @@ -301,6 +306,7 @@ export function contentSecurityPolicyTest( ); // Check to see if the test passed or failed + // If it passed, report any duplicate report-uri/report-to directives if ( [ expectation, @@ -310,10 +316,17 @@ export function contentSecurityPolicyTest( ].includes(output.result) ) { output.pass = true; + if (csp.has(DUPLICATE_WARNINGS_KEY)) { + output.result = Expectation.CspImplementedButDuplicateDirectives; + } } output.data = {}; for (const [key, value] of csp) { + // filter out the duplicate warnings key from the parsed CSP + if (key === DUPLICATE_WARNINGS_KEY) { + continue; + } output.data[key] = [...value].sort(); } diff --git a/src/grader/charts.js b/src/grader/charts.js index 38f1b03..1387618 100644 --- a/src/grader/charts.js +++ b/src/grader/charts.js @@ -214,6 +214,16 @@ export const SCORE_TABLE = new Map([

`, }, ], + [ + Expectation.CspImplementedButDuplicateDirectives, + { + description: `

+ Content Security Policy (CSP) implemented, but contains duplicate directives. +

`, + modifier: 0, + recommendation: `

Remove duplicate directives from the CSP

`, + }, + ], // Cookies [ diff --git a/src/types.js b/src/types.js index 0d9b4b9..31a93bb 100644 --- a/src/types.js +++ b/src/types.js @@ -106,6 +106,8 @@ export const Expectation = { CspNotImplemented: "csp-not-implemented", CspNotImplementedButReportingEnabled: "csp-not-implemented-but-reporting-enabled", + CspImplementedButDuplicateDirectives: + "csp-implemented-but-duplicate-directives", // SUBRESOURCE INTEGRITY diff --git a/test/csp-parser.test.js b/test/csp-parser.test.js index f6a6173..1051040 100644 --- a/test/csp-parser.test.js +++ b/test/csp-parser.test.js @@ -196,11 +196,28 @@ describe("Content Security Policy Parser", function () { } }); - it("should parse this header correctly", function () { + it("should parse this example header correctly", function () { let policy = [ "default-src 'self' blob: https://*.cnn.com:* http://*.cnn.com:* *.cnn.io:* *.cnn.net:* *.turner.com:* *.turner.io:* *.ugdturner.com:* courageousstudio.com *.vgtf.net:*; script-src 'unsafe-eval' 'unsafe-inline' 'self' *; style-src 'unsafe-inline' 'self' blob: *; child-src 'self' blob: *; frame-src 'self' *; object-src 'self' *; img-src 'self' data: blob: *; media-src 'self' data: blob: *; font-src 'self' data: *; connect-src 'self' data: *; frame-ancestors 'self' https://*.cnn.com:* http://*.cnn.com https://*.cnn.io:* http://*.cnn.io:* *.turner.com:* courageousstudio.com;", ]; const res = parseCsp(policy); assert(res); }); + + it("should parse a policy with duplicate report-uri entries and report those duplicates", function () { + let policy = [ + "report-uri https://www.dropbox.com/csp_log?policy_name=metaserver-whitelist ; report-uri https://www.dropbox.com/csp_log?policy_name=metaserver-dynamic", + ]; + const res = parseCsp(policy); + assert(res); + assert(res.has("_observatory_duplicate_key_warnings")); + assert(res.get("_observatory_duplicate_key_warnings")?.has("report-uri")); + }); + it("should parse a policy with duplicate report-to entries and report those duplicates", function () { + let policy = ["report-to some_name ; report-to some_other_name"]; + const res = parseCsp(policy); + assert(res); + assert(res.has("_observatory_duplicate_key_warnings")); + assert(res.get("_observatory_duplicate_key_warnings")?.has("report-to")); + }); });