From cfdebc9c47179a434875624ef16d023546915052 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Wed, 11 Sep 2019 10:03:11 -0500 Subject: [PATCH] policy: add startup benchmark and make SRI lazier --- benchmark/policy/policy-startup.js | 51 ++++++++++++++++ lib/internal/policy/manifest.js | 78 ++++++++++++++++--------- lib/internal/policy/sri.js | 14 +++-- test/benchmark/test-benchmark-policy.js | 9 +++ 4 files changed, 119 insertions(+), 33 deletions(-) create mode 100644 benchmark/policy/policy-startup.js create mode 100644 test/benchmark/test-benchmark-policy.js diff --git a/benchmark/policy/policy-startup.js b/benchmark/policy/policy-startup.js new file mode 100644 index 00000000000000..1588123d8007d9 --- /dev/null +++ b/benchmark/policy/policy-startup.js @@ -0,0 +1,51 @@ +// Tests the impact on eager operations required for policies affecting +// general startup, does not test lazy operations +'use strict'; +const common = require('../common.js'); + +const configs = { + n: [1024] +}; + +const options = { + flags: ['--expose-internals'] +}; + +const bench = common.createBenchmark(main, configs, options); + +function main(conf) { + const hash = (str, algo) => { + const hash = require('crypto').createHash(algo); + return hash.update(str).digest('base64'); + }; + const resources = Object.fromEntries( + // Simulate graph of 1k modules + Array.from({ length: 1024 }, (_, i) => { + return [`./_${i}`, { + integrity: `sha256-${hash(`// ./_${i}`, 'sha256')}`, + dependencies: Object.fromEntries(Array.from({ + // Average 3 deps per 4 modules + length: Math.floor((i % 4) / 2) + }, (_, ii) => { + return [`_${ii}`, `./_${i - ii}`]; + })), + }]; + }) + ); + const json = JSON.parse(JSON.stringify({ resources }), (_, o) => { + if (o && typeof o === 'object') { + Reflect.setPrototypeOf(o, null); + Object.freeze(o); + } + return o; + }); + const { Manifest } = require('internal/policy/manifest'); + + bench.start(); + + for (let i = 0; i < conf.n; i++) { + new Manifest(json, 'file://benchmark/policy-relative'); + } + + bench.end(conf.n); +} diff --git a/lib/internal/policy/manifest.js b/lib/internal/policy/manifest.js index 672eab1c3837a6..0531f198f21971 100644 --- a/lib/internal/policy/manifest.js +++ b/lib/internal/policy/manifest.js @@ -4,6 +4,7 @@ const { ArrayIsArray, Map, MapPrototypeSet, + ObjectCreate, ObjectEntries, ObjectFreeze, ObjectSetPrototypeOf, @@ -31,7 +32,6 @@ const { URL } = require('internal/url'); const { createHash, timingSafeEqual } = crypto; const HashUpdate = uncurryThis(crypto.Hash.prototype.update); const HashDigest = uncurryThis(crypto.Hash.prototype.digest); -const BufferEquals = uncurryThis(Buffer.prototype.equals); const BufferToString = uncurryThis(Buffer.prototype.toString); const kRelativeURLStringPattern = /^\.{0,2}\//; const { getOptionValue } = require('internal/options'); @@ -56,9 +56,47 @@ function REACTION_LOG(error) { } class Manifest { + /** + * @type {Map} + * + * Used to compare a resource to the content body at the resource. + * `true` is used to signify that all integrities are allowed, otherwise, + * SRI strings are parsed to compare with the body. + * + * This stores strings instead of eagerly parsing SRI strings + * and only converts them to SRI data structures when needed. + * This avoids needing to parse all SRI strings at startup even + * if some never end up being used. + */ #integrities = new SafeMap(); + /** + * @type {Map true | URL>} + * + * Used to find where a dependency is located. + * + * This stores functions to lazily calculate locations as needed. + * `true` is used to signify that the location is not specified + * by the manifest and default resolution should be allowed. + */ #dependencies = new SafeMap(); + /** + * @type {(err: Error) => void} + * + * Performs default action for what happens when a manifest encounters + * a violation such as abort()ing or exiting the process, throwing the error, + * or logging the error. + */ #reaction = null; + /** + * `obj` should match the policy file format described in the docs + * it is expected to not have prototype pollution issues either by reassigning + * the prototype to `null` for values or by running prior to any user code. + * + * `manifestURL` is a URL to resolve relative locations against. + * + * @param {object} obj + * @param {string} manifestURL + */ constructor(obj, manifestURL) { const integrities = this.#integrities; const dependencies = this.#dependencies; @@ -98,35 +136,14 @@ class Manifest { let integrity = manifestEntries[i][1].integrity; if (!integrity) integrity = null; if (integrity != null) { - debug(`Manifest contains integrity for url ${originalHREF}`); + debug('Manifest contains integrity for url %s', originalHREF); if (typeof integrity === 'string') { - const sri = ObjectFreeze(SRI.parse(integrity)); if (integrities.has(resourceHREF)) { - const old = integrities.get(resourceHREF); - let mismatch = false; - - if (old.length !== sri.length) { - mismatch = true; - } else { - compare: - for (let sriI = 0; sriI < sri.length; sriI++) { - for (let oldI = 0; oldI < old.length; oldI++) { - if (sri[sriI].algorithm === old[oldI].algorithm && - BufferEquals(sri[sriI].value, old[oldI].value) && - sri[sriI].options === old[oldI].options) { - continue compare; - } - } - mismatch = true; - break compare; - } - } - - if (mismatch) { + if (integrities.get(resourceHREF) !== integrity) { throw new ERR_MANIFEST_INTEGRITY_MISMATCH(resourceURL); } } - integrities.set(resourceHREF, sri); + integrities.set(resourceHREF, integrity); } else if (integrity === true) { integrities.set(resourceHREF, true); } else { @@ -138,7 +155,7 @@ class Manifest { let dependencyMap = manifestEntries[i][1].dependencies; if (dependencyMap === null || dependencyMap === undefined) { - dependencyMap = {}; + dependencyMap = ObjectCreate(null); } if (typeof dependencyMap === 'object' && !ArrayIsArray(dependencyMap)) { /** @@ -200,13 +217,18 @@ class Manifest { assertIntegrity(url, content) { const href = `${url}`; - debug(`Checking integrity of ${href}`); + debug('Checking integrity of %s', href); const integrities = this.#integrities; const realIntegrities = new Map(); if (integrities.has(href)) { - const integrityEntries = integrities.get(href); + let integrityEntries = integrities.get(href); if (integrityEntries === true) return true; + if (typeof integrityEntries === 'string') { + const sri = ObjectFreeze(SRI.parse(integrityEntries)); + integrities.set(href, sri); + integrityEntries = sri; + } // Avoid clobbered Symbol.iterator for (let i = 0; i < integrityEntries.length; i++) { const { diff --git a/lib/internal/policy/sri.js b/lib/internal/policy/sri.js index d70df5c1aa1f7b..c4b5e7f1ca209a 100644 --- a/lib/internal/policy/sri.js +++ b/lib/internal/policy/sri.js @@ -1,17 +1,19 @@ 'use strict'; -// Value of https://w3c.github.io/webappsec-subresource-integrity/#the-integrity-attribute +// Utility to parse the value of +// https://w3c.github.io/webappsec-subresource-integrity/#the-integrity-attribute const { ObjectDefineProperty, ObjectFreeze, + ObjectGetPrototypeOf, ObjectSeal, + ObjectSetPrototypeOf, RegExp, RegExpPrototypeExec, RegExpPrototypeTest, StringPrototypeSlice, } = primordials; -// Returns [{algorithm, value (in base64 string), options,}] const { ERR_SRI_PARSE } = require('internal/errors').codes; @@ -21,7 +23,8 @@ const kHASH_ALGO = 'sha(?:256|384|512)'; // Base64 const kHASH_VALUE = '[A-Za-z0-9+/]+[=]{0,2}'; const kHASH_EXPRESSION = `(${kHASH_ALGO})-(${kHASH_VALUE})`; -const kOPTION_EXPRESSION = `(${kVCHAR}*)`; +// Ungrouped since unused +const kOPTION_EXPRESSION = `(?:${kVCHAR}*)`; const kHASH_WITH_OPTIONS = `${kHASH_EXPRESSION}(?:[?](${kOPTION_EXPRESSION}))?`; const kSRIPattern = RegExp(`(${kWSP}*)(?:${kHASH_WITH_OPTIONS})`, 'g'); ObjectSeal(kSRIPattern); @@ -29,9 +32,10 @@ const kAllWSP = RegExp(`^${kWSP}*$`); ObjectSeal(kAllWSP); const BufferFrom = require('buffer').Buffer.from; +const RealArrayPrototype = ObjectGetPrototypeOf([]); +// Returns {algorithm, value (in base64 string), options,}[] const parse = (str) => { - kSRIPattern.lastIndex = 0; let prevIndex = 0; let match; const entries = []; @@ -62,7 +66,7 @@ const parse = (str) => { throw new ERR_SRI_PARSE(str, str.charAt(prevIndex), prevIndex); } } - return entries; + return ObjectSetPrototypeOf(entries, RealArrayPrototype); }; module.exports = { diff --git a/test/benchmark/test-benchmark-policy.js b/test/benchmark/test-benchmark-policy.js new file mode 100644 index 00000000000000..7eb0992b1f1eda --- /dev/null +++ b/test/benchmark/test-benchmark-policy.js @@ -0,0 +1,9 @@ +'use strict'; + +require('../common'); + +const runBenchmark = require('../common/benchmark'); + +runBenchmark('policy', [ + 'n=1', +]);