Ubuntu
firefox package

Bug #1022741
Comment #26

Comment 26 for bug 1022741

Revision history for this message

In Mozilla Bugzilla #341604, Khuey (khuey) wrote on 2011-12-03:

#26

Comment on attachment 578799
WIP - experimental prototype

Review of attachment 578799:
-----------------------------------------------------------------

Drive by comments. These should definitely not be taken as exhaustive.

Looks like this doesn't attempt to cover plugins? Also looks like this doesn't attempt to deal with scripts? I expect those are going to be a significant amount of work.

We should detect the situation called out in the first warning (and maybe the second too?) at http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-iframe-sandbox and send a warning to the error console.

Reading the spec and the proto-patch here, I think storing the sandbox flags on both the docshell and the document is going to be a source of great pain. I think you're going to want one canonical set of sandbox flags (on the outer window, perhaps?).

Also, you really should request f? from a specific person. I gave an f-, but at this stage that is to be expected.

Finally, this is going to need lots of tests ...

You could make aForceOwner default to false here.

@@ +1864,5 @@
> + *
> + * @param aAttribute the value of the sandbox attribute as an nsIDOMDOMSettableTokenList
> + * @return the set of flags
> + */
> + static PRUint32 ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute);

80 character lines please.

::: content/base/public/nsIDocument.h
@@ +462,5 @@
> }
>
> /**
> + * Set the sandbox flags for this document. This should only happen at
> + * at document load, the flags are immutable after being set at load time.

Can you assert that this is being called during load? (by checking for READYSTATE_LOADING or something?)

@@ +471,5 @@
> + mSandboxFlags = aSandboxFlags;
> + }
> +
> + /**
> + * Get the sandox flags for this document

sandbox

@@ +1786,5 @@
>
> + // The sandbox flags on the document. These reflect the value of the sandbox attribute of the
> + // associated IFRAME or CSP-protectable content, if existent. These are set at load time and
> + //are immutable - see nsDocShell.idl for the possible flags
> + PRUint32 mSandboxFlags;

nsIDocument needs an IID bump for this.

::: content/base/src/nsContentUtils.cpp
@@ +699,5 @@
> + * @param aAttribute the value of the sandbox attribute as an nsIDOMDOMSettableTokenList
> + * @return the set of flags
> + */
> +PRUint32
> +nsContentUtils::ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute)

You should rewrite this function to use nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace>. As it is it'll match things like "allow-scriptsallow-same-origin" which are not allowed per spec.

I'd give extra points for putting a

typedef nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace> HTMLSplitOnSpacesTokenizer (or something)

in nsContentUtils.h and changing the existing users in the tree.

@@ +702,5 @@
> +PRUint32
> +nsContentUtils::ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute)
> +{
> + // TODO: how to handle errors in this function ? if we're here, something has a sandbox
> + // attribute set and expects content to be sandboxed... perhaps consider failing closed ??

You can do this one of three ways:

1) Return an explicit nsresult and pass the flags back in a PRUint32 outparam.
2) Return a clearly bogus value for failure (e.g. -1)
3) Fail closed (-1 effectively does this if somebody forgets the check).

What in here is fallible though? Simple tokenization and comparison should be infallible, I think.

@@ +711,5 @@
> + out |= nsIDocShell::SANDBOX_FLAGS_SANDBOXED;
> +
> + bool allowed = false;
> +
> + // TODO: should i try to use atoms even though Contains wants a string ?

Yes, you should use the atoms and do 'if (atom->Equals(token)) ...'

::: content/base/src/nsDocument.cpp
@@ +2394,5 @@
> + nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(aContainer);
> +
> + if (docShell) {
> + nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags);
> + NS_ENSURE_SUCCESS(rv, rv);

ugh tabs

