Closed Bug 1420507 (CVE-2018-5119) Opened 7 years ago Closed 7 years ago

Reader view ignores crossorigin attribute

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- verified
firefox59 --- verified

People

(Reporter: s.h.h.n.j.k, Assigned: Gijs)

Details

(Keywords: reporter-external, sec-low, Whiteboard: [adv-main58+])

Attachments

(1 file)

Patch
872 bytes, patch
freddy
: review+
francois
: review+
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

1. Go to https://test.shhnjk.com/vidcors.html.
2. Observe an alert saying "Blocked by CORS".
3. Click on open in reader view button.


Actual results:

Cross-origin video loaded (crossorigin attribute ignored).


Expected results:

Crossorigin attribute in reader view should be respected as normal page.
Seems the crossorigin attribute is getting stripped somewhere/somehow. I don't know why, or if not stripping it would actually help given the weird stuff that about:reader does. :-\

Paul, can you help assess the severity of this issue?
Group: firefox-core-security → toolkit-core-security
Component: Untriaged → Reader Mode
Flags: needinfo?(ptheriault)
Product: Firefox → Toolkit
Flags: needinfo?(ptheriault)
Ha its actually kind of a security reason why this attribute is lost. Reader mode uses parserUtils to filter 'dangerous' attributes - I'm not familiar with the flags but from the looks of this line [1] my guess is that parserUtils.parseFragment is the culprit here. From testing:

var html = `<video src="https://shhnjk.com/mov_bbb.mp4" crossorigin='anonymous'=""></video>`
var parserUtils = Cc["@mozilla.org/parserutils;1"].getService(Ci.nsIParserUtils);
contentFragment = parserUtils.sanitize(html,Ci.nsIParserUtils.SanitizerDropForms | Ci.nsIParserUtils.SanitizerAllowStyle)

The resulting output is:

"<html><head></head><body><video src=\"https://shhnjk.com/mov_bbb.mp4\" controls=\"controls\"></video></body></html>"

Note that the crossorigin attribute is stripped.

So the obvious risk here is that an attacker crafts a malicious webpage, coerces user to open it in reader mode, and then use the CORS bypass to steal data. But reader blocks JavaScript, and I think also CSS, so its difficult for the page to ex-filtrate data. I can't think of a way that an attacker could abuse this this specific issue to steal data protected.  So its probably a sec-low, unless there are more broad implications here (are there other security critical attributes that parserUtils strips?) 


[1] https://searchfox.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm#826
Keywords: sec-moderate
I feel like I'm missing something, but can anyone else come up with a scenario in which crossorigin='anonymous' is a security feature for a website?

parserUtils has a whitelist of tags and attributes, which indeed shouldn't contain crossorigin.
(If we needed it here (not sure), we could introduce a new flag for parserUtils and supply it in the sanitize() call)
Scenario:

Website wants to build a feature which displays a video with user supplied URL. But the page contains video can be viewed by any user in the website. Since HTTP video can be loaded to HTTPS websites, this website made sure to not to send user's cookie over HTTP by adding crossorigin='anonymous' (this will omit any credential to be sent with the request).

This scenario can be bypassed with Reader Mode.
(In reply to Frederik Braun [:freddyb] from comment #3)
> I feel like I'm missing something, but can anyone else come up with a
> scenario in which crossorigin='anonymous' is a security feature for a
> website?
> 
> parserUtils has a whitelist of tags and attributes, which indeed shouldn't
> contain crossorigin.

So comment #4 answers this.


> (If we needed it here (not sure), we could introduce a new flag for
> parserUtils and supply it in the sanitize() call)

Conversely though, as I understand it, when is crossorigin=anonymous ever something that parserutils needs to strip out? You suggested a flag, but it seems to me we should just fix parserutils so it doesn't strip out crossorigin=anonymous (though it should potentially strip out other values, I don't know).
Flags: needinfo?(fbraun)
Flags: sec-bounty?
(In reply to Jun from comment #4)
> Scenario:
> 
> Website wants to build a feature which displays a video with user supplied
> URL. But the page contains video can be viewed by any user in the website.
> Since HTTP video can be loaded to HTTPS websites, this website made sure to
> not to send user's cookie over HTTP by adding crossorigin='anonymous' (this
> will omit any credential to be sent with the request).
> 
> This scenario can be bypassed with Reader Mode.

