490983 - Make schema-validation build with Firefox 3.5

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Philipp Wagner [:imphil]]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.0.9) Gecko/2009041500 SUSE/3.0.9-0.1.1 Firefox/3.0.9
Build Identifier: 

schema-validation as it currently is in the new hg repository does not build due to changes to mozilla core.

Reproducible: Always

Comment 1

[Philipp Wagner [:imphil]]

Attachment: only get it building (not ready for review)

This patch is not yet ready for review as it only comments out the cc->SetExceptionWasThrown(PR_TRUE); line. But I won't bother with this code until I know what it exactly does (see bug #490701).

Comment 2

[Philipp Wagner [:imphil]]

Attachment: Fix security problems and schema-validation build with FF 3.5 (Gecko 1.9.1)

Comment 3

[Philipp Wagner [:imphil]]

The attached bug fixes bug #462717, bug #490701 and this one. I'm sorry for "merging" these bugs into one, but they are closely related and require the same fix.

1) In XForms we allow schema loading from any location (see bug #462717 comment 14). This means, XForms needs to use the system principal for schema loading, which is done in nsXFormsModelElement::InitializeInstances(). XForms uses  nsSchemaLoader::LoadAsyncWithPrincipal(), which is noscript.

All other code using the schema-validation extension cannot be allowed to load schema data from anywhere, because the user can retrieve the loaded schema data using JS. These users need to use LoadAsync() or Load(), which uses the subject principal.

This also means we need to use the same principal for loading xsd:include/xsd:import as we did for loading the initial schema. This makes the patch in bug #462717 a security problem and is the reason why I introduced the mPrincipal member variable.

2) schema-validation does not need to do its own security checking. We use XMLHttpRequest, which already does security checking if we pass the right principal. This means we can safely remove all security checking code from the schema-validation extension, which was done by removing GetResolvedURI.

I hope I understood everything correctly and didn't introduce any new security problem.

Comment 5

[aaronr]

Comment on attachment 385595 [details] [diff] [review]
Fix security problems and schema-validation build with FF 3.5 (Gecko 1.9.1)

>diff --git a/src/nsSchemaLoader.cpp b/src/nsSchemaLoader.cpp

