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

Detect static securecookies to trivialize more rules #16029

Merged
merged 10 commits into from
Aug 22, 2018
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
96 changes: 93 additions & 3 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 @@ -99,6 +100,7 @@ files.fork().zipAll([ sources.fork(), rules ]).map(([name, source, ruleset]) =>
const fail = createTag('FAIL', chalk.red, console.error);

let targets = ruleset.target.map(target => target.$.host);
let securecookies = ruleset.securecookie ? ruleset.securecookie.map(sc => sc.$) : new Array();
let rules = ruleset.rule.map(rule => rule.$);

if (rules.length === 1 && isTrivial(rules[0])) {
Expand Down Expand Up @@ -181,9 +183,98 @@ files.fork().zipAll([ sources.fork(), rules ]).map(([name, source, ruleset]) =>

domains = Array.from(domains);

// It is assumed that if all securecookies are static,
// they can be safely ignored.
//
// A securecookie is called to be static either it is a trivial securecookie
// or ALL of the following conditions are satisfied:
//
// 1. securecookie.host match cookie.host from the beginning ^ to the end $.
// Otherwise, it might match subdomains/ partial patterns, thus a non-trivial
// securecookie.
//
// 2. securecookie.host will not throw an error when passed to explodeRegExp().
// Otherwise, it might match patterns too complicated for our interests.
//
// 3. Each exploded securecookie.host should be included in ruleset.target/
// exploded target. Otherwise, this ruleset is likely problematic itself. It
// is dangerous for a rewrite.
function isStaticCookie(securecookie) {
if (securecookie.host === '.+' && securecookie.name === '.+') {
return [true, false];
}

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

let localDomains = new Set();
let unsupportedDomains = new Set();

try {
explodeRegExp(securecookie.host, domain => {
if (domain.startsWith('.')) {
domain = domain.slice(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this sufficient munging to ensure it matches the target domains?
If not, at worst it will not match and result in a false result, which isn't terrible but not desirable.

Copy link
Collaborator Author

@cschanaj cschanaj Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hainish this should be good enough. As mentioned in #16029 (comment), securecookie.host only match cookie.domain if there is a rule covers cookie.domain. Given the target are trivial, cookie.domain cannot be anything other than domain.example.com and .domain.example.com for a securecookie rule to works. With condition (1) effective, securecookie.host should explode to either one of them. Otherwise, the securecookie will never be applied (dangling rules?) under the current implementation.

With the above logic in mind, I have also add 4bbd731 to remove some of these securecookie rules that will never be effective.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, it would be great to have #15998 and #16000 merged before running this against the master.

}
localDomains.add(domain);
});
} catch (e) {
if (!(e instanceof UnsupportedRegExp)) {
throw e;
}
warn`Unsupported regexp part ${e.message} while traversing securecookie : ${JSON.stringify(securecookie)}`;
return [false, false];
}

for (const domain of localDomains) {
if (domains.indexOf(domain) === -1) {
warn`Ruleset does not cover target ${domain} for securecookie : ${JSON.stringify(securecookie)}`;
unsupportedDomains.add(domain);
}
}

// For cookies to be covered, there must be at least one rule covering the
// same domain. This is guaranteed by safeToSecureCookie(cookie) in rules.js
//
// Since securecookie.host will only match cookie.domain if there there is
// a rule covers cookie.domain. Given the target are trivial, cookie.domain
// cannot be anything other than domain.example.com and .domain.example.com
// (possibly with more leading dots) for a securecookie rule ever to take
// place.
//
// With condition (1) effective, securecookie.host should explode to either
// one of the aforementioned patterns. Otherwise, the securecookie rules
// will never be applied. Such dangling securecookie rules can be removed
// safely.
if (unsupportedDomains.size > 0) {
if (unsupportedDomains.size === localDomains.size) {
return [true, true];
}
// TODO: Can we remove partial dangling securecookie rules???
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think so. This means that the author intended at least some of the target domains to use secure cookies, we shouldn't throw out that information

fail`Tag securecookie ${JSON.stringify(securecookie)} matches ${unsupportedDomains} which are not in targets ${targets}`;
}
return [true, false];
}

if (domains.slice().sort().join('\n') !== targets.sort().join('\n')) {
if (ruleset.securecookie) {
return;
// For each securecookie rule, check if it is a static securecookie.
// If it is non-static, we do not trivialize the ruleset; Otherwise,
// we remove the securecookie if it contain only unsupported hosts.
// This removal is better done one by one to avoid 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);
source = source.replace(scRe, '');
}
} else {
// Skip this ruleset as it contain non-static securecookies
return;
}
}

source = replaceXML(source, 'target', domains.map(domain => `<target host="${domain}" />`));
Expand All @@ -194,7 +285,6 @@ files.fork().zipAll([ sources.fork(), rules ]).map(([name, source, ruleset]) =>
info`trivialized`;

return writeFile(`${rulesDir}/${name}`, source);

})
.filter(Boolean)
.parallel(10)
Expand Down