186538 - Touching the ownerElement of an attribute through XPath generates a security error.

Status: VERIFIED FIXED

(Core :: XSLT, defect, P2)

Description

[Laurens van den Oever]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20021222
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20021222

The following code will reproduce the problem:

var dp = new DOMParser();
var doc = dp.parseFromString("<element/>", "text/xml");
var attr = doc.createAttribute("attr");
doc.documentElement.setAttributeNode(attr);
var xpe = new XPathEvaluator();

// This will generate "Security error"  code: "1000" nsresult: "0x805303e8
(NS_ERROR_DOM_SECURITY_ERR)".
var res = xpe.evaluate("..", attr, xpe.createNSResolver(attr),
XPathResult.FIRST_ORDERED_NODE_TYPE, null);

// So will the xpath "ancestor::*" and "../*"

Reproducible: Always

Steps to Reproduce:
1. Evaluate an XPath with an attribute as context node.
2. Touch its ownerElement with ".." or "ancestor::*" etc.

Actual Results:  
"Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"

Expected Results:  
Return the ownerElement.

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

XPath belongs under XSLT component, reaqssigning.

Comment 2

[Peter Van der Beken [:peterv]]

CanCallerAccess needs to be able to handle attribute nodes.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: testcase

I had to slightly adapt the testcase to use getAttribute since otherwise the
attribute doesn't have an ownerDocument. I'll let sicking fix that when he
rewrites attributes ;-).

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 111707 [details] [diff] [review]
v1

- In nsContentUtils.cpp:

+	 if (!ni) {
+	     // aNode is not part of a document, let any caller access it.
+
+	     return PR_TRUE;
+	 }
+
+	 ni->GetDocumentPrincipal(getter_AddRefs(unTrustedPrincipal));
+
+	 if (!unTrustedPrincipal) {
+	   // we can't get to the principal, let any caller access it.
+
+	   return NS_OK;

Huh? Don't you mean PR_TRUE (which is != NS_OK)?

Other than that, sr=jst

Comment 6

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

ugh, this is a lot of additional code :(. Could (for CheckSameOrigin) instead
declare |nsCOMPtr<nsIAttribute> attr| where |nsCOMPtr<nsIContent> content| is
declared. And then in the |if (!unTrustedDoc) {| just add.

    attr = do_QueryInterface(aUnTrustedNode);
    if (!attr) {
      // aUnTrustedNode is not a nsIContent, a nsIAttribute or a nsIDocument,
      // something weird is going on...
      NS_ERROR("aUnTrustedNode is not nsIContent, nsIAttribute or nsIDocument!");
      return NS_ERROR_UNEXPECTED;
    }

Then change the |else| (currently on line 487) to |if (!unTrustedDoc)| and
change the |content->GetNodeInfo| to use either |content| or |attr| depending on
which one is available.

Same for the other two places where you add the attr-stuff.

Pretty please! This code is hairy enough anyway :(

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 8

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

Comment on attachment 111997 [details] [diff] [review]
v1.1

>+nsresult
>+nsContentUtils::GetDocumentAndPrincipal(nsIDOMNode* aNode,
>+                                        nsIDocument** aDocument,
>+                                        nsIPrincipal** aPrincipal)
...
>+      if (!ni) {
>+        // we can't get to the principal so we'll give up and give the caller
>+        // access
>+
>+        return NS_OK;
>+      }
...
>+      if (!aPrincipal) {
>+        // we can't get to the principal so we'll give up and give the caller access
>+
>+        return NS_OK;

You should probably remove the "and give the caller access"-part from these
comments since this isn't really where we actually allow the access, that's
done in the callers to this function.

This is a very nice patch indeed. Nice to see that codeduplication removed!

r=sicking

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 111997 [details] [diff] [review]
v1.1

Rerequesting jst's sr, the patch changed a bit to consolidate duplicate code.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 111997 [details] [diff] [review]
v1.1

- In nsIAttribute.h:

+  NS_IMETHOD GetNodeInfo(nsINodeInfo** aNodeInfo) = 0;

While I realize that this is the "right way", I'd say you should make this pass
the out parameter as a reference to a pointer, for consistency's sake since the
same method in nsIContent uses a reference to a pointer...

- In nsContentUtils::GetDocumentAndPrincipal():

+      if (content) {
+	 content->GetNodeInfo(*getter_AddRefs(ni));
+      }
+      else {
+	 attr->GetNodeInfo(getter_AddRefs(ni));
+      }

... and consequently this wouldn't look this goofy :-)

+  if (!aPrincipal) {
+    (*aDocument)->GetPrincipal(aPrincipal);
+  }

Shouldn't that be if (!*aPrincipal)? I.e. missing '*'?

+  rv = sSecurityManager->CheckSameOriginPrincipal(subjectPrincipal,
+						  principal);

Fix the indentation-off-by-one.

- In txURIUtils.cpp:

	 if (!domDoc) {
	     nsCOMPtr<nsINodeInfo> ni;
-	     content->GetNodeInfo(*getter_AddRefs(ni));
+	     if (content) {
+	       content->GetNodeInfo(*getter_AddRefs(ni));
+	     }
+	     else {
+	       attr->GetNodeInfo(getter_AddRefs(ni));
+	     }

Don't mix 2-space indentation with 4-space indentation. More of this further
down...

sr=jst if you fix that.

Comment 11

[Peter Van der Beken [:peterv]]

Checked in.

Comment 12

[Axel Hecht]

mass verifying