1248444 - splitwise.com fails to load because it relies on writing to cross origin style sheets (which throws a SecurityError in Gecko)

Status: RESOLVED FIXED

(Web Compatibility :: Site Reports, defect)

Description

[Josh Matthews [:jdm]]

mozregression traced this to bug 1241021. To reproduce, create a Splitwise account and log in. On FF 45, you get a usable interface. On any more recent version (since bug 1241021 has been uplifted) you get a big "Loading" message that never goes away, and the web console contains a "SecurityError: The operation is insecure." that I haven't been able to track down.

Comment 1

[Daniel Holbert [:dholbert]]

webkit prefix support is behind an RELEASE_BUILD #ifdef guard for now (bug 1238827), which means this won't ship past Aurora for now.  So while DevEdition 46 is affected, Beta/Release 46 should not be.

So, I'm not sure it's worth tracking this for Firefox46 via tracking flags, but I'll leave those untouched for now.  (I'm also not sure what the correct setting of "status-firefox46" is in this scenario; not worrying about that for now.)

Anyway -- Mike, would you be up for diagnosing this?

Comment 2

[Mike Taylor [:miketaylr]]

Yep, will take a look first thing in the morning (leaving ni?).

Comment 3

[Mike Taylor [:miketaylr]]

Actually, this stuff keeps me up at night. The security error is coming from `document.styleSheets[0].insertRule(s, 0)` in:

if (document.createElement('div').style.WebkitAnimationName !== undefined) {
  var i = {
  };
  n = function (e, t, n) {
    if (!i[t]) {
      var r = 'spin' + t,
      s = '@-webkit-keyframes ' + r + ' {';
      for (var o = 0; o < t; o++) {
        var u = Math.round(100000 / t * o) / 1000,
        a = Math.round(100000 / t * (o + 1) - 1) / 1000,
        f = '% { -webkit-transform:rotate(' + Math.round(360 / t * o) + 'deg); }\n';
        s += u + f + a + f
      }
      s += '100% { -webkit-transform:rotate(100deg); }\n}',
      document.styleSheets[0].insertRule(s, 0),
      i[t] = r
    }
    e.css('-webkit-animation', i[t] + ' ' + n + 's linear infinite')
  }
}


document.styleSheets[0].href is "https://fonts.googleapis.com/css?family=Lato:400,400italic,700", time to learn about cross origin styleSheet manipulation differences between Chrome and Firefox...

Comment 4

[Mike Taylor [:miketaylr]]

According to Boris in http://stackoverflow.com/a/15233597, Chrome can write to a cross origin style sheet but not read. And we don't allow either due to it being a potential XSS vector.

Comment 5

[Mike Taylor [:miketaylr]]

Attachment: cross-origin-stylesheet-write.html

Comment 6

[Mike Taylor [:miketaylr]]

It seems IE11, Edge, WebKit, Blink and Presto all allow this, based on the simple attached test case. How much work would it be to allow this and have it be safe?

Comment 7

[Mike Taylor [:miketaylr]]

It seems like we're doing the right thing according to https://drafts.csswg.org/cssom/#dom-cssstylesheet-insertrule, but that doesn't match the reality of other major implementations (and apparently sites that rely on this behavior).

Comment 8

[Daniel Holbert [:dholbert]]

