Removed forced sandbox flags.

With real users, we are seeing unexpected difference in between the
sandbox flags computed from the renderer and the browser process. This
is implemented by those two functions:
- content::NavigationRequest::ComputeSandboxFlagsToCommit()
- blink::FrameLoader::CalculateSandboxFlags

After a navigation, the new document gets sandbox_flags from:
- FramePolicy.sandbox, which contains everything except CSP.
- The response's CSP sandbox.

That's basically it. Both function does the same and are fed from the
same data. The only difference is blink, which used
"forced_sandbox_flags". This looked a bit mysterious at first. It turns
out to be needed only for setting the sandbox flags for the initial
empty document and the second 'initial empty document'. (e.g. not via
navigation).

Removing forced_sandbox_flags from CalculateSandboxFlags helps
guaranteeing both browser side and renderer side computation always
match. This make it explicit in the code from where the inheritance
happens.

This patch aligns inheriting sandbox from the opener with
"OpenerFeatureState" (1), by moving it toward Frame, instead of the
FrameLoader.

(1) Turns out this will be removed:
https://chromium-review.googlesource.com/c/chromium/src/+/2564396

Change-Id: I7098b459e485485630ab9edc99d392fcdbfdef4e
Bug: 1153708
Fixed: 1153708
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2573362
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834660}

third_party/blink/renderer/core/frame/csp/content_security_policy_fuzzer.cc