::: content/base/src/nsFrameLoader.cpp
@@ +506,5 @@
> + // and it may not be - maybe fail closed (all restrictions) ???
> + }
> +
> + if (parentSandboxFlags) {
> + // do the intersection with the current flags in the docshell!

I didn't look at this too closely, I assume that permissions can only be removed, not added, for children?

e.g. if I have:

- Top Level Doc (A)
- Sandboxed Iframe (B)
- Sandboxed Iframe (C)

And B has permissions 1 and 2 there's no way for C to have permission 3, right?

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +691,5 @@
> + PRUint32 sandboxFlags = doc->GetSandboxFlags();
> +
> + if (!(sandboxFlags & nsIDocShell::SANDBOX_FLAGS_SANDBOXED) ||
> + (sandboxFlags & nsIDocShell::SANDBOX_FLAGS_ALLOW_FORM_SUBMIT))
> + rv = DoSubmit(aEvent);

Does this check avoid doing form validation and the like when sandboxed? (I think it does). You'll want tests for that.

::: content/html/content/src/nsHTMLIFrameElement.cpp
@@ +306,5 @@
> + bool aNotify)
> +{
> + if (aName == nsGkAtoms::sandbox && aNameSpaceID == kNameSpaceID_None) {
> + // parse the new value of the sandbox attribute, and if we have a docshell
> + // set its sandbox flags appropriately

When is a sandbox attribute set supposed to take effect?

"While the sandbox attribute is specified, the iframe element's nested browsing context must have the flags given in the following list set. In addition, any browsing contexts nested within an iframe, either directly or indirectly, must have all the flags set on them as were set on the iframe's Document's browsing context when the iframe's Document was created."

So it sounds like a sandbox attr change should only take effect for the next document in the iframe. Setting them on the docshell immediately means they'll take affect for future subloads even on the same document, right? You'll want either tests to show that this isn't the case or to change the code so that it isn't (and then tests :-)).

::: docshell/base/nsDocShell.cpp
@@ +755,5 @@
> mIsBeingDestroyed(false),
> mIsExecutingOnLoadHandler(false),
> mIsPrintingOrPP(false),
> + mSavingOldViewer(false),
> + mSandboxFlags(0)

Please order intializers in the same order variables are declared (doing otherwise will give warnings on gcc).

@@ +2887,5 @@
> + // If our document is sandboxed, we need to do some extra checks
> + PRUint32 sandboxFlags = 0;
> +
> + if (mContentViewer) {
> + nsCOMPtr<nsIDocument> doc = mContentViewer->GetDocument();

No need to use an nsCOMPtr here. nsIDocument* doc is fine.

@@ +2944,5 @@
> + parentAsItem = tmp;
> + }
> + }
> + }
> +

Somebody who understand docshell (read bz, smaug) should look at this stuff. In my five minutes of looking at docshell it looks like this logic might fit better in CanAccessItem.

@@ +4996,5 @@
> +
> +NS_IMETHODIMP
> +nsDocShell::GetSandboxFlags(PRUint32 *aSandboxFlags)
> +{
> + NS_ENSURE_ARG_POINTER(aSandboxFlags);

Usually we don't bother with this. If you pass in null, you crash.

@@ +8201,5 @@
> + PRUint32 sandboxFlags = 0;
> + sandboxFlags = doc->GetSandboxFlags();
> + if (sandboxFlags & nsIDocShell::SANDBOX_FLAGS_SANDBOXED) {
> + return NS_ERROR_FAILURE;
> + }

Same comment as above about somebody who knows docshell looking at this.

@@ +8996,5 @@
> + nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, true);
> + }
> + else {
> + nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, false);
> + }

I think this would be easier to follow with one callsite whose final parameter varies.

::: docshell/base/nsIDocShell.idl
@@ +561,5 @@
> + * They are only used when loading new content, sandbox flags are also immutably set on the document
> + * when it is loaded. The sandbox flags of a document depend on the sandbox flags on its docshell
> + * and of its parent document, if any.
> + */
> + attribute unsigned long sandboxFlags;

nsIDocShell needs an IID bump for this.

@@ +576,5 @@
> +
> + /**
> + * The content is sandboxed but allowed to have its natural domain.
> + */
> + const unsigned long SANDBOX_FLAGS_ALLOW_SAME_DOMAIN = 0x4;

please make this ALLOW_SAME_ORIGIN so that the terminology is consistent.

::: dom/interfaces/html/nsIDOMHTMLIFrameElement.idl
@@ +68,5 @@
> readonly attribute nsIDOMDocument contentDocument;
> readonly attribute nsIDOMWindow contentWindow;
> +
> + // HTML5 sandbox attribute
> + readonly attribute nsIDOMDOMSettableTokenList sandbox;

Needs an IID bump.

Comment on attachment 578799
WIP - experimental prototype

Review of attachment 578799:
-----------------------------------------------------------------

Drive by comments.  These should definitely not be taken as exhaustive.

Looks like this doesn't attempt to cover plugins?  Also looks like this doesn't attempt to deal with scripts?  I expect those are going to be a significant amount of work.

Reading the spec and the proto-patch here, I think storing the sandbox flags on both the docshell and the document is going to be a source of great pain.  I think you're going to want one canonical set of sandbox flags (on the outer window, perhaps?).

Also, you really should request f? from a specific person.  I gave an f-, but at this stage that is to be expected.

Finally, this is going to need lots of tests ...

::: content/base/public/nsContentUtils.h
@@ +1838,5 @@
>    static bool SetUpChannelOwner(nsIPrincipal* aLoadingPrincipal,
>                                  nsIChannel* aChannel,
>                                  nsIURI* aURI,
> +                                bool aSetUpForAboutBlank,
> +                                bool aForceOwner);

You could make aForceOwner default to false here.

@@ +1864,5 @@
> +   *
> +   * @param aAttribute 	the value of the sandbox attribute as an nsIDOMDOMSettableTokenList
> +   * @return 			the set of flags
> +   */
> +  static PRUint32 ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute);

80 character lines please.

::: content/base/public/nsIDocument.h
@@ +462,5 @@
>    }
>    
>    /**
> +   * Set the sandbox flags for this document. This should only happen at 
> +   * at document load, the flags are immutable after being set at load time.

Can you assert that this is being called during load? (by checking for READYSTATE_LOADING or something?)

@@ +471,5 @@
> +    mSandboxFlags = aSandboxFlags;
> +  }
> +  
> +  /** 
> +   * Get the sandox flags for this document

sandbox

@@ +1786,5 @@
>  
> +  // The sandbox flags on the document. These reflect the value of the sandbox attribute of the 
> +  // associated IFRAME or CSP-protectable content, if existent. These are set at load time and
> +  //are immutable - see nsDocShell.idl for the possible flags   
> +  PRUint32 mSandboxFlags;

nsIDocument needs an IID bump for this.

::: content/base/src/nsContentUtils.cpp
@@ +699,5 @@
> + * @param aAttribute    the value of the sandbox attribute as an nsIDOMDOMSettableTokenList
> + * @return 			    the set of flags
> + */
> +PRUint32
> +nsContentUtils::ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute)

You should rewrite this function to use nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace>.  As it is it'll match things like "allow-scriptsallow-same-origin" which are not allowed per spec.

I'd give extra points for putting a

typedef nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace> HTMLSplitOnSpacesTokenizer (or something)

in nsContentUtils.h and changing the existing users in the tree.