paging bz for thoughts on comment 6 (sounds like he's thought about this before, given comment 4)

Comment 9

[Boris Zbarsky [:bzbarsky]]

> How much work would it be to allow this and have it be safe?

Allowing it is trivial.  Having it be safe is ... unclear.  We'd have to check all the places where the sheet principal is used, figure out what it's used for, and then decide what issues would arise as a result.  But a priori it seems like any place where we use the sheet principal, not the document one, becomes an attack vector.  This is particularly bad if the page ever gets its hands on a privileged enough sheet (e.g. injected by an extension).  I would _really_ like to know what other browsers do in those situations, once we identify what the relevant situations are.

Anyway, the spec does in fact say to do what we do right now, as comment 12 notes.

Comment 10

[Mike Taylor [:miketaylr]]

> We'd have to check all the places where the sheet principal is used, figure out what it's used for, and then decide what issues would arise as a result.

Boris, can you give me a pointer to where I can look to compile such a list?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm.  At least the callers of CSSStyleSheet::Principal, whatever the CSS parser does with mSheetPrincipal, and whatever places URLValue's mOriginPrincipal member is used.

If you're willing to try to put this list together, that sounds awesome; I can help sort through it.  If it's being too annoying, let me know and I can try to do it.

Comment 12

[Mike Taylor [:miketaylr]]

OK, sorry for the delay here.

CSSStyleSheet::Principal

passed to parser.ParseSheet by Loader::ParseSheet
https://dxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#1719

nsCSSCounterStyleRule::SetDescriptor stores as `principal`
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSRules.cpp#3250
which is passed to parser.ParseCounterDescriptor in the same method.

nsDOMCSSDeclaration::GetCSSParsingEnvironmentForRule
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsDOMCSSDeclaration.cpp#322

existence checked in nsDOMCSSDeclaration::SetCssText
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsDOMCSSDeclaration.cpp#133

then passed to cssParser.ParseDeclarations in the same method

passed to cssParser.ParseProperty in nsDOMCSSDeclaration::ParsePropertyValue
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsDOMCSSDeclaration.cpp#352

existence checked in nsDOMCSSDeclaration::ParseCustomPropertyValue
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsDOMCSSDeclaration.cpp#376
then passed to cssParser.ParseVariable in same method

nsCSSParser.cpp usage:

CSSParserImpl::InitScanner gets passed in an aSheetPrincipal.
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.h#66

CSSParserImpl::ParseSheet
CSSParserImpl::ParseDeclarations
CSSParserImpl::ParseRule
CSSParserImpl::ParseLonghandProperty
CSSParserImpl::ParseProperty
CSSParserImpl::ParseVariable
CSSParserImpl::ParseCounterDescriptor
CSSParserImpl::ParseFontFaceDescriptor

All but ParseLonghandProperty (which calls ParseProperty) pass aSheetPrincipal into
CSSParserImpl::InitScanner

URLValue’s mOriginPrincipal is referenced here:

Element::WrapObject
https://dxr.mozilla.org/mozilla-central/source/dom/base/Element.cpp#495

URLValue’s mOriginPrincipal member is initialized in css::URLValue::URLValue
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#2460

https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#2471

Used by
css::URLValue::operator==
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#2487

Which is used by nsCSSValue::operator==
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#248
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#251

Used by css::URLValue::URIEquals
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#2504
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#2505

which is called by nsCSSFrameConstructor::CaptureStateForFramesOf
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9096

callers of URLValue constructor:
nsGenericHTMLElement::ParseBackgroundAttribute
https://dxr.mozilla.org/mozilla-central/source/dom/html/nsGenericHTMLElement.cpp#1067

StyleAnimationValue::ExtractComputedValue,
for eCSSProperty_filter case
https://dxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp#3471

for eStyleAnimType_PaintServer case:
https://dxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp#3644

css::ImageValue (which inherits from URLValue)
constructed by nsAttrValue::LoadImage
https://dxr.mozilla.org/mozilla-central/source/dom/base/nsAttrValue.cpp#1647
and nsCSSValue::StartImageLoad
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#706

nsCSSFrameConstructor::ConstructDocElementFrame, calls
xblService->LoadBindings
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#2389

nsCSSFrameConstructor::AddFrameConstructionItemsInternal, calls
xblService->LoadBindings
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#5586

FontFaceSet::FindOrCreateUserFontEntryFromFontFace
eCSSUnit_URL case:
https://dxr.mozilla.org/mozilla-central/source/layout/style/FontFaceSet.cpp#1115

Comment 13

[Mike Taylor [:miketaylr]]

Looking into the following CSSStyleSheet methods, we check SOP-ness (via SubjectSubsumesInnerPrincipal) first:

CSSStyleSheet::GetCssRules
CSSStyleSheet::InsertRule
CSSStyleSheet::DeleteRule

Per current CSSOM draft, that’s the right thing to do.

Per DOM2 insertRule and deleteRule have no notion of origin restrictions.

insertRule in Blink:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/css/CSSStyleSheet.cpp&l=283

deleteRule:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/css/CSSStyleSheet.cpp&l=320

cssRules does check `canAccessRules`
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/css/CSSStyleSheet.cpp&l=364

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/css/CSSStyleSheet.cpp&l=261


The https://www.w3.org/TR/2011/WD-cssom-20110712/ CSSOM draft doesn't enforce origins, but https://www.w3.org/TR/2013/WD-cssom-20131205/ does.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Thank you for going through those...  I'm hoping that list is complete.  Looking at those endpoint uses:

1) Element::WrapObject

This is going to pass the principal to the XBL binding loader.  Jonas, what URLs do we actually allow to be loaded in those cases?  As in, if an extension adds a system sheet to a document, and the page can then inject -moz-binding rules into that sheet, can it trigger loads of bindings over http:// that it controls?

2) Looks like we also propagate the URLValue principal to font faces, and hence used for the font-load security checks, right?  So injecting a CSS rule into a cross-origin sheet would allow you to load fonts that that sheet can load (but you normally cannot).  Does Chrome actually end up doing that?  That is, in Chrome a page at http://A can't load a font from http://B unless the latter opts in via CORS, I believe.  Can it do it if the font is linked to from a stylesheet on http://B?  Can it insert new font-face rules into that sheet?

3) ImageValue::ImageValue (not mentioned above, really) passes the principal to StyleImageLoader()->LoadImage(), looks like.  This is then used for security checks of various sorts.  Again, this could be an attack against a sufficiently privileged sheet, I believe (e.g. using generated content images and their sizing to exfiltrate information that you can't normally get, like sizes of images on the user's hard drive).

Comment 15

[Mike Taylor [:miketaylr]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> 2) Looks like we also propagate the URLValue principal to font faces, and
> hence used for the font-load security checks, right?  So injecting a CSS
> rule into a cross-origin sheet would allow you to load fonts that that sheet
> can load (but you normally cannot).  Does Chrome actually end up doing that?
> That is, in Chrome a page at http://A can't load a font from http://B unless
> the latter opts in via CORS, I believe.  Can it do it if the font is linked
> to from a stylesheet on http://B?  Can it insert new font-face rules into
> that sheet?

Took a while to make a TC for this (had to learn how to install a letsencrypt cert on another domain).

https://miketaylr.com/bzla/font-face-origin.html

The TC on http://A loads a stylesheet from http://B which has two @font-face rules for has-cors.otf and no-cors.otf, which have and don't have Access-Control-Allow-Origin: * set, respectively.

Clicking the first button inserts a rule to set text's font-family to has-cors.otf. That works in Chrome and in Edge
Clicking the second button inserts a rule to set text's font-family to no-cors.otf. That fails in Chrome and in Edge.

(I couldn't test in Safari because it hates the new cert right now ¯\_(ツ)_/¯)

Comment 16

[Mike Taylor [:miketaylr]]

Rick, before we go too far down the insanity rabbit hole, does Chrome intend to match CSSOM and throw a SecurityError for insertRule and deleteRule on crossorigin stylesheets? Or, would it be willing to? I didn't see any bugs open in the Chromium or WebKit bug trackers... (in case this is an inheritied WebKit-ism).

See Comment #3 for why we're even here.

Comment 17

[Boris Zbarsky [:bzbarsky]]

The testcase in comment 15 doesn't really test what I was interested in.

What I'm interested in is this, on http://A/index.html :

  <div style="font-family: cross-site">Some text</div>

then do three tests:

1)  Insert an @font-face rule into a sheet at http://A/test.css that tries to load a font with name "cross-site" from http://B/test.woff.

2)  A link to a sheet at http://B/test1.css which has an @font-face rule that loads a font with name "cross-site" from http://B/test.woff.

3)  Insert (in a browser that supports that) an @font-face rule into a sheet at http://B/test.css that tries to load a font with name "cross-site" from http://B/test.woff.

item 3 will throw in Firefox.  I _believe_ item 1 will not load the font and item 2 will.  What I want to know is which of these load the font in Chrome....

Comment 18

[Boris Zbarsky [:bzbarsky]]

Oh, and for purposes of that test server B should not be sending any CORS headers.

Comment 19

[Mike Taylor [:miketaylr]]

OK, let me move some things around...

Comment 20

[Mike Taylor [:miketaylr]]

Alright, I *think* I got all the details right: https://miketaylr.com/bzla/font-face-origin-redux.html

All 3 fail in in Chrome and Edge, due to missing CORS headers. They fail in Firefox too, but we throw on #3 before it even tries to fetch the font.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Hmm. Then it sounds like we're not using the principal of the sheet for the CORS check for fonts?  Is that correct?

Comment 22

[Cameron McCormack (:heycam)]

This code is about to change with bug 1195172 (with no change in behaviour), but yes, it's the principal of the document that gets used.

For the aRequestingPrincipal value passed in to nsCORSListenerProxy here:

https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/layout/style/FontFaceSet.cpp#657

we take the gfxUserFontEntry's principal, which is set here:

https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/gfx/thebes/gfxUserFontSet.cpp#486

from the value returned by FontFaceSet::CheckFontLoad:

https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/layout/style/FontFaceSet.cpp#1330

which is the document's principal (except for some special user style sheet cases).

Comment 23

[Boris Zbarsky [:bzbarsky]]

Then why do we need the principal from the sheet in the font face?

Comment 24

[Cameron McCormack (:heycam)]

