Closed Bug 998007 Opened 6 years ago Closed 6 years ago

use safebrowsing app id for application reputation remote query

Categories

(Core :: DOM: Security, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: mmc, Assigned: mmc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Assignee: nobody → mmc
Status: NEW → ASSIGNED
Attachment #8408552 - Attachment is obsolete: true
Comment on attachment 8408553 [details] [diff] [review]
Use Safebrowsing app id for application reputation checks (

Review of attachment 8408553 [details] [diff] [review]:
-----------------------------------------------------------------

I can't include anything that relies on ipc/chromium in the toolkit/components/downloads directory, because it interacts badly with existing Windows code. Refactor LoadContext.h not to depend on SerializedLoadContext which depends on ipc/chromium.
Attachment #8408553 - Flags: review?(mozilla)
Attachment #8408553 - Flags: review?(gpascutto)
Comment on attachment 8408553 [details] [diff] [review]
Use Safebrowsing app id for application reputation checks (

Review of attachment 8408553 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Olli,

I notice you previously did some reviews on LoadContext.h. If you have time, please review that part of it.

Thanks,
Monica
Attachment #8408553 - Flags: review?(bugs)
Comment on attachment 8408553 [details] [diff] [review]
Use Safebrowsing app id for application reputation checks (

Review of attachment 8408553 [details] [diff] [review]:
-----------------------------------------------------------------

I don't really know this code but the mIsNotNull thing looks really strange.

::: docshell/base/SimpleLoadContext.h
@@ +46,5 @@
> +    , mUsePrivateBrowsing(aUsePrivateBrowsing)
> +    , mUseRemoteTabs(aUseRemoteTabs)
> +    , mIsInBrowserElement(aIsInBrowserElement)
> +#ifdef DEBUG
> +    , mIsNotNull(true)

This makes no sense. If you're going to fix it to true, then you should remove all the debugging checks in SimpleLoadContext that assert if it is null. But if they're there for a reason, why not pass it down?
Attachment #8408553 - Flags: review?(gpascutto) → review-
Comment on attachment 8408553 [details] [diff] [review]
Use Safebrowsing app id for application reputation checks (

Review of attachment 8408553 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/SimpleLoadContext.h
@@ +46,5 @@
> +    , mUsePrivateBrowsing(aUsePrivateBrowsing)
> +    , mUseRemoteTabs(aUseRemoteTabs)
> +    , mIsInBrowserElement(aIsInBrowserElement)
> +#ifdef DEBUG
> +    , mIsNotNull(true)

It's only ever not null in LoadContext.h. I don't think I can only have mIsNotNull in LoadContext.h, though because it's just a constructor for SimpleLoadContext that knows about SerializedLoadContext, and SimpleLoadContext.cpp is the only implementation of them both.

This is the original:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/LoadContext.h#58
Comment on attachment 8408553 [details] [diff] [review]
Use Safebrowsing app id for application reputation checks (

Review of attachment 8408553 [details] [diff] [review]:
-----------------------------------------------------------------

WriteUp from IRC chat:
It seems you are only changing LoadContext to SimpleLoadContext, right? Can you summarize real quick, why using the regular LoadContext does not do the trick? You said, it required some IPC/chromium changes that undefed some windows apis? What for example? Still don't fully understand why we can't use the regular LoadContext.
Other than that, this looks good! I still don't think i should be reviewing this, but of course am available for feedback.

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +769,5 @@
>      NS_LITERAL_CSTRING("POST"), false);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Set the Safebrowsing cookie jar.
> +  nsCOMPtr<nsIInterfaceRequestor> loadContext =

Nit: Please extend that comment; why are we setting the safebrowsing id here.
Attachment #8408553 - Flags: review?(mozilla)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> Comment on attachment 8408553 [details] [diff] [review]
> Use Safebrowsing app id for application reputation checks (
> 
> Review of attachment 8408553 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> WriteUp from IRC chat:
> It seems you are only changing LoadContext to SimpleLoadContext, right? Can
> you summarize real quick, why using the regular LoadContext does not do the
> trick? You said, it required some IPC/chromium changes that undefed some
> windows apis? What for example? Still don't fully understand why we can't
> use the regular LoadContext.
> Other than that, this looks good! I still don't think i should be reviewing
> this, but of course am available for feedback.
> 
> ::: toolkit/components/downloads/ApplicationReputation.cpp
> @@ +769,5 @@
> >      NS_LITERAL_CSTRING("POST"), false);
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +  // Set the Safebrowsing cookie jar.
> > +  nsCOMPtr<nsIInterfaceRequestor> loadContext =
> 
> Nit: Please extend that comment; why are we setting the safebrowsing id here.

Thanks, Chris -- I mostly asked you since you were the original author of the SAFEBROWSING_APP_ID stuff :)

Including LoadContext forces inclusion of ipc/chromium which contains #undef CreateEvent (https://mxr.mozilla.org/mozilla-central/search?string=CreateEvent&find=.h%24&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central) which breaks nsDownloadScanner in this directory with "unknown identifier: CreateEvent".
Isn't that something that you could work around by reordering the order of includes and potentially disabling unified building for that file? If the #undef happens after the place where CreateEvent is used, it should work?
(In reply to Gian-Carlo Pascutto [:gcp] from comment #9)
> Isn't that something that you could work around by reordering the order of
> includes and potentially disabling unified building for that file? If the
> #undef happens after the place where CreateEvent is used, it should work?

I tried including a bunch of these headers at the top of DownloadScanner but it still failed.

http://msdn.microsoft.com/en-us/library/windows/desktop/ms682396(v=vs.85).aspx
Comment on attachment 8408553 [details] [diff] [review]
Use Safebrowsing app id for application reputation checks (

This feels complicated. To have LoadContext and SimpleLoadContext.
Could you not just un-inline LoadContext's ctor and 
forward declare IPC::SerializedLoadContext in the .h file?
Attachment #8408553 - Flags: review?(bugs) → review-
Here's the approach from my comment, i.e. disable the unified build for that file so the #undef doesn't spread: https://tbpl.mozilla.org/?tree=Try&rev=460d7fde7ed4
Not sure if gcp can review this since he modified the patch.
Attachment #8408553 - Attachment is obsolete: true
Attachment #8410503 - Flags: review?(sstamm)
Comment on attachment 8410503 [details] [diff] [review]
Use safebrowsing app-id for application reputation check

Review of attachment 8410503 [details] [diff] [review]:
-----------------------------------------------------------------

This looks somewhat reasonable.  Can you test it easily?  I'd like ckerschb to review this as well, since he's familiar with the code.
Attachment #8410503 - Flags: review?(mozilla)
Comment on attachment 8410503 [details] [diff] [review]
Use safebrowsing app-id for application reputation check

Review of attachment 8410503 [details] [diff] [review]:
-----------------------------------------------------------------

gps: can you sanity-check the changes to moz.build?
Attachment #8410503 - Flags: feedback?(gps)
I think the cookie separation is tested pretty well in the cookie tests that ckerschb already wrote. Is that sufficient?

https://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_cookiejars_safebrowsing.js
Comment on attachment 8410503 [details] [diff] [review]
Use safebrowsing app-id for application reputation check

Review of attachment 8410503 [details] [diff] [review]:
-----------------------------------------------------------------

If I am understanding everything correctly, we use the SAFEBROWSING_APP_ID since Google maintains an extension to Safebrowsing that tracks binary file reputation and we want those cookies to be separated into the same jar as safebrowsing cookies, correct? As you mentioned, we already have tests that verify correct cookie-jar separation for safebrowsing cookies, so in my opinion that should be sufficient in terms of testing the cookie-jar separation. I assume you have a testcase the verifies correct behavior of the application reputation service in a different bug, right?

As I mentioned in a previous comment, maybe you want to extend the comment in ApplicationReputation.cpp a little, before you set the SAFEBROWSING_APP_ID.
Other than that, this looks good to me and ready to go.
Attachment #8410503 - Flags: review?(mozilla) → review+
Comment on attachment 8410503 [details] [diff] [review]
Use safebrowsing app-id for application reputation check

Thanks, ckerschb!

From froydnj, this counts as a trivial build change that does not require build peer review. Clearing flags.

<mmc> glandium, does a build change to disable unified builds in toolkit/components/url-classifier require build peer review?
10:47 AM <froydnj> mmc: if you're just shuffling things between SOURCES and UNIFIED_SOURCES, I don't think it does
10:48 AM <froydnj> since that falls under "adding and removing from existing moz.build lists"
Attachment #8410503 - Flags: review?(sstamm)
Attachment #8410503 - Flags: feedback?(gps)
Great, land it.
Attachment #8410503 - Attachment is obsolete: true
Comment on attachment 8411200 [details] [diff] [review]
Use SafeBrowsing app id for application reputation checks.

Review of attachment 8411200 [details] [diff] [review]:
-----------------------------------------------------------------

Updated comment to be:

+  // Set the Safebrowsing cookie jar, so that the regular Google cookie is not
+  // sent with this request. See bug 897516.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17)
> If I am understanding everything correctly, we use the SAFEBROWSING_APP_ID
> since Google maintains an extension to Safebrowsing that tracks binary file
> reputation and we want those cookies to be separated into the same jar as
> safebrowsing cookies, correct?

Yes, same as your previous bug for safebrowsing cookie isolation.

> As you mentioned, we already have tests that
> verify correct cookie-jar separation for safebrowsing cookies, so in my
> opinion that should be sufficient in terms of testing the cookie-jar
> separation. I assume you have a testcase the verifies correct behavior of
> the application reputation service in a different bug, right?

Yes, existing tests are in the same directory under test_app_rep.js and test_app_rep_windows.js.

Marking checkin-needed since the tree is closed. Try was green at https://tbpl.mozilla.org/?tree=Try&rev=460d7fde7ed4
Attachment #8411200 - Flags: review+
Attachment #8411200 - Flags: checkin+
Comment on attachment 8411200 [details] [diff] [review]
Use SafeBrowsing app id for application reputation checks.

Review of attachment 8411200 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3954977ec62
Attachment #8411200 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/c3954977ec62
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.