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
Merged

Detect static securecookies to trivialize more rules #16029

Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
77f08cf
Detect static securecookies to trivialize more rules
Jul 11, 2018
ca2844f
Add comments on static cookie terminology
Jul 12, 2018
4bbd731
Remove securecookie tags if they are not effective
Jul 12, 2018
ca52ba2
Add comments on the safety removing dangling securecookies
Jul 13, 2018
5c149aa
Fix a bug for handling the dangling securecookie rules
Aug 7, 2018
a37df21
semistandard --fix explode-regexp.js trivialize-rules.js
Aug 7, 2018
db2b8b6
Revert "semistandard --fix explode-regexp.js trivialize-rules.js"
Aug 7, 2018
e8f27ee
Remove unused variables and fix syntax manually
Aug 9, 2018
6e82204
Update comments and remove unnecessary checks
Aug 13, 2018
e5ad3c5
Update comments on partial dangling securecookie
Aug 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 82 additions & 1 deletion utils/trivialize-rules/trivialize-rules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,11 @@ 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.$);

let shouldRemoveSecurecookies = false;

if (rules.length === 1 && isTrivial(rules[0])) {
return;
}
Expand Down Expand Up @@ -181,11 +184,89 @@ 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;
}

if (!securecookie.host.startsWith('^') || !securecookie.host.endsWith('$')) {
return 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 •
edited
Loading

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;
}

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) {
shouldRemoveSecurecookies = true;
Copy link
Contributor

@bcyphers bcyphers Aug 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If any one of the securecookies is unsupported, this will cause all of them to be removed. I don't think that's the intended behavior.

For example, for the following XML, the script will trivialize the file and remove all securecookies:

<target host="a.foo.com" />
<supercookie host="a.foo.com" />
<supercookie host="b.foo.com" />
...

But if the two securecookie entries are combined into one (<securecookie host="^(a|b)\.foo\.com$" />), the script will refuse to trivialize the XML.

It seems like we should remove one securecookie entry at a time, not drop them all at once. If that's risky (not sure why it would be), we can just refuse to trivialize any XML with unsupported securecookie domains.

return true;
}
return false;
}

return true;
}

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

if (shouldRemoveSecurecookies) {
source = replaceXML(source, 'securecookie', []);
}

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

Expand Down