Avoid unsafe static_cast<network::mojom::ReferrerPolicy>(i) casts.

Casting a raw integer (e.g. an integer deserialized from session restore
files, from a protobug, or coming from the other side of JNI boundary)
to an enum class value may potentially cause the following problems:

- Invalid enum values may cause Chromium code to misbehave.

- It is not entirely clear if C++ spec allows such casts (i.e. if such
  casts may lead to undefined behavior).

Because of the above, this CL replaces:
  static_cast<network::mojom::ReferrerPolicy>(raw_integer)
casts with calls to a new content::Referrer::ConvertToPolicy static
helper method, which tries to ensure that the cast is done in a safe
way.

Bug: 1006409
Change-Id: I2a660c4caf961ca52503b084a7c0abe93e9d5eb2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1816134Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706044}

android_webview/browser/state_serializer.cc

@@ -4,7 +4,9 @@
 
 #include "android_webview/browser/state_serializer.h"
 
+#include <memory>
 #include <string>
+#include <vector>
 
 #include "base/pickle.h"
 #include "base/time/time.h"
@@ -14,6 +16,7 @@
 #include "content/public/browser/restore_type.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/page_state.h"
+#include "content/public/common/referrer.h"
 
 // Reasons for not re-using TabNavigation under chrome/ as of 20121116:
 // * Android WebView has different requirements for fields to store since
