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 1 commit
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
38 changes: 37 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,6 +99,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,8 +182,43 @@ files.fork().zipAll([ sources.fork(), rules ]).map(([name, source, ruleset]) =>

domains = Array.from(domains);

function isStaticCookie(securecookie) {
if (securecookie.host === '.+' && securecookie.name === '.+') {
return true;
}

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

let localDomains = 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)}`;
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;
}

Expand Down