Skip to content
Permalink
Browse files

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/+/1816134
Reviewed-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}
  • Loading branch information
anforowicz authored and Commit Bot committed Oct 15, 2019
1 parent a1aacaa commit a192eb95c0aae837fad27339caac07354eb1a3fb
@@ -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);
}

{
@@ -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);

@@ -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(
@@ -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.
@@ -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);

@@ -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.
@@ -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,16 +382,15 @@ 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);
}

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>();
@@ -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());
@@ -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) {
@@ -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
@@ -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
@@ -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",
@@ -0,0 +1,66 @@
// 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_
@@ -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);

0 comments on commit a192eb9

Please sign in to comment.
You can’t perform that action at this time.