1270678 - Make sure favicon requests obey OriginAttributes

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Tanvi Vyas[:tanvi]]

Make sure that favicons are properly segregated based on usercontextID.  If not, fix the code.  If so, write a test.

+++ This bug was initially created as a clone of Bug #1264564 +++

Comment 1

[Tanvi Vyas[:tanvi]]

Kamil or Paul,
please test to see that requests for favicons in different user context only have the cookies associated with the user context they are in.  A reference to the code would also be great if you can find one.

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I just tested this, and this seems like an issue. From testing it looks like the cookie that is sent for favicon request is any cookie set outside containers (ie not in work,personal etc, just a regular tab) I guess that means the request is made from a context with no userContext set. Digging for the code now...

Comment 3

[Paul Theriault [:pauljt] (no longer reading bugmail)]

This probably depends on or is at least related to bug 1227289.

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Actually Christoph informs that we aren't using asyncopen2 yet for loading favicons (bug 1196013). This may or may not fix this bug, it depends on exactly how the cookie is attached. I haven't been able to track this down yet.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Whoever is going to fix that bug, *please* make sure to add a test to our codebase because historically favicons have always been an attack vector within browsers.

Comment 6

[Kamil Jozwiak [:kjozwiak]]

Thanks for going through this Paul! Someone in IRC mentioned that the favicon service might be running from XUL? This could be similar to the awesome bar autocomplete issue that's also running using XUL via bug # 1244340 comment #11?

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Ok Im closer, but still looking. In Tabbrowser.xml [1] the correct principal seems to be passed to the setIcon function. Breaking at in this function while in the "banking" container I see:


XPCWrappedNative_NoHelper { URI: XPCWrappedNative_NoHelper, isNullPrincipal: false, cspJSON: "{}", jarPrefix: "", originAttributes: Object, origin: "https://misuse.co^userContextId=2", originNoSuffix: "https://misuse.co", originSuffix: "^userContextId=2", baseDomain: "misuse.co", appStatus: 0 }

as the loading principal, right before:

this.mFaviconService.setAndFetchFaviconForPage(
                browser.currentURI, aURI, false, loadType, null, loadingPrincipal);
            }

Still digging...

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#856

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Im still stuck here - I'll try Bill tomorrow. From talking with Christoph, there definitely is a problem relating to session restore see [1] and also this comment in the code [2].

But in the regular load case, from "setIcon" in the tabbrowser.xml, I traced the code down into nsFaviconService::SetAndFetchFaviconForPage then into the RunnableMethod(?) AsyncFetchAndSetIconForPage, which has a task called AsyncFetchAndSetIconForPage::FetchFromNetwork() [4]. In this method,NS_NewChannel is called, with the mLoadingPrincipal from above (afaict). 

I don't know what's next? I assume if you pass the correct principal to the channel then it should have the correct cookie, but maybe there is code here that isn't origin aware? I'll escalate to folks in US when they wake up.







[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1119386#c55 
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#869
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsFaviconService.cpp#209
[4] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/FaviconHelpers.cpp#402
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/FaviconHelpers.cpp#419

Comment 9

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Though I very much wonder if the patch in bug 1196013 will fix this initial problem:

https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1196013&attachment=8746679

There is then the first issue (session restore etc).

Comment 10

[Tim Huang[:timhuang]]

The reason why the favicon does not obey originAttributes is that the loading channel of the favicon does not have a loadcontext. This causes NS_GetOriginAttributes()[1] fails and the nsCookieService will utilize the default user context instead. 

But I don't know why the channel of loading favicon doesn't have a loadcontext. I will keep digging on it.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#1265

Comment 11

[Tim Huang[:timhuang]]

After better understanding the code here, it appears that it is not the channel doesn't have the load context. It is the notification callbacks doesn't have the load context, and the notification callback is AsyncFetchAndSetIconForPage[1] for the loading channel of the favicon.

BTW, the code which calls NS_GetOriginAttributes() is located at nsCookieService.cpp[2].

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/FaviconHelpers.h#101
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#1906

Comment 12

[Tim Huang[:timhuang]]

When loading a page, there will be two loads of the favicon that I could see from the browser console. One load is triggered by the PlacesUIUtils.loadFavicon()[1], and another one is originated from the xul:image[2]. For the first case, I will submit a patch to fix. But for the second one, it is hard to be resolved because the xul:image will use the system princial to load the image, and the system principal follows the default originAttributes. The bug 1166891 is targeting this issue, so I will focus on the first case here.  

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#868
[2] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#6192

Comment 13

[Tim Huang[:timhuang]]

Attachment: Make the NS_GetOriginAttributes() acquires originAttributes from the loadInfo when loadContext is not available.

Jonas, could you help on review this patch? And Should I find a necko peer to look this as well?

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8758173 [details] [diff] [review]
Make the NS_GetOriginAttributes() acquires originAttributes from the loadInfo when loadContext is not available.

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

::: netwerk/base/nsNetUtil.cpp
@@ +1270,2 @@
>      NS_QueryNotificationCallbacks(aChannel, loadContext);
> +    loadInfo = aChannel->GetLoadInfo();

You can squeeze these into one line
nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();

@@ +1276,5 @@
> +    if (loadContext) {
> +        DocShellOriginAttributes doa;
> +        loadContext->GetOriginAttributes(doa);
> +        aAttributes.InheritFromDocShellToNecko(doa);
> +    } else {

You have to add a comment when this is the case? What side effects might this change cause?

Jonas, can't we always rely on the loadInfo instead of the loadContext? Originattributes should match, right?

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

> 
> Jonas, can't we always rely on the loadInfo instead of the loadContext?
> Originattributes should match, right?

I've tried to use loadInfo in NS_GetOriginAttributes just 1~2 weeks ago and the try looks horrible....
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a086bcd6c23

But I didn't spend time to figure out how much work to fix that yet.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Yoshi Huang[:allstars.chh] from comment #15)
> I've tried to use loadInfo in NS_GetOriginAttributes just 1~2 weeks ago and
> the try looks horrible....
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a086bcd6c23
> 
> But I didn't spend time to figure out how much work to fix that yet.

Ah, that's good to know - thanks. Then I suppose we should special case the favicon case here (which is what the patch already does), but it also might return loadinfo originattributes in other cases than favicons in case there is no loadcontext.

Comment 17

[Tim Huang[:timhuang]]

Attachment: Make the NS_GetOriginAttributes() acquires originAttributes from the loadInfo when loadContext is not available.

Christoph, thanks for your feedback. This patch has made changes according to what you said.

Comment 18

[Tim Huang[:timhuang]]

Attachment: Part 1:Make the NS_GetOriginAttributes() acquires originAttributes from the loadInfo when loadContext is not available.

Valentin, could you help on reviewing this patch? Thanks.

Comment 19

[Tim Huang[:timhuang]]

Attachment: Part 2: Add a test case to verify that the loading of favicon obey originAttributes.

Comment 20

[Tim Huang[:timhuang]]

Attachment: Part 2: Add a test case to verify that the loading of favicon obey originAttributes.

Fix a problem that the test will timeout in non-e10s mode.

Comment 21

[Tim Huang[:timhuang]]

Attachment: Part 1: Make the NS_GetOriginAttributes() acquires originAttributes from the loadInfo when loadContext is not available.

Rebase from central.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8760065 [details] [diff] [review]
Part 2: Add a test case to verify that the loading of favicon obey originAttributes.

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

LGTM, r=me

Comment 23

[Tanvi Vyas[:tanvi]]

Is this ready to land?

Comment 24

[Tim Huang[:timhuang]]

Attachment: Part 2: Add a test case to verify that the loading of favicon obey originAttributes.

Update commit message.

Comment 25

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e00e8be2e52

Comment 26

[Tim Huang[:timhuang]]

Try looks good.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2238c0c342a9
Part 1: Make the NS_GetOriginAttributes() acquires originAttributes from the loadInfo when loadContext is not available. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a5b19ff2f41
Part 2: Add a test case to verify that the loading of favicon obey originAttributes. r=ckerschb

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2238c0c342a9
https://hg.mozilla.org/mozilla-central/rev/3a5b19ff2f41

Comment 29

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Do we care about favicon requests for the test pilot? It would be better if it worked, but doesnt seem like the end of the world if it doesn't and not sure if its worth the risk of uplift?

Comment 30

[Ethan Tseng [:ethan]]

(In reply to Paul Theriault [:pauljt] from comment #29)
> Do we care about favicon requests for the test pilot? It would be better if
> it worked, but doesnt seem like the end of the world if it doesn't and not
> sure if its worth the risk of uplift?

Talked with Tim offline.
It's better not to uplift this bug at this moment because it does not completely resolve the favicon
issue. There are two follow-up bugs, which I added them to the See Also field.