325426 - Don't rely on baseuri for same-origin checks

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

We currently have security checks when setting the baseuri on html elements. However, we don't have similar checks for the xml:base. We also (until recently) didn't have these checks in the html fragment sink.

These checks should not be needed. Same-origin checks should always be done against the document uri, not the document base-uri. I'm currently looking through all users of GetBaseURI to make sure that is the case.

So far it seems like nsGenericElement::TriggerLink (or arguably the two users in nsGenericHTMLElement::HandleDOMEventForAnchors) are doing the wrong thing.

Comment 1

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

Many other clients of nsGenericElement::TriggerLink has the same issue as nsGenericHTMLElement::HandleDOMEventForAnchors. We should just remove the aOrigin argument from TriggerLink and let the function grab the origin from the document itself.

Note to self: more places to check are

CSSParser::ParseStyleAttribute
CSSParser::ParseProperty
nsXMLDocument::Load (not sure where the security check for this lives)
nsXULContentUtils::MakeElementURI (seems like this should use GetDocumentURI
                                   rather then GetBaseURI)
nsXULContentUtils::MakeElementID (same as above)
txExecutionState::retrieveDocument (this uses the wrong referrer)
nsGlobalWindow::BuildURIfromBase


These functions seem wrong:

TX_CompileStylesheet, as does most of the xslt compiler checks
SendPing in nsWebShell.cpp
nsDOMParser::ParseFromStream

I've done up until nsXFormsModelElement.


Given how many places we seem to be doing this wrong (though luckily most of them seem to use the document where we do have same-origin policies), maybe we need to keep the current checks :(
It is technically wrong, but it might be safest.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> However, we don't have similar checks for the xml:base.

Er... We sure seem to in nsGenericElement::GetBaseURI().  What am I missing?

> Same-origin checks should always be done against the document uri, not the
> document base-uri.

Agreed on that, but this has always seemed like a belt-and-suspenders kinda thing to me.  If we change this we have to audit all document or content base users, of course.  I guess that's what you're doing, eh?

> So far it seems like nsGenericElement::TriggerLink (or arguably the two users
> in nsGenericHTMLElement::HandleDOMEventForAnchors) are doing the wrong thing.

It certainly is, but the patch in bug 324600 fixes it.  Want to review?  ;)

And yeah, I think we should fix the existing misusers but probably keep the checks on setting the base, just in case.

Comment 3

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

Doh, not sure how I missed the check in nsGenericElement::GetBaseURI.

However, all the checks we do (both in nsGenericElement::GetBaseURI and nsDocument::SetBaseURI) are CheckLoadURI check rather then same-origin checks. This means that you can set any http-uri as your base uri, so we must not rely on it for same-origin checks. So all the places i've listed above are more or less vulnerable :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, yes.  Using the base URI for same-origin checks is broken.  So is using the document URI.  All same-origin checks should use the document principal.  Otherwise they're not accounting for document.domain, for example.

Comment 5

[Boris Zbarsky [:bzbarsky]]

In fact, I'd really like to remove the URI version of CheckSameOrigin completely....  We need to make principals scriptable and accessible from script first, though.

Comment 6

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

So I'm slightly mutating this bug. Since the checks we do have are very lenient, there is no point in removing them.

What we should do is to audit all users of GetBaseURI to make sure they do the right thing. The places I've mentioned above needs fixing, and I'll audit the remaining ones too.

Comment 7

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

Attachment: Patch to fix

This should fix it. Some of this is just renaming variables to more clearly indicate that they contain the baseuri or documenturi.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 214277 [details] [diff] [review]
Patch to fix

>Index: content/xslt/src/xslt/txMozillaStylesheetCompiler.cpp
>+    // XXX This would be simpler if GetBaseURI lived on nsINode

File a bug to do that, reference it here?  I guess the issue is the difference in method signatures, eh?  :(

>+    uri->GetSpec(spec);

Yuck.  Any chance this code could move towards using nsIURI instead of strings?  In the future?

>Index: content/xul/templates/src/nsXULContentUtils.cpp
>-        nsIURI *docURL = aDocument->GetBaseURI();
>+        nsIURI *docURL = aDocument->GetDocumentURI();

Why?  This looks to me like it wants to be using the base URI, no?  Or is this code actually using URIs when it doesn't mean URIs?

> nsXULContentUtils::MakeElementID(nsIDocument* aDocument, const nsAString& aURI, nsAString& aElementID)
>-    aDocument->GetBaseURI()->GetSpec(spec);
>+    aDocument->GetDocumentURI()->GetSpec(spec);

Same.

r=bzbarsky with the questions answered.

Comment 9

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

(In reply to comment #8)
> (From update of attachment 214277 [details] [diff] [review] [edit])
> >Index: content/xslt/src/xslt/txMozillaStylesheetCompiler.cpp
> >+    // XXX This would be simpler if GetBaseURI lived on nsINode
> 
> File a bug to do that, reference it here?  I guess the issue is the difference
> in method signatures, eh?  :(

So far this is the only usecase and i'd rather that we waited with filing a bug until we know that having it on nsINode is what we really want. I'll change the comment to that effect.

> >+    uri->GetSpec(spec);
> 
> Yuck.  Any chance this code could move towards using nsIURI instead of 
> strings? In the future?

Yeah, we really should. It's tricky to get that sensible in standalone transformiix though.

> >Index: content/xul/templates/src/nsXULContentUtils.cpp
> >-        nsIURI *docURL = aDocument->GetBaseURI();
> >+        nsIURI *docURL = aDocument->GetDocumentURI();
> 
> Why?  This looks to me like it wants to be using the base URI, no?  Or is this
> code actually using URIs when it doesn't mean URIs?

These two function is creating/parsing uris that refer to elements in the current document. So to get to these elements you need to go through the current documents uri, not base uri.

Makes sense?

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 214277 [details] [diff] [review]
Patch to fix

sr=jst

Comment 11

[Boris Zbarsky [:bzbarsky]]

Sounds good.

Comment 12

[Darin Fisher]

Please get this fix on the trunk and 1.8.1 branch so it can bake before we consider it for the next 1.8.0.x train.  Thanks!

Comment 13

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

Attachment: Patch that landed on 1.8.1 branch

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 214277 [details] [diff] [review]
Patch to fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 15

[Bob Clary [:bc] (inactive)]

sicking, any idea on how to test this?

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 214277 [details] [diff] [review]
Patch to fix

Clearing stale requests.