Skip to content

Commit

Permalink
Implement net/ support for Android's NetworkSecurityPolicy
Browse files Browse the repository at this point in the history
NetworkSecurityPolicy allows apps to disallow cleartext requests,
globally (starting from M) or by hostname (starting from N). This CL
adds support for this feature in URLRequestHttpJob, and a flag on the
URLRequestContext to enable it. It's off by default and only prevents
requests using the "http" and "ws" schemes. Uses will be added in future
CLs.

BUG=474197, 473848

Review-Url: https://codereview.chromium.org/2546213003
Cr-Commit-Position: refs/heads/master@{#438661}
  • Loading branch information
mgersh authored and Commit bot committed Dec 14, 2016
1 parent 8573cb8 commit d21d6d1
Show file tree
Hide file tree
Showing 11 changed files with 159 additions and 26 deletions.
1 change: 1 addition & 0 deletions net/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1104,6 +1104,7 @@ if (is_android) {
generate_jni("net_test_jni_headers") {
sources = [
"android/javatests/src/org/chromium/net/AndroidKeyStoreTestUtil.java",
"android/javatests/src/org/chromium/net/AndroidNetworkLibraryTestUtil.java",
"test/android/javatests/src/org/chromium/net/test/DummySpnegoAuthenticator.java",
"test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java",
]
Expand Down
1 change: 1 addition & 0 deletions net/android/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ android_library("net_javatests") {
testonly = true
java_files = [
"javatests/src/org/chromium/net/AndroidKeyStoreTestUtil.java",
"javatests/src/org/chromium/net/AndroidNetworkLibraryTestUtil.java",
"javatests/src/org/chromium/net/AndroidProxySelectorTest.java",
"javatests/src/org/chromium/net/NetErrorsTest.java",
"javatests/src/org/chromium/net/NetworkChangeNotifierTest.java",
Expand Down
18 changes: 18 additions & 0 deletions net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import android.net.wifi.WifiManager;
import android.os.Build;
import android.security.KeyChain;
import android.security.NetworkSecurityPolicy;
import android.telephony.TelephonyManager;
import android.util.Log;

Expand Down Expand Up @@ -243,4 +244,21 @@ public static String getWifiSSID(Context context) {
}
return "";
}

/**
* Returns true if cleartext traffic to |host| is allowed by the current app. Always true on L
* and older.
*/
@TargetApi(Build.VERSION_CODES.N)
@CalledByNative
private static boolean isCleartextPermitted(String host) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
NetworkSecurityPolicy policy = NetworkSecurityPolicy.getInstance();
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
return policy.isCleartextTrafficPermitted(host);
}
return policy.isCleartextTrafficPermitted();
}
return true;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright 2016 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.

package org.chromium.net;

import android.annotation.TargetApi;
import android.os.Build;
import android.security.NetworkSecurityPolicy;

import org.chromium.base.annotations.CalledByNative;

import java.lang.reflect.Method;

/**
* Utility functions for testing features implemented in AndroidNetworkLibrary.
*/
public class AndroidNetworkLibraryTestUtil {
/**
* Helper for tests that simulates an app disallowing cleartext traffic entirely on M and newer.
*/
@TargetApi(Build.VERSION_CODES.M)
@CalledByNative
private static void setUpSecurityPolicyForTesting(boolean cleartextPermitted) throws Exception {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
Method setCleartextTrafficPermitted = NetworkSecurityPolicy.class.getDeclaredMethod(
"setCleartextTrafficPermitted", boolean.class);
setCleartextTrafficPermitted.invoke(
NetworkSecurityPolicy.getInstance(), cleartextPermitted);
}
}
}
6 changes: 6 additions & 0 deletions net/android/network_library.cc
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ bool StoreKeyPair(const uint8_t* public_key,
return ret;
}

bool IsCleartextPermitted(const std::string& host) {
JNIEnv* env = AttachCurrentThread();
ScopedJavaLocalRef<jstring> host_string = ConvertUTF8ToJavaString(env, host);
return Java_AndroidNetworkLibrary_isCleartextPermitted(env, host_string);
}

bool HaveOnlyLoopbackAddresses() {
JNIEnv* env = AttachCurrentThread();
return Java_AndroidNetworkLibrary_haveOnlyLoopbackAddresses(env);
Expand Down
4 changes: 4 additions & 0 deletions net/android/network_library.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ bool StoreKeyPair(const uint8_t* public_key,
const uint8_t* private_key,
size_t private_len);

// Returns true if cleartext traffic to |host| is allowed by the app. Always
// true on L and older.
bool IsCleartextPermitted(const std::string& host);

// Returns true if it can determine that only loopback addresses are configured.
// i.e. if only 127.0.0.1 and ::1 are routable.
// Also returns false if it cannot determine this.
Expand Down
4 changes: 4 additions & 0 deletions net/base/net_error_list.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ NET_ERROR(BLOCKED_BY_RESPONSE, -27)
// heuristics that point to the possiblility of a cross-site scripting attack.
NET_ERROR(BLOCKED_BY_XSS_AUDITOR, -28)

// The request was blocked by system policy disallowing some or all cleartext
// requests. Used for NetworkSecurityPolicy on Android.
NET_ERROR(CLEARTEXT_NOT_PERMITTED, -29)

// A connection was closed (corresponding to a TCP FIN).
NET_ERROR(CONNECTION_CLOSED, -100)

Expand Down
4 changes: 3 additions & 1 deletion net/url_request/url_request_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ URLRequestContext::URLRequestContext()
sdch_manager_(nullptr),
network_quality_estimator_(nullptr),
url_requests_(new std::set<const URLRequest*>),
enable_brotli_(false) {
enable_brotli_(false),
check_cleartext_permitted_(false) {
base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
this, "URLRequestContext", base::ThreadTaskRunnerHandle::Get());
}
Expand Down Expand Up @@ -76,6 +77,7 @@ void URLRequestContext::CopyFrom(const URLRequestContext* other) {
set_http_user_agent_settings(other->http_user_agent_settings_);
set_network_quality_estimator(other->network_quality_estimator_);
set_enable_brotli(other->enable_brotli_);
set_check_cleartext_permitted(other->check_cleartext_permitted_);
}

const HttpNetworkSession::Params* URLRequestContext::GetNetworkSessionParams(
Expand Down
12 changes: 12 additions & 0 deletions net/url_request/url_request_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,15 @@ class NET_EXPORT URLRequestContext

bool enable_brotli() const { return enable_brotli_; }

// Sets the |check_cleartext_permitted| flag, which controls whether to check
// system policy before allowing a cleartext http or ws request.
void set_check_cleartext_permitted(bool check_cleartext_permitted) {
check_cleartext_permitted_ = check_cleartext_permitted;
}

// Returns current value of the |check_cleartext_permitted| flag.
bool check_cleartext_permitted() const { return check_cleartext_permitted_; }

// Sets a name for this URLRequestContext. Currently the name is used in
// MemoryDumpProvier to annotate memory usage. The name does not need to be
// unique.
Expand Down Expand Up @@ -285,6 +294,9 @@ class NET_EXPORT URLRequestContext

// Enables Brotli Content-Encoding support.
bool enable_brotli_;
// Enables checking system policy before allowing a cleartext http or ws
// request. Only used on Android.
bool check_cleartext_permitted_;

// An optional name which can be set to describe this URLRequestContext.
// Used in MemoryDumpProvier to annotate memory usage. The name does not need
Expand Down
57 changes: 32 additions & 25 deletions net/url_request/url_request_http_job.cc
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@
#include "net/websockets/websocket_handshake_stream_base.h"
#include "url/origin.h"

#if defined(OS_ANDROID)
#include "net/android/network_library.h"
#endif

static const char kAvailDictionaryHeader[] = "Avail-Dictionary";

namespace {
Expand Down Expand Up @@ -169,27 +173,6 @@ void LogChannelIDAndCookieStores(const GURL& url,
EPHEMERALITY_MAX);
}

net::URLRequestRedirectJob* MaybeInternallyRedirect(
net::URLRequest* request,
net::NetworkDelegate* network_delegate) {
const GURL& url = request->url();
if (url.SchemeIsCryptographic())
return nullptr;

net::TransportSecurityState* hsts =
request->context()->transport_security_state();
if (!hsts || !hsts->ShouldUpgradeToSSL(url.host()))
return nullptr;

GURL::Replacements replacements;
replacements.SetSchemeStr(url.SchemeIs(url::kHttpScheme) ? url::kHttpsScheme
: url::kWssScheme);
return new net::URLRequestRedirectJob(
request, network_delegate, url.ReplaceComponents(replacements),
// Use status code 307 to preserve the method, so POST requests work.
net::URLRequestRedirectJob::REDIRECT_307_TEMPORARY_REDIRECT, "HSTS");
}

} // namespace

namespace net {
Expand All @@ -208,10 +191,34 @@ URLRequestJob* URLRequestHttpJob::Factory(URLRequest* request,
request, network_delegate, ERR_INVALID_ARGUMENT);
}

URLRequestRedirectJob* redirect =
MaybeInternallyRedirect(request, network_delegate);
if (redirect)
return redirect;
const GURL& url = request->url();

// Check for reasons not to return a URLRequestHttpJob. These don't apply to
// https and wss requests.
if (!url.SchemeIsCryptographic()) {
// Check for HSTS upgrade.
TransportSecurityState* hsts =
request->context()->transport_security_state();
if (hsts && hsts->ShouldUpgradeToSSL(url.host())) {
GURL::Replacements replacements;
replacements.SetSchemeStr(
url.SchemeIs(url::kHttpScheme) ? url::kHttpsScheme : url::kWssScheme);
return new URLRequestRedirectJob(
request, network_delegate, url.ReplaceComponents(replacements),
// Use status code 307 to preserve the method, so POST requests work.
URLRequestRedirectJob::REDIRECT_307_TEMPORARY_REDIRECT, "HSTS");
}

#if defined(OS_ANDROID)
// Check whether the app allows cleartext traffic to this host, and return
// ERR_BLOCKED_BY_CLIENT if not.
if (request->context()->check_cleartext_permitted() &&
!android::IsCleartextPermitted(url.host())) {
return new URLRequestErrorJob(request, network_delegate,
ERR_CLEARTEXT_NOT_PERMITTED);
}
#endif
}

return new URLRequestHttpJob(request,
network_delegate,
Expand Down
46 changes: 46 additions & 0 deletions net/url_request/url_request_http_job_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@
#include "url/gurl.h"
#include "url/url_constants.h"

#if defined(OS_ANDROID)
#include "base/android/build_info.h"
#include "base/android/jni_android.h"
#include "jni/AndroidNetworkLibraryTestUtil_jni.h"
#endif

using net::test::IsError;
using net::test::IsOk;

Expand Down Expand Up @@ -938,6 +944,46 @@ TEST_F(URLRequestHttpJobWithBrotliSupportTest, BrotliAdvertisement) {
request->GetTotalReceivedBytes());
}

#if defined(OS_ANDROID)
TEST_F(URLRequestHttpJobTest, AndroidCleartextPermittedTest) {
context_.set_check_cleartext_permitted(true);

struct TestCase {
const char* url;
bool cleartext_permitted;
bool should_block;
} cases[] = {
{"http://blocked.test/", true, false},
{"https://blocked.test/", true, false},
{"http://blocked.test/", false, true},
{"https://blocked.test/", false, false},
};

for (const TestCase& test : cases) {
JNIEnv* env = base::android::AttachCurrentThread();
Java_AndroidNetworkLibraryTestUtil_setUpSecurityPolicyForTesting(
env, test.cleartext_permitted);

TestDelegate delegate;
std::unique_ptr<URLRequest> request =
context_.CreateRequest(GURL(test.url), DEFAULT_PRIORITY, &delegate);
request->Start();
base::RunLoop().Run();

int sdk_int = base::android::BuildInfo::GetInstance()->sdk_int();
bool expect_blocked = (sdk_int >= base::android::SDK_VERSION_MARSHMALLOW &&
test.should_block);
if (expect_blocked) {
EXPECT_THAT(delegate.request_status(),
IsError(ERR_CLEARTEXT_NOT_PERMITTED));
} else {
// Should fail since there's no test server running
EXPECT_THAT(delegate.request_status(), IsError(ERR_FAILED));
}
}
}
#endif

// This base class just serves to set up some things before the TestURLRequest
// constructor is called.
class URLRequestHttpJobWebSocketTestBase : public ::testing::Test {
Expand Down

0 comments on commit d21d6d1

Please sign in to comment.