Annotate OCSP requests by first party domain. (Tor 13670.2)

RESOLVED FIXED in Firefox 52

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: arthuredelstein, Assigned: jhao)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [tor][domsecurity-active])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

In Bug 1264562, we isolated the OCSP cache by first-party domain (when "privacy.firstparty.isolate" is true). Tor Browser also sends each HTTP request over a Tor circuit corresponding to the by first-party domain, which means each OCSP request object needs to provide a way to retrieve that first-party domain. I imagine the right way is to create give the OCSP request with a new OriginAttribute containing the correct first-party domain.

Comment 1

2 years ago
Ideally we want to land this bug into Firefox 52, otherwise Tor has to uplift it.
Priority: -- → P1
Target Milestone: --- → mozilla52
Ethan, you made this a P1, can you please also assign it to someone and [domsecurity-active] in case it's really a P1? Thanks!
Flags: needinfo?(ettseng)

Updated

2 years ago
Assignee: nobody → jhao
Flags: needinfo?(ettseng)

Comment 3

2 years ago
(In reply to Christoph Kerschbaumer [:ckerschb (limited availability)] from comment #2)
> Ethan, you made this a P1, can you please also assign it to someone and
> [domsecurity-active] in case it's really a P1? Thanks!

Chris, thanks for the reminder.
Jonathan is the best person to take this bug.  He's on PTO and will be back in two days.
I expect he can work on this bug pretty soon.  We'll set it as [domsecurity-active] then.
Whiteboard: [tor] → [tor][domsecurity-active]
(Assignee)

Comment 4

2 years ago
(In reply to Arthur Edelstein [:arthuredelstein] from comment #0)
> In Bug 1264562, we isolated the OCSP cache by first-party domain (when
> "privacy.firstparty.isolate" is true). Tor Browser also sends each HTTP
> request over a Tor circuit corresponding to the by first-party domain, which
> means each OCSP request object needs to provide a way to retrieve that
> first-party domain. I imagine the right way is to create give the OCSP
> request with a new OriginAttribute containing the correct first-party domain.

The loading principal of the OCSP request is system principal. http://searchfox.org/mozilla-central/rev/2142de26c16c05f23e543be4fa1a651c4d29604e/security/manager/ssl/nsNSSCallbacks.cpp#98

I don't think it's a good idea to put origin attributes in the loading principal in this case.  Arthur, do you think we can put the first party domain in the triggering principal?

Christoph, can we use triggering principal to carry first party information for Tor (only when pref is on)?
Flags: needinfo?(ckerschb)
Flags: needinfo?(arthuredelstein)
(In reply to Jonathan Hao [:jhao] from comment #4)
> The loading principal of the OCSP request is system principal.
> http://searchfox.org/mozilla-central/rev/
> 2142de26c16c05f23e543be4fa1a651c4d29604e/security/manager/ssl/nsNSSCallbacks.
> cpp#98
> 
> I don't think it's a good idea to put origin attributes in the loading
> principal in this case.

What do you think would be wrong with this approach? Could we create a new loading principal, derived from the system principal, that contains the origin attributes we need?

> Arthur, do you think we can put the first party
> domain in the triggering principal?

I think for predicability, HTTP requests should offer a consistent way to retrieve the first party domain for every HTTP request. In the tests for isolating content (Bug 1264577) and favicon (Bug 1277803) requests, the method used for retrieving it is

> channel.loadInfo.originAttributes.firstPartyDomain

I wonder if it is somehow possible to ensure it works the same for OCSP requests?
Flags: needinfo?(arthuredelstein)
(Assignee)

Comment 6

2 years ago
Hmm, I'll try to do that first.
Flags: needinfo?(ckerschb)
Comment hidden (mozreview-request)
Comment on attachment 8806648 [details]
Bug 1312794 - Annotate OCSP requests by first party domain. (adapted from Tor Browser patch #13670)

https://reviewboard.mozilla.org/r/90024/#review89858

This looks good (just a few comments), but it needs some tests.

::: security/manager/ssl/nsNSSCallbacks.cpp:118
(Diff revision 1)
>  
> +  if (!mRequestSession->mFirstPartyDomain.IsEmpty()) {
> +    NeckoOriginAttributes attrs;
> +    attrs.mFirstPartyDomain =
> +      NS_ConvertUTF8toUTF16(mRequestSession->mFirstPartyDomain);
> +    

nit: this "blank" line has unnecessary spaces in it

::: security/manager/ssl/nsNSSCallbacks.cpp:119
(Diff revision 1)
> +  if (!mRequestSession->mFirstPartyDomain.IsEmpty()) {
> +    NeckoOriginAttributes attrs;
> +    attrs.mFirstPartyDomain =
> +      NS_ConvertUTF8toUTF16(mRequestSession->mFirstPartyDomain);
> +    
> +    nsCOMPtr<nsILoadInfo> loadInfo = chan->GetLoadInfo();

GetLoadInfo doesn't necessarily seem to be infallible - we should probably null-check loadInfo before using it (unless I'm wrong about the guarantees here).

::: security/manager/ssl/nsNSSCallbacks.cpp:120
(Diff revision 1)
> +    NeckoOriginAttributes attrs;
> +    attrs.mFirstPartyDomain =
> +      NS_ConvertUTF8toUTF16(mRequestSession->mFirstPartyDomain);
> +    
> +    nsCOMPtr<nsILoadInfo> loadInfo = chan->GetLoadInfo();
> +    loadInfo->SetOriginAttributes(attrs);

nit: check for failure here
Attachment #8806648 - Flags: review?(dkeeler)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 10

2 years ago
mozreview-review
Comment on attachment 8806648 [details]
Bug 1312794 - Annotate OCSP requests by first party domain. (adapted from Tor Browser patch #13670)

https://reviewboard.mozilla.org/r/90024/#review91400

Great - just some nits.

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:228
(Diff revision 2)
>  
>    // This test makes sure that OCSP cache are isolated by firstPartyDomain.
>  
> +  let gObservedCnt = 0;
> +  let protocolProxyService = Cc["@mozilla.org/network/protocol-proxy-service;1"]
> +    .getService(Ci.nsIProtocolProxyService);

nit: indent this so the first '.' is under the '[' from the previous line

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:236
(Diff revision 2)
> +  // origin attributes are aFirstPartyDomain.
> +  function startObservingChannels(aFirstPartyDomain) {
> +    // We use a dummy proxy filter to catch all channels, even those that do not
> +    // generate an "http-on-modify-request" notification.
> +    let proxyFilter = {
> +      applyFilter : function (aProxyService, aChannel, aProxy) {

nit: no space before ':'
Attachment #8806648 - Flags: review?(dkeeler) → review+
Comment hidden (mozreview-request)

Comment 12

2 years ago
Pushed by jhao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b988d56154b
Annotate OCSP requests by first party domain. (adapted from Tor Browser patch #13670) r=keeler
(Assignee)

Comment 13

2 years ago
Thank you, David.  And Arthur, if this patch doesn't do what you want, please let me know.  Thanks.
Hi Jonathan -- sorry I didn't find the time to review this one. But it does look good to me as well. Thanks so much for writing this patch!

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b988d56154b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.