@@ -226,8 +229,7 @@ bool RestoreNavigationEntryFromPickle(uint32_t state_version,
       return false;
 
     deserialized_referrer.url = GURL(referrer_url);
-    deserialized_referrer.policy =
-        static_cast<network::mojom::ReferrerPolicy>(policy);
+    deserialized_referrer.policy = content::Referrer::ConvertToPolicy(policy);
   }
 
   {

chrome/browser/android/customtabs/detached_resource_request_android.cc

@@ -51,7 +51,7 @@ static void JNI_CustomTabsConnection_CreateAndStartDetachedResourceRequest(
   // Java only knows about the blink referrer policy.
   net::URLRequest::ReferrerPolicy url_request_referrer_policy =
       content::Referrer::ReferrerPolicyForUrlRequest(
-          static_cast<network::mojom::ReferrerPolicy>(referrer_policy));
+          content::Referrer::ConvertToPolicy(referrer_policy));
   DetachedResourceRequest::Motivation request_motivation =
       static_cast<DetachedResourceRequest::Motivation>(motivation);
 

chrome/browser/android/tab_android.cc

@@ -6,6 +6,10 @@
 
 #include <stddef.h>
 
+#include <string>
+#include <utility>
+#include <vector>
+
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/bind.h"
@@ -50,6 +54,7 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_user_data.h"
+#include "content/public/common/referrer.h"
 #include "content/public/common/resource_request_body_android.h"
 #include "services/service_manager/public/cpp/interface_provider.h"
 #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
@@ -383,7 +388,7 @@ TabAndroid::TabLoadStatus TabAndroid::LoadUrl(
     if (j_referrer_url) {
       load_params.referrer = content::Referrer(
           GURL(base::android::ConvertJavaStringToUTF8(env, j_referrer_url)),
-          static_cast<network::mojom::ReferrerPolicy>(referrer_policy));
+          content::Referrer::ConvertToPolicy(referrer_policy));
     }
     if (j_initiator_origin) {
       load_params.initiator_origin = url::Origin::Create(GURL(

chrome/browser/android/tab_state.cc

@@ -9,6 +9,8 @@
 
 #include <limits>
 #include <memory>
+#include <string>
+#include <utility>
 #include <vector>
 
 #include "base/android/jni_android.h"
@@ -30,6 +32,7 @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/restore_type.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/referrer.h"
 
 using base::android::ConvertUTF16ToJavaString;
 using base::android::ConvertUTF8ToJavaString;
@@ -133,7 +136,6 @@ void UpgradeNavigationFromV0ToV2(
       LOG(ERROR) << "Failed to read SerializedNavigationEntry from pickle "
                  << "(index=" << i << ", url=" << virtual_url_spec;
     }
-
   }
 
   for (int i = 0; i < entry_count; ++i) {
@@ -534,7 +536,7 @@ WebContentsState::CreateSingleNavigationStateAsByteBuffer(
   if (referrer_url) {
     referrer = content::Referrer(
         GURL(base::android::ConvertJavaStringToUTF8(env, referrer_url)),
-        static_cast<network::mojom::ReferrerPolicy>(referrer_policy));
+        content::Referrer::ConvertToPolicy(referrer_policy));
   }
   // TODO(nasko,tedchoc): https://crbug.com/980641: Don't use String to store
   // initiator origin, as it is a lossy format.

chrome/browser/sessions/chrome_serialized_navigation_driver.cc

@@ -49,9 +49,9 @@ ChromeSerializedNavigationDriver::GetInstance() {
 
 void ChromeSerializedNavigationDriver::Sanitize(
     sessions::SerializedNavigationEntry* navigation) const {
-  content::Referrer old_referrer(navigation->referrer_url(),
-                                 static_cast<network::mojom::ReferrerPolicy>(
-                                     navigation->referrer_policy()));
+  content::Referrer old_referrer(
+      navigation->referrer_url(),
+      content::Referrer::ConvertToPolicy(navigation->referrer_policy()));
   content::Referrer new_referrer = content::Referrer::SanitizeForRequest(
       navigation->virtual_url(), old_referrer);
 

components/sessions/content/content_serialized_navigation_builder.cc

@@ -130,9 +130,9 @@ ContentSerializedNavigationBuilder::ToNavigationEntry(
     // PageState set above + drop the SetReferrer call below.  This will
     // slightly change the legacy behavior, but will make PageState and
     // Referrer consistent.
-    content::Referrer referrer(navigation->referrer_url(),
-                               static_cast<network::mojom::ReferrerPolicy>(
-                                   navigation->referrer_policy()));
+    content::Referrer referrer(
+        navigation->referrer_url(),
+        content::Referrer::ConvertToPolicy(navigation->referrer_policy()));
     entry->SetReferrer(referrer);
   } else {
     // Note that PageState covers some of the values inside |navigation| (e.g.

content/browser/frame_host/navigation_controller_android.cc

@@ -16,6 +16,7 @@
 #include "content/public/android/content_jni_headers/NavigationControllerImpl_jni.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/ssl_host_state_delegate.h"
+#include "content/public/common/referrer.h"
 #include "content/public/common/resource_request_body_android.h"
 #include "net/base/data_url.h"
 #include "ui/gfx/android/java_bitmap.h"
@@ -282,9 +283,9 @@ void NavigationControllerAndroid::LoadUrl(
   }
 
   if (j_referrer_url) {
-    params.referrer = content::Referrer(
-        GURL(ConvertJavaStringToUTF8(env, j_referrer_url)),
-        static_cast<network::mojom::ReferrerPolicy>(referrer_policy));
+    params.referrer =
+        Referrer(GURL(ConvertJavaStringToUTF8(env, j_referrer_url)),
+                 Referrer::ConvertToPolicy(referrer_policy));
   }
 
   navigation_controller_->LoadURLWithParams(params);
@@ -336,7 +337,7 @@ void NavigationControllerAndroid::GetDirectedNavigationHistory(
 void NavigationControllerAndroid::ClearSslPreferences(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {
-  content::SSLHostStateDelegate* delegate =
+  SSLHostStateDelegate* delegate =
       navigation_controller_->GetBrowserContext()->GetSSLHostStateDelegate();
   if (delegate)
     delegate->Clear(base::Callback<bool(const std::string&)>());
@@ -381,8 +382,7 @@ NavigationControllerAndroid::GetEntryAtIndex(JNIEnv* env,
   if (index < 0 || index >= navigation_controller_->GetEntryCount())
     return base::android::ScopedJavaLocalRef<jobject>();
 
-  content::NavigationEntry* entry =
-      navigation_controller_->GetEntryAtIndex(index);
+  NavigationEntry* entry = navigation_controller_->GetEntryAtIndex(index);
   return JNI_NavigationControllerImpl_CreateJavaNavigationEntry(env, entry,
                                                                 index);
 }
@@ -390,7 +390,7 @@ NavigationControllerAndroid::GetEntryAtIndex(JNIEnv* env,
 base::android::ScopedJavaLocalRef<jobject>
 NavigationControllerAndroid::GetVisibleEntry(JNIEnv* env,
                                              const JavaParamRef<jobject>& obj) {
-  content::NavigationEntry* entry = navigation_controller_->GetVisibleEntry();
+  NavigationEntry* entry = navigation_controller_->GetVisibleEntry();
 
   if (!entry)
     return base::android::ScopedJavaLocalRef<jobject>();
@@ -402,7 +402,7 @@ NavigationControllerAndroid::GetVisibleEntry(JNIEnv* env,
 base::android::ScopedJavaLocalRef<jobject>
 NavigationControllerAndroid::GetPendingEntry(JNIEnv* env,
                                              const JavaParamRef<jobject>& obj) {
-  content::NavigationEntry* entry = navigation_controller_->GetPendingEntry();
+  NavigationEntry* entry = navigation_controller_->GetPendingEntry();
 
   if (!entry)
     return base::android::ScopedJavaLocalRef<jobject>();

content/common/fetch/fetch_api_request_proto.cc

@@ -5,6 +5,7 @@
 #include "content/common/fetch/fetch_api_request_proto.h"
 
 #include "content/common/fetch/fetch_api_request.pb.h"
+#include "content/public/common/referrer.h"
 
 namespace content {
 
@@ -54,10 +55,10 @@ blink::mojom::FetchAPIRequestPtr DeserializeFetchRequestFromString(
   request_ptr->method = request_proto.method();
   request_ptr->headers = {request_proto.headers().begin(),
                           request_proto.headers().end()};
-  request_ptr->referrer =
-      blink::mojom::Referrer::New(GURL(request_proto.referrer().url()),
-                                  static_cast<network::mojom::ReferrerPolicy>(
-                                      request_proto.referrer().policy()));
+  request_ptr->referrer = blink::mojom::Referrer::New(
+      GURL(request_proto.referrer().url()),
+
+      Referrer::ConvertToPolicy(request_proto.referrer().policy()));
   request_ptr->is_reload = request_proto.is_reload();
   request_ptr->credentials_mode = static_cast<network::mojom::CredentialsMode>(
       request_proto.credentials_mode());

content/common/page_state_serialization.cc

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <limits>
+#include <utility>
 
 #include "base/pickle.h"
 #include "base/strings/string_number_conversions.h"
@@ -14,6 +15,7 @@
 #include "build/build_config.h"
 #include "content/common/page_state.mojom.h"
 #include "content/common/unique_name_helper.h"
+#include "content/public/common/referrer.h"
 #include "ipc/ipc_message_utils.h"
 #include "mojo/public/cpp/base/string16_mojom_traits.h"
 #include "mojo/public/cpp/base/time_mojom_traits.h"
@@ -465,7 +467,7 @@ void ReadResourceRequestBody(
         std::string blob_uuid = ReadStdString(obj);
         AppendBlobToRequestBody(request_body, blob_uuid);
       } else {
-        ReadGURL(obj); // Skip the obsolete blob url value.
+        ReadGURL(obj);  // Skip the obsolete blob url value.
       }
     }
   }
@@ -545,14 +547,13 @@ void ReadFrameState(
   state->item_sequence_number = ReadInteger64(obj);
   state->document_sequence_number = ReadInteger64(obj);
   if (obj->version >= 21 && obj->version < 23)
-    ReadInteger64(obj); // Skip obsolete frame sequence number.
+    ReadInteger64(obj);  // Skip obsolete frame sequence number.
 
   if (obj->version >= 17 && obj->version < 19)
-    ReadInteger64(obj); // Skip obsolete target frame id number.
+    ReadInteger64(obj);  // Skip obsolete target frame id number.
 
   if (obj->version >= 18) {
-    state->referrer_policy =
-        static_cast<network::mojom::ReferrerPolicy>(ReadInteger(obj));
+    state->referrer_policy = Referrer::ConvertToPolicy(ReadInteger(obj));
   }
 
   if (obj->version >= 20 && state->did_save_scroll_or_scale_state) {

content/public/common/referrer.cc

@@ -5,9 +5,11 @@
 #include "content/public/common/referrer.h"
 
 #include <string>
+#include <type_traits>
 
 #include "base/numerics/safe_conversions.h"
 #include "content/public/common/content_features.h"
+#include "mojo/public/cpp/bindings/enum_utils.h"
 #include "net/base/features.h"
 #include "services/network/loader_util.h"
 #include "services/network/public/cpp/features.h"
@@ -191,4 +193,10 @@ net::URLRequest::ReferrerPolicy Referrer::GetDefaultReferrerPolicy() {
   return net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE;
 }
 
+// static
+network::mojom::ReferrerPolicy Referrer::ConvertToPolicy(int32_t policy) {
+  return mojo::ConvertIntToMojoEnum<network::mojom::ReferrerPolicy>(policy)
+      .value_or(network::mojom::ReferrerPolicy::kDefault);
+}
+
 }  // namespace content

content/public/common/referrer.h

@@ -54,6 +54,11 @@ struct CONTENT_EXPORT Referrer {
       net::URLRequest::ReferrerPolicy net_policy);
 
   static net::URLRequest::ReferrerPolicy GetDefaultReferrerPolicy();
+
+  // Validates |policy| to make sure it represents one of the valid
+  // net::mojom::ReferrerPolicy enum values and returns it.  The relatively safe
+  // |kNever| value is returned if |policy| is not a valid value.
+  static network::mojom::ReferrerPolicy ConvertToPolicy(int32_t policy);
 };
 
 }  // namespace content

mojo/public/cpp/bindings/BUILD.gn

@@ -41,6 +41,7 @@ component("bindings_base") {
     "deprecated_interface_types_forward.h",
     "disconnect_reason.h",
     "enum_traits.h",
+    "enum_utils.h",
     "equals_traits.h",
     "features.cc",
     "features.h",

mojo/public/cpp/bindings/enum_utils.h

+// Copyright 2019 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.
+
+#ifndef MOJO_PUBLIC_CPP_BINDINGS_ENUM_UTILS_H_
+#define MOJO_PUBLIC_CPP_BINDINGS_ENUM_UTILS_H_
+
+#include <type_traits>
+
+#include "base/numerics/safe_conversions.h"
+#include "base/optional.h"
+
+namespace mojo {
+
+// Converts |int_value| to |TMojoEnum|.  If |int_value| represents a known enum
+// value, then a corresponding |TMojoEnum| value will be returned.  Returns
+// |base::nullopt| otherwise.
+//
+// Using base::StrictNumeric as the parameter type prevents callers from
+// accidentally using an implicit narrowing conversion when calling this
+// function (e.g. calling it with an int64_t argument, when the enum's
+// underlying type is int32_t).
+template <typename TMojoEnum>
+base::Optional<TMojoEnum> ConvertIntToMojoEnum(
+    base::StrictNumeric<int32_t> int_value) {
+  // Today all mojo enums use |int32_t| as the underlying type, so the code
+  // can simply use |int32_t| rather than |std::underlying_type_t<TMojoEnum>|.
+  static_assert(std::is_same<int32_t, std::underlying_type_t<TMojoEnum>>::value,
+                "Assumming that all mojo enums use int32_t as the underlying "
+                "type");
+
+  // The static cast from int32_t to TMojoEnum should be safe from undefined
+  // behavior.  In particular, TMojoEnums have a fixed underlying type
+  // (int32_t), so only the first part of the following spec snippet applies:
+  // http://www.eel.is/c++draft/expr.static.cast#10:
+  //     [...] If the enumeration type has a fixed underlying type, the value is
+  //     first converted to that type by integral conversion, if necessary, and
+  //     then to the enumeration type. If the enumeration type does not have a
+  //     fixed underlying type, the value is unchanged if the original value is
+  //     within the range of the enumeration values ([dcl.enum]), and otherwise,
+  //     the behavior is undefined. [...]
+  //
+  // Also, note that using a list initializer to covert an integer to an enum
+  // value is explicitly called out as safe in C++17 - see
+  // http://www.eel.is/c++draft/dcl.init.list#3.8:
+  //     enum class Handle : std::uint32_t { Invalid = 0 };
+  //     Handle h { 42 }; // OK as of C++17
+  // We may want to switch to this syntax in the future (once C++17 is adopted).
+  TMojoEnum enum_value =
+      static_cast<TMojoEnum>(static_cast<int32_t>(int_value));
+
+  // Verify whether |int_value| was one of known enum values.
+  //
+  // IsKnownEnumValue comes from code generated from .mojom files and is present
+  // in somenamespace::mojom namespace (the same namespace as the namespace of
+  // TMojoEnum and |enum_value|) - we rely on ADL (argument-dependent lookup) to
+  // find the right overload below.
+  if (!IsKnownEnumValue(enum_value))
+    return base::nullopt;
+
+  return enum_value;
+}
+
+}  // namespace mojo
+
+#endif  // MOJO_PUBLIC_CPP_BINDINGS_ENUM_UTILS_H_

third_party/blink/renderer/bindings/core/v8/referrer_script_info.cc

@@ -4,6 +4,7 @@
 
 #include "third_party/blink/renderer/bindings/core/v8/referrer_script_info.h"
 
+#include "mojo/public/cpp/bindings/enum_utils.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"
 #include "v8/include/v8.h"
 
@@ -58,9 +59,12 @@ ReferrerScriptInfo ReferrerScriptInfo::FromV8HostDefinedOptions(
   v8::Local<v8::Primitive> referrer_policy_value =
       host_defined_options->Get(isolate, kReferrerPolicy);
   SECURITY_CHECK(referrer_policy_value->IsUint32());
+  int32_t referrer_policy_int32 = base::saturated_cast<int32_t>(
+      referrer_policy_value->IntegerValue(context).ToChecked());
   network::mojom::ReferrerPolicy referrer_policy =
-      static_cast<network::mojom::ReferrerPolicy>(
-          referrer_policy_value->IntegerValue(context).ToChecked());
+      mojo::ConvertIntToMojoEnum<network::mojom::ReferrerPolicy>(
+          referrer_policy_int32)
+          .value_or(network::mojom::ReferrerPolicy::kDefault);
 
   return ReferrerScriptInfo(base_url, credentials_mode, nonce, parser_state,
                             referrer_policy);