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

VERIFIED FIXED in mozilla1.3beta

Status

()

Core
XSLT
P2
major
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: Laurens van den Oever, Assigned: peterv)

Tracking

Trunk
mozilla1.3beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

16 years ago
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.
(Reporter)

Updated

16 years ago
Blocks: 171034
XPath belongs under XSLT component, reaqssigning.
Assignee: heikki → peterv
Component: XML → XSLT
QA Contact: rakeshmishra → keith
(Assignee)

Comment 2

16 years ago
CanCallerAccess needs to be able to handle attribute nodes.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows 2000 → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → mozilla1.3beta
(Assignee)

Comment 3

16 years ago
Created attachment 111706 [details]
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 ;-).
(Assignee)

Updated

16 years ago
Attachment #111707 - Flags: superreview?(jst)
Attachment #111707 - Flags: review?(bugmail)
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
Attachment #111707 - Flags: superreview?(jst) → superreview+
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 :(

(Assignee)

Comment 7

16 years ago
Created attachment 111997 [details] [diff] [review]
v1.1
Attachment #111707 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #111997 - Flags: review?(bugmail)
(Assignee)

Updated

16 years ago
Attachment #111707 - Flags: review?(bugmail)
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
Attachment #111997 - Flags: review?(bugmail) → review+
(Assignee)

Comment 9

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

Rerequesting jst's sr, the patch changed a bit to consolidate duplicate code.
Attachment #111997 - Flags: superreview?(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.
Attachment #111997 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 11

16 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 12

15 years ago
mass verifying
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.