Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

HSTS Fingerprinting #13437

Merged
merged 14 commits into from
Mar 27, 2018
Merged

HSTS Fingerprinting #13437

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8fd9fce
lint
jumde Mar 13, 2018
2d95a75
update
jumde Mar 14, 2018
4b6f90a
lint
jumde Mar 14, 2018
1f71cee
lint
jumde Mar 14, 2018
e582571
lint
jumde Mar 14, 2018
a399cb9
lint
jumde Mar 14, 2018
3a8d985
compare hosts not urls
jumde Mar 14, 2018
27ba205
compare hosts not urls
jumde Mar 14, 2018
c40994b
update per comments
jumde Mar 14, 2018
3a2640b
update
jumde Mar 14, 2018
6b40aa7
added tests for HSTS data cleanup
jumde Mar 14, 2018
0d90ece
lint
jumde Mar 14, 2018
97ab150
easy tests
jumde Mar 14, 2018
96c4fad
update
jumde Mar 21, 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
20 changes: 20 additions & 0 deletions app/filtering.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,17 @@ function registerForHeadersReceived (session, partition) {
muonCb({ cancel: true })
return
}

let parsedTargetUrl = urlParse(details.url || '')
let parsedFirstPartyUrl = urlParse(firstPartyUrl)
let trackingHeaders = ['Strict-Transport-Security', 'Expect-CT', 'Public-Key-Pins']
Copy link
Member

Choose a reason for hiding this comment

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

these should be const not let

Copy link
Member

Choose a reason for hiding this comment

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

minor: rename trackingHeaders to something like trackableSecurityHeaders so it's more clear why we are deleting them

Copy link
Member

Choose a reason for hiding this comment

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

i think these should also include https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Public-Key-Pins-Report-Only

if (isThirdPartyHost(parsedFirstPartyUrl.hostname, parsedTargetUrl.hostname)) {
trackingHeaders.forEach(function (header) {
delete details.responseHeaders[header]
delete details.responseHeaders[header.toLowerCase()]
Copy link
Member

Choose a reason for hiding this comment

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

this line shouldn't be needed because details.responseHeaders capitalization is already normalized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked yesterday, and I did see some headers in lowercase. Strict-Transport-Security and strict-transport-security

Copy link
Member

@diracdeltas diracdeltas Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

thanks for checking - i think i was confused because Cookie and Referer have only a single capitalization mode, but that's because they are request headers not response headers

this seems potentially problematic because HTTP headers are case-insensitive according to the RFC. so someone could create a header called sTriCT-TransPort-SecUrITY and the browser would still process it (in theory)

})
}

for (let i = 0; i < headersReceivedFilteringFns.length; i++) {
let results = headersReceivedFilteringFns[i](details, isPrivate)
if (!module.exports.isResourceEnabled(results.resourceName, firstPartyUrl, isPrivate)) {
Expand Down Expand Up @@ -844,6 +855,15 @@ module.exports.clearStorageData = () => {
}
}

module.exports.clearHSTSData = () => {
for (let partition in registeredSessions) {
let ses = registeredSessions[partition]
setImmediate(() => {
ses.clearHSTSData.bind(ses)(() => {})
Copy link
Member

Choose a reason for hiding this comment

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

i think this should just be ses.clearHSTSData.bind(ses)() since clearHSTSData doesn't take a callback. (or even just ses.clearHSTSData() - not sure why the bind is necessary)

})
}
}

/**
* Clears all session caches.
*/
Expand Down
6 changes: 6 additions & 0 deletions app/sessionStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,8 +825,14 @@ module.exports.runPreMigrations = (data) => {
if (data.lastAppVersion) {
// Force WidevineCdm to be upgraded when last app version <= 0.18.25
let runWidevineCleanup = false
let runHSTSCleanup = false

try { runWidevineCleanup = compareVersions(data.lastAppVersion, '0.18.25') < 1 } catch (e) {}
try { runHSTSCleanup = compareVersions(data.lastAppVersion, '0.22.00') < 1 } catch (e) {}

if (runHSTSCleanup) {
filtering.clearHSTSData()
}

if (runWidevineCleanup) {
const fs = require('fs-extra')
Expand Down
153 changes: 100 additions & 53 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/unit/app/sessionStoreTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ describe('sessionStore unit tests', function () {
}
}
const fakeFiltering = {
clearHSTSData: () => {},
clearStorageData: () => {},
clearCache: () => {},
clearHistory: () => {}
Expand Down