Closed
Bug 325814
Opened 18 years ago
Closed 18 years ago
Instance document loads don't do security or content policy checks
Categories
(Core Graveyard :: XForms, defect)
Core Graveyard
XForms
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: allan)
Details
(Keywords: fixed1.8.0.5, fixed1.8.1)
Attachments
(1 file, 2 obsolete files)
16.23 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
nsXFormsInstanceElement::LoadExternalInstance doesn't do security or content policy checks. This means it can be used for things ranging from hanging the user's browser to crashing the user's OS to reading data off the user's hard drive. This should really be fixed, trunk and branches... Was this code on the 1.7 branch?
Comment 1•18 years ago
|
||
Note that XForms never shipped as part of Mozilla or Firefox. And no, this wasn't on 1.7.
Comment 2•18 years ago
|
||
We actually do have a security check, http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsUtils.cpp#1110. But: bz so that still needs to do a content policy check bz but it doesn't need to checkloaduri, I guess bz fwiw, on trunk if this did a content policy check and after this one patch sicking and I are planning you wouldn't have to worry about instance documents loading other instances... ;) bz no, content policy check being nsIContentPolicy bz ShouldLoad doron right now we use IsCapabilityEnabled because we have some crazu russian compay using chrome xforms/xul apps. So the nsIPrincipal stuff can be replaced with the contentpoilicy? bz no bz the two are orthogonal bz content policy allows extensions to register to disallow loads bz eg we do our image disabling that way bz and some other stuff bz various whitelists bz so somewhat similar to the TestPermission thing doron so the core security cross-domain check is still CheckSameOriginPrincipal bz So you'd have 4 different checks, I guess. IsCapabilityEnabled, CheckSameOriginPrincipal, TestPermission, and content policy bz yeah
Comment 3•18 years ago
|
||
Since this is doing a same-origin check the remaining missing content-policy calls don't constitute a vulnerability. Can I clear the security bit or am I missing something?
Whiteboard: [sg:needinfo]
![]() |
Reporter | |
Comment 4•18 years ago
|
||
Yeah this is probably not a security issue. Bug 325005 doesn't really apply here. I'd missed the same-origin check when I was reading the function. :(
Group: security
Whiteboard: [sg:needinfo]
Assignee | ||
Updated•18 years ago
|
Assignee: aaronr → xforms
Assignee | ||
Comment 5•18 years ago
|
||
Adds NS_CheckContentLoadPolicy() checks to XForms loading of content. I just use nsIContentPolicy::TYPE_OTHER all the time. Do we need to distinguish between the different types, and use f.x. TYPE_SUBDOCUMENT for submissions?
![]() |
Reporter | |
Comment 6•18 years ago
|
||
Comment on attachment 223689 [details] [diff] [review] Patch >Index: nsXFormsUtils.cpp >+#include "nsContentUtils.h" >+#include "nsContentPolicyUtils.h" Why can you use these outside the layout module? And can you? I'm not convinced. >+nsXFormsUtils::CheckSameOrigin(nsIDocument *aBaseDocument, >+ nsIPrincipal* basePrincipal = aBaseDocument->NodePrincipal(); So basePrincipal is guaranteed non-null... >- if (NS_SUCCEEDED(rv)) { >+ if (basePrincipal && NS_SUCCEEDED(rv)) { ... and I don't understand this change. >+ aDoc, // context You really don't know what XForms element is trying to do the load?
Assignee | ||
Comment 7•18 years ago
|
||
(In reply to comment #6) > (From update of attachment 223689 [details] [diff] [review] [edit]) > >Index: nsXFormsUtils.cpp > > >+#include "nsContentUtils.h" > >+#include "nsContentPolicyUtils.h" > > Why can you use these outside the layout module? And can you? I'm not > convinced. My compiler is not complaining :-). It is used in modules/plugin too? > >+ aDoc, // context > > You really don't know what XForms element is trying to do the load? I do, I just did not check the parameters well enough. I assumed it wanted a document.
Assignee | ||
Comment 8•18 years ago
|
||
I now pass on the actual XForms (DOM) element. I am using GetOwnerDocument() though, I am not sure whether I should use GetCurrentDoc() (ie bug 303625)?
Attachment #223689 -
Attachment is obsolete: true
Attachment #223751 -
Flags: review?(bzbarsky)
Attachment #223689 -
Flags: review?(bzbarsky)
![]() |
Reporter | |
Comment 9•18 years ago
|
||
Comment on attachment 223751 [details] [diff] [review] Patch v2 >Index: nsXFormsInstanceElement.cpp >@@ -563,17 +556,17 @@ nsXFormsInstanceElement::LoadExternalIns Unrelated to this bug, but should this really use the document's URI instead of the node's base URI? This applies to several places in this code... Might want to file a separate bug on this. >Index: nsXFormsUtils.cpp > /* static */ PRBool >+nsXFormsUtils::CheckConnectionAllowed(nsIDOMElement *aElement, >+ NS_ENSURE_STATE(doc); That returns NS_ERROR_UNEXPECTED if !doc. But this method returns a PRBool. Please fix to return a boolean. Using GetOwnerDoc is correct -- for security check purposes that's what you want. >+ nsIPrincipal *basePrincipal = doc->NodePrincipal(); For what it's worth, you could use the NodePrincipal() of the nsINode that got passed in here... And also in CheckSameOrigin. Unless there are other callers of CheckSameOrigin? Not a big deal either way. >+nsXFormsUtils::CheckSameOrigin(nsIDocument *aBaseDocument, This documents that aBaseDocument must be non-null, right? >+nsXFormsUtils::CheckContentPolicy(nsIDOMElement *aElement, >+ nsIDocument *aDoc, This should assert that aElement is non-null. And document that both that and aDoc must be non-null. r=bzbarsky with the NS_ENSURE_STATE thing fixed and the comments added.
Attachment #223751 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 10•18 years ago
|
||
(In reply to comment #9) > (From update of attachment 223751 [details] [diff] [review] [edit]) > >Index: nsXFormsInstanceElement.cpp > >@@ -563,17 +556,17 @@ nsXFormsInstanceElement::LoadExternalIns > > Unrelated to this bug, but should this really use the document's URI instead of > the node's base URI? This applies to several places in this code... Might want > to file a separate bug on this. Filed bug 339777. > >Index: nsXFormsUtils.cpp > > /* static */ PRBool > >+nsXFormsUtils::CheckConnectionAllowed(nsIDOMElement *aElement, > > >+ NS_ENSURE_STATE(doc); > > That returns NS_ERROR_UNEXPECTED if !doc. But this method returns a PRBool. > Please fix to return a boolean. Doh! Classic error.
Assignee | ||
Comment 11•18 years ago
|
||
Attachment #223751 -
Attachment is obsolete: true
Attachment #223893 -
Flags: review?
Assignee | ||
Updated•18 years ago
|
Attachment #223893 -
Flags: review? → review?(Olli.Pettay)
Updated•18 years ago
|
Attachment #223893 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 12•18 years ago
|
||
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Assignee | ||
Updated•18 years ago
|
Keywords: fixed1.8.0.5
Assignee | ||
Updated•18 years ago
|
Keywords: fixed1.8.1
Assignee | ||
Updated•18 years ago
|
Whiteboard: xf-to-branch
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•