Skip to content

Commit

Permalink
android: attempting to respect Android's cleartext policies (#2318)
Browse files Browse the repository at this point in the history
Rejecting requests which don't have an https scheme if Android disallows cleartext

Risk Level: High
Testing: TODO (envoyproxy/envoy-mobile#2341)
Docs Changes: n/a
Release Notes: inline
Fixes #1572

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: JP Simard <jp@jpsim.com>
  • Loading branch information
alyssawilk authored and jpsim committed Nov 28, 2022
1 parent 97b5b03 commit ec3b0cc
Show file tree
Hide file tree
Showing 6 changed files with 80 additions and 0 deletions.
1 change: 1 addition & 0 deletions mobile/docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Breaking changes:

- api: replace the ``drainConnections()`` method with a broader ``resetConnectivityState()``. (:issue:`#2225 <2225>`).
- api: disallow setting 'host' header directly (:issue:`#2275 <2275>`)
- android: respect Android's NetworkSecurityPolicy isCleartextTrafficPermitted APIs.
- net: enable happy eyeballs by default (:issue:`#2272 <2272>`)
- iOS: remove support for installing via CocoaPods, which had not worked since 2020 (:issue:`#2215 <2215>`)
- iOS: enable usage of ``NWPathMonitor`` by default (:issue:`#2329 <2329>`)
Expand Down
1 change: 1 addition & 0 deletions mobile/library/common/http/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ envoy_cc_library(
"//library/common/extensions/filters/http/local_error:local_error_filter_lib",
"//library/common/extensions/filters/http/network_configuration:network_configuration_filter_lib",
"//library/common/http:header_utility_lib",
"//library/common/jni:android_jni_utility_lib",
"//library/common/network:configurator_lib",
"//library/common/network:synthetic_address_lib",
"//library/common/stream_info:extra_stream_info_lib",
Expand Down
11 changes: 11 additions & 0 deletions mobile/library/common/http/client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "library/common/data/utility.h"
#include "library/common/http/header_utility.h"
#include "library/common/http/headers.h"
#include "library/common/jni/android_jni_utility.h"
#include "library/common/network/configurator.h"
#include "library/common/stream_info/extra_stream_info.h"

Expand Down Expand Up @@ -479,6 +480,16 @@ void Client::sendHeaders(envoy_stream_t stream, envoy_headers headers, bool end_
if (direct_stream) {
ScopeTrackerScopeState scope(direct_stream.get(), scopeTracker());
RequestHeaderMapPtr internal_headers = Utility::toRequestHeaders(headers);

// This is largely a check for the android platform: is_cleartext_permitted
// is a no-op for other platforms.
if (internal_headers->getSchemeValue() != "https" &&
!is_cleartext_permitted(internal_headers->getHostValue())) {
direct_stream->request_decoder_->sendLocalReply(
Http::Code::BadRequest, "Cleartext is not permitted", nullptr, absl::nullopt, "");
return;
}

setDestinationCluster(*internal_headers);
// Set the x-forwarded-proto header to https because Envoy Mobile only has clusters with TLS
// enabled. This is done here because the ApiListener's synthetic connection would make the
Expand Down
22 changes: 22 additions & 0 deletions mobile/library/common/jni/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,25 @@ cc_library(
"//library/common/jni/import:jni_import_lib",
],
)

cc_library(
name = "android_jni_utility_lib",
srcs = [
"android_jni_utility.cc",
],
hdrs = [
"android_jni_utility.h",
],
deps = [
"@envoy//source/common/common:assert_lib",
"//library/common/types:c_types_lib",
] + select({
"@envoy//bazel:android": [
":ndk_jni_support",
":jni_utility_lib",
"//library/common/data:utility_lib",
"//library/common/jni/import:jni_import_lib",
],
"//conditions:default": [],
}),
)
34 changes: 34 additions & 0 deletions mobile/library/common/jni/android_jni_utility.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#include "library/common/jni/android_jni_utility.h"

#include <stdlib.h>
#include <string.h>

#include "source/common/common/assert.h"

#if defined(__ANDROID_API__)
#include "library/common/data/utility.h"
#include "library/common/jni/import/jni_import.h"
#include "library/common/jni/jni_support.h"
#include "library/common/jni/jni_version.h"
#include "library/common/jni/jni_utility.h"
#endif

// NOLINT(namespace-envoy)

bool is_cleartext_permitted(absl::string_view hostname) {
#if defined(__ANDROID_API__)
envoy_data host = Envoy::Data::Utility::copyToBridgeData(hostname);
JNIEnv* env = get_env();
jstring java_host = native_data_to_string(env, host);
jclass jcls_Boolean = env->FindClass("org/chromium/net/AndroidNetworkLibrary");
jmethodID jmid_isCleartextTrafficPermitted =
env->GetMethodID(jcls_Boolean, "isCleartextTrafficPermitted", "(Ljava/lang/String;)Z");
jboolean result = env->CallBooleanMethod(java_host, jmid_isCleartextTrafficPermitted);
env->DeleteLocalRef(java_host);
release_envoy_data(host);
return result == JNI_TRUE;
#else
UNREFERENCED_PARAMETER(hostname);
return true;
#endif
}
11 changes: 11 additions & 0 deletions mobile/library/common/jni/android_jni_utility.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#pragma once

#include "absl/strings/string_view.h"

// NOLINT(namespace-envoy)

/* For android, calls up through JNI to see if cleartext is permitted for this
* host.
* For other platforms simply returns true.
*/
bool is_cleartext_permitted(absl::string_view hostname);

0 comments on commit ec3b0cc

Please sign in to comment.