HSTS Fingerprinting #13437
HSTS Fingerprinting #13437
Conversation
app/filtering.js
Outdated
| @@ -364,6 +364,12 @@ function registerForHeadersReceived (session, partition) { | |||
| muonCb({ cancel: true }) | |||
| return | |||
| } | |||
|
|
|||
| if (firstPartyUrl !== details.url) { | |||
There was a problem hiding this comment.
please use isThirdPartyHost instead on the URL hostnames since we want to check this based on the host, not the full URL
Codecov Report
@@ Coverage Diff @@
## master #13437 +/- ##
==========================================
+ Coverage 56.25% 56.59% +0.33%
==========================================
Files 283 284 +1
Lines 28353 28764 +411
Branches 4674 4749 +75
==========================================
+ Hits 15950 16278 +328
- Misses 12403 12486 +83
|
app/filtering.js
Outdated
|
|
||
| let parsedTargetUrl = urlParse(details.url || '') | ||
| let parsedFirstPartyUrl = urlParse(firstPartyUrl) | ||
| let trackingHeaders = ['Strict-Transport-Security', 'Expect-CT', 'Public-Key-Pins'] |
There was a problem hiding this comment.
these should be const not let
There was a problem hiding this comment.
minor: rename trackingHeaders to something like trackableSecurityHeaders so it's more clear why we are deleting them
There was a problem hiding this comment.
i think these should also include https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Public-Key-Pins-Report-Only
app/filtering.js
Outdated
| if (isThirdPartyHost(parsedFirstPartyUrl.hostname, parsedTargetUrl.hostname)) { | ||
| trackingHeaders.forEach(function (header) { | ||
| delete details.responseHeaders[header] | ||
| delete details.responseHeaders[header.toLowerCase()] |
There was a problem hiding this comment.
this line shouldn't be needed because details.responseHeaders capitalization is already normalized
There was a problem hiding this comment.
I checked yesterday, and I did see some headers in lowercase. Strict-Transport-Security and strict-transport-security
There was a problem hiding this comment.
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 partition in registeredSessions) { | ||
| let ses = registeredSessions[partition] | ||
| setImmediate(() => { | ||
| ses.clearHSTSData.bind(ses)(() => {}) |
There was a problem hiding this comment.
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)
|
lgtm. will test after brave/muon#532 is released |
|
@diracdeltas this is now ready for review, as Muon code was merged. We don't have a new Muon build yet- so if you'd like to try that out, we'll have to wait for it to finish |
|
Awesome work. Here is the test plan I did:
|
Revert "Merge pull request #13437 from brave/hsts-fingerprinting"
Revert "Merge pull request #13437 from brave/hsts-fingerprinting"
Revert "Merge pull request #13437 from brave/hsts-fingerprinting"
Fixes: #12223
Test Plan:
Alternative test plan if proxy site is not working
repeat steps above, but replace safebrowsing-proxy.com with https://jsfiddle.net/pqwdgr5x/5/