It is at least used for the user style sheet case.  I don't know if it's used beyond that.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 47 but not 46 (since 46 moves to beta in a week and then won't be affected).

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

I do notice no one is assigned here.  Does anyone want to take this bug?

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

My understanding is that we're supposed to use the principal of the *document* when doing CORS security checks during font loading.

Which makes sense because it's the document which is getting access to read the font data (using canvas, layout metrics etc), which is what CORS was intended to be all about.

Additionally, I believe that many font hosting services host not just the font, but also the stylesheet to load the correct font. If we were to use the principal of the stylesheet, that would mean that all websites would now be able to load the font by simply linking to the correct stylesheet.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Jonas, the needinfo for you was about XBL.

Comment 29

[Rick Byers]

> Rick, before we go too far down the insanity rabbit hole, does Chrome intend to match CSSOM and throw a SecurityError for insertRule and deleteRule on crossorigin stylesheets?

I haven't followed this entire thread (sorry), sounds like maybe the thinking has evolved?  Do you still believe there's a bug in Chrome's behavior?  If so I'm happy to file a crbug and get the right CSS experts to chime in.  But in general, if the spec says we should fire a SecurityError, other engines do, and it's web compatible, then yes of course we should change :-).

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> 1) Element::WrapObject
> 
> This is going to pass the principal to the XBL binding loader.  Jonas, what URLs do we actually allow to
> be loaded in those cases?  As in, if an extension adds a system sheet to a document, and the page can
> then inject -moz-binding rules into that sheet, can it trigger loads of bindings over http:// that it
> controls?

I'm not sure what "those cases" refer to.

I'm also not fully updated on what we do with system stylesheets these days. I think you and Christoph might have changed the rules here so that anytime a system stylesheet links to something, we allow the load no matter what.

There's a meeting on monday to discuss if we'll keep this rule.

However, if we do keep that rule, we'd definitely need to ensure that a webpage can't mutate system stylesheets that are linked into that page.

Comment 31

[Mike Taylor [:miketaylr]]

(In reply to Rick Byers from comment #29)
> > Rick, before we go too far down the insanity rabbit hole, does Chrome intend to match CSSOM and throw a SecurityError for insertRule and deleteRule on crossorigin stylesheets?
> 
> I haven't followed this entire thread (sorry), sounds like maybe the
> thinking has evolved?  Do you still believe there's a bug in Chrome's
> behavior?  If so I'm happy to file a crbug and get the right CSS experts to
> chime in.  But in general, if the spec says we should fire a SecurityError,
> other engines do, and it's web compatible, then yes of course we should
> change :-).

Right now it's unclear if the spec behavior is web compatible (and to what extent), but it seems to be the safest thing for users.

I've tweeted at some splitwise devs (https://twitter.com/miketaylr/status/703420567399981056) to see if we can't get them to fix this -- this is the only known broken site (for now).

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Based on the discussions yesterday, stylesheets with system principals will definitely have more abilities that stylesheets without system principals. Even when inserted into normal web documents. (There's a proposal for what exactly the difference will be, but we'll see if that changes based on further discussions. Either way there will be *some* difference).

So, given that, we should definitely make sure that webpages can't mutate stylesheets that have a system principal using the CSSOM.

Let me know if there's further questions for me.

Comment 33

[Steve Workman [:sworkman] (INACTIVE)]

Liz, Daniel - is this still firefox46: affected? According to comment #1 the offending code is behind a RELEASE_BUILD flag, but there have been a lot of comments since then - is is still a correct statement that beta is unaffected?

Comment 34

[Daniel Holbert [:dholbert]]

firefox46 is now unaffected, and it's still a correct statement that beta is unaffected.

(Specifically, it seems that SplitWise only relies on writing to cross-origin style sheets when it detects that a browser supports webkit-prefixed features. Or something like that. And as you noted, our webkit prefixing support is behind a RELEASE_BUILD guard at the moment.)

Comment 35

[Steve Workman [:sworkman] (INACTIVE)]

Thanks Daniel!

Comment 36

[Mike Taylor [:miketaylr]]

jdm, can you verify that this is fixed?

(per https://twitter.com/rofreg/status/707363839042576384, it seems they deployed a fix to create a new stylesheet, rather than add rules to the Google font one).

That said, the larger issue won't be resolved, and we still don't have a good idea of how many sites rely on it.

Comment 37

[Josh Matthews [:jdm]]

This works for me on the latest nightly.

Comment 38

[Andrew Overholt [:overholt]]

Since the specific site bug here has been fixed, should we dupe this to bug 1262963? Or otherwise refine the summary?

Comment 39

[Daniel Holbert [:dholbert]]

I suspect we should convert this to a Tech Evang but and resolve it as FIXED (since we did outreach & the site deployed a fix).

(And any changes that we want to do on our end here can be tracked separately in bug 1262963.)

Leaving ni=jdm and adding ni=miketalyr (who did the tech evang), to make sure I'm understanding correctly.

Comment 40

[Daniel Holbert [:dholbert]]

> convert this to a Tech Evang but

(sorry, s/but/bug/)

Comment 41

[Mike Taylor [:miketaylr]]

Yeah, I agree with what Daniel is suggesting. We'll keep our eye on similar issues going forward and revisit more generally if we need to, but so far we haven't found evidence that this is a popular pattern.

Comment 42

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Given that the site was updated and the issue was fixed on their end, I am updating this tracked bug for Fx47 to status-fx47 as unaffected.