Part 3.1: Is policy list subsumed under subsuming policy?

third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/frame/csp/CSPDirectiveList.h"
 
 #include "bindings/core/v8/SourceLocation.h"
 #include "core/dom/Document.h"
 #include "core/dom/SecurityContext.h"
 #include "core/dom/SpaceSplitString.h"
@@ skipping 1146 matching lines @@
              ContentSecurityPolicy::DirectiveType::TreatAsPublicAddress) {
     treatAsPublicAddress(name, value);
   } else if (type == ContentSecurityPolicy::DirectiveType::RequireSRIFor &&
              m_policy->experimentalFeaturesEnabled()) {
     parseRequireSRIFor(name, value);
   } else {
     m_policy->reportUnsupportedDirective(name);
   }
 }
 
+SourceListDirective* CSPDirectiveList::operativeDirective(

[amalika, 2016/11/23 13:57:11]

Not sure if we should add a case for `DefaultSrc` here or just return nullptr
since we don't use `default-src` for subsumption?

I probably should have added it since it might be expected based on the name
that actual `default-src` is returned and we whitelist directives in `subsumes`
method anyways?

[Mike West, 2016/11/24 13:07:46]

`default-src`'s operative directive is `default-src`. I agree that you're not
going to use it for subsumption, but someone's going to use it for something
someday. :)

[amalika, 2016/11/24 14:32:29]

Updated!

+    const ContentSecurityPolicy::DirectiveType& type) {
+  switch (type) {
+    // Directives that do not have a default directive.
+    case ContentSecurityPolicy::DirectiveType::BaseURI:
+      return m_baseURI.get();
+    case ContentSecurityPolicy::DirectiveType::DefaultSrc:
+      return m_defaultSrc.get();
+    case ContentSecurityPolicy::DirectiveType::FrameAncestors:
+      return m_frameAncestors.get();
+    case ContentSecurityPolicy::DirectiveType::FormAction:
+      return m_formAction.get();
+    // Directives that have one default directive.
+    case ContentSecurityPolicy::DirectiveType::ChildSrc:
+      return operativeDirective(m_childSrc.get());
+    case ContentSecurityPolicy::DirectiveType::ConnectSrc:
+      return operativeDirective(m_connectSrc.get());
+    case ContentSecurityPolicy::DirectiveType::FontSrc:
+      return operativeDirective(m_fontSrc.get());
+    case ContentSecurityPolicy::DirectiveType::ImgSrc:
+      return operativeDirective(m_imgSrc.get());
+    case ContentSecurityPolicy::DirectiveType::ManifestSrc:
+      return operativeDirective(m_manifestSrc.get());
+    case ContentSecurityPolicy::DirectiveType::MediaSrc:
+      return operativeDirective(m_mediaSrc.get());
+    case ContentSecurityPolicy::DirectiveType::ObjectSrc:
+      return operativeDirective(m_objectSrc.get());
+    case ContentSecurityPolicy::DirectiveType::ScriptSrc:
+      return operativeDirective(m_scriptSrc.get());
+    case ContentSecurityPolicy::DirectiveType::StyleSrc:
+      return operativeDirective(m_styleSrc.get());
+    case ContentSecurityPolicy::DirectiveType::WorkerSrc:

[Mike West, 2016/11/24 13:07:46]

1. `worker-src` is currently defined as sitting on top of `child-src`, just like
`frame-src`.

2. We're probably going to change it to sit on top of `script-src`. Would you
mind adding a `TODO(mkwst): Reevaluate this` comment?

[amalika, 2016/11/24 14:32:29]

Addressed.

+      return operativeDirective(m_workerSrc.get());
+    // frame-src defaults to child-src, which defaults to default-src.
+    case ContentSecurityPolicy::DirectiveType::FrameSrc:
+      return operativeDirective(m_frameSrc,
+                                operativeDirective(m_childSrc.get()));
+    default:
+      return nullptr;
+  }
+}
+
+SourceListDirectiveVector CSPDirectiveList::getSourceVector(
+    const ContentSecurityPolicy::DirectiveType& type,
+    CSPDirectiveListVector policies) {

[Mike West, 2016/11/24 13:07:46]

`const CSPDirectiveListVector&`?

+  SourceListDirectiveVector sourceListDirectives;
+  for (const auto& policy : policies) {
+    if (SourceListDirective* directive = policy->operativeDirective(type))
+      sourceListDirectives.append(directive);
+  }
+
+  return sourceListDirectives;
+}
+
+bool CSPDirectiveList::subsumes(CSPDirectiveListVector other) {

[Mike West, 2016/11/24 13:07:46]

`const CSPDirectiveListVector&`?

+  ContentSecurityPolicy::DirectiveType directives[] = {
+      // Fetch Directives
+      ContentSecurityPolicy::DirectiveType::ChildSrc,
+      ContentSecurityPolicy::DirectiveType::ConnectSrc,
+      ContentSecurityPolicy::DirectiveType::FontSrc,
+      ContentSecurityPolicy::DirectiveType::FrameSrc,
+      ContentSecurityPolicy::DirectiveType::ImgSrc,
+      ContentSecurityPolicy::DirectiveType::ManifestSrc,
+      ContentSecurityPolicy::DirectiveType::MediaSrc,
+      ContentSecurityPolicy::DirectiveType::ObjectSrc,
+      ContentSecurityPolicy::DirectiveType::ScriptSrc,
+      ContentSecurityPolicy::DirectiveType::StyleSrc,
+      ContentSecurityPolicy::DirectiveType::WorkerSrc,
+      // Document Directives
+      ContentSecurityPolicy::DirectiveType::BaseURI,
+      // Navigation Directives

[Mike West, 2016/11/24 13:07:46]

I don't think these comments add much. You're not doing anything differently for
the different groups, so you might as well lump them together with a comment
about "These are the source-list directives." or something, linking to
https://w3c.github.io/webappsec-csp/#framework-directive-source-list.

[amalika, 2016/11/24 14:32:29]

Changed!

+      ContentSecurityPolicy::DirectiveType::FrameAncestors,
+      ContentSecurityPolicy::DirectiveType::FormAction};
+
+  for (const auto& directive : directives) {
+    // There should only be one SourceListDirective for each directive in
+    // Embedding-CSP.
+    SourceListDirectiveVector requiredList =
+        getSourceVector(directive, CSPDirectiveListVector(1, this));
+    if (requiredList.size() == 0)
+      continue;
+    SourceListDirective* required = requiredList[0];
+    // Aggregate all serialized source lists of the returned CSP into a vector
+    // based on a directive type, defaulting accordingly (for example, to
+    // `default-src`).
+    SourceListDirectiveVector returned = getSourceVector(directive, other);
+    // TODO(amalika): Add checks for plugin-types, sandbox, disown-opener,
+    // navigation-to, worker-src.
+    if (!required->subsumes(returned))
+      return false;
+  }
+
+  return true;
+}
+
 DEFINE_TRACE(CSPDirectiveList) {
   visitor->trace(m_policy);
   visitor->trace(m_pluginTypes);
   visitor->trace(m_baseURI);
   visitor->trace(m_childSrc);
   visitor->trace(m_connectSrc);
   visitor->trace(m_defaultSrc);
   visitor->trace(m_fontSrc);
   visitor->trace(m_formAction);
   visitor->trace(m_frameAncestors);
   visitor->trace(m_frameSrc);
   visitor->trace(m_imgSrc);
   visitor->trace(m_mediaSrc);
   visitor->trace(m_manifestSrc);
   visitor->trace(m_objectSrc);
   visitor->trace(m_scriptSrc);
   visitor->trace(m_styleSrc);
   visitor->trace(m_workerSrc);
 }
 
 }  // namespace blink