Seems like a pretty specific scenario (but then I guess its why crossorigin exists). I wonder if we have any telementry on its usage. In any case this scenario would seem to be just as much an issue of the website serving credentialed resources over HTTP (and/or setting sensitive cookies on HTTP schemed origins). 

Revising sec rating in light of limited attack scenario.
Keywords: sec-moderate → sec-low
(In reply to :Gijs from comment #5)
> (In reply to Frederik Braun [:freddyb] from comment #3)
> > I feel like I'm missing something, but can anyone else come up with a
> > scenario in which crossorigin='anonymous' is a security feature for a
> > website?
> > 
> > parserUtils has a whitelist of tags and attributes, which indeed shouldn't
> > contain crossorigin.
> 
> So comment #4 answers this.
> 

Yeah, but crossorigin=anonymous isn't really a mitigation for missing HTTPS. I share Paul's view that this is sec-low.

> 
> > (If we needed it here (not sure), we could introduce a new flag for
> > parserUtils and supply it in the sanitize() call)
> 
> Conversely though, as I understand it, when is crossorigin=anonymous ever
> something that parserutils needs to strip out? You suggested a flag, but it
> seems to me we should just fix parserutils so it doesn't strip out
> crossorigin=anonymous (though it should potentially strip out other values,
> I don't know).

You're right, crossorigin could be on the whitelist of allowed attributes (regardless of its value, even), but I'm lacking context for most other uses of parserutils to know whether this holds true universally. Are you confident enough, or do you think we should investigate other usage?
Flags: needinfo?(fbraun)
I manually applied crossorigin attribute on Reader Mode to see what happens, and it sends "Origin: null", so fix might not be that useful...
(In reply to Frederik Braun [:freddyb] from comment #7)
> (In reply to :Gijs from comment #5)
> > (In reply to Frederik Braun [:freddyb] from comment #3)
> > > (If we needed it here (not sure), we could introduce a new flag for
> > > parserUtils and supply it in the sanitize() call)
> > 
> > Conversely though, as I understand it, when is crossorigin=anonymous ever
> > something that parserutils needs to strip out? You suggested a flag, but it
> > seems to me we should just fix parserutils so it doesn't strip out
> > crossorigin=anonymous (though it should potentially strip out other values,
> > I don't know).
> 
> You're right, crossorigin could be on the whitelist of allowed attributes
> (regardless of its value, even), but I'm lacking context for most other uses
> of parserutils to know whether this holds true universally. Are you
> confident enough, or do you think we should investigate other usage?

I don't know enough about the attribute to be confident in assessing the security implications of leaving/removing it in all circumstances.

Generally, from what I can tell, there are 3 effects:

1) the request includes may include various bits of information that could be considered "private" that either the embedding site or the browser is trying to protect. crossorigin=anonymous allows the embedding site to avoid sending cookies. crossorigin=use-credentials, on the other hand, could be used to force the browser to send stored credentials of some description which it wouldn't ordinarily do;
2) the request will be different, and so the server serving the request for the image/script/video could potentially respond differently;
3) the resource will behave differently within the embedding page, in terms of canvas tainting and onerror error information propagation.

This bug seems to be about the anonymous case for (1), and maybe (2) (though 2 isn't really a security issue).

In terms of the sanitizer, from reading https://dxr.mozilla.org/mozilla-central/source/parser/html/nsIParserUtils.idl I think we always strip script and anything script-ish, so it seems (3) shouldn't be a concern.

I can see theoretical cases where we ought to strip out use-credentials to avoid leaking of credentials -- except that because reader mode always uses an existing page, if someone were abusing `use-credentials` they would presumably just not (encourage the victim to) use reader mode...

From looking at https://dxr.mozilla.org/mozilla-central/search?q=nsIParserUtils+-path%3Atest+-file%3AnsTreeSanitizer+-path%3Aobj-x86+-path%3Aparser%2Fhtml&redirect=false we have 3 main uses of the sanitizer code:

1) reader mode (toolkit/components/reader/ ) and print's "simplify" mode (basically the same as reader mode) (toolkit/content/browser-content.js)
2) feed reader code (toolkit/components/feeds)
3) editor code sanitizing incoming paste/dnd data ( https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/HTMLEditorDataTransfer.cpp#2167 )

For the first 2, keeping crossorigin=anonymous definitely seems correct to me. Again, less sure about crossorigin=use-credentials, but probably OK?

I'm hardly an editor expert, but I expect the same applies in that case - if the user copy/pastes code that has the attribute, we probably shouldn't be stripping it (and anyway, if this is a contenteditable the "receiving" site could just put it back).

The only other edgecase I can think of there is convincing a user to copy/paste or dnd HTML containing an image with the crossorigin attribute into a receiving target page for which the attacker does not know the URL, and the Origin CORS headers revealing the domain (even if the target page had set a meta referrer tag to avoid the referrer URL leaking for any requests). That seems pretty far-fetched as attacks go, though...

So the short answer is "I'm not sure, but leaning towards saying we could keep crossorigin=anonymous without a flag".

From https://bugzilla.mozilla.org/buglist.cgi?quicksearch=FIX%20crossorigin%20attribute&list_id=13904840 , Joe Drew and bz would the obvious candidates to ask if this made sense, but Joe is gone. bz? (Sorry to be adding to your pile!)

(In reply to Jun from comment #8)
> I manually applied crossorigin attribute on Reader Mode to see what happens,
> and it sends "Origin: null", so fix might not be that useful...

I though the threat model you're suggesting in comment #4 revolves around cookies etc. being sent. Why would the origin header being null make that "not useful"? I mean, I understand that this is not the right value to send from a user perspective, but it isn't relevant for your suggestion in comment 4, right?

FWIW, this would be fixed if we fixed bug 1204818 in such a way that the document we frame has the principal of the original website. It's not immediately clear to me that that wouldn't cause its own set of issues, though.
Flags: needinfo?(s.h.h.n.j.k)
Flags: needinfo?(bzbarsky)
From security point of view, it will solve the problem. But usability point of view, it would not load the things which needs to be loaded.
Flags: needinfo?(s.h.h.n.j.k)
I don't understand the security benefit in removing crossorigin attributes or what the attack vector would be. Exfiltration happens by creating requests, not by using CORS.
(In reply to :Gijs from comment #5)
> So the short answer is "I'm not sure, but leaning towards saying we could
> keep crossorigin=anonymous without a flag".

I share your hesitance, but I lean towards the same conclusion :)
(In reply to Anne (:annevk) from comment #12)
> I don't understand the security benefit in removing crossorigin attributes

The sanitizer removes everything that isn't on a list of "safe" things (which is arguably the *safest* way to build an HTML sanitizer in the face of an ever-changing web). When the crossorigin attribute was implemented, it was not added to the list of "safe" things at the time, so it's getting removed.

So it's less that I or anyone else is saying (or ever said) "we must remove this for security's sake", but that we're trying to establish whether/that this attribute is always safe and there's no possible way that keeping the attribute could affect user privacy or security, in all cases where this sanitizer is used, and so it must be kept. Given relative lack of familiarity with the attribute, I (and, it seems, others) are cautious. :-)

> or what the attack vector would be. Exfiltration happens by creating
> requests, not by using CORS.

But exfiltration is not the only potential vector, and CORS vs. non-CORS requests, at least as far as I can tell, differ in what they reveal in an Origin header and/or credentials. I gave some examples in comment 9:

(In reply to :Gijs from comment #9)
> I can see theoretical cases where we ought to strip out use-credentials to
> avoid leaking of credentials -- except that because reader mode always uses
> an existing page, if someone were abusing `use-credentials` they would
> presumably just not (encourage the victim to) use reader mode...

> I'm hardly an editor expert, but I expect the same applies in that case - if
> the user copy/pastes code ...

> The only other edgecase I can think of there is convincing a user to
> copy/paste or dnd HTML containing an image with the crossorigin attribute
> into a receiving target page for which the attacker does not know the URL,
> and the Origin CORS headers revealing the domain (even if the target page
> had set a meta referrer tag to avoid the referrer URL leaking for any
> requests). That seems pretty far-fetched as attacks go, though...

If you, as someone who undoubtedly knows more about this attribute, thinks those aren't realistic vectors, and it's impossible for there to be any other ones, then sure, we can just add the attribute to the list of attributes the sanitizer should keep and move on. If all values are safe, it's going to be a 1-line patch...
That is the way to write a sanitizer, yes. 
My gut feeling here is that all values of the crossorigin attribute should be safe to pass through the sanitizer...
Flags: needinfo?(bzbarsky)
It seems part of my comment got dropped. Can you basically not write <img src=cross-origin-url> in a Bugzilla comment?
Hmm okay. In any event, credentials also leak through <img> and similar such constructs. Origin leaks through Referer (couple minor exceptions, but if the page wants to leak the origin it can). So yeah, crossorigin is safe.
Attached patch Patch — — Splinter Review 
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

        Attachment #8932969 -
        Flags: review?(ptheriault)

    
  
Group: toolkit-core-security → core-security-release
Component: Reader Mode → DOM: Security
Product: Toolkit → Core
Group: core-security-release → dom-core-security

          status-firefox57:
          --- → wontfix

          status-firefox58:
          --- → fix-optional

          status-firefox59:
          --- → affected

          status-firefox-esr52:
          --- → wontfix

        Attachment #8932969 -
        Flags: review?(ptheriault) → review?(fbraun)

    
    

    
  
Comment on attachment 8932969 [details] [diff] [review]
Patch

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

looks good to me!
Note that I'm not a DOM Security peer (and neither is pauljt), so you'll need another review.

        Attachment #8932969 -
        Flags: review?(fbraun) → review+

    
    

    
  
(For those curious, part of my comment got dropped due to bug 1253535. There's no HTML stripping going on. Just no smiling allowed.)

    
    

    
  
Comment on attachment 8932969 [details] [diff] [review]
Patch

Xidorn, can you review this? If not, please forward appropriately.

        Attachment #8932969 -
        Flags: review?(xidorn+moz)

    
    

    
  
Comment on attachment 8932969 [details] [diff] [review]
Patch

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

I'm not a peer of any DOM module. Sounds like this should fall into Content Security module, so redirect to francois.

        Attachment #8932969 -
        Flags: review?(xidorn+moz) → review?(francois)

    
    

    
  
Comment on attachment 8932969 [details] [diff] [review]
Patch

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

Sounds like there's general agreement that all values of this field are safe.

        Attachment #8932969 -
        Flags: review?(francois) → review+

    
    

    
  
(In reply to :Gijs from comment #14)
> When the crossorigin attribute was implemented, it
> was not added to the list of "safe" things at the time, so it's getting
> removed.

That's also true of the "integrity" attribute. We should probably not be stripping that out.

    
    

    
  
(In reply to François Marier [:francois] from comment #25)
> (In reply to :Gijs from comment #14)
> > When the crossorigin attribute was implemented, it
> > was not added to the list of "safe" things at the time, so it's getting
> > removed.
> 
> That's also true of the "integrity" attribute. We should probably not be
> stripping that out.

Makes sense. I'll add that to the patch while we're here...

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/f3ea44ff63eec1d2e6edafe94c213860a8cfe11f

(via https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=f3ea44ff63ee )
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox59:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

    
    

    
  
Comment on attachment 8932969 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: for some subresources in reader mode, we're not verifying integrity of and/or treating them as CORS requests, which in some very limited cases may cause security and/or privacy issues.
[Is this code covered by automated tests?]: reader mode as a whole is, this particular aspect of it is not.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: Using the steps from comment 0, check that the outgoing request for https://shhnjk.com/mov_bbb.mp4 contains an "Origin" header.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not too risky
[Why is the change risky/not risky?]: it's a trivial patch, we have lots of beta runway still, and there was extensive discussion (with consensus) about whether this was the right thing to do.
[String changes made/needed]: n/a

        Attachment #8932969 -
        Flags: approval-mozilla-beta?

    
    

    
  
(In reply to Jun from comment #8)
> I manually applied crossorigin attribute on Reader Mode to see what happens,
> and it sends "Origin: null", so fix might not be that useful...

I filed bug 1422519 (non-sec-sensitive, because we already don't send a "Referer" header, either, and that doesn't seem sec-sensitive to me) to address this.

    
    

    
  
Comment on attachment 8932969 [details] [diff] [review]
Patch

Fix a security issue related to crossorigin attribute. Beta58+.

        Attachment #8932969 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
Thank you for reporting this bug. Unfortunately it does not meet the bug bounty severity criteria.
Group: dom-core-security
Flags: sec-bounty? → sec-bounty-
Flags: qe-verify+

    
    

    
  
I have managed to reproduce the issue mentioned in comment 0 using Firefox 59.0a1 (BuildId:20171124220317).

This issue is verified fixed using Firefox 59.0a1 (BuildId:20171205100345) and Firefox 58.0b9 (BuildId:20171204150510) on Windows 10 64bit, macOS 10.11 and Ubuntu 16.04 64bit.
Status: RESOLVED → VERIFIED

          status-firefox58:
          fixed → verified

          status-firefox59:
          fixed → verified
Flags: qe-verify+
Whiteboard: [adv-main58+]
Alias: CVE-2018-5119

    
  
Flags: sec-bounty-hof+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: