Bug 32368 - Add defensive initialization of iframe sandbox flags
Summary: Add defensive initialization of iframe sandbox flags
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-10 01:50 PST by Patrik Persson
Modified: 2010-01-10 16:48 PST (History)
2 users (show)

See Also:


Attachments
Work in progress (11.02 KB, patch)
2010-01-10 00:58 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (14.22 KB, patch)
2010-01-10 13:12 PST, Adam Barth
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrik Persson 2009-12-10 01:50:38 PST
This is a followup to bug 21288, which concerned the implementation of
the HTML5 iframe sandbox attribute.  I'm curious whether it would be
possible to use more defensive initial values for the sandbox flags
than the current default of "SandboxNone" (indicating no sandboxing at
all).

This defensive technique is, to me, primarily about finding current
bugs and preventing future ones.  It is a design challenge rather than
a functional bug, so I don't have a test case to share.

Three classes contain such sandbox flags: HTMLFrameOwnerElement,
FrameLoader and SecurityOrigin.  I imagine three steps:

  1. On object instantiation, the sandbox flags are set to a defensive
     default value (SandboxAll, possibly including a special flag
     indicating an illegal value).

  2. At some point between 1 and 3, we know of a better value for the
     sandbox flags, and assign that value to the flags.

  3. We now start making decisions based on the sandbox flags.  To
     ensure that step 2 actually happened, we could ASSERT that the
     flag for an illegal value is not set.

The design challenge is to ensure step 2 always happens before step 3.
It was straight-forward for the FrameLoader (that part was included in
the landed patch for bug 21288), but not for HTMLFrameOwnerElement and
SecurityOrigin.  I wrote down some observations on this in the thread
for bug 21288, comment #58:

https://bugs.webkit.org/show_bug.cgi?id=21288#c58
Comment 1 Darin Adler 2009-12-10 17:25:55 PST
I think for HTMLFrameOwnerElement this issue probably doesn't apply. Using the DOM you can create a frame element a step at a time, and you can start using it at any point, so there is no real "illegal value" time for the DOM elements.

For SecurityOrigin I think we might be able to make it happen, though.
Comment 2 Adam Barth 2010-01-10 00:58:35 PST
Created attachment 46230 [details]
Work in progress
Comment 3 Adam Barth 2010-01-10 13:12:19 PST
Created attachment 46242 [details]
Patch
Comment 4 Darin Adler 2010-01-10 16:11:32 PST
Comment on attachment 46242 [details]
Patch

>      if (shouldTreatURLSchemeAsNoAccess(m_protocol))
>          m_isUnique = true;
>  
> -    // If this ASSERT becomes false in the future, please consider the impact
> -    // of m_sandoxFlags on m_isUnique.
> -    ASSERT(m_sandboxFlags == SandboxNone);
> +    if (isSandboxed(SandboxOrigin))
> +        m_isUnique = true;

I suggest initializing m_unique to the correct value from the outset.

    m_isUnique(shouldTreatURLSchemeAsNoAccess(m_protocol) || isSandboxed(SandboxOrigin))

To some the if statements may be slightly clearer, so it seems OK to leave it this way if you think that way.

> +    static PassRefPtr<SecurityOrigin> create(const KURL&, SandboxFlags sandboxFlags = SandboxNone);

The argument name isn't needed here because the type speaks for itself.
Comment 5 Adam Barth 2010-01-10 16:13:38 PST
> > +    static PassRefPtr<SecurityOrigin> create(const KURL&, SandboxFlags sandboxFlags = SandboxNone);
> 
> The argument name isn't needed here because the type speaks for itself.

Oh, I didn't know you could omit the argument name and provide a default value.

Thanks for the review.
Comment 6 Adam Barth 2010-01-10 16:48:54 PST
Comments addressed and landed in http://trac.webkit.org/changeset/53056