>@@ -452,17 +453,23 @@ nsBuiltinSchemaCollection::GetBuiltinTyp
>     }
>     else if (typeName == nsSchemaAtoms::sNMTOKEN_atom) {
>       typeVal = nsISVSchemaBuiltinType::BUILTIN_TYPE_NMTOKEN;
>     }
>     else if (typeName == nsSchemaAtoms::sNMTOKENS_atom) {
>       typeVal = nsISVSchemaBuiltinType::BUILTIN_TYPE_NMTOKENS;
>     }
>     else {
>-      NS_ERROR("Unknown builtin type");
>+      nsAutoString errorMsg(NS_LITERAL_STRING("nsSchemaLoader::GetType: "));
>+      errorMsg.AppendLiteral("Unknown builtin type {");
>+      errorMsg.Append(aNamespace);
>+      errorMsg.AppendLiteral("}:");
>+      errorMsg.Append(aName);
>+      errorMsg.AppendLiteral("\"");
>+      NS_WARNING(NS_ConvertUTF16toUTF8(errorMsg).get());

nit: looks like you are outputting:
  nsSchemaLoader::GetType: Unknown builtin type {namespace}:name"

what is that final double quote for?  I assume you are missing a first double quote that it matches?


>@@ -590,20 +597,22 @@ nsSchemaLoader::GetType(const nsAString 
> {
>   nsresult rv = NS_OK;
> 
>   if (IsSchemaNamespace(aNamespace) || IsSOAPNamespace(aNamespace)) {
>     rv = mBuiltinCollection->GetType(aName, aNamespace, _retval);
> 
>     if (NS_FAILED(rv)) {
>       nsAutoString errorMsg(NS_LITERAL_STRING("nsSchemaLoader::GetType: "));
>-      errorMsg.AppendLiteral("Failure processing schema: cannot get schema type \"");
>+      errorMsg.AppendLiteral("Failure processing schema: cannot get schema type {");
>+      errorMsg.Append(aNamespace);
>+      errorMsg.AppendLiteral("}:");
>       errorMsg.Append(aName);
>       errorMsg.AppendLiteral("\"");

nit: you got rid of the first double quote, can get rid of this one

>@@ -611,94 +620,49 @@ nsSchemaLoader::GetType(const nsAString 
>   if (NS_FAILED(rv)) {
>     return rv;
>   }
> 
>   rv = schema->GetTypeByName(aName, _retval);
> 
>   if (NS_FAILED(rv)) {
>     nsAutoString msg(NS_LITERAL_STRING("nsSchemaLoader::GetType: "));
>-    msg.AppendLiteral("Failure processing schema: ");
>-    msg.AppendLiteral("cannot get schema type \"");
>+    msg.AppendLiteral("Failure processing schema: cannot get schema type {");
>+    msg.Append(aNamespace);
>+    msg.AppendLiteral("}:");
>     msg.Append(aName);
>     msg.AppendLiteral("\"");

same here

Idea sounds good to me.  With those, r=me

Comment 6

[Philipp Wagner [:imphil]]

Attachment: from Aaron's review: add missing double quotes

Comment 7

[alexander :surkov (:asurkov)]

It sounds your patch is a bit obsolete, at least I can't find removed code in nsSchemaLoader::ProcessSchemaElement (http://mxr.mozilla.org/mozilla/source/extensions/schema-validation/src/nsSchemaLoader.cpp#888)

Comment 8

[alexander :surkov (:asurkov)]

And actually I'm not best person to review neither schema-validation nor security related code. It's worth to ask Doron for schema-validation, probably Boris or Olli for security issues.

Comment 9

[Philipp Wagner [:imphil]]

Attachment: The right patch ...

The previous patch was on top of the one from bug #462717, sorry.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'm not the right reviewer for this either.  I just have no idea whether the preconditions in comment 3 are correct, which means I can't tell whether the conclusions are correct.

If someone can describe the precise security policy we're trying to enforce, I might be able to tell you whether that's sensible, and if you describe exactly the steps you're taking to enforce it I might be able to tell you whether those steps might be sufficient.  But no guarantees on that last; you really need to know exactly which parts of this code do what to review that sort of thing.  And I don't, nor do I plan to spend the time trying to learn it...

Comment 11

[Philipp Wagner [:imphil]]

The security policy is simple:
1) For XForms, allow loading a schema from all anywhere without enforcing a same-origin policy. This was discussed in bug #462717 comment
14 and found to be ok, because no schema data can be extracted.

The implementation is inside  LoadAsyncWithPrincipal() (it's noscript). XForms calls this method with the system principal to load a schema.

2) The schema-validation extension can be used directly from JS, without XForms. For these users, we want to have the same restrictions as with a normal XMLHttpRequest.

That's the methods Load() and LoadAsync(), which always use the subject principal for loading.

Comment 12

[Doron Rosenberg (IBM)]

The patch looks ok, but I've been out of the mozilla loop for a while, so I can't safely say "this is secure".

First, please add a comment to where we used to do GetResolvedURI to say something like "no need to check security here as we are passing the principal to xmlhttp which will do the check for us".

Secondly, since we now store the principal as a member, we have to be careful to make sure it can't be exploited somehow.  Is there anywhere someone can call into the code and use the previously loaded principal?

Comment 13

[Philipp Wagner [:imphil]]

mPrincipal is used in two methods: GetDocumentFromURI and LoadAsyncWithPrincipal

GetDocumentFromURI is called from ProcessSchemaElement

Comment 14

[Philipp Wagner [:imphil]]

I'm sorry, I didn't intend to submit the previous comment yet.

mPrincipal is used in two methods: GetDocumentFromURI() and
LoadAsyncWithPrincipal()

GetDocumentFromURI() is called from ProcessSchemaElement() and Load(). 
- Load() always sets mPrincipal to the subject principal. 
- ProcessSchemaElement() is called to process a xsd:import element, then it continues to use the previously set principal (which was set in one of the Load* methods). But it is not marked noscript, so it can be called directly from JS. This would result in a crash right now (mPrincipal is not set). The upcoming patch resolves this by using the subject principal in that case.

LoadAsyncWithPrincipal() always sets mPrincipal to the given principal. This can be any principal if called from C++ or the subject principal when called from JavaScript via the LoadAsync() method.

Comment 15

[Philipp Wagner [:imphil]]

Attachment: Patch v4: add comments when we rely on XMLHttpRequest for security checks and fix possible crash

Comment 16

[aaronr]

(In reply to comment #12)
> 
> Secondly, since we now store the principal as a member, we have to be careful
> to make sure it can't be exploited somehow.  Is there anywhere someone can call
> into the code and use the previously loaded principal?

Yeah, I thought of that when I did my review but couldn't figure out a way that it could be abused.  'Course, my security knowledge is very minimal when it comes to the real world.

Comment 17

[Doron Rosenberg (IBM)]

Comment on attachment 387088 [details] [diff] [review]
Patch v4: add comments when we rely on XMLHttpRequest for security checks and fix possible crash

Given that we are getting principals for loads, this should be a safe patch.  I would still have someone more knowledgeable give it a security ok.

Or we could just noscript everything, I don't believe anyone is calling the apis from js.

Comment 18

[Philipp Wagner [:imphil]]

bz, does the basic idea look good to you? That is:

* Use the subject principal for all document loading from JS
* Allow C++ code to use any principal using a noscript method
* Let XMLHttpRequest (which does all document loading) do all security checks based on the given principal

It was already found to be ok that we do schema loading from XForms without same-origin limitations in bug #462717.

Comment 19

[Boris Zbarsky [:bzbarsky]]

What happens if multiple load() or loadAsync() calls happen, with different principals?  Can GetDocumentFromURI() for one of those calls happen after another one has already changed mPrincipal?  Should we be bailing out of GetDocumentFromURI if mPrincipal is null?  Why is it ok to remove the ProcessSchemaElement security check?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Shouldn't there be some sort of change to LoadAsync too?

Comment 21

[Boris Zbarsky [:bzbarsky]]

For that matter, what happens if privileged script makes a Load() call on a schema loader and then unprivileged script makes a ProcessSchemaElement call on the same loader?

Comment 22

[Philipp Wagner [:imphil]]

bz, thanks for your comments.

I made the following assumptions:

* It is not possible to get a nsISVSchemaLoader object in JS that was initialized inside C++ context. This means, all nsISVSchemaLoader objects which the user can get hold of in JS use the subject principal (possible entry points exported in the IDL file are Load(), LoadAsync() or ProcessSchemaElement(), all of which initialize with the subject principal).

* Using the subject principal is always safe.

* XMLHttpRequest does all required security checking if we pass the right principal.

I don't know enough about the code base to be sure that these assumptions are always right. 

Now to your questions:

> (In reply to comment #19)
> What happens if multiple load() or loadAsync() calls happen, with different
> principals?  

Load() and LoadAsync() always initializes mPrincipal with the current subject principal. Even if you call these methods from C++ after you've called LoadAsyncWithPrincipal() with the system principal, you'll always end up with the subject principal for that load request.

> Can GetDocumentFromURI() for one of those calls happen after
> another one has already changed mPrincipal?  

Only if you pass the nsISVSchemaLoader object around after initializing it somewhere else. Even this should be no problem in JS, since you only initialize it with the subject principal. I don't know if you could exchange the object through e.g. iframe boundaries and that way get it into another domain context. But wouldn't that be the same problem as passing a XMLHttpRequest object around?

> Should we be bailing out of
> GetDocumentFromURI if mPrincipal is null?  

Would aid debugging, but I don't see how mPrincipal could be null inside GetDocumentFromURI.
It's called from 
- Load() (which sets it to the subject principal)
- ProcessSchemaElement() (which uses the existing mPrincipal or sets it to the subject principal)

> Why is it ok to remove the
> ProcessSchemaElement security check?

It's actually not removed - it's only passed on to the XMLHttpRequest which is done inside GetDocumentFromURI.

(In reply to comment #20)
> Shouldn't there be some sort of change to LoadAsync too?

LoadAsync() gets the subject principal and calls LoadAsyncWithPrincipal(), which sets mPrincipal to the given (i.e. subject) principal. What other change did you mean?


(In reply to comment #21)
> For that matter, what happens if privileged script makes a Load() call on a
> schema loader and then unprivileged script makes a ProcessSchemaElement call on
> the same loader?

Again, if they are able to exchange the nsISVSchemaLoader object, they get the old subject principal. But what do you exactly mean by "privileged script" - are there ways to get more rights for your subject principal?

Comment 23

[Boris Zbarsky [:bzbarsky]]

> * It is not possible to get a nsISVSchemaLoader object in JS that was
> initialized inside C++ context.

I have no idea whether this assumption is correct.

> * Using the subject principal is always safe.

This depends on how it's used.

> Only if you pass the nsISVSchemaLoader object around after initializing it
> somewhere else. 

Really?  Two loadAsync calls can't race?  That said, what stops someone from passing such an object around?

> But wouldn't that be the same problem as passing a XMLHttpRequest object
> around?

Somewhat; but people know to not do that.

> Would aid debugging, but I don't see how mPrincipal could be null inside
> GetDocumentFromURI.

The subject principal is null if no JS is on the callstack.

> LoadAsync() gets the subject principal and calls LoadAsyncWithPrincipal()

Ah, so it was already doing that.  OK.

> Again, if they are able to exchange the nsISVSchemaLoader object

My problem is that I have no way to evaluate whether they can do so "by accident" from the system script's point of view.  Someone familiar with this code needs to evaluate that.

Comment 24

[Philipp Wagner [:imphil]]

First, all that we're talking here is mostly theoretical, as I don't know of anybody that uses the schema-validation extension from JS.

(In reply to comment #23)
> > * It is not possible to get a nsISVSchemaLoader object in JS that was
> > initialized inside C++ context.
> 
> I have no idea whether this assumption is correct.

That would be clearly a security problem if that would be done. I don't see such a problem in the XForms code.

> > * Using the subject principal is always safe.
> 
> This depends on how it's used.
> 
> > Only if you pass the nsISVSchemaLoader object around after initializing it
> > somewhere else. 
> 
> Really?  Two loadAsync calls can't race?  That said, what stops someone from
> passing such an object around?
> 
> > But wouldn't that be the same problem as passing a XMLHttpRequest object
> > around?
> 
> Somewhat; but people know to not do that.

OK, so we have the same problem here as with XMLHttpRequest objects. Considering that these are used everywhere and schema-validation is not used at all, I guess this problem is negligible.

> > Would aid debugging, but I don't see how mPrincipal could be null inside
> > GetDocumentFromURI.
> 
> The subject principal is null if no JS is on the callstack.

OK, I'll add a check for that in an upcoming patch.

> > Again, if they are able to exchange the nsISVSchemaLoader object
> 
> My problem is that I have no way to evaluate whether they can do so "by
> accident" from the system script's point of view.  Someone familiar with this
> code needs to evaluate that.

I don't see such a problem. Doron, could you confirm? 

All in all I guess we're at least as secure as we were before. Doron, are you comfortable with the changes or should we mark everything noscript, and thus render the schema-validation extension unusable for JS, but make sure that we don't have any security problems left?

Comment 25

[Philipp Wagner [:imphil]]

Actually, we're already checking the pointer in ProcessSchemaElement() with the NS_ENSURE_TRUE(principal, NS_ERROR_FAILURE);
line, so we should be ok here.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> as I don't know of anybody that uses the schema-validation extension from JS.

Why is it scriptable at all, then?  Could we reduce attack surface by changing that?

> Considering that these are used everywhere and schema-validation is not used
> at all, I guess this problem is negligible.

Other way around: something that is commonly used is better understood in terms of its pitfalls...

In any case, I think I've said about all I can without further study of this code.

Comment 27

[Philipp Wagner [:imphil]]

(In reply to comment #26)
> > as I don't know of anybody that uses the schema-validation extension from JS.
> 
> Why is it scriptable at all, then?  Could we reduce attack surface by changing
> that?

schema-validation was developed as a standalone extension, xforms only has a dependency on it. As it turns out, currently xforms seems to be the only user of schema-validation.

> > Considering that these are used everywhere and schema-validation is not used
> > at all, I guess this problem is negligible.
> 
> Other way around: something that is commonly used is better understood in terms
> of its pitfalls...
> 
> In any case, I think I've said about all I can without further study of this
> code.

thank you for your (fast!) help, I guess it's now up to Doron for the final word on how to proceed.

Comment 28

[Doron Rosenberg (IBM)]

Philipp - I think we should mark processSchemaElement as noscript, as it would be the biggest security concern.

Keeping load around for js should be safe, since we aren't exposing anything that could be exploited as far as I can tell.  The only concern would be cross domain-type attacks, and those should be covered already.

The only js user I know of was daniel glazman's company, and I believe they've switched away from xsd.

Comment 29

[Philipp Wagner [:imphil]]

Attachment: Patch v5: make ProcessSchemaElement noscript

Thanks Doron! I marked ProcessSchemaElement noscript and regenerated the UUID. I guess with that we're ready for checkin?

Comment 30

[alexander :surkov (:asurkov)]

checked in, http://hg.mozilla.org/schema-validation/rev/14c6bc8f8946