Closed Bug 1320402 Opened 3 years ago Closed 3 years ago

Move url-classifier off of using appIds

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: ehsan, Assigned: dimi)

References

Details

Attachments

(1 file, 2 obsolete files)

It would be really nice if we can remove OriginAttributes::mAppId, and currently the only thing in the code base that uses appIds in any meaningful way is the urlclassifier.

Now that OriginAttributes has a firstPartyDomain field, we can define a well-known ID (maybe with a UUID inside it to prevent accidental clashes with other consumers) and move urlclassifier off of NECKO_SAFEBROWSING_APP_ID.

One question is whether we should port any data from NECKO_SAFEBROWSING_APP_ID to the well-known firstPartyDomain we define here.  If we decide to not do this, this is effectively like "clearing history" after this lands as far as the cookie jar for these requests is concerned.  That's a lot easier to implement and doesn't need anything sophisticated such as database upgrades, but if clearing the history for those requests is unacceptable, then we may need to port over some data, in which case, another question is which data to port.

gcp, what do you think?  Anybody else who we need to consult about this?
Flags: needinfo?(gpascutto)
Blocks: 1320404
(In reply to :Ehsan Akhgari from comment #0)
> One question is whether we should port any data from
> NECKO_SAFEBROWSING_APP_ID to the well-known firstPartyDomain we define here.
> If we decide to not do this, this is effectively like "clearing history"
> after this lands as far as the cookie jar for these requests is concerned. 

As I understand it, Google uses them as an identifier that's stable over a week or two in order to detect abuse and misbehaving clients.

Resetting this identifier once shouldn't cause any problems. I will however let them know that this is happening.
Flags: needinfo?(gpascutto)
Bug 1305479 is trying to replace the appId with userContextId in SafeBrowsing
Duplicate of this bug: 1305479
Priority: -- → P2
I slightly prefer using firstPartyDomain with a unique string rather than using userContextId.

Andrea, what do you think?  Which one do you prefer?
Flags: needinfo?(amarchesini)
Blocks: 1321912
FirstPartyDomain is better. But please, use a URL. We should force firstPartyDomain to be an URL. I'll file a bug for this.
Flags: needinfo?(amarchesini)
Hi Ehasn,
Do you mind if i take this bug if you are not working on it ?
Flags: needinfo?(ehsan)
Go for it!

Some notes about what I was planning to do for this:

* Switch NECKO_SAFEBROWSING_APP_ID to a special well-known value for firstPartyDomain, something like "chrome://safe-browsing".
* Write a cookie migration pass to go over the list of the cookies we have stored, looking for ones with "appId=4294967294" in the originAttributes field, and port them to "firstPartyDomain=chrome://safe-browsing".  This would essentially port the possible exiting cookies that people may have stored in their local profile to the new well-known firstPartyDomain value to alleviate comment 1.

I have written a patch for bug 1321912 which I will post now which you can use as an example of the changes required to implement a cookie database migration pass.
Assignee: nobody → dlee
Flags: needinfo?(ehsan)
Regarding the cookie migration, I checked with Google and they are ok with us resetting everybody's Safe Browsing cookies as a one-off.

So I think we can simply clear the existing cookies (or leave them there until they expire).
Status: NEW → ASSIGNED
Hi Ehsan,
Could you help check this patch? thanks. I'll ask gcp or francois to review after you r+ the patch.

I didn't handle cookie migration according to Comment 8.
Also the reason that i didn't use "chrome://" as you suggested in safebrowsing first-party domain
is becuase current first-party domain doesn't include "scheme". Adding scheme will trigger assertion[1].

[1] https://dxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.cpp#175
(In reply to François Marier [:francois] from comment #8)
> Regarding the cookie migration, I checked with Google and they are ok with
> us resetting everybody's Safe Browsing cookies as a one-off.
> 
> So I think we can simply clear the existing cookies (or leave them there
> until they expire).

OK, sure, that's simpler and better indeed.
Comment on attachment 8819805 [details]
Bug 1320402 - Part 1 : Remove appId from NeckoOriginAttribute constructor.

https://reviewboard.mozilla.org/r/99470/#review99940

When landing you should probably fold both patches into the same patch since this isn't standalone.
Attachment #8819805 - Flags: review?(ehsan) → review+
Comment on attachment 8819806 [details]
Bug 1320402 - Part 2 : Replace NECKO_SAFEBROWSING_APP_ID to a well-known first-party domain value.

https://reviewboard.mozilla.org/r/99472/#review99942

Looks great overall, but I have several small comments below.

::: caps/nsIScriptSecurityManager.idl:266
(Diff revision 2)
>  
>      /**
> +     * This is a unique const value used by safebrowsing to separate
> +     * its cookie. Value is defined in nsNetUtil.h.
> +     */
> +    readonly attribute DOMString safebrowsingFirstPartyDomain;

I think it's important for us to *not* expose the value that we use for this purpose to add-ons.  Therefore, I think you should remove this attribute.

::: docshell/base/LoadContext.h:28
(Diff revision 2)
>   *
>   * Note: this is not the "normal" or "original" nsILoadContext.  That is
>   * typically provided by nsDocShell.  This is only used when the original
>   * docshell is in a different process and we need to copy certain values from
>   * it.
>   *

Nit: might as well remove this line also!

::: netwerk/base/nsNetUtil.h:678
(Diff revision 2)
> -// special app id reserved for separating the safebrowsing cookie
> -#define NECKO_SAFEBROWSING_APP_ID UINT32_MAX - 1
>  
> +// Unique first-party domain for separating the safebrowsing cookie
> +#define NECKO_SAFEBROWSING_FIRST_PARTY_DOMAIN \
> +  "safebrowsing.86868755-6b82-4842-b301-72671a0db32e.com"

This is "magical enough" but at least in theory one could go ahead and register this domain.  ;-)

How about "safebrowsing.86868755-6b82-4842-b301-72671a0db32e.mozilla"?

::: netwerk/test/unit/test_cookiejars_safebrowsing.js:155
(Diff revision 2)
>  
>  add_test(function test_safebrowsing_cookie() {
>  
>    var cookieName = 'sbCookie_id4294967294';
> -  var originAttributes = new OriginAttributes(Ci.nsIScriptSecurityManager.SAFEBROWSING_APP_ID, false, 0);
> +  var originAttributes = new OriginAttributes(0, false, 0);
> +  originAttributes.firstPartyDomain = secMan.safebrowsingFirstPartyDomain;

Here we can just hardcode our magic string.

::: toolkit/components/downloads/ApplicationReputation.cpp:1283
(Diff revision 2)
>  
>    nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo();
>    if (loadInfo) {
> -    loadInfo->SetOriginAttributes(
> -      mozilla::NeckoOriginAttributes(false));
> +    mozilla::NeckoOriginAttributes neckoAttrs(false);
> +    neckoAttrs.mFirstPartyDomain.AssignLiteral(NECKO_SAFEBROWSING_FIRST_PARTY_DOMAIN);
> +    loadInfo->SetOriginAttributes(neckoAttrs);

Why aren't you setting neckoAttrs' mFirstPartyDomain here?

::: toolkit/components/downloads/ApplicationReputation.cpp:1304
(Diff revision 2)
>    // Set the Safebrowsing cookie jar, so that the regular Google cookie is not
>    // sent with this request. See bug 897516.
>    DocShellOriginAttributes attrs;
> -  attrs.mAppId = NECKO_SAFEBROWSING_APP_ID;
> +  attrs.mFirstPartyDomain.AssignLiteral(NECKO_SAFEBROWSING_FIRST_PARTY_DOMAIN);
>    nsCOMPtr<nsIInterfaceRequestor> loadContext = new mozilla::LoadContext(attrs);
>    rv = mChannel->SetNotificationCallbacks(loadContext);

I wonder if we would need to do this at all if we set the first party domain on the channel's loadInfo.  Can you try to remove this whole paragraph and see if that breaks things?

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:186
(Diff revision 2)
> -  // cookies in a separate jar.
> +  // safebrowsing cookies in a separate jar.
>    DocShellOriginAttributes attrs;
> -  attrs.mAppId = NECKO_SAFEBROWSING_APP_ID;
> +  attrs.mFirstPartyDomain.AssignLiteral(NECKO_SAFEBROWSING_FIRST_PARTY_DOMAIN);
>    nsCOMPtr<nsIInterfaceRequestor> sbContext = new mozilla::LoadContext(attrs);
>    rv = mChannel->SetNotificationCallbacks(sbContext);
>    NS_ENSURE_SUCCESS(rv, rv);

Similarly I wonder if we can eliminate this paragraph as well?
Attachment #8819806 - Flags: review?(ehsan) → review-
Thanks for review! I'll merge both patches when push.

(In reply to :Ehsan Akhgari from comment #15)
> ::: toolkit/components/downloads/ApplicationReputation.cpp:1283
> (Diff revision 2)
> >  
> >    nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo();
> >    if (loadInfo) {
> > -    loadInfo->SetOriginAttributes(
> > -      mozilla::NeckoOriginAttributes(false));
> > +    mozilla::NeckoOriginAttributes neckoAttrs(false);
> > +    neckoAttrs.mFirstPartyDomain.AssignLiteral(NECKO_SAFEBROWSING_FIRST_PARTY_DOMAIN);
> > +    loadInfo->SetOriginAttributes(neckoAttrs);
> 
> Why aren't you setting neckoAttrs' mFirstPartyDomain here?
> 

I am not sure what do you mean here, neckoAttrs'mFirstPartyDomain is set before calling SetOriginAttributes, could you explain more ? thanks!

> ::: toolkit/components/downloads/ApplicationReputation.cpp:1304
> (Diff revision 2)
> >    // Set the Safebrowsing cookie jar, so that the regular Google cookie is not
> >    // sent with this request. See bug 897516.
> >    DocShellOriginAttributes attrs;
> > -  attrs.mAppId = NECKO_SAFEBROWSING_APP_ID;
> > +  attrs.mFirstPartyDomain.AssignLiteral(NECKO_SAFEBROWSING_FIRST_PARTY_DOMAIN);
> >    nsCOMPtr<nsIInterfaceRequestor> loadContext = new mozilla::LoadContext(attrs);
> >    rv = mChannel->SetNotificationCallbacks(loadContext);
> 
> I wonder if we would need to do this at all if we set the first party domain
> on the channel's loadInfo.  Can you try to remove this whole paragraph and
> see if that breaks things?
> 

Yes, you are right, I think we don't need this now. Originally it was added because |NS_CompareLoadInfoAndLoadContext| will check if appId is matched between LoadContext and LoadInfo, but we are using firstPartyDomain now.
Also I have checked that testcase can be run successfully after removing this paragraph.
(In reply to Dimi Lee[:dimi][:dlee] from comment #18)
> Thanks for review! I'll merge both patches when push.
> 
> (In reply to :Ehsan Akhgari from comment #15)
> > ::: toolkit/components/downloads/ApplicationReputation.cpp:1283
> > (Diff revision 2)
> > >  
> > >    nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo();
> > >    if (loadInfo) {
> > > -    loadInfo->SetOriginAttributes(
> > > -      mozilla::NeckoOriginAttributes(false));
> > > +    mozilla::NeckoOriginAttributes neckoAttrs(false);
> > > +    neckoAttrs.mFirstPartyDomain.AssignLiteral(NECKO_SAFEBROWSING_FIRST_PARTY_DOMAIN);
> > > +    loadInfo->SetOriginAttributes(neckoAttrs);
> > 
> > Why aren't you setting neckoAttrs' mFirstPartyDomain here?
> > 
> 
> I am not sure what do you mean here, neckoAttrs'mFirstPartyDomain is set
> before calling SetOriginAttributes, could you explain more ? thanks!

Hmm.  What I said clearly makes no sense.  :-)  Sorry about that!

I think what happened here was that I didn't see the |neckoAttrs.mFirstPartyDomain.AssignLiteral(NECKO_SAFEBROWSING_FIRST_PARTY_DOMAIN);| line, at least that's what I remember I thought your code is doing.  My mistake.

> > ::: toolkit/components/downloads/ApplicationReputation.cpp:1304
> > (Diff revision 2)
> > >    // Set the Safebrowsing cookie jar, so that the regular Google cookie is not
> > >    // sent with this request. See bug 897516.
> > >    DocShellOriginAttributes attrs;
> > > -  attrs.mAppId = NECKO_SAFEBROWSING_APP_ID;
> > > +  attrs.mFirstPartyDomain.AssignLiteral(NECKO_SAFEBROWSING_FIRST_PARTY_DOMAIN);
> > >    nsCOMPtr<nsIInterfaceRequestor> loadContext = new mozilla::LoadContext(attrs);
> > >    rv = mChannel->SetNotificationCallbacks(loadContext);
> > 
> > I wonder if we would need to do this at all if we set the first party domain
> > on the channel's loadInfo.  Can you try to remove this whole paragraph and
> > see if that breaks things?
> > 
> 
> Yes, you are right, I think we don't need this now. Originally it was added
> because |NS_CompareLoadInfoAndLoadContext| will check if appId is matched
> between LoadContext and LoadInfo, but we are using firstPartyDomain now.
> Also I have checked that testcase can be run successfully after removing
> this paragraph.

Nice, makes sense.
Comment on attachment 8819806 [details]
Bug 1320402 - Part 2 : Replace NECKO_SAFEBROWSING_APP_ID to a well-known first-party domain value.

https://reviewboard.mozilla.org/r/99472/#review100280

r=me with the below addressed.  Thanks for fixing this!

::: netwerk/test/unit/test_cookiejars_safebrowsing.js:153
(Diff revision 3)
>  add_test(function test_safebrowsing_cookie() {
>  
>    var cookieName = 'sbCookie_id4294967294';
> -  var originAttributes = new OriginAttributes(Ci.nsIScriptSecurityManager.SAFEBROWSING_APP_ID, false, 0);
> +  var originAttributes = new OriginAttributes(0, false, 0);
> +  originAttributes.firstPartyDomain =
> +    "safebrowsing.86868755-6b82-4842-b301-72671a0db32e.mozilla";

Please add a comment to where NECKO_SAFEBROWSING_FIRST_PARTY_DOMAIN is defined saying that if that value is changed, this code in test_cookiejars_safebrowsing.js must also changed, and please add a similar comment here to make copy/pasting like this a little bit less bad.

::: toolkit/components/url-classifier/content/xml-fetcher.js:45
(Diff revision 3)
>  this.PROT_XMLFetcher = function PROT_XMLFetcher() {
>    this.debugZone = "xmlfetcher";
>    this._request = PROT_NewXMLHttpRequest();
>    // implements nsILoadContext
> -  this.appId = Ci.nsIScriptSecurityManager.SAFEBROWSING_APP_ID;
> +  this.firstPartyDomain =
> +    "safebrowsing.86868755-6b82-4842-b301-72671a0db32e.mozilla";

This location also needs comments similar to the above case.

::: toolkit/components/url-classifier/content/xml-fetcher.js:70
(Diff revision 3)
>      this._request.abort();                // abort() is asynchronous, so
>      this._request = PROT_NewXMLHttpRequest();
>      this._callback = callback;
>      var asynchronous = true;
>      this._request.loadInfo.originAttributes = {
>        appId: this.appId,

You need to update this part too.  It's pretty worrying that missing this didn't cause a test failure.  Can you also file a follow-up for investigating why this path is untested and adding a test for it?  Thanks!
Attachment #8819806 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari from comment #20)
> ::: toolkit/components/url-classifier/content/xml-fetcher.js:70
> (Diff revision 3)
> >      var asynchronous = true;
> >      this._request.loadInfo.originAttributes = {
> >        appId: this.appId,
> 
> You need to update this part too.  It's pretty worrying that missing this
> didn't cause a test failure.  Can you also file a follow-up for
> investigating why this path is untested and adding a test for it?  Thanks!

File bug 1324962
Comment on attachment 8819806 [details]
Bug 1320402 - Part 2 : Replace NECKO_SAFEBROWSING_APP_ID to a well-known first-party domain value.

https://reviewboard.mozilla.org/r/99472/#review101836

::: netwerk/base/nsNetUtil.h:678
(Diff revision 4)
> -// special app id reserved for separating the safebrowsing cookie
> -#define NECKO_SAFEBROWSING_APP_ID UINT32_MAX - 1
>  
> +// Unique first-party domain for separating the safebrowsing cookie.
> +// Note if this value is changed, code in test_cookiejars_safebrowsing.js and
> +// xml-fetcher.js should also be changed.

Can remove the xml-fetcher remark.
Attachment #8819806 - Flags: review?(gpascutto) → review+
Attachment #8819805 - Attachment is obsolete: true
Attachment #8819806 - Attachment is obsolete: true
Attached patch (r+) PatchSplinter Review
- Merge patch according to comment 14.
- Address comment 24
Attachment #8823177 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/306a058214a2
Move url-classifier off of using appIds. r=ehsan, gcp
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/306a058214a2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.