@@ -26,12 +26,7 @@ int LLVMFuzzerInitialize(int* argc, char*** argv) {
   LEAK_SANITIZER_DISABLED_SCOPE;
   g_page_holder = std::make_unique<DummyPageHolder>().release();
 
-  // Set loader sandbox flags and install a new document so the document
-  // has all possible sandbox flags set on the document already when the
-  // CSP is bound.
   scoped_refptr<SharedBuffer> empty_document_data = SharedBuffer::Create();
-  g_page_holder->GetFrame().Loader().ForceSandboxFlags(
-      network::mojom::blink::WebSandboxFlags::kAll);
   g_page_holder->GetFrame().ForceSynchronousDocumentInstall(
       "text/html", empty_document_data);
   return 0;

third_party/blink/renderer/core/frame/frame.h

@@ -32,6 +32,7 @@
 #include "base/i18n/rtl.h"
 #include "base/optional.h"
 #include "base/unguessable_token.h"
+#include "services/network/public/mojom/web_sandbox_flags.mojom-blink.h"
 #include "third_party/blink/public/common/feature_policy/document_policy_features.h"
 #include "third_party/blink/public/common/feature_policy/feature_policy_features.h"
 #include "third_party/blink/public/common/frame/user_activation_state.h"
@@ -253,6 +254,23 @@ class CORE_EXPORT Frame : public GarbageCollected<Frame> {
     return navigation_rate_limiter_;
   }
 
+  // Called to get the opener's sandbox flags if any. This works with disowned
+  // openers, i.e., even if WebFrame::Opener() is nullptr,
+  network::mojom::blink::WebSandboxFlags OpenerSandboxFlags() const {
+    return opener_sandbox_flags_;
+  }
+
+  // Sets the opener's sandbox_flags for the main frame. Once a non-empty
+  // |opener_feature_state| is set, it can no longer be modified (due to the
+  // fact that the original opener which passed down the FeatureState cannot be
+  // modified either).
+  void SetOpenerSandboxFlags(network::mojom::blink::WebSandboxFlags flags) {
+    DCHECK(IsMainFrame());
+    DCHECK_EQ(network::mojom::blink::WebSandboxFlags::kNone,
+              opener_sandbox_flags_);
+    opener_sandbox_flags_ = flags;
+  }
+
   const DocumentPolicyFeatureState& GetRequiredDocumentPolicy() const {
     return required_document_policy_;
   }
@@ -427,6 +445,10 @@ class CORE_EXPORT Frame : public GarbageCollected<Frame> {
 
   NavigationRateLimiter navigation_rate_limiter_;
 
+  // Sandbox flags inherited from an opener. It is always empty for child
+  // frames.
+  network::mojom::blink::WebSandboxFlags opener_sandbox_flags_;
+
   // The required document policy for any subframes of this frame.
   // Note: current frame's document policy might not conform to
   // |required_document_policy_| here, as the Require-Document-Policy HTTP

third_party/blink/renderer/core/frame/local_frame_client_impl.cc

@@ -744,6 +744,18 @@ void LocalFrameClientImpl::BeginNavigation(
   navigation_info->frame_policy =
       owner ? owner->GetFramePolicy() : FramePolicy();
 
+  // owner->GetFramePolicy() above only contains the sandbox flags defined by
+  // the <iframe> element. It doesn't take into account inheritance from the
+  // parent or the opener. This is not a problem in the general case, because
+  // this attribute is simply dropped! It matter only for the "fake" navigation
+  // to the "fake" initial empty document. It is:
+  // RenderFrameImpl::CommitInitialEmptyDocument().
+  // This one doesn't go toward the browser process, it commits synchronously.
+  // The sandbox flags must be defined. They correspond to the one already in
+  // use for the 'real' initial empty document.
+  navigation_info->frame_policy.sandbox_flags =
+      web_frame_->GetFrame()->Loader().PendingEffectiveSandboxFlags();
+
   navigation_info->href_translate = href_translate;
 
   web_frame_->Client()->BeginNavigation(std::move(navigation_info));

third_party/blink/renderer/core/frame/web_local_frame_impl.cc

@@ -1985,7 +1985,20 @@ void WebLocalFrameImpl::InitializeCoreFrameInternal(
       previous_sibling_frame, insert_type, GetFrameToken(),
       window_agent_factory, interface_registry_, std::move(policy_container)));
   frame_->Tree().SetName(name);
-  frame_->Loader().ForceSandboxFlags(sandbox_flags);
+
+  // See sandbox inheritance: content/browser/renderer_host/sandbox_flags.md
+  //
+  // New documents are either:
+  // 1. The initial empty document:
+  //   a. In a new iframe.
+  //   b. In a new popup.
+  // 2. A document replacing the previous, one via a navigation.
+  //
+  // This is about 1.b. This is used to define sandbox flags for the initial
+  // empty document in a new popup.
+  if (frame_->IsMainFrame())
+    frame_->SetOpenerSandboxFlags(sandbox_flags);
+
   Frame* opener_frame = opener ? ToCoreFrame(*opener) : nullptr;
 
   // We must call init() after frame_ is assigned because it is referenced

third_party/blink/renderer/core/loader/document_loader.cc

@@ -1441,9 +1441,15 @@ void DocumentLoader::DidCommitNavigation() {
 }
 
 network::mojom::blink::WebSandboxFlags DocumentLoader::CalculateSandboxFlags() {
-  auto sandbox_flags = GetFrameLoader().GetForcedSandboxFlags() |
-                       content_security_policy_->GetSandboxMask() |
-                       frame_policy_.sandbox_flags;
+  // The snapshot of the FramePolicy taken, when the navigation started. This
+  // contains the sandbox of the parent/opener and also the sandbox of the
+  // iframe element owning this new document.
+  auto sandbox_flags = frame_policy_.sandbox_flags;
+
+  // The new document's response can further restrict sandbox using the
+  // Content-Security-Policy: sandbox directive:
+  sandbox_flags |= content_security_policy_->GetSandboxMask();
+
   if (archive_) {
     // The URL of a Document loaded from a MHTML archive is controlled by
     // the Content-Location header. This would allow UXSS, since

third_party/blink/renderer/core/loader/frame_loader.cc

@@ -216,14 +216,6 @@ ResourceRequest FrameLoader::ResourceRequestForReload(
 FrameLoader::FrameLoader(LocalFrame* frame)
     : frame_(frame),
       progress_tracker_(MakeGarbageCollected<ProgressTracker>(frame)),
-      // Frames need to inherit the sandbox flags of their parent frame.
-      // These can be fixed at construction time, because the only actions that
-      // trigger a sandbox flags change in the parent will necessarily detach
-      // this frame.
-      forced_sandbox_flags_(
-          frame_->Tree().Parent()
-              ? frame_->Tree().Parent()->GetSecurityContext()->GetSandboxFlags()
-              : network::mojom::blink::WebSandboxFlags::kNone),
       dispatching_did_clear_window_object_in_main_world_(false),
       virtual_time_pauser_(
           frame_->GetFrameScheduler()->CreateWebScopedVirtualTimePauser(
@@ -249,10 +241,13 @@ void FrameLoader::Trace(Visitor* visitor) const {
 void FrameLoader::Init() {
   ScriptForbiddenScope forbid_scripts;
 
+  // Load the initial empty document:
   auto navigation_params = std::make_unique<WebNavigationParams>();
   navigation_params->url = KURL(g_empty_string);
   navigation_params->frame_policy =
       frame_->Owner() ? frame_->Owner()->GetFramePolicy() : FramePolicy();
+  navigation_params->frame_policy->sandbox_flags =
+      PendingEffectiveSandboxFlags();
 
   DocumentLoader* new_document_loader = Client()->CreateDocumentLoader(
       frame_, kWebNavigationTypeOther, CreateCSPForInitialEmptyDocument(),
@@ -1552,11 +1547,6 @@ void FrameLoader::RunScriptsAtDocumentElementAvailable() {
   // The frame might be detached at this point.
 }
 
-void FrameLoader::ForceSandboxFlags(
-    network::mojom::blink::WebSandboxFlags flags) {
-  forced_sandbox_flags_ |= flags;
-}
-
 void FrameLoader::DispatchDidClearDocumentOfWindowObject() {
   if (state_ == State::kUninitialized)
     return;
@@ -1593,10 +1583,12 @@ void FrameLoader::DispatchDidClearWindowObjectInMainWorld() {
 
 network::mojom::blink::WebSandboxFlags
 FrameLoader::PendingEffectiveSandboxFlags() const {
-  network::mojom::blink::WebSandboxFlags flags = forced_sandbox_flags_;
-  if (FrameOwner* frame_owner = frame_->Owner())
-    flags |= frame_owner->GetFramePolicy().sandbox_flags;
-  return flags;
+  if (Frame* parent = frame_->Tree().Parent()) {
+    return parent->GetSecurityContext()->GetSandboxFlags() |
+           frame_->Owner()->GetFramePolicy().sandbox_flags;
+  } else {
+    return frame_->OpenerSandboxFlags();
+  }
 }
 
 void FrameLoader::ModifyRequestForCSP(

third_party/blink/renderer/core/loader/frame_loader.h

@@ -145,16 +145,11 @@ class CORE_EXPORT FrameLoader final {
   void DispatchDocumentElementAvailable();
   void RunScriptsAtDocumentElementAvailable();
 
-  // The following sandbox flags will be forced, regardless of changes to the
-  // sandbox attribute of any parent frames.
-  void ForceSandboxFlags(network::mojom::blink::WebSandboxFlags flags);
-
-  network::mojom::blink::WebSandboxFlags GetForcedSandboxFlags() const {
-    return forced_sandbox_flags_;
-  }
-
-  // Includes the collection of forced, inherited, and FrameOwner's sandbox
-  // flags.
+  // See content/browser/renderer_host/sandbox_flags.md
+  // This contains the sandbox flags to commit for new documents.
+  // - For main documents, it contains the sandbox inherited from the opener.
+  // - For nested documents, it contains the sandbox flags inherited from the
+  //   parent and the one defined in the <iframe>'s sandbox attribute.
   network::mojom::blink::WebSandboxFlags PendingEffectiveSandboxFlags() const;
 
   // Modifying itself is done based on |fetch_client_settings_object|.
@@ -298,8 +293,6 @@ class CORE_EXPORT FrameLoader final {
   };
   std::unique_ptr<ClientNavigationState> client_navigation_;
 
-  network::mojom::blink::WebSandboxFlags forced_sandbox_flags_;
-
   // The state is set to kInitialized when Init() completes, and kDetached
   // during teardown in Detach().
   enum class State { kUninitialized, kInitialized, kDetached };