283006 - XForms seems vulnerable to http-redirect attacks

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

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

From a brief look it looks like xforms is vulnerable to http-redirect attacks.
I.e. that you can point the instancedata url to your server and then send a
redirect response redirecting to some other server.

It's no huge deal since no 'real' xforms release has been done yet. But it
should still be fixed pretty quickly if the attac is in fact possible.

Comment 1

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

Btw, I don't know if xforms fetches other 'external' data then the instancedata.
If so that code would need to be fixed too.

Comment 2

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Is it enough to add a SameOriginCheck to the channel we are actually reading
data from?

Comment 3

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

Don't know. Check with darin. One thing to keep in mind is that you don't want
to make it possible to even check the existance of a resource. Though maybe that
is already possible using iframes...

Another thing that defenetly is not possible to stop that way is submitting data
to a server inside a firewall. Though this is probably possible anyway.

Comment 4

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Taking. Going to use nsIHttpEventSink for this.

Comment 5

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Attachment: v.1

Something like this. I'm not too happy copying the same methods in too places,
but fortunately they are quite small ;)

Comment 6

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Attachment: v.1.0.1

Added some null checks.

Comment 7

[Darin Fisher]

Good catch jonas!  nsXFormsSubmissionElement::OnRedirect should implement the
same security logic as the sending code.  In other words, if the submission is
simple GET w/ replace=all, then there is no reason for same origin checks (in
fact, you'd be breaking things in that case).  The other thing to keep in mind
with this bug is that some HTTP redirects are OK.  Sometimes they just redirect
from POST to GET.  In those cases, there shouldn't be a same origin concern IMO
when replace != instance.  But, that is maybe a minor detail.

The rule I think should be this:

* If the page is sending a XML document, then restrict.
* If the page is fetching a XML document, then restrict.
* Otherwise, allow.

Comment 8

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Comment on attachment 175029 [details] [diff] [review]
v.1.0.1

This doesn't work :(

Comment 9

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Attachment: v.1.1

darin, any comments on this one?
This uses same security check in ::OnRedirect as ::SendData.

Comment 10

[Darin Fisher]

could the page trick us out by modifying the replace attribute on the submission
element between the time it submits the document and the redirect is received? 
i think the answer is yes :(

Comment 11

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Attachment: v.1.2

(In reply to comment #10)
> could the page trick us out by modifying the replace attribute on the
submission
> element between the time it submits the document and the redirect is
received? 
> i think the answer is yes :(
Blaah, of course.

Comment 12

[Doron Rosenberg (IBM)]

Comment on attachment 175063 [details] [diff] [review]
v.1.2

>+
>+  if (mFormat & (ENCODING_XML | ENCODING_MULTIPART_RELATED) ||
>+      mIsReplaceInstance) {
>+    if (!nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), newURI))
>+      return NS_ERROR_ABORT;
>+  }
>+  return NS_OK;
>+}
nit: you could make it all one if, no need for nested if statements really.

>@@ -482,16 +535,20 @@ nsXFormsSubmissionElement::Submit()
> 
>   // 1. ensure that we are not currently processing a xforms-submit (see E37)
>   NS_ENSURE_STATE(!mSubmissionActive);
>   mSubmissionActive = PR_TRUE;
>   
>   if (mActivator)
>     mActivator->SetDisabled(PR_TRUE);
  if mActivator is null, will things break?


>   nsAutoString replace;
>   mElement->GetAttribute(NS_LITERAL_STRING("replace"), replace);
>-  if (format & (ENCODING_XML | ENCODING_MULTIPART_RELATED) ||
>-      replace.EqualsLiteral("instance"))
>+  if (mFormat & (ENCODING_XML | ENCODING_MULTIPART_RELATED) ||
>+      mIsReplaceInstance)
>   {
>     if (!nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), uri))
>       return NS_ERROR_ABORT;
>   }

nit: You could probably save the 2nd if and just have one if, unless its too
unreadable?

DOM usage and changes to existing code look good, not too sure about the necko
usage, darin should be able to nitpick that.

Comment 13

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

