847715 - [B2G] Calendar app potential insecure origin checking

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Calendar, defect)

Description

[David Chan [:dchan]]

The origin checking code in page.js may be unsafe. the page onclick handler performs an origin check before dispatching events. It may be possible for a specially crafted <a href> to bypass this check. I was unable to create a working exploit on my phone.

The specific concern is if the anchor href is a prefix of the expected location. e.g. 
<a href="http://foo.bar.my.domain"> will pass the same origin check for
http://foo.bar
since
"http://foo.bar.my.domain".indexOf("http://foo.bar") == 0

Switching the order of the check arguments should solve the issue. origin.indexOf(href) instead of href.indexOf(origin)

This may not be an issue if location is always app:// since user installed apps are not able to obtain a non-GUID app:// hostname from what I know.


360   function onclick(e) {
361     if (e.defaultPrevented) return;
362     var el = e.target;
363     while (el && 'A' != el.nodeName) el = el.parentNode;
364     if (!el || 'A' != el.nodeName) return;
365     var href = el.href;
366     var path = el.pathname + el.search;
367     if (el.hash) return;
368     if (!sameOrigin(href)) return;
369     var orig = path;
370     path = path.replace(base, '');
371     if (base && orig == path) return;
372     e.preventDefault();
373     page.show(orig);
374   }
...
380   function sameOrigin(href) {
381     var origin = location.protocol + '//' + location.hostname;
382     if (location.port) origin += ':' + location.port;
383     return 0 == href.indexOf(origin);
384   }

Comment 1

[Jason Smith [:jsmith]]

Sounds like this should be sec-sensitive.

Comment 2

[James Lal [:lightsofapollo]]

Not sure how this would happen inside our calendar app context ( we don't display random links from anyone) but I can work to upstream a fix to page.js.

Comment 3

[David Chan [:dchan]]

I'm not that familiar with how the calendar app page routing works so it may not be exploitable. If both href and location are trusted input then it shouldn't be a problem.

It sounds like the usecase for page.js in the Calendar app is to handle the widgets/frames used for navigating around the app. If that is the case, then my concern would be if the future roadmap allows interacting with HTML content / links in calendar events.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to James Lal [:lightsofapollo] from comment #2)
> Not sure how this would happen inside our calendar app context ( we don't
> display random links from anyone) but I can work to upstream a fix to
> page.js.

Cool. If there is an upstream bug please add it to the "See Also" field.

Comment 5

[James Lal [:lightsofapollo]]

Given that we only display links for in-app content (third-parties cannot expose links ever) I don't think this should be leo+ we should fix this upstream in 1.2 (or whenever we support links in third-party content).

Comment 6

[Al Billings [:abillings - ex-MoCo]]

We're moving beyond 1.2 now. Did we want to address this?

Comment 7

[Jason Smith [:jsmith]]

(In reply to Al Billings [:abillings] from comment #6)
> We're moving beyond 1.2 now. Did we want to address this?

I think this remains as a non-blocker for the same reasons as comment 5. We're way too late in the game to go after this at this point.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

(In reply to Jason Smith [:jsmith] from comment #7)
> 
> I think this remains as a non-blocker for the same reasons as comment 5.
> We're way too late in the game to go after this at this point.

Too bad we did nothing with it for over six months then. When do people plan to address it?

Comment 9

[Al Billings [:abillings - ex-MoCo]]

To put this another way, this is a sec-high rated security issue that has been ignored for over six months. It would be good to get a patch, regardless of when we'll check it in.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Al Billings [:abillings] from comment #9)
> To put this another way, this is a sec-high rated security issue that has
> been ignored for over six months. It would be good to get a patch,
> regardless of when we'll check it in.

I think James's comment implies that this might not be a sec-high bug as marked right now. Dan might want to discuss with James directly to understand why.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Cool. James could you elaborate on what the real security risk is here? (Maybe just a more verbose comment 5)

Comment 12

[Daniel Veditz [:dveditz]]

This certainly isn't sec-high in the current implementation given comment 5. We can re-evaluate if we start to expose this for arbitrary content.