@@ +702,5 @@
> +PRUint32
> +nsContentUtils::ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute)
> +{
> +  // TODO: how to handle errors in this function ? if we're here, something has a sandbox
> +  // attribute set and expects content to be sandboxed... perhaps consider failing closed ??

You can do this one of three ways:

What in here is fallible though?  Simple tokenization and comparison should be infallible, I think.

@@ +711,5 @@
> +  out |= nsIDocShell::SANDBOX_FLAGS_SANDBOXED;
> +  
> +  bool allowed = false;
> +  
> +  // TODO: should i try to use atoms even though Contains wants a string ?

Yes, you should use the atoms and do 'if (atom->Equals(token)) ...'

::: content/base/src/nsDocument.cpp
@@ +2394,5 @@
> +  nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(aContainer);
> +  
> +  if (docShell) {
> +     nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags);
> +	 NS_ENSURE_SUCCESS(rv, rv);

ugh tabs

::: content/base/src/nsFrameLoader.cpp
@@ +506,5 @@
> +      // and it may not be - maybe fail closed (all restrictions) ??? 
> +    }
> +     
> +    if (parentSandboxFlags) { 
> +      // do the intersection with the current flags in the docshell!

I didn't look at this too closely, I assume that permissions can only be removed, not added, for children?

e.g. if I have:

- Top Level Doc (A)
  - Sandboxed Iframe (B)
     - Sandboxed Iframe (C)

And B has permissions 1 and 2 there's no way for C to have permission 3, right?

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +691,5 @@
> +      PRUint32 sandboxFlags = doc->GetSandboxFlags();
> +      
> +      if (!(sandboxFlags & nsIDocShell::SANDBOX_FLAGS_SANDBOXED) || 
> +          (sandboxFlags & nsIDocShell::SANDBOX_FLAGS_ALLOW_FORM_SUBMIT))
> +        rv = DoSubmit(aEvent);

Does this check avoid doing form validation and the like when sandboxed? (I think it does).  You'll want tests for that.

::: content/html/content/src/nsHTMLIFrameElement.cpp
@@ +306,5 @@
> +                                  bool aNotify)
> +{
> +  if (aName == nsGkAtoms::sandbox && aNameSpaceID == kNameSpaceID_None) {
> +    // parse the new value of the sandbox attribute, and if we have a docshell
> +    // set its sandbox flags appropriately

When is a sandbox attribute set supposed to take effect?

So it sounds like a sandbox attr change should only take effect for the next document in the iframe.  Setting them on the docshell immediately means they'll take affect for future subloads even on the same document, right?  You'll want either tests to show that this isn't the case or to change the code so that it isn't (and then tests :-)).

::: docshell/base/nsDocShell.cpp
@@ +755,5 @@
>      mIsBeingDestroyed(false),
>      mIsExecutingOnLoadHandler(false),
>      mIsPrintingOrPP(false),
> +    mSavingOldViewer(false),
> +    mSandboxFlags(0)

Please order intializers in the same order variables are declared (doing otherwise will give warnings on gcc).

@@ +2887,5 @@
> +            // If our document is sandboxed, we need to do some extra checks
> +            PRUint32 sandboxFlags = 0;
> +           
> +            if (mContentViewer) {
> +                nsCOMPtr<nsIDocument> doc = mContentViewer->GetDocument();

No need to use an nsCOMPtr here.  nsIDocument* doc is fine.

@@ +2944,5 @@
> +                        parentAsItem = tmp;
> +                    }
> +                }
> +            } 
> +

Somebody who understand docshell (read bz, smaug) should look at this stuff.  In my five minutes of looking at docshell it looks like this logic might fit better in CanAccessItem.

@@ +4996,5 @@
> +
> +NS_IMETHODIMP
> +nsDocShell::GetSandboxFlags(PRUint32  *aSandboxFlags)
> +{
> +  NS_ENSURE_ARG_POINTER(aSandboxFlags);

Usually we don't bother with this.  If you pass in null, you crash.

@@ +8201,5 @@
> +            PRUint32 sandboxFlags = 0;
> +            sandboxFlags = doc->GetSandboxFlags();
> +            if (sandboxFlags & nsIDocShell::SANDBOX_FLAGS_SANDBOXED) {
> +              return NS_ERROR_FAILURE;
> +            }

Same comment as above about somebody who knows docshell looking at this.

@@ +8996,5 @@
> +      nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, true);
> +    }
> +    else {
> +      nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, false);
> +    }

I think this would be easier to follow with one callsite whose final parameter varies.

::: docshell/base/nsIDocShell.idl
@@ +561,5 @@
> +   * They are only used when loading new content, sandbox flags are also immutably set on the document
> +   * when it is loaded. The sandbox flags of a document depend on the sandbox flags on its docshell
> +   * and of its parent document, if any. 
> +   */   
> +  attribute unsigned long sandboxFlags;

nsIDocShell needs an IID bump for this.

@@ +576,5 @@
> +   
> +  /**
> +   * The content is sandboxed but allowed to have its natural domain. 
> +   */
> +  const unsigned long SANDBOX_FLAGS_ALLOW_SAME_DOMAIN = 0x4;

please make this ALLOW_SAME_ORIGIN so that the terminology is consistent.

::: dom/interfaces/html/nsIDOMHTMLIFrameElement.idl
@@ +68,5 @@
>    readonly attribute nsIDOMDocument   contentDocument;
>    readonly attribute nsIDOMWindow     contentWindow;
> +  
> +  // HTML5 sandbox attribute
> +  readonly attribute nsIDOMDOMSettableTokenList sandbox;

Needs an IID bump.

Ubuntufirefox package

Comment 26 for bug 1022741

Ubuntu
firefox package