policy: add startup benchmark and make SRI lazier

bfarias-godaddy · addaleax · commit 0ede223fa83c · 2020-09-22T19:53:44.000+02:00
PR-URL: #29527 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git a/benchmark/policy/policy-startup.js 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
@@ -4,6 +4,7 @@ const{
 ArrayIsArray,
 Map,
 MapPrototypeSet,
+ ObjectCreate,
 ObjectEntries,
 ObjectFreeze,
 ObjectSetPrototypeOf,
@@ -29,7 +30,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');
@@ -54,9 +54,47 @@ function REACTION_LOG(error){
 }
 
 class Manifest{
+ /**
+ * @type{Map<string, true | string | SRI[]>}
+ *
+ * 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<string, (specifier: string) => 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;
@@ -96,35 +134,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{
@@ -136,7 +153,7 @@ class Manifest{
 
 let dependencyMap = manifestEntries[i][1].dependencies;
 if (dependencyMap === null || dependencyMap === undefined){
- dependencyMap ={};
+ dependencyMap = ObjectCreate(null);
 }
 if (typeof dependencyMap === 'object' && !ArrayIsArray(dependencyMap)){
 /**
@@ -198,13 +215,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
@@ -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,17 +23,19 @@ 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);
 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
@@ -0,0 +1,9 @@
+'use strict';
+
+require('../common');
+
+const runBenchmark = require('../common/benchmark');
+
+runBenchmark('policy', [
+ 'n=1',
+]);

-Original file line number
+Diff line change
@@ @@ -0,0 +1,51 @@ @@
 +// Tests the impact on eager operations required for policies affecting
 +// general startup, does not test lazy operations
 +'use strict';
 +constcommon=require('../common.js');
++
 +constconfigs={
 +n: [1024]
 +};
++
 +constoptions={
 +flags: ['--expose-internals']
 +};
++
 +constbench=common.createBenchmark(main,configs,options);
++
 +functionmain(conf){
 +consthash=(str,algo)=>{
 +consthash=require('crypto').createHash(algo);
 +returnhash.update(str).digest('base64');
 +};
 +constresources=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}`];
 +})),
 +}];
 +})
 +);
 +constjson=JSON.parse(JSON.stringify({ resources }),(_,o)=>{
 +if(o&&typeofo==='object'){
 +Reflect.setPrototypeOf(o,null);
 +Object.freeze(o);
 +}
 +returno;
 +});
 +const{ Manifest }=require('internal/policy/manifest');
++
 +bench.start();
++
 +for(leti=0;i<conf.n;i++){
 +newManifest(json,'file://benchmark/policy-relative');
 +}
++
 +bench.end(conf.n);
 +}