(In reply to comment #12)
> >   
> >   if (mActivator)
> >     mActivator->SetDisabled(PR_TRUE);
>   if mActivator is null, will things break?

No. mActivator is used only to mark <xf:submit> element disabled.

Comment 14

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Comment on attachment 175063 [details] [diff] [review]
v.1.2

Doron, modifiying those |if|s makes them quite unreadable,
but I can change them (if needed) before this is checked in.

Darin, could you sr(+|-)?

Comment 15

[Darin Fisher]

Comment on attachment 175063 [details] [diff] [review]
v.1.2

>Index: nsXFormsInstanceElement.cpp

>+NS_IMETHODIMP
>+nsXFormsInstanceElement::GetInterface(const nsIID & aIID, void **aResult)
>+{
>+  *aResult = nsnull;
>+  nsresult rv = QueryInterface(aIID, aResult);
>+  if (NS_FAILED(rv)) {

how about doing:

    if (NS_SUCCEEDED(rv))
      return rv;

that way you can reduce indenting of the remainder of the function :)


>+    NS_ENSURE_STATE(mElement);
>+    nsCOMPtr<nsIDOMDocument> domDoc;
>+    mElement->GetOwnerDocument(getter_AddRefs(domDoc));
>+    nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));
>+    NS_ENSURE_STATE(doc);
>+
>+    nsCOMPtr<nsISupports> container = doc->GetContainer();
>+    nsCOMPtr<nsIInterfaceRequestor> callbacks = do_QueryInterface(container);
>+    if (callbacks)
>+      return callbacks->GetInterface(aIID, aResult);

So, I'm curious.  Why is this needed here?  We didn't do 
this before (the channel would query callbacks from the
loadgroup if necessary), so why do it now?


>+nsXFormsInstanceElement::OnRedirect(nsIHttpChannel *aHttpChannel,
>+                                    nsIChannel *aNewChannel)
>+{
>+  NS_ENSURE_ARG_POINTER(aNewChannel);
>+  nsCOMPtr<nsIURI> newURI;
>+  nsresult rv = aNewChannel->GetURI(getter_AddRefs(newURI));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  NS_ENSURE_STATE(mElement);
>+  nsCOMPtr<nsIDOMDocument> domDoc;
>+  mElement->GetOwnerDocument(getter_AddRefs(domDoc));
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));
>+  NS_ENSURE_STATE(doc);
>+  return nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), newURI) ?
>+    NS_OK : NS_ERROR_ABORT;
>+}

incidentally, you could also just check that aHttpChannel
and aNewChannel have the same origin.  that might be less
code.  of course, what you have here is fine too.


>Index: nsXFormsSubmissionElement.h
...
>+  PRPackedBool   mSubmissionActive;
>+  PRPackedBool   mIsReplaceInstance;
>+  PRUint32       mFormat;

I think you should comment that these two new member variables
are valid over the course of a single submission.  So, I presume
they are valid only when mSubmissionActive is true, right?


>Index: nsXFormsSubmissionElement.cpp
...
>+nsXFormsSubmissionElement::GetInterface(const nsIID & aIID, void **aResult)

same comments apply to this guy.


>+nsXFormsSubmissionElement::OnRedirect(nsIHttpChannel *aHttpChannel,

and this guy.


>+  if (mFormat & (ENCODING_XML | ENCODING_MULTIPART_RELATED) ||
>+      mIsReplaceInstance) {

maybe this logic could be moved to a helper function that is
called in both places.	it'd be terrible if we ever got those
two places out of sync! :)


>@@ -482,16 +535,20 @@ nsXFormsSubmissionElement::Submit()
...
>+  nsAutoString replace;
>+  mElement->GetAttribute(NS_LITERAL_STRING("replace"), replace);
>+  mIsReplaceInstance = replace.EqualsLiteral("instance");

maybe mention why you are stashing this parameter.


>+  mFormat = GetSubmissionFormat(mElement);
>+  NS_ENSURE_STATE(mFormat != 0);

ditto

Comment 16

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

(In reply to comment #15)
> So, I'm curious.  Why is this needed here?  We didn't do 
> this before (the channel would query callbacks from the
> loadgroup if necessary), so why do it now?
> 

Oh, no need for that code. Removing it...

Comment 17

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Attachment: v.1.3

Comment 18

[Doron Rosenberg (IBM)]

checked in