Closed Bug 1270678 Opened 6 years ago Closed 6 years ago

Make sure favicon requests obey OriginAttributes

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: tanvi, Assigned: timhuang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [OA][domsecurity-active][userContextId][uplift49-])

Attachments

(2 files, 5 obsolete files)

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 +++
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.
Flags: needinfo?(ptheriault)
Flags: needinfo?(kjozwiak)
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...
Flags: needinfo?(ptheriault)
This probably depends on or is at least related to bug 1227289.
Depends on: 1227289
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.
Depends on: 1196013
No longer depends on: 1227289
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.
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?
Flags: needinfo?(kjozwiak)
Assignee: nobody → jhao
Assignee: jhao → nobody
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
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
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).
Assignee: nobody → jhao
Status: NEW → ASSIGNED
Assignee: jhao → tihuang
Blocks: 1276412
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
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
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
Jonas, could you help on review this patch? And Should I find a necko peer to look this as well?
Attachment #8758173 - Flags: review?(jonas)
Whiteboard: [OA][domsecurity-backlog][userContextId] → [OA][domsecurity-active][userContextId]
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?
Attachment #8758173 - Flags: feedback+
> 
> 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.
(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.
Christoph, thanks for your feedback. This patch has made changes according to what you said.
Attachment #8758233 - Flags: review?(jonas)
Attachment #8758173 - Attachment is obsolete: true
Attachment #8758173 - Flags: review?(jonas)
Valentin, could you help on reviewing this patch? Thanks.
Attachment #8759180 - Flags: review?(valentin.gosu)
Attachment #8758233 - Attachment is obsolete: true
Attachment #8758233 - Flags: review?(jonas)
Attachment #8759181 - Flags: review?(ckerschb)
Fix a problem that the test will timeout in non-e10s mode.
Attachment #8760065 - Flags: review?(ckerschb)
Attachment #8759181 - Attachment is obsolete: true
Attachment #8759181 - Flags: review?(ckerschb)
Attachment #8759180 - Attachment is obsolete: true
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
Attachment #8760065 - Flags: review?(ckerschb) → review+
Is this ready to land?
Priority: -- → P1
Attachment #8760065 - Attachment is obsolete: true
Try looks good.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2238c0c342a9
https://hg.mozilla.org/mozilla-central/rev/3a5b19ff2f41
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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?
Whiteboard: [OA][domsecurity-active][userContextId] → [OA][domsecurity-active][userContextId][uplift49?]
(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.
See Also: → 1277803, 1277808
Whiteboard: [OA][domsecurity-active][userContextId][uplift49?] → [OA][domsecurity-active][userContextId][uplift49-]
You need to log in before you can comment on or make changes to this bug.