Skip to content
This repository has been archived by the owner on Nov 6, 2023. It is now read-only.

Commit

Permalink
Fix a bug for handling the dangling securecookie rules
Browse files Browse the repository at this point in the history
See comments in code for more details.
  • Loading branch information
Chan Chak Shing committed Aug 7, 2018
1 parent ca52ba2 commit 5c149aa
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 14 deletions.
1 change: 1 addition & 0 deletions utils/trivialize-rules/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"main": "trivialize-rules.js",
"dependencies": {
"chalk": "^2.1.0",
"escape-string-regexp": "^1.0.5",
"highland": "^2.7.1",
"regulex": "0.0.2",
"xml2js": "^0.4.16"
Expand Down
46 changes: 32 additions & 14 deletions utils/trivialize-rules/trivialize-rules.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const readFile = _.wrapCallback(fs.readFile);
const writeFile = _.wrapCallback(fs.writeFile);
const parseXML = _.wrapCallback(require('xml2js').parseString);
const { explodeRegExp, UnsupportedRegExp } = require('./explode-regexp');
const escapeStringRegexp = require('escape-string-regexp');
const chalk = require('chalk');

const rulesDir = `${__dirname}/../../src/chrome/content/rules`;
Expand Down Expand Up @@ -202,11 +203,11 @@ files.fork().zipAll([ sources.fork(), rules ]).map(([name, source, ruleset]) =>
// is dangerous for a rewrite.
function isStaticCookie(securecookie) {
if (securecookie.host === '.+' && securecookie.name === '.+') {
return true;
return [true, false];
}

if (!securecookie.host.startsWith('^') || !securecookie.host.endsWith('$')) {
return false;
return [false, false];
}

let localDomains = new Set();
Expand All @@ -224,7 +225,7 @@ files.fork().zipAll([ sources.fork(), rules ]).map(([name, source, ruleset]) =>
throw e;
}
warn`Unsupported regexp part ${e.message} while traversing securecookie : ${JSON.stringify(securecookie)}`;
return false;
return [false, false];
}

for (const domain of localDomains) {
Expand All @@ -249,22 +250,39 @@ files.fork().zipAll([ sources.fork(), rules ]).map(([name, source, ruleset]) =>
// safely.
if (unsupportedDomains.size > 0) {
if (unsupportedDomains.size === localDomains.size) {
shouldRemoveSecurecookies = true;
return true;
return [true, true];
}
return false;
// TODO: Can we remove partial dangling securecookie rules???
fail`Tag securecookie ${JSON.stringify(securecookie)} matches ${unsupportedDomains} which are not in targets ${targets}`;
}

return true;
return [true, false];
}

if (domains.slice().sort().join('\n') !== targets.sort().join('\n')) {
if (securecookies.length > 0 && !securecookies.every(isStaticCookie)) {
return;
}

if (shouldRemoveSecurecookies) {
source = replaceXML(source, 'securecookie', []);
// For each securecookie rule, check if it is a static securecookie.
// If it is non-static, remove the securecookie rule if it contains

This comment has been minimized.

Copy link
@bcyphers

bcyphers Aug 13, 2018

Contributor

I think this line is a little confused. If the cookie is non-static, we short out and do not trivialize the ruleset. If a securecookie is static but matches any unsupported hosts, we remove that securecookie.

// dangling rules. This removal is better done one by one to avoid
// unwanted side effects.
// Else if ALL securecookie rules are static, trivialize the targets.
for (const securecookie of securecookies) {
let [isStatic, shouldRemove] = isStaticCookie(securecookie);

if (isStatic) {
if (shouldRemove) {
let scReSrc = `\n([\t ]*)<securecookie\\s*host=\\s*"${escapeStringRegexp(securecookie.host)}"(\\s*)name=\\s*"${escapeStringRegexp(securecookie.name)}"\\s*?/>[\t ]*\n`;
let scRe = new RegExp(scReSrc);

if (scRe && scRe.test(source)) {
source = source.replace(scRe, '');
} else {
fail`Failed to construct regexp which matches securecookie: ${JSON.stringify(securecookie)}`;

This comment has been minimized.

Copy link
@bcyphers

bcyphers Aug 13, 2018

Contributor

Will this failure ever happen in practice? This seems fine for figuring out what scReSrc should be, but once we have that, I'm not sure why scRe.test(source) would fail. Can we remove the check?

return ;
}
}
} else {
// Skip this ruleset as it contain non-static securecookies
return ;
}
}

source = replaceXML(source, 'target', domains.map(domain => `<target host="${domain}" />`));
Expand Down

0 comments on commit 5c149aa

Please sign in to comment.