From 51cf7ca0d260f9ac7c4cb02b24d2e8f968d0ca83 Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Thu, 5 May 2022 18:05:23 +0100 Subject: [PATCH 01/12] Cronvoy: preparation to unittest certificate verification JNI Properly testing Android certificate verification logic requires calling into framework code. That cannot be done through Robolectric (framework APIs are faked). Hence, actual instrumentation tests are needed. Having said that, after some exploration on the state of Bazel/Android instrumentation tests, I've realized that the current state of things is non-optimal. As a stopgap solution (until the infrastructure improves), we've decided to test the JNI layer through unittests which fake X509Util. This should be a reasonable middle ground, as X509Util is mostly a compatibility layer on top of Android's API. This CL introduces FakeX509Util and adds allows tests to swap that in by adding a test-only API to AndroidNetworkLibrary. FakeX509Util is not particularly clever: from its perspective a certificate is just a string and for a verification to succeeds, all certificates in a chain must be root certificates (weird, but we don't really care about its logic). Signed-off-by: Stefano Duo --- .../chromium/net/AndroidNetworkLibrary.java | 78 ++++++++----------- .../java/org/chromium/net/FakeX509Util.java | 40 ++++++++++ 2 files changed, 71 insertions(+), 47 deletions(-) create mode 100644 library/java/org/chromium/net/FakeX509Util.java diff --git a/library/java/org/chromium/net/AndroidNetworkLibrary.java b/library/java/org/chromium/net/AndroidNetworkLibrary.java index 4fe6909dfd..28909665dc 100644 --- a/library/java/org/chromium/net/AndroidNetworkLibrary.java +++ b/library/java/org/chromium/net/AndroidNetworkLibrary.java @@ -1,47 +1,8 @@ package org.chromium.net; -import android.Manifest; -import android.content.Context; -import android.content.Intent; -import android.content.IntentFilter; -import android.content.pm.PackageManager; -import android.net.ConnectivityManager; -import android.net.LinkProperties; -import android.net.Network; -import android.net.NetworkCapabilities; -import android.net.NetworkInfo; -import android.net.TrafficStats; -import android.net.TransportInfo; -import android.net.wifi.WifiInfo; -import android.net.wifi.WifiManager; -import android.os.Build; -import android.os.Build.VERSION_CODES; -import android.os.ParcelFileDescriptor; -import android.os.Process; -import android.telephony.TelephonyManager; -import android.util.Log; - -import androidx.annotation.RequiresApi; -import androidx.annotation.VisibleForTesting; - -import java.io.FileDescriptor; -import java.io.IOException; -import java.io.InputStream; -import java.io.OutputStream; -import java.lang.reflect.InvocationTargetException; -import java.lang.reflect.Method; -import java.net.InetAddress; -import java.net.NetworkInterface; -import java.net.Socket; -import java.net.SocketAddress; -import java.net.SocketException; -import java.net.SocketImpl; -import java.net.URLConnection; import java.security.KeyStoreException; import java.security.NoSuchAlgorithmException; import java.security.cert.CertificateException; -import java.util.Enumeration; -import java.util.List; /** * This class implements net utilities required by the net component. @@ -49,19 +10,35 @@ public final class AndroidNetworkLibrary { private static final String TAG = "AndroidNetworkLibrary"; + private static boolean mUseFakeCertificateVerification; + + /** + * Wheter a fake should be used in place of X509Util. This allows to easily test the JNI + * calls interaction in robolectric tests. + * + * @param useFakeCertificateVerification Wheter FakeX509Util should be used or not. + */ + public static void + setFakeCertificateVerificationForTesting(boolean useFakeCertificateVerification) { + mUseFakeCertificateVerification = useFakeCertificateVerification; + } + /** * Validate the server's certificate chain is trusted. Note that the caller * must still verify the name matches that of the leaf certificate. + * This is called from native code. * * @param certChain The ASN.1 DER encoded bytes for certificates. * @param authType The key exchange algorithm name (e.g. RSA). * @param host The hostname of the server. * @return Android certificate verification result code. */ - // TODO(stefanoduo): Hook envoy-mobile JNI. - //@CalledByNative public static AndroidCertVerifyResult verifyServerCertificates(byte[][] certChain, String authType, String host) { + if (mUseFakeCertificateVerification) { + return FakeX509Util.verifyServerCertificates(certChain, authType, host); + } + try { return X509Util.verifyServerCertificates(certChain, authType, host); } catch (KeyStoreException e) { @@ -75,23 +52,30 @@ public static AndroidCertVerifyResult verifyServerCertificates(byte[][] certChai /** * Adds a test root certificate to the local trust store. + * This is called from native code. + * * @param rootCert DER encoded bytes of the certificate. */ - // TODO(stefanoduo): Hook envoy-mobile JNI. - //@CalledByNativeUnchecked public static void addTestRootCertificate(byte[] rootCert) throws CertificateException, KeyStoreException, NoSuchAlgorithmException { - X509Util.addTestRootCertificate(rootCert); + if (mUseFakeCertificateVerification) { + FakeX509Util.addTestRootCertificate(rootCert); + } else { + X509Util.addTestRootCertificate(rootCert); + } } /** * Removes all test root certificates added by |addTestRootCertificate| calls from the local * trust store. + * This is called from native code. */ - // TODO(stefanoduo): Hook envoy-mobile JNI. - //@CalledByNativeUnchecked public static void clearTestRootCertificates() throws NoSuchAlgorithmException, CertificateException, KeyStoreException { - X509Util.clearTestRootCertificates(); + if (mUseFakeCertificateVerification) { + FakeX509Util.clearTestRootCertificates(); + } else { + X509Util.clearTestRootCertificates(); + } } } diff --git a/library/java/org/chromium/net/FakeX509Util.java b/library/java/org/chromium/net/FakeX509Util.java new file mode 100644 index 0000000000..27a83a9ddb --- /dev/null +++ b/library/java/org/chromium/net/FakeX509Util.java @@ -0,0 +1,40 @@ +package org.chromium.net; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + +/** + * Fake utility functions for verifying X.509 certificates. + */ +public final class FakeX509Util { + private static final String TAG = "FakeX509Util"; + + private static final Set validFakeCerts = new HashSet(); + + public static void addTestRootCertificate(byte[] rootCertBytes) { + String fakeCertificate = new String(rootCertBytes); + validFakeCerts.add(fakeCertificate); + } + + public static void clearTestRootCertificates() { validFakeCerts.clear(); } + + public static AndroidCertVerifyResult verifyServerCertificates(byte[][] certChain, + String authType, String host) { + if (certChain == null || certChain.length == 0 || certChain[0] == null) { + throw new IllegalArgumentException( + "Expected non-null and non-empty certificate " + + "chain passed as |certChain|. |certChain|=" + Arrays.deepToString(certChain)); + } + + for (byte[] cert : certChain) { + String fakeCert = new String(cert); + if (!validFakeCerts.contains(fakeCert)) { + return new AndroidCertVerifyResult(CertVerifyStatusAndroid.NO_TRUSTED_ROOT); + } + } + + return new AndroidCertVerifyResult(CertVerifyStatusAndroid.OK); + } +} From 232f12c9c0612d0028dfcd81972205f87f787235 Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Thu, 5 May 2022 18:21:31 +0100 Subject: [PATCH 02/12] Add utility functions to translate between C++ and Java std constructs Add utility functions to translate between: - C++ strings and Java strings - C++ vector of bytes and Java array of bytes These utility functions will be used in the next commit to implement the JNI layer for certificate verifications (and its unit tests). Signed-off-by: Stefano Duo --- library/common/jni/jni_utility.cc | 60 +++++++++++++++++++++++++++++++ library/common/jni/jni_utility.h | 19 ++++++++++ 2 files changed, 79 insertions(+) diff --git a/library/common/jni/jni_utility.cc b/library/common/jni/jni_utility.cc index 1bb6c7e905..ed401a5ae7 100644 --- a/library/common/jni/jni_utility.cc +++ b/library/common/jni/jni_utility.cc @@ -3,6 +3,8 @@ #include #include +#include + #include "source/common/common/assert.h" #include "library/common/jni/jni_support.h" @@ -255,3 +257,61 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries) { envoy_map native_map = {length / 2, entry_array}; return native_map; } + +jstring ConvertUTF8ToJavaString(JNIEnv* env, const std::string& str) { + std::u16string utf16 = + std::wstring_convert, char16_t>{}.from_bytes(str); + const jchar* jutf16 = reinterpret_cast(utf16.data()); + return env->NewString(jutf16, utf16.length()); +} + +jobjectArray ToJavaArrayOfByteArray(JNIEnv* env, const std::vector& v) { + jclass jcls_byte_array = env->FindClass("[B"); + jobjectArray joa = env->NewObjectArray(v.size(), jcls_byte_array, nullptr); + + for (size_t i = 0; i < v.size(); ++i) { + jbyteArray byte_array = + ToJavaByteArray(env, reinterpret_cast(v[i].data()), v[i].length()); + env->SetObjectArrayElement(joa, i, byte_array); + } + return joa; +} + +jbyteArray ToJavaByteArray(JNIEnv* env, const uint8_t* bytes, size_t len) { + jbyteArray byte_array = env->NewByteArray(len); + + env->SetByteArrayRegion(byte_array, 0, len, reinterpret_cast(bytes)); + + return byte_array; +} + +void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, + std::vector* out) { + size_t len = env->GetArrayLength(array); + out->resize(len); + for (size_t i = 0; i < len; ++i) { + jbyteArray bytes_array = static_cast(env->GetObjectArrayElement(array, i)); + jsize bytes_len = env->GetArrayLength(bytes_array); + jbyte* bytes = env->GetByteArrayElements(bytes_array, nullptr); + (*out)[i].assign(reinterpret_cast(bytes), bytes_len); + env->ReleaseByteArrayElements(bytes_array, bytes, JNI_ABORT); + } +} + +void JavaArrayOfByteToBytesVector(JNIEnv* env, jbyteArray array, std::vector* out) { + const size_t len = env->GetArrayLength(array); + out->resize(len); + jbyte* jbytes = env->GetByteArrayElements(array, nullptr); + uint8_t* bytes = reinterpret_cast(jbytes); + std::copy(bytes, bytes + len, out->begin()); + env->ReleaseByteArrayElements(array, jbytes, JNI_ABORT); +} + +void ConvertJavaStringToUTF8(JNIEnv* env, jstring str, std::string* result) { + const jsize length = env->GetStringLength(str); + const jchar* chars = env->GetStringChars(str, NULL); + std::string utf8 = std::wstring_convert, char16_t>{}.to_bytes( + reinterpret_cast(chars)); + *result = utf8; + env->ReleaseStringChars(str, chars); +} diff --git a/library/common/jni/jni_utility.h b/library/common/jni/jni_utility.h index f75529a3a5..a9c237f045 100644 --- a/library/common/jni/jni_utility.h +++ b/library/common/jni/jni_utility.h @@ -1,5 +1,8 @@ #pragma once +#include +#include + #include "library/common/jni/import/jni_import.h" #include "library/common/types/c_types.h" @@ -63,3 +66,19 @@ envoy_headers* to_native_headers_ptr(JNIEnv* env, jobjectArray headers); envoy_stats_tags to_native_tags(JNIEnv* env, jobjectArray tags); envoy_map to_native_map(JNIEnv* env, jobjectArray entries); + +/** + * Utilities to translate C++ std library constructs to their Java counterpart. + */ +jstring ConvertUTF8ToJavaString(JNIEnv* env, const std::string& str); + +jobjectArray ToJavaArrayOfByteArray(JNIEnv* env, const std::vector& v); + +jbyteArray ToJavaByteArray(JNIEnv* env, const uint8_t* bytes, size_t len); + +void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, + std::vector* out); + +void JavaArrayOfByteToBytesVector(JNIEnv* env, jbyteArray array, std::vector* out); + +void ConvertJavaStringToUTF8(JNIEnv* env, jstring str, std::string* result); From d3ddc192e449b02e6b0ebeb906714922f5cc431f Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Thu, 5 May 2022 18:25:48 +0100 Subject: [PATCH 03/12] Implement JNI layer for Android's certificate verification Add native hooks for AndroidNetworkLibrary Java APIs. Write unittests that mimic calls from native to Java for these new hooks, by making use of a fake X509Util and additional test-only Java -> C++ JNI calls. Signed-off-by: Stefano Duo --- library/common/jni/jni_interface.cc | 119 ++++++++++++++++++ library/common/types/c_types.h | 24 ++++ .../envoymobile/engine/JniLibrary.java | 26 ++++ test/java/org/chromium/net/BUILD | 16 +++ .../net/CertificateVerificationTest.java | 72 +++++++++++ 5 files changed, 257 insertions(+) create mode 100644 test/java/org/chromium/net/CertificateVerificationTest.java diff --git a/library/common/jni/jni_interface.cc b/library/common/jni/jni_interface.cc index 9ebe1c0e70..03ecf1b26e 100644 --- a/library/common/jni/jni_interface.cc +++ b/library/common/jni/jni_interface.cc @@ -1,6 +1,7 @@ #include #include +#include #include "library/common/api/c_types.h" #include "library/common/extensions/filters/http/platform_bridge/c_types.h" @@ -1031,3 +1032,121 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_setPreferredNetwork(JNIEnv* env return set_preferred_network(static_cast(engine), static_cast(network)); } + +bool Java_AndroidCertVerifyResult_isIssuedByKnownRoot(JNIEnv* env, jobject result) { + jclass jcls_AndroidCertVerifyResult = env->FindClass("org/chromium/net/AndroidCertVerifyResult"); + jmethodID jmid_isIssuedByKnownRoot = + env->GetMethodID(jcls_AndroidCertVerifyResult, "isIssuedByKnownRoot", "()Z"); + return env->CallBooleanMethod(jcls_AndroidCertVerifyResult, jmid_isIssuedByKnownRoot, result); +} + +envoy_cert_verify_status_t Java_AndroidCertVerifyResult_getStatus(JNIEnv* env, jobject result) { + jclass jcls_AndroidCertVerifyResult = env->FindClass("org/chromium/net/AndroidCertVerifyResult"); + jmethodID jmid_getStatus = env->GetMethodID(jcls_AndroidCertVerifyResult, "getStatus", "()I"); + + return static_cast( + env->CallIntMethod(jcls_AndroidCertVerifyResult, jmid_getStatus, result)); +} + +jobjectArray Java_AndroidCertVerifyResult_getCertificateChainEncoded(JNIEnv* env, jobject result) { + jclass jcls_AndroidCertVerifyResult = env->FindClass("org/chromium/net/AndroidCertVerifyResult"); + jmethodID jmid_getCertificateChainEncoded = + env->GetMethodID(jcls_AndroidCertVerifyResult, "getCertificateChainEncoded", "()[[B"); + + return static_cast( + env->CallObjectMethod(jcls_AndroidCertVerifyResult, jmid_getCertificateChainEncoded, result)); +} + +void ExtractCertVerifyResult(JNIEnv* env, jobject result, envoy_cert_verify_status_t* status, + bool* is_issued_by_known_root, + std::vector* verified_chain) { + *status = Java_AndroidCertVerifyResult_getStatus(env, result); + + *is_issued_by_known_root = Java_AndroidCertVerifyResult_isIssuedByKnownRoot(env, result); + + jobjectArray chain_byte_array = + Java_AndroidCertVerifyResult_getCertificateChainEncoded(env, result); + JavaArrayOfByteArrayToStringVector(env, chain_byte_array, verified_chain); +} + +static jobject jvm_verify_x509_cert_chain(const std::vector& cert_chain, + std::string auth_type, std::string host) { + jni_log("[Envoy]", "jvm_verify_x509_cert_chain"); + JNIEnv* env = get_env(); + jclass jcls_AndroidNetworkLibrary = env->FindClass("org/chromium/net/AndroidNetworkLibrary"); + jmethodID jmid_verifyServerCertificates = env->GetStaticMethodID( + jcls_AndroidNetworkLibrary, "verifyServerCertificates", + "([[BLjava/lang/String;Ljava/lang/String;)Lorg/chromium/net/AndroidCertVerifyResult;"); + + jobjectArray chain_byte_array = ToJavaArrayOfByteArray(env, cert_chain); + jstring auth_string = ConvertUTF8ToJavaString(env, auth_type); + jstring host_string = ConvertUTF8ToJavaString(env, host); + + jobject result = + env->CallStaticObjectMethod(jcls_AndroidNetworkLibrary, jmid_verifyServerCertificates, + chain_byte_array, auth_string, host_string); + + env->DeleteLocalRef(chain_byte_array); + env->DeleteLocalRef(auth_string); + env->DeleteLocalRef(host_string); + return result; +} + +static void verify_x509_cert_chain(const std::vector& cert_chain, + std::string auth_type, std::string host, + envoy_cert_verify_status_t* status, + bool* is_issued_by_known_root, + std::vector* verified_chain) { + jobject result = jvm_verify_x509_cert_chain(cert_chain, auth_type, host); + ExtractCertVerifyResult(get_env(), result, status, is_issued_by_known_root, verified_chain); +} + +static void jvm_add_test_root_certificate(const uint8_t* cert, size_t len) { + jni_log("[Envoy]", "jvm_add_test_root_certificate"); + JNIEnv* env = get_env(); + jclass jcls_AndroidNetworkLibrary = env->FindClass("org/chromium/net/AndroidNetworkLibrary"); + jmethodID jmid_addTestRootCertificate = + env->GetStaticMethodID(jcls_AndroidNetworkLibrary, "addTestRootCertificate", "([B)V"); + + jbyteArray cert_array = ToJavaByteArray(env, cert, len); + env->CallStaticVoidMethod(jcls_AndroidNetworkLibrary, jmid_addTestRootCertificate, cert_array); + env->DeleteLocalRef(cert_array); +} + +static void jvm_clear_test_root_certificate() { + jni_log("[Envoy]", "jvm_clear_test_root_certificate"); + JNIEnv* env = get_env(); + jclass jcls_AndroidNetworkLibrary = env->FindClass("org/chromium/net/AndroidNetworkLibrary"); + jmethodID jmid_clearTestRootCertificates = + env->GetStaticMethodID(jcls_AndroidNetworkLibrary, "clearTestRootCertificates", "()V"); + + env->CallStaticVoidMethod(jcls_AndroidNetworkLibrary, jmid_clearTestRootCertificates); +} + +extern "C" JNIEXPORT jobject JNICALL +Java_io_envoyproxy_envoymobile_engine_JniLibrary_callCertificateVerificationFromNative( + JNIEnv* env, jclass, jobjectArray certChain, jstring authType, jstring jhost) { + std::vector cert_chain; + std::string auth_type; + std::string host; + + JavaArrayOfByteArrayToStringVector(env, certChain, &cert_chain); + ConvertJavaStringToUTF8(env, authType, &auth_type); + ConvertJavaStringToUTF8(env, jhost, &host); + + return jvm_verify_x509_cert_chain(cert_chain, auth_type, host); +} + +extern "C" JNIEXPORT void JNICALL +Java_io_envoyproxy_envoymobile_engine_JniLibrary_callAddTestRootCertificateFromNative( + JNIEnv* env, jclass, jbyteArray jcert) { + std::vector cert; + JavaArrayOfByteToBytesVector(env, jcert, &cert); + jvm_add_test_root_certificate(cert.data(), cert.size()); +} + +extern "C" JNIEXPORT void JNICALL +Java_io_envoyproxy_envoymobile_engine_JniLibrary_callClearTestRootCertificateFromNative(JNIEnv*, + jclass) { + jvm_clear_test_root_certificate(); +} diff --git a/library/common/types/c_types.h b/library/common/types/c_types.h index 22f1fc4dba..27f069adec 100644 --- a/library/common/types/c_types.h +++ b/library/common/types/c_types.h @@ -488,3 +488,27 @@ typedef struct { // Context passed through to callbacks to provide dispatch and execution state. const void* context; } envoy_event_tracker; + +/** + * The list of certificate verification results returned from Java side to the + * C++ side. + * A Java counterpart lives in org.chromium.net.CertVerifyStatusAndroid.java + */ +typedef enum { + // Certificate is trusted. + CERT_VERIFY_STATUS_ANDROID_OK = 0, + // Certificate verification could not be conducted. + CERT_VERIFY_STATUS_ANDROID_FAILED = -1, + // Certificate is not trusted due to non-trusted root of the certificate + // chain. + CERT_VERIFY_STATUS_ANDROID_NO_TRUSTED_ROOT = -2, + // Certificate is not trusted because it has expired. + CERT_VERIFY_STATUS_ANDROID_EXPIRED = -3, + // Certificate is not trusted because it is not valid yet. + CERT_VERIFY_STATUS_ANDROID_NOT_YET_VALID = -4, + // Certificate is not trusted because it could not be parsed. + CERT_VERIFY_STATUS_ANDROID_UNABLE_TO_PARSE = -5, + // Certificate is not trusted because it has an extendedKeyUsage field, but + // its value is not correct for a web server. + CERT_VERIFY_STATUS_ANDROID_INCORRECT_KEY_USAGE = -6, +} envoy_cert_verify_status_t; diff --git a/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java b/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java index 0729dd1c07..55edd24e74 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java +++ b/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java @@ -331,4 +331,30 @@ protected static native int registerStringAccessor(String accessorName, * @return The resulting status of the operation. */ protected static native int setPreferredNetwork(long engine, int network); + + /** + * Mimic a call to AndroidNetworkLibrary#verifyServerCertificates from native code. + * To be used for testing only. + * + * @param certChain The ASN.1 DER encoded bytes for certificates. + * @param authType The key exchange algorithm name (e.g. RSA). + * @param host The hostname of the server. + * @return Android certificate verification result code. + */ + public static native Object callCertificateVerificationFromNative(byte[][] certChain, + String authType, String host); + /** + * Mimic a call to AndroidNetworkLibrary#addTestRootCertificate from native code. + * To be used for testing only. + * + * @param rootCert DER encoded bytes of the certificate. + */ + public static native void callAddTestRootCertificateFromNative(byte[] cert); + + /** + * Mimic a call to AndroidNetworkLibrary#clearTestRootCertificate from native code. + * To be used for testing only. + * + */ + public static native void callClearTestRootCertificateFromNative(); } diff --git a/test/java/org/chromium/net/BUILD b/test/java/org/chromium/net/BUILD index 17f33ec87f..97fc92370c 100644 --- a/test/java/org/chromium/net/BUILD +++ b/test/java/org/chromium/net/BUILD @@ -36,6 +36,22 @@ envoy_mobile_android_test( ], ) +envoy_mobile_android_test( + name = "certificate_verification_tests", + srcs = [ + "CertificateVerificationTest.java", + ], + native_deps = [ + "//library/common/jni:libndk_envoy_jni.so", + "//library/common/jni:libndk_envoy_jni.jnilib", + ], + deps = [ + "//library/java/io/envoyproxy/envoymobile/engine:envoy_base_engine_lib", + "//library/java/io/envoyproxy/envoymobile/engine:envoy_engine_lib", + "//library/java/org/chromium/net", + ], +) + envoy_mobile_android_test( name = "cronet_url_request_context_test", srcs = [ diff --git a/test/java/org/chromium/net/CertificateVerificationTest.java b/test/java/org/chromium/net/CertificateVerificationTest.java new file mode 100644 index 0000000000..a65c1f87b8 --- /dev/null +++ b/test/java/org/chromium/net/CertificateVerificationTest.java @@ -0,0 +1,72 @@ +package org.chromium.net; + +import static org.junit.Assert.assertEquals; + +import io.envoyproxy.envoymobile.engine.AndroidJniLibrary; +import io.envoyproxy.envoymobile.engine.JniLibrary; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; + +/** + * Simple test for Certificate verification. + */ +@RunWith(RobolectricTestRunner.class) +public class CertificateVerificationTest { + static { + AndroidJniLibrary.loadTestLibrary(); + JniLibrary.load(); + } + + @Before + public void setUp() throws Exception { + AndroidNetworkLibrary.setFakeCertificateVerificationForTesting(true); + } + + @After + public void tearDown() throws Exception { + JniLibrary.callClearTestRootCertificateFromNative(); + AndroidNetworkLibrary.setFakeCertificateVerificationForTesting(true); + } + + @Test + public void testVerifyCertificateCall() throws Exception { + String[] fakeCertChain1 = new String[] {"fake cert"}; + byte[][] certChain1 = new byte[][] {fakeCertChain1[0].getBytes()}; + + String[] fakeCertChain2 = new String[] {"fake cert", "another fake cert"}; + byte[][] certChain2 = new byte[][] {fakeCertChain2[0].getBytes(), fakeCertChain2[1].getBytes()}; + + AndroidCertVerifyResult result = + (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain1, "", + ""); + assertEquals(result.getStatus(), CertVerifyStatusAndroid.NO_TRUSTED_ROOT); + result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain2, + "", ""); + assertEquals(result.getStatus(), CertVerifyStatusAndroid.NO_TRUSTED_ROOT); + + JniLibrary.callAddTestRootCertificateFromNative(certChain1[0]); + result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain1, + "", ""); + assertEquals(result.getStatus(), CertVerifyStatusAndroid.OK); + result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain2, + "", ""); + assertEquals(result.getStatus(), CertVerifyStatusAndroid.NO_TRUSTED_ROOT); + + JniLibrary.callAddTestRootCertificateFromNative(certChain2[1]); + result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain2, + "", ""); + assertEquals(result.getStatus(), CertVerifyStatusAndroid.OK); + + JniLibrary.callClearTestRootCertificateFromNative(); + result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain1, + "", ""); + assertEquals(result.getStatus(), CertVerifyStatusAndroid.NO_TRUSTED_ROOT); + result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain2, + "", ""); + assertEquals(result.getStatus(), CertVerifyStatusAndroid.NO_TRUSTED_ROOT); + } +} From fda54b81cb79a346fc650ec39eccf65f2170fa2f Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Fri, 6 May 2022 15:33:52 +0100 Subject: [PATCH 04/12] Address comments. Signed-off-by: Stefano Duo --- library/common/jni/jni_interface.cc | 36 +++--- library/common/jni/jni_utility.cc | 28 +++-- library/common/jni/jni_utility.h | 1 + library/common/types/c_types.h | 2 +- .../java/org/chromium/net/FakeX509Util.java | 14 ++- .../net/CertificateVerificationTest.java | 106 +++++++++++++----- 6 files changed, 129 insertions(+), 58 deletions(-) diff --git a/library/common/jni/jni_interface.cc b/library/common/jni/jni_interface.cc index 03ecf1b26e..e52b272f4c 100644 --- a/library/common/jni/jni_interface.cc +++ b/library/common/jni/jni_interface.cc @@ -1033,22 +1033,22 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_setPreferredNetwork(JNIEnv* env static_cast(network)); } -bool Java_AndroidCertVerifyResult_isIssuedByKnownRoot(JNIEnv* env, jobject result) { +bool jvm_cert_is_issued_by_known_root(JNIEnv* env, jobject result) { jclass jcls_AndroidCertVerifyResult = env->FindClass("org/chromium/net/AndroidCertVerifyResult"); jmethodID jmid_isIssuedByKnownRoot = env->GetMethodID(jcls_AndroidCertVerifyResult, "isIssuedByKnownRoot", "()Z"); return env->CallBooleanMethod(jcls_AndroidCertVerifyResult, jmid_isIssuedByKnownRoot, result); } -envoy_cert_verify_status_t Java_AndroidCertVerifyResult_getStatus(JNIEnv* env, jobject result) { +envoy_cert_verify_status_android_t jvm_cert_get_status(JNIEnv* env, jobject result) { jclass jcls_AndroidCertVerifyResult = env->FindClass("org/chromium/net/AndroidCertVerifyResult"); jmethodID jmid_getStatus = env->GetMethodID(jcls_AndroidCertVerifyResult, "getStatus", "()I"); - return static_cast( + return static_cast( env->CallIntMethod(jcls_AndroidCertVerifyResult, jmid_getStatus, result)); } -jobjectArray Java_AndroidCertVerifyResult_getCertificateChainEncoded(JNIEnv* env, jobject result) { +jobjectArray jvm_cert_get_certificate_chain_encoded(JNIEnv* env, jobject result) { jclass jcls_AndroidCertVerifyResult = env->FindClass("org/chromium/net/AndroidCertVerifyResult"); jmethodID jmid_getCertificateChainEncoded = env->GetMethodID(jcls_AndroidCertVerifyResult, "getCertificateChainEncoded", "()[[B"); @@ -1057,20 +1057,20 @@ jobjectArray Java_AndroidCertVerifyResult_getCertificateChainEncoded(JNIEnv* env env->CallObjectMethod(jcls_AndroidCertVerifyResult, jmid_getCertificateChainEncoded, result)); } -void ExtractCertVerifyResult(JNIEnv* env, jobject result, envoy_cert_verify_status_t* status, +void ExtractCertVerifyResult(JNIEnv* env, jobject result, + envoy_cert_verify_status_android_t* status, bool* is_issued_by_known_root, std::vector* verified_chain) { - *status = Java_AndroidCertVerifyResult_getStatus(env, result); + *status = jvm_cert_get_status(env, result); - *is_issued_by_known_root = Java_AndroidCertVerifyResult_isIssuedByKnownRoot(env, result); + *is_issued_by_known_root = jvm_cert_is_issued_by_known_root(env, result); - jobjectArray chain_byte_array = - Java_AndroidCertVerifyResult_getCertificateChainEncoded(env, result); + jobjectArray chain_byte_array = jvm_cert_get_certificate_chain_encoded(env, result); JavaArrayOfByteArrayToStringVector(env, chain_byte_array, verified_chain); } -static jobject jvm_verify_x509_cert_chain(const std::vector& cert_chain, - std::string auth_type, std::string host) { +static jobject call_jvm_verify_x509_cert_chain(const std::vector& cert_chain, + std::string auth_type, std::string host) { jni_log("[Envoy]", "jvm_verify_x509_cert_chain"); JNIEnv* env = get_env(); jclass jcls_AndroidNetworkLibrary = env->FindClass("org/chromium/net/AndroidNetworkLibrary"); @@ -1092,12 +1092,12 @@ static jobject jvm_verify_x509_cert_chain(const std::vector& cert_c return result; } -static void verify_x509_cert_chain(const std::vector& cert_chain, - std::string auth_type, std::string host, - envoy_cert_verify_status_t* status, - bool* is_issued_by_known_root, - std::vector* verified_chain) { - jobject result = jvm_verify_x509_cert_chain(cert_chain, auth_type, host); +static void jvm_verify_x509_cert_chain(const std::vector& cert_chain, + std::string auth_type, std::string host, + envoy_cert_verify_status_android_t* status, + bool* is_issued_by_known_root, + std::vector* verified_chain) { + jobject result = call_jvm_verify_x509_cert_chain(cert_chain, auth_type, host); ExtractCertVerifyResult(get_env(), result, status, is_issued_by_known_root, verified_chain); } @@ -1134,7 +1134,7 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_callCertificateVerificationFrom ConvertJavaStringToUTF8(env, authType, &auth_type); ConvertJavaStringToUTF8(env, jhost, &host); - return jvm_verify_x509_cert_chain(cert_chain, auth_type, host); + return call_jvm_verify_x509_cert_chain(cert_chain, auth_type, host); } extern "C" JNIEXPORT void JNICALL diff --git a/library/common/jni/jni_utility.cc b/library/common/jni/jni_utility.cc index ed401a5ae7..d9887cc29f 100644 --- a/library/common/jni/jni_utility.cc +++ b/library/common/jni/jni_utility.cc @@ -279,9 +279,8 @@ jobjectArray ToJavaArrayOfByteArray(JNIEnv* env, const std::vector& jbyteArray ToJavaByteArray(JNIEnv* env, const uint8_t* bytes, size_t len) { jbyteArray byte_array = env->NewByteArray(len); - - env->SetByteArrayRegion(byte_array, 0, len, reinterpret_cast(bytes)); - + const jbyte* jbytes = reinterpret_cast(bytes); + env->SetByteArrayRegion(byte_array, /*start=*/0, len, jbytes); return byte_array; } @@ -289,29 +288,40 @@ void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, std::vector* out) { size_t len = env->GetArrayLength(array); out->resize(len); + for (size_t i = 0; i < len; ++i) { jbyteArray bytes_array = static_cast(env->GetObjectArrayElement(array, i)); jsize bytes_len = env->GetArrayLength(bytes_array); - jbyte* bytes = env->GetByteArrayElements(bytes_array, nullptr); + // We don't care whether the returned array is a copy or not as we're simply + // copying it into C++ owned memory. + jbyte* bytes = env->GetByteArrayElements(bytes_array, /*isCopy=*/nullptr); (*out)[i].assign(reinterpret_cast(bytes), bytes_len); + // There is nothing to write back, it is always safe to JNI_ABORT. env->ReleaseByteArrayElements(bytes_array, bytes, JNI_ABORT); + // Explicitly delete to keep the local ref count low. + env->DeleteLocalRef(bytes_array); } } void JavaArrayOfByteToBytesVector(JNIEnv* env, jbyteArray array, std::vector* out) { const size_t len = env->GetArrayLength(array); out->resize(len); - jbyte* jbytes = env->GetByteArrayElements(array, nullptr); + + // We don't care whether the returned array is a copy or not as we're simply + // copying it into C++ owned memory. + jbyte* jbytes = env->GetByteArrayElements(array, /*isCopy=*/nullptr); uint8_t* bytes = reinterpret_cast(jbytes); std::copy(bytes, bytes + len, out->begin()); + // There is nothing to write back, it is always safe to JNI_ABORT. env->ReleaseByteArrayElements(array, jbytes, JNI_ABORT); } void ConvertJavaStringToUTF8(JNIEnv* env, jstring str, std::string* result) { - const jsize length = env->GetStringLength(str); - const jchar* chars = env->GetStringChars(str, NULL); + // We don't care whether the returned string is a copy or not as we're simply + // copying it into C++ owned memory. + const jchar* utf16 = env->GetStringChars(str, /*isCopy=*/nullptr); std::string utf8 = std::wstring_convert, char16_t>{}.to_bytes( - reinterpret_cast(chars)); + reinterpret_cast(utf16)); *result = utf8; - env->ReleaseStringChars(str, chars); + env->ReleaseStringChars(str, utf16); } diff --git a/library/common/jni/jni_utility.h b/library/common/jni/jni_utility.h index a9c237f045..0895dbe6b8 100644 --- a/library/common/jni/jni_utility.h +++ b/library/common/jni/jni_utility.h @@ -69,6 +69,7 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries); /** * Utilities to translate C++ std library constructs to their Java counterpart. + * The underlying data is always copied to disentangle C++ and Java objects lifetime. */ jstring ConvertUTF8ToJavaString(JNIEnv* env, const std::string& str); diff --git a/library/common/types/c_types.h b/library/common/types/c_types.h index 27f069adec..114e655ca3 100644 --- a/library/common/types/c_types.h +++ b/library/common/types/c_types.h @@ -511,4 +511,4 @@ typedef enum { // Certificate is not trusted because it has an extendedKeyUsage field, but // its value is not correct for a web server. CERT_VERIFY_STATUS_ANDROID_INCORRECT_KEY_USAGE = -6, -} envoy_cert_verify_status_t; +} envoy_cert_verify_status_android_t; diff --git a/library/java/org/chromium/net/FakeX509Util.java b/library/java/org/chromium/net/FakeX509Util.java index 27a83a9ddb..447c3e5aa2 100644 --- a/library/java/org/chromium/net/FakeX509Util.java +++ b/library/java/org/chromium/net/FakeX509Util.java @@ -6,11 +6,15 @@ import java.util.Set; /** - * Fake utility functions for verifying X.509 certificates. + * Fake utility functions to verify X.509 certificates. + * + * FakeX509Util is not particularly clever: from its perspective a certificate is just a string and + * its contents have no particular meaning. For a verification to succeed all certificates in a + * chain must have been previously registered as root certificates. This doesn't make much sense + * w.r.t. how X.509 certificates are really validated, but we're not interested in mimicking that, + * we just want something to confirm that JNI calls have taken place. */ public final class FakeX509Util { - private static final String TAG = "FakeX509Util"; - private static final Set validFakeCerts = new HashSet(); public static void addTestRootCertificate(byte[] rootCertBytes) { @@ -20,6 +24,10 @@ public static void addTestRootCertificate(byte[] rootCertBytes) { public static void clearTestRootCertificates() { validFakeCerts.clear(); } + /* Fake certificate chain verification. Returns CertVerifyStatusAndroid.OK only if all + * certificates in the chain have been previously registered as root certificates, + * CertVerifyStatusAndroid.NO_TRUSTED_ROOT otherwise. + */ public static AndroidCertVerifyResult verifyServerCertificates(byte[][] certChain, String authType, String host) { if (certChain == null || certChain.length == 0 || certChain[0] == null) { diff --git a/test/java/org/chromium/net/CertificateVerificationTest.java b/test/java/org/chromium/net/CertificateVerificationTest.java index a65c1f87b8..6ef8b7b78f 100644 --- a/test/java/org/chromium/net/CertificateVerificationTest.java +++ b/test/java/org/chromium/net/CertificateVerificationTest.java @@ -12,10 +12,13 @@ import org.robolectric.RobolectricTestRunner; /** - * Simple test for Certificate verification. + * Simple test for Certificate verification JNI layer. + * The objective is not to test the certificate verification logic (which is faked) but instead to + * confirm that all JNI calls go through (confirmed by checking for the fake implementation side + * effects). */ @RunWith(RobolectricTestRunner.class) -public class CertificateVerificationTest { +public final class CertificateVerificationTest { static { AndroidJniLibrary.loadTestLibrary(); JniLibrary.load(); @@ -33,40 +36,89 @@ public void tearDown() throws Exception { } @Test - public void testVerifyCertificateCall() throws Exception { - String[] fakeCertChain1 = new String[] {"fake cert"}; - byte[][] certChain1 = new byte[][] {fakeCertChain1[0].getBytes()}; - - String[] fakeCertChain2 = new String[] {"fake cert", "another fake cert"}; - byte[][] certChain2 = new byte[][] {fakeCertChain2[0].getBytes(), fakeCertChain2[1].getBytes()}; + public void testChainWithNonRootCertificate() throws Exception { + final String[] fakeCertChain = new String[] {"fake cert"}; + final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes()}; + final String authType = ""; + final String host = ""; AndroidCertVerifyResult result = - (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain1, "", - ""); - assertEquals(result.getStatus(), CertVerifyStatusAndroid.NO_TRUSTED_ROOT); - result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain2, - "", ""); + (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, + authType, host); assertEquals(result.getStatus(), CertVerifyStatusAndroid.NO_TRUSTED_ROOT); + } - JniLibrary.callAddTestRootCertificateFromNative(certChain1[0]); - result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain1, - "", ""); - assertEquals(result.getStatus(), CertVerifyStatusAndroid.OK); - result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain2, - "", ""); - assertEquals(result.getStatus(), CertVerifyStatusAndroid.NO_TRUSTED_ROOT); + @Test + public void testChainWithRootCertificate() throws Exception { + final String[] fakeCertChain = new String[] {"fake cert"}; + final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes()}; + final String authType = ""; + final String host = ""; - JniLibrary.callAddTestRootCertificateFromNative(certChain2[1]); - result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain2, - "", ""); + JniLibrary.callAddTestRootCertificateFromNative(certChain[0]); + AndroidCertVerifyResult result = + (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, + authType, host); assertEquals(result.getStatus(), CertVerifyStatusAndroid.OK); + } + + @Test + public void testClearTestRootCertificate() throws Exception { + final String[] fakeCertChain = new String[] {"fake cert"}; + final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes()}; + final String authType = ""; + final String host = ""; + JniLibrary.callAddTestRootCertificateFromNative(certChain[0]); JniLibrary.callClearTestRootCertificateFromNative(); - result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain1, - "", ""); + AndroidCertVerifyResult result = + (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, + authType, host); + assertEquals(result.getStatus(), CertVerifyStatusAndroid.NO_TRUSTED_ROOT); + } + + @Test + public void testChainWithMultipleNonRootCertificates() throws Exception { + final String[] fakeCertChain = new String[] {"fake cert", "another fake cert"}; + final byte[][] certChain = + new byte[][] {fakeCertChain[0].getBytes(), fakeCertChain[1].getBytes()}; + final String authType = ""; + final String host = ""; + + AndroidCertVerifyResult result = + (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, + authType, host); assertEquals(result.getStatus(), CertVerifyStatusAndroid.NO_TRUSTED_ROOT); - result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain2, - "", ""); + } + + @Test + public void testChainWithMixedCertificates() throws Exception { + final String[] fakeCertChain = new String[] {"fake cert", "another fake cert"}; + final byte[][] certChain = + new byte[][] {fakeCertChain[0].getBytes(), fakeCertChain[1].getBytes()}; + final String authType = ""; + final String host = ""; + + JniLibrary.callAddTestRootCertificateFromNative(certChain[0]); + AndroidCertVerifyResult result = + (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, + authType, host); assertEquals(result.getStatus(), CertVerifyStatusAndroid.NO_TRUSTED_ROOT); } + + @Test + public void testChainWithMultipleRootCertificates() throws Exception { + final String[] fakeCertChain = new String[] {"fake cert", "another fake cert"}; + final byte[][] certChain = + new byte[][] {fakeCertChain[0].getBytes(), fakeCertChain[1].getBytes()}; + final String authType = ""; + final String host = ""; + + JniLibrary.callAddTestRootCertificateFromNative(certChain[0]); + JniLibrary.callAddTestRootCertificateFromNative(certChain[1]); + AndroidCertVerifyResult result = + (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, + authType, host); + assertEquals(result.getStatus(), CertVerifyStatusAndroid.OK); + } } From 4f1206426fb5c87b106c7f6c7da0e185a36c08ce Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Fri, 6 May 2022 15:44:14 +0100 Subject: [PATCH 05/12] Typo Signed-off-by: Stefano Duo --- library/java/org/chromium/net/AndroidNetworkLibrary.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/java/org/chromium/net/AndroidNetworkLibrary.java b/library/java/org/chromium/net/AndroidNetworkLibrary.java index 28909665dc..256b644c37 100644 --- a/library/java/org/chromium/net/AndroidNetworkLibrary.java +++ b/library/java/org/chromium/net/AndroidNetworkLibrary.java @@ -13,10 +13,10 @@ public final class AndroidNetworkLibrary { private static boolean mUseFakeCertificateVerification; /** - * Wheter a fake should be used in place of X509Util. This allows to easily test the JNI - * calls interaction in robolectric tests. + * Whether a fake should be used in place of X509Util. This allows to easily test the JNI + * call interaction in robolectric tests. * - * @param useFakeCertificateVerification Wheter FakeX509Util should be used or not. + * @param useFakeCertificateVerification Whether FakeX509Util should be used or not. */ public static void setFakeCertificateVerificationForTesting(boolean useFakeCertificateVerification) { From 15d9bc7eed1cce17116a2a510baeeaa23981496c Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Fri, 6 May 2022 18:09:07 +0100 Subject: [PATCH 06/12] Address comments. Signed-off-by: Stefano Duo --- library/common/jni/jni_utility.cc | 8 ++++---- library/java/org/chromium/net/FakeX509Util.java | 3 ++- .../chromium/net/CertificateVerificationTest.java | 15 +++------------ 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/library/common/jni/jni_utility.cc b/library/common/jni/jni_utility.cc index d9887cc29f..9a6c331ed2 100644 --- a/library/common/jni/jni_utility.cc +++ b/library/common/jni/jni_utility.cc @@ -292,8 +292,8 @@ void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, for (size_t i = 0; i < len; ++i) { jbyteArray bytes_array = static_cast(env->GetObjectArrayElement(array, i)); jsize bytes_len = env->GetArrayLength(bytes_array); - // We don't care whether the returned array is a copy or not as we're simply - // copying it into C++ owned memory. + // It doesn't matter if the array returned by GetByteArrayElements is a copy + // or not, as the data will be simply be copied into C++ owned memory below. jbyte* bytes = env->GetByteArrayElements(bytes_array, /*isCopy=*/nullptr); (*out)[i].assign(reinterpret_cast(bytes), bytes_len); // There is nothing to write back, it is always safe to JNI_ABORT. @@ -307,8 +307,8 @@ void JavaArrayOfByteToBytesVector(JNIEnv* env, jbyteArray array, std::vectorGetArrayLength(array); out->resize(len); - // We don't care whether the returned array is a copy or not as we're simply - // copying it into C++ owned memory. + // It doesn't matter if the array returned by GetByteArrayElements is a copy + // or not, as the data will be simply be copied into C++ owned memory below. jbyte* jbytes = env->GetByteArrayElements(array, /*isCopy=*/nullptr); uint8_t* bytes = reinterpret_cast(jbytes); std::copy(bytes, bytes + len, out->begin()); diff --git a/library/java/org/chromium/net/FakeX509Util.java b/library/java/org/chromium/net/FakeX509Util.java index 447c3e5aa2..af323bdd15 100644 --- a/library/java/org/chromium/net/FakeX509Util.java +++ b/library/java/org/chromium/net/FakeX509Util.java @@ -24,7 +24,8 @@ public static void addTestRootCertificate(byte[] rootCertBytes) { public static void clearTestRootCertificates() { validFakeCerts.clear(); } - /* Fake certificate chain verification. Returns CertVerifyStatusAndroid.OK only if all + /** + * Performs fake certificate chain verification. Returns CertVerifyStatusAndroid.OK only if all * certificates in the chain have been previously registered as root certificates, * CertVerifyStatusAndroid.NO_TRUSTED_ROOT otherwise. */ diff --git a/test/java/org/chromium/net/CertificateVerificationTest.java b/test/java/org/chromium/net/CertificateVerificationTest.java index 6ef8b7b78f..9c72977698 100644 --- a/test/java/org/chromium/net/CertificateVerificationTest.java +++ b/test/java/org/chromium/net/CertificateVerificationTest.java @@ -24,6 +24,9 @@ public final class CertificateVerificationTest { JniLibrary.load(); } + private static final String authType = ""; + private static final String host = ""; + @Before public void setUp() throws Exception { AndroidNetworkLibrary.setFakeCertificateVerificationForTesting(true); @@ -39,8 +42,6 @@ public void tearDown() throws Exception { public void testChainWithNonRootCertificate() throws Exception { final String[] fakeCertChain = new String[] {"fake cert"}; final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes()}; - final String authType = ""; - final String host = ""; AndroidCertVerifyResult result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, @@ -52,8 +53,6 @@ public void testChainWithNonRootCertificate() throws Exception { public void testChainWithRootCertificate() throws Exception { final String[] fakeCertChain = new String[] {"fake cert"}; final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes()}; - final String authType = ""; - final String host = ""; JniLibrary.callAddTestRootCertificateFromNative(certChain[0]); AndroidCertVerifyResult result = @@ -66,8 +65,6 @@ public void testChainWithRootCertificate() throws Exception { public void testClearTestRootCertificate() throws Exception { final String[] fakeCertChain = new String[] {"fake cert"}; final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes()}; - final String authType = ""; - final String host = ""; JniLibrary.callAddTestRootCertificateFromNative(certChain[0]); JniLibrary.callClearTestRootCertificateFromNative(); @@ -82,8 +79,6 @@ public void testChainWithMultipleNonRootCertificates() throws Exception { final String[] fakeCertChain = new String[] {"fake cert", "another fake cert"}; final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes(), fakeCertChain[1].getBytes()}; - final String authType = ""; - final String host = ""; AndroidCertVerifyResult result = (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, @@ -96,8 +91,6 @@ public void testChainWithMixedCertificates() throws Exception { final String[] fakeCertChain = new String[] {"fake cert", "another fake cert"}; final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes(), fakeCertChain[1].getBytes()}; - final String authType = ""; - final String host = ""; JniLibrary.callAddTestRootCertificateFromNative(certChain[0]); AndroidCertVerifyResult result = @@ -111,8 +104,6 @@ public void testChainWithMultipleRootCertificates() throws Exception { final String[] fakeCertChain = new String[] {"fake cert", "another fake cert"}; final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes(), fakeCertChain[1].getBytes()}; - final String authType = ""; - final String host = ""; JniLibrary.callAddTestRootCertificateFromNative(certChain[0]); JniLibrary.callAddTestRootCertificateFromNative(certChain[1]); From 68f24dff7cd2910818ce0e4980f1258ec63242e7 Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Mon, 9 May 2022 17:14:00 +0100 Subject: [PATCH 07/12] Address comments * Extend FakeX509Util with host and authType checks * Add tests for host and authType * Refactor UTF conversion in its own library * Add unit tests for them Signed-off-by: Stefano Duo --- library/common/jni/BUILD | 2 ++ library/common/jni/jni_interface.cc | 6 ++++ library/common/jni/jni_utility.cc | 14 +++++---- library/common/strings/BUILD | 10 +++++++ library/common/strings/string_conversions.cc | 14 +++++++++ library/common/strings/string_conversions.h | 7 +++++ .../java/org/chromium/net/FakeX509Util.java | 23 +++++++++----- test/common/strings/BUILD | 14 +++++++++ .../common/strings/string_conversions_test.cc | 11 +++++++ .../net/CertificateVerificationTest.java | 30 +++++++++++++++++-- 10 files changed, 115 insertions(+), 16 deletions(-) create mode 100644 library/common/strings/BUILD create mode 100644 library/common/strings/string_conversions.cc create mode 100644 library/common/strings/string_conversions.h create mode 100644 test/common/strings/BUILD create mode 100644 test/common/strings/string_conversions_test.cc diff --git a/library/common/jni/BUILD b/library/common/jni/BUILD index ac721be063..5ba513514d 100644 --- a/library/common/jni/BUILD +++ b/library/common/jni/BUILD @@ -21,6 +21,7 @@ cc_library( ], deps = [ "//library/common/jni/import:jni_import_lib", + "//library/common/strings:string_conversions_lib", "//library/common/types:c_types_lib", "@envoy//source/common/common:assert_lib", ], @@ -160,6 +161,7 @@ cc_library( ], deps = [ ":java_jni_support", + "//library/common/strings:string_conversions_lib", "//library/common/jni/import:jni_import_lib", "//library/common:envoy_main_interface_lib", "//library/common/types:c_types_lib", diff --git a/library/common/jni/jni_interface.cc b/library/common/jni/jni_interface.cc index e52b272f4c..5d84f880d7 100644 --- a/library/common/jni/jni_interface.cc +++ b/library/common/jni/jni_interface.cc @@ -1057,6 +1057,12 @@ jobjectArray jvm_cert_get_certificate_chain_encoded(JNIEnv* env, jobject result) env->CallObjectMethod(jcls_AndroidCertVerifyResult, jmid_getCertificateChainEncoded, result)); } +// Once we have a better picture of how Android's certificate verification will +// be plugged into EM, we should decide where this function should really live. +// Context: as of now JNI functions declared in this file are not exported through any +// header files, instead they are stored as callbacks into plain function +// tables. For this reason, this function, which would ideally be defined in +// jni_utility.cc, is currently defined here. void ExtractCertVerifyResult(JNIEnv* env, jobject result, envoy_cert_verify_status_android_t* status, bool* is_issued_by_known_root, diff --git a/library/common/jni/jni_utility.cc b/library/common/jni/jni_utility.cc index 9a6c331ed2..45c57d47a2 100644 --- a/library/common/jni/jni_utility.cc +++ b/library/common/jni/jni_utility.cc @@ -3,12 +3,11 @@ #include #include -#include - #include "source/common/common/assert.h" #include "library/common/jni/jni_support.h" #include "library/common/jni/jni_version.h" +#include "library/common/strings/string_conversions.h" // NOLINT(namespace-envoy) @@ -259,8 +258,9 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries) { } jstring ConvertUTF8ToJavaString(JNIEnv* env, const std::string& str) { - std::u16string utf16 = - std::wstring_convert, char16_t>{}.from_bytes(str); + // JNI's NewStringUTF expects "modified" UTF8 so instead convert the string to UTF16 and pass it + // to NewString. + std::u16string utf16 = UTF8ToUTF16(str.data(), str.size()); const jchar* jutf16 = reinterpret_cast(utf16.data()); return env->NewString(jutf16, utf16.length()); } @@ -317,11 +317,13 @@ void JavaArrayOfByteToBytesVector(JNIEnv* env, jbyteArray array, std::vectorGetStringChars(str, /*isCopy=*/nullptr); - std::string utf8 = std::wstring_convert, char16_t>{}.to_bytes( - reinterpret_cast(utf16)); + size_t len = env->GetStringLength(str); + std::string utf8 = UTF16ToUTF8(reinterpret_cast(utf16), len); *result = utf8; env->ReleaseStringChars(str, utf16); } diff --git a/library/common/strings/BUILD b/library/common/strings/BUILD new file mode 100644 index 0000000000..4597914ffb --- /dev/null +++ b/library/common/strings/BUILD @@ -0,0 +1,10 @@ +load("@rules_cc//cc:defs.bzl", "cc_library") + +licenses(["notice"]) # Apache 2 + +cc_library( + name = "string_conversions_lib", + srcs = ["string_conversions.cc"], + hdrs = ["string_conversions.h"], + visibility = ["//visibility:public"], +) diff --git a/library/common/strings/string_conversions.cc b/library/common/strings/string_conversions.cc new file mode 100644 index 0000000000..0b62faf821 --- /dev/null +++ b/library/common/strings/string_conversions.cc @@ -0,0 +1,14 @@ +#include "library/common/strings/string_conversions.h" + +#include +#include + +std::string UTF16ToUTF8(const char16_t* utf16, size_t len) { + return std::wstring_convert, char16_t>{}.to_bytes(utf16, + utf16 + len); +} + +std::u16string UTF8ToUTF16(const char* utf8, size_t len) { + return std::wstring_convert, char16_t>{}.from_bytes(utf8, + utf8 + len); +} diff --git a/library/common/strings/string_conversions.h b/library/common/strings/string_conversions.h new file mode 100644 index 0000000000..98794466d8 --- /dev/null +++ b/library/common/strings/string_conversions.h @@ -0,0 +1,7 @@ +#pragma once + +#include + +std::string UTF16ToUTF8(const char16_t* utf16, size_t len); + +std::u16string UTF8ToUTF16(const char* utf8, size_t len); diff --git a/library/java/org/chromium/net/FakeX509Util.java b/library/java/org/chromium/net/FakeX509Util.java index af323bdd15..efdfa36d6f 100644 --- a/library/java/org/chromium/net/FakeX509Util.java +++ b/library/java/org/chromium/net/FakeX509Util.java @@ -9,14 +9,18 @@ * Fake utility functions to verify X.509 certificates. * * FakeX509Util is not particularly clever: from its perspective a certificate is just a string and - * its contents have no particular meaning. For a verification to succeed all certificates in a - * chain must have been previously registered as root certificates. This doesn't make much sense - * w.r.t. how X.509 certificates are really validated, but we're not interested in mimicking that, - * we just want something to confirm that JNI calls have taken place. + * its contents have no particular meaning. For a verification to succeed: + * - all certificates in a chain must have been previously registered as root certificates + * - host and authentication type must match the expected hardcoded values + * This doesn't make much sense w.r.t. how X.509 certificates are really validated, but we're not + * interested in mimicking that, we just want something to confirm that JNI calls have taken place. */ public final class FakeX509Util { private static final Set validFakeCerts = new HashSet(); + public static final String expectedAuthType = "RSA"; + public static final String expectedHost = "www.example.com"; + public static void addTestRootCertificate(byte[] rootCertBytes) { String fakeCertificate = new String(rootCertBytes); validFakeCerts.add(fakeCertificate); @@ -25,9 +29,10 @@ public static void addTestRootCertificate(byte[] rootCertBytes) { public static void clearTestRootCertificates() { validFakeCerts.clear(); } /** - * Performs fake certificate chain verification. Returns CertVerifyStatusAndroid.OK only if all - * certificates in the chain have been previously registered as root certificates, - * CertVerifyStatusAndroid.NO_TRUSTED_ROOT otherwise. + * Performs fake certificate chain verification. Returns CertVerifyStatusAndroid.NO_TRUSTED_ROOT + * if at least one of the certificates in the chain has not been previously registered as a root. + * Returns CertVerifyStatusAndroid.OK if authType and host match respectively expectedAuthType and + * expectedHost; CertVerifyStatusAndroid.FAILED otherwise. */ public static AndroidCertVerifyResult verifyServerCertificates(byte[][] certChain, String authType, String host) { @@ -44,6 +49,8 @@ public static AndroidCertVerifyResult verifyServerCertificates(byte[][] certChai } } - return new AndroidCertVerifyResult(CertVerifyStatusAndroid.OK); + return authType.equals(expectedAuthType) && host.equals(expectedHost) + ? new AndroidCertVerifyResult(CertVerifyStatusAndroid.OK) + : new AndroidCertVerifyResult(CertVerifyStatusAndroid.FAILED); } } diff --git a/test/common/strings/BUILD b/test/common/strings/BUILD new file mode 100644 index 0000000000..dc445ddc7d --- /dev/null +++ b/test/common/strings/BUILD @@ -0,0 +1,14 @@ +load("@envoy//bazel:envoy_build_system.bzl", "envoy_cc_test", "envoy_package") + +licenses(["notice"]) # Apache 2 + +envoy_package() + +envoy_cc_test( + name = "string_conversions_test", + srcs = ["string_conversions_test.cc"], + repository = "@envoy", + deps = [ + "//library/common/strings:string_conversions_lib", + ], +) diff --git a/test/common/strings/string_conversions_test.cc b/test/common/strings/string_conversions_test.cc new file mode 100644 index 0000000000..c5c2882e1d --- /dev/null +++ b/test/common/strings/string_conversions_test.cc @@ -0,0 +1,11 @@ +#include + +#include "gtest/gtest.h" +#include "library/common/strings/string_conversions.h" + +TEST(StringConversions, SameEncoding) { + std::string utf8 = u8"z\u00df\u6c34\U0001f34c"; + std::u16string utf16 = u"z\u00df\u6c34\U0001f34c"; + + EXPECT_EQ(utf16, UTF8ToUTF16(utf8.data(), utf8.size())); +} diff --git a/test/java/org/chromium/net/CertificateVerificationTest.java b/test/java/org/chromium/net/CertificateVerificationTest.java index 9c72977698..906ba5d088 100644 --- a/test/java/org/chromium/net/CertificateVerificationTest.java +++ b/test/java/org/chromium/net/CertificateVerificationTest.java @@ -24,8 +24,8 @@ public final class CertificateVerificationTest { JniLibrary.load(); } - private static final String authType = ""; - private static final String host = ""; + private static final String authType = FakeX509Util.expectedAuthType; + private static final String host = FakeX509Util.expectedHost; @Before public void setUp() throws Exception { @@ -61,6 +61,32 @@ public void testChainWithRootCertificate() throws Exception { assertEquals(result.getStatus(), CertVerifyStatusAndroid.OK); } + @Test + public void testChainWithRootCertificateWrongHostname() throws Exception { + final String[] fakeCertChain = new String[] {"fake cert"}; + final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes()}; + final String host = "wrong host"; + + JniLibrary.callAddTestRootCertificateFromNative(certChain[0]); + AndroidCertVerifyResult result = + (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, + authType, host); + assertEquals(result.getStatus(), CertVerifyStatusAndroid.FAILED); + } + + @Test + public void testChainWithRootCertificateWrongAuthType() throws Exception { + final String[] fakeCertChain = new String[] {"fake cert"}; + final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes()}; + final String authType = "wrong auth type"; + + JniLibrary.callAddTestRootCertificateFromNative(certChain[0]); + AndroidCertVerifyResult result = + (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, + authType, host); + assertEquals(result.getStatus(), CertVerifyStatusAndroid.FAILED); + } + @Test public void testClearTestRootCertificate() throws Exception { final String[] fakeCertChain = new String[] {"fake cert"}; From f8a6a46e4948a86947ce00aba47f194e0a8f97c3 Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Wed, 11 May 2022 17:45:29 +0100 Subject: [PATCH 08/12] Address comments Signed-off-by: Stefano Duo --- library/common/jni/jni_utility.cc | 4 +-- library/common/strings/string_conversions.cc | 6 ++++ library/common/strings/string_conversions.h | 18 ++++++++++ .../common/strings/string_conversions_test.cc | 36 +++++++++++++++++-- 4 files changed, 59 insertions(+), 5 deletions(-) diff --git a/library/common/jni/jni_utility.cc b/library/common/jni/jni_utility.cc index 45c57d47a2..3f3218e34c 100644 --- a/library/common/jni/jni_utility.cc +++ b/library/common/jni/jni_utility.cc @@ -260,7 +260,7 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries) { jstring ConvertUTF8ToJavaString(JNIEnv* env, const std::string& str) { // JNI's NewStringUTF expects "modified" UTF8 so instead convert the string to UTF16 and pass it // to NewString. - std::u16string utf16 = UTF8ToUTF16(str.data(), str.size()); + std::u16string utf16 = Envoy::Strings::UTF8ToUTF16(str.data(), str.size()); const jchar* jutf16 = reinterpret_cast(utf16.data()); return env->NewString(jutf16, utf16.length()); } @@ -323,7 +323,7 @@ void ConvertJavaStringToUTF8(JNIEnv* env, jstring str, std::string* result) { // copying it into C++ owned memory. const jchar* utf16 = env->GetStringChars(str, /*isCopy=*/nullptr); size_t len = env->GetStringLength(str); - std::string utf8 = UTF16ToUTF8(reinterpret_cast(utf16), len); + std::string utf8 = Envoy::Strings::UTF16ToUTF8(reinterpret_cast(utf16), len); *result = utf8; env->ReleaseStringChars(str, utf16); } diff --git a/library/common/strings/string_conversions.cc b/library/common/strings/string_conversions.cc index 0b62faf821..1861552416 100644 --- a/library/common/strings/string_conversions.cc +++ b/library/common/strings/string_conversions.cc @@ -3,6 +3,9 @@ #include #include +namespace Envoy { +namespace Strings { + std::string UTF16ToUTF8(const char16_t* utf16, size_t len) { return std::wstring_convert, char16_t>{}.to_bytes(utf16, utf16 + len); @@ -12,3 +15,6 @@ std::u16string UTF8ToUTF16(const char* utf8, size_t len) { return std::wstring_convert, char16_t>{}.from_bytes(utf8, utf8 + len); } + +} // namespace Strings +} // namespace Envoy diff --git a/library/common/strings/string_conversions.h b/library/common/strings/string_conversions.h index 98794466d8..7cc0a83c20 100644 --- a/library/common/strings/string_conversions.h +++ b/library/common/strings/string_conversions.h @@ -2,6 +2,24 @@ #include +namespace Envoy { +namespace Strings { + +/** + * Converts a UTF-16 string to an UTF-8 one. + * @param utf16, the array of code units composing the UTF-16 string + * @param len, the number of code units + * @return the input string encoded in UTF-8 + */ std::string UTF16ToUTF8(const char16_t* utf16, size_t len); +/** + * Converts a UTF-8 string to an UTF-16 one. + * @param utf8, the array of code units composing the UTF-8 string + * @param len, the number of code units + * @return the input string encoded in UTF-16 + */ std::u16string UTF8ToUTF16(const char* utf8, size_t len); + +} // namespace Strings +} // namespace Envoy diff --git a/test/common/strings/string_conversions_test.cc b/test/common/strings/string_conversions_test.cc index c5c2882e1d..cb6f9aa1fd 100644 --- a/test/common/strings/string_conversions_test.cc +++ b/test/common/strings/string_conversions_test.cc @@ -1,11 +1,41 @@ +#include +#include #include #include "gtest/gtest.h" #include "library/common/strings/string_conversions.h" -TEST(StringConversions, SameEncoding) { - std::string utf8 = u8"z\u00df\u6c34\U0001f34c"; - std::u16string utf16 = u"z\u00df\u6c34\U0001f34c"; +namespace Envoy { +namespace Strings { + +namespace { + +TEST(StringConversions, UTF8ToUTF16) { + std::string utf8 = u8"zß水🍌"; + std::u16string utf16 = u"zß水🍌"; + + // Some sanity checking before proceeding, the strings should: + // - Have different code unit sizes when UTF-8 encoded vs UTF-16. + // - Refer to the same abstract unicode characters. + ASSERT_NE(utf8.size(), utf16.size()); + ASSERT_TRUE(std::lexicographical_compare(utf8.begin(), utf8.end(), utf16.begin(), utf16.end())); EXPECT_EQ(utf16, UTF8ToUTF16(utf8.data(), utf8.size())); } + +TEST(StringConversions, UTF16ToUTF8) { + std::string utf8 = u8"zß水🍌"; + std::u16string utf16 = u"zß水🍌"; + + // Some sanity checking before proceeding, the strings should: + // - Have different code unit sizes when UTF-8 encoded vs UTF-16. + // - Refer to the same abstract unicode characters. + ASSERT_NE(utf8.size(), utf16.size()); + ASSERT_TRUE(std::lexicographical_compare(utf8.begin(), utf8.end(), utf16.begin(), utf16.end())); + + EXPECT_EQ(utf8, UTF16ToUTF8(utf16.data(), utf16.size())); +} + +} // namespace +} // namespace Strings +} // namespace Envoy From aa0beeea7079dbc6c9021f9bd6fd4801dbb1381a Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Mon, 16 May 2022 15:17:33 +0100 Subject: [PATCH 09/12] Generalize cert verify result enum and delete local refs Signed-off-by: Stefano Duo --- library/common/jni/jni_interface.cc | 28 +++++++++++++++++----------- library/common/types/c_types.h | 16 ++++++++-------- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/library/common/jni/jni_interface.cc b/library/common/jni/jni_interface.cc index 5d84f880d7..6f26b1aaf2 100644 --- a/library/common/jni/jni_interface.cc +++ b/library/common/jni/jni_interface.cc @@ -1037,24 +1037,29 @@ bool jvm_cert_is_issued_by_known_root(JNIEnv* env, jobject result) { jclass jcls_AndroidCertVerifyResult = env->FindClass("org/chromium/net/AndroidCertVerifyResult"); jmethodID jmid_isIssuedByKnownRoot = env->GetMethodID(jcls_AndroidCertVerifyResult, "isIssuedByKnownRoot", "()Z"); - return env->CallBooleanMethod(jcls_AndroidCertVerifyResult, jmid_isIssuedByKnownRoot, result); + bool is_issued_by_known_root = + env->CallBooleanMethod(jcls_AndroidCertVerifyResult, jmid_isIssuedByKnownRoot, result); + env->DeleteLocalRef(jcls_AndroidCertVerifyResult); + return is_issued_by_known_root; } -envoy_cert_verify_status_android_t jvm_cert_get_status(JNIEnv* env, jobject result) { +envoy_cert_verify_status_t jvm_cert_get_status(JNIEnv* env, jobject j_result) { jclass jcls_AndroidCertVerifyResult = env->FindClass("org/chromium/net/AndroidCertVerifyResult"); jmethodID jmid_getStatus = env->GetMethodID(jcls_AndroidCertVerifyResult, "getStatus", "()I"); - - return static_cast( - env->CallIntMethod(jcls_AndroidCertVerifyResult, jmid_getStatus, result)); + envoy_cert_verify_status_t result = static_cast( + env->CallIntMethod(jcls_AndroidCertVerifyResult, jmid_getStatus, j_result)); + env->DeleteLocalRef(jcls_AndroidCertVerifyResult); + return result; } jobjectArray jvm_cert_get_certificate_chain_encoded(JNIEnv* env, jobject result) { jclass jcls_AndroidCertVerifyResult = env->FindClass("org/chromium/net/AndroidCertVerifyResult"); jmethodID jmid_getCertificateChainEncoded = env->GetMethodID(jcls_AndroidCertVerifyResult, "getCertificateChainEncoded", "()[[B"); - - return static_cast( + jobjectArray certificate_chain = static_cast( env->CallObjectMethod(jcls_AndroidCertVerifyResult, jmid_getCertificateChainEncoded, result)); + env->DeleteLocalRef(jcls_AndroidCertVerifyResult); + return certificate_chain; } // Once we have a better picture of how Android's certificate verification will @@ -1063,8 +1068,7 @@ jobjectArray jvm_cert_get_certificate_chain_encoded(JNIEnv* env, jobject result) // header files, instead they are stored as callbacks into plain function // tables. For this reason, this function, which would ideally be defined in // jni_utility.cc, is currently defined here. -void ExtractCertVerifyResult(JNIEnv* env, jobject result, - envoy_cert_verify_status_android_t* status, +void ExtractCertVerifyResult(JNIEnv* env, jobject result, envoy_cert_verify_status_t* status, bool* is_issued_by_known_root, std::vector* verified_chain) { *status = jvm_cert_get_status(env, result); @@ -1087,7 +1091,6 @@ static jobject call_jvm_verify_x509_cert_chain(const std::vector& c jobjectArray chain_byte_array = ToJavaArrayOfByteArray(env, cert_chain); jstring auth_string = ConvertUTF8ToJavaString(env, auth_type); jstring host_string = ConvertUTF8ToJavaString(env, host); - jobject result = env->CallStaticObjectMethod(jcls_AndroidNetworkLibrary, jmid_verifyServerCertificates, chain_byte_array, auth_string, host_string); @@ -1095,12 +1098,13 @@ static jobject call_jvm_verify_x509_cert_chain(const std::vector& c env->DeleteLocalRef(chain_byte_array); env->DeleteLocalRef(auth_string); env->DeleteLocalRef(host_string); + env->DeleteLocalRef(jcls_AndroidNetworkLibrary); return result; } static void jvm_verify_x509_cert_chain(const std::vector& cert_chain, std::string auth_type, std::string host, - envoy_cert_verify_status_android_t* status, + envoy_cert_verify_status_t* status, bool* is_issued_by_known_root, std::vector* verified_chain) { jobject result = call_jvm_verify_x509_cert_chain(cert_chain, auth_type, host); @@ -1117,6 +1121,7 @@ static void jvm_add_test_root_certificate(const uint8_t* cert, size_t len) { jbyteArray cert_array = ToJavaByteArray(env, cert, len); env->CallStaticVoidMethod(jcls_AndroidNetworkLibrary, jmid_addTestRootCertificate, cert_array); env->DeleteLocalRef(cert_array); + env->DeleteLocalRef(jcls_AndroidNetworkLibrary); } static void jvm_clear_test_root_certificate() { @@ -1127,6 +1132,7 @@ static void jvm_clear_test_root_certificate() { env->GetStaticMethodID(jcls_AndroidNetworkLibrary, "clearTestRootCertificates", "()V"); env->CallStaticVoidMethod(jcls_AndroidNetworkLibrary, jmid_clearTestRootCertificates); + env->DeleteLocalRef(jcls_AndroidNetworkLibrary); } extern "C" JNIEXPORT jobject JNICALL diff --git a/library/common/types/c_types.h b/library/common/types/c_types.h index 114e655ca3..acc127f112 100644 --- a/library/common/types/c_types.h +++ b/library/common/types/c_types.h @@ -496,19 +496,19 @@ typedef struct { */ typedef enum { // Certificate is trusted. - CERT_VERIFY_STATUS_ANDROID_OK = 0, + CERT_VERIFY_STATUS_OK = 0, // Certificate verification could not be conducted. - CERT_VERIFY_STATUS_ANDROID_FAILED = -1, + CERT_VERIFY_STATUS_FAILED = -1, // Certificate is not trusted due to non-trusted root of the certificate // chain. - CERT_VERIFY_STATUS_ANDROID_NO_TRUSTED_ROOT = -2, + CERT_VERIFY_STATUS_NO_TRUSTED_ROOT = -2, // Certificate is not trusted because it has expired. - CERT_VERIFY_STATUS_ANDROID_EXPIRED = -3, + CERT_VERIFY_STATUS_EXPIRED = -3, // Certificate is not trusted because it is not valid yet. - CERT_VERIFY_STATUS_ANDROID_NOT_YET_VALID = -4, + CERT_VERIFY_STATUS_NOT_YET_VALID = -4, // Certificate is not trusted because it could not be parsed. - CERT_VERIFY_STATUS_ANDROID_UNABLE_TO_PARSE = -5, + CERT_VERIFY_STATUS_UNABLE_TO_PARSE = -5, // Certificate is not trusted because it has an extendedKeyUsage field, but // its value is not correct for a web server. - CERT_VERIFY_STATUS_ANDROID_INCORRECT_KEY_USAGE = -6, -} envoy_cert_verify_status_android_t; + CERT_VERIFY_STATUS_INCORRECT_KEY_USAGE = -6, +} envoy_cert_verify_status_t; From 153320a939692a23d298b1256148f83a0acb90e8 Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Wed, 18 May 2022 18:23:20 +0100 Subject: [PATCH 10/12] JNI: pass strings as arrays through their UTF-8 encoding Drop string conversion utilities since they're no longer needed. The conversion is now handled on the Java side. Document the expectation that strings passed to jvm_verify_x509_cert_chain and call_jvm_verify_x509_cert_chain must be UTF-8 encoded. Also add a missing DeleteLocalRef. Signed-off-by: Stefano Duo --- library/common/jni/BUILD | 1 - library/common/jni/jni_interface.cc | 25 ++++++----- library/common/jni/jni_utility.cc | 32 +++++---------- library/common/jni/jni_utility.h | 6 +-- library/common/strings/BUILD | 10 ----- library/common/strings/string_conversions.cc | 20 --------- library/common/strings/string_conversions.h | 25 ----------- .../envoymobile/engine/JniLibrary.java | 2 +- .../chromium/net/AndroidNetworkLibrary.java | 13 ++++-- test/common/strings/BUILD | 14 ------- .../common/strings/string_conversions_test.cc | 41 ------------------- .../net/CertificateVerificationTest.java | 21 ++++++---- 12 files changed, 51 insertions(+), 159 deletions(-) delete mode 100644 library/common/strings/BUILD delete mode 100644 library/common/strings/string_conversions.cc delete mode 100644 library/common/strings/string_conversions.h delete mode 100644 test/common/strings/BUILD delete mode 100644 test/common/strings/string_conversions_test.cc diff --git a/library/common/jni/BUILD b/library/common/jni/BUILD index 5ba513514d..906085e544 100644 --- a/library/common/jni/BUILD +++ b/library/common/jni/BUILD @@ -161,7 +161,6 @@ cc_library( ], deps = [ ":java_jni_support", - "//library/common/strings:string_conversions_lib", "//library/common/jni/import:jni_import_lib", "//library/common:envoy_main_interface_lib", "//library/common/types:c_types_lib", diff --git a/library/common/jni/jni_interface.cc b/library/common/jni/jni_interface.cc index 6f26b1aaf2..451dd807a7 100644 --- a/library/common/jni/jni_interface.cc +++ b/library/common/jni/jni_interface.cc @@ -1068,9 +1068,9 @@ jobjectArray jvm_cert_get_certificate_chain_encoded(JNIEnv* env, jobject result) // header files, instead they are stored as callbacks into plain function // tables. For this reason, this function, which would ideally be defined in // jni_utility.cc, is currently defined here. -void ExtractCertVerifyResult(JNIEnv* env, jobject result, envoy_cert_verify_status_t* status, - bool* is_issued_by_known_root, - std::vector* verified_chain) { +static void ExtractCertVerifyResult(JNIEnv* env, jobject result, envoy_cert_verify_status_t* status, + bool* is_issued_by_known_root, + std::vector* verified_chain) { *status = jvm_cert_get_status(env, result); *is_issued_by_known_root = jvm_cert_is_issued_by_known_root(env, result); @@ -1079,18 +1079,19 @@ void ExtractCertVerifyResult(JNIEnv* env, jobject result, envoy_cert_verify_stat JavaArrayOfByteArrayToStringVector(env, chain_byte_array, verified_chain); } +// `auth_type` and `host` are expected to be UTF-8 encoded. static jobject call_jvm_verify_x509_cert_chain(const std::vector& cert_chain, std::string auth_type, std::string host) { jni_log("[Envoy]", "jvm_verify_x509_cert_chain"); JNIEnv* env = get_env(); jclass jcls_AndroidNetworkLibrary = env->FindClass("org/chromium/net/AndroidNetworkLibrary"); - jmethodID jmid_verifyServerCertificates = env->GetStaticMethodID( - jcls_AndroidNetworkLibrary, "verifyServerCertificates", - "([[BLjava/lang/String;Ljava/lang/String;)Lorg/chromium/net/AndroidCertVerifyResult;"); + jmethodID jmid_verifyServerCertificates = + env->GetStaticMethodID(jcls_AndroidNetworkLibrary, "verifyServerCertificates", + "([[B[B[B)Lorg/chromium/net/AndroidCertVerifyResult;"); jobjectArray chain_byte_array = ToJavaArrayOfByteArray(env, cert_chain); - jstring auth_string = ConvertUTF8ToJavaString(env, auth_type); - jstring host_string = ConvertUTF8ToJavaString(env, host); + jbyteArray auth_string = ToJavaByteArray(env, auth_type); + jbyteArray host_string = ToJavaByteArray(env, host); jobject result = env->CallStaticObjectMethod(jcls_AndroidNetworkLibrary, jmid_verifyServerCertificates, chain_byte_array, auth_string, host_string); @@ -1102,6 +1103,7 @@ static jobject call_jvm_verify_x509_cert_chain(const std::vector& c return result; } +// `auth_type` and `host` are expected to be UTF-8 encoded. static void jvm_verify_x509_cert_chain(const std::vector& cert_chain, std::string auth_type, std::string host, envoy_cert_verify_status_t* status, @@ -1109,6 +1111,7 @@ static void jvm_verify_x509_cert_chain(const std::vector& cert_chai std::vector* verified_chain) { jobject result = call_jvm_verify_x509_cert_chain(cert_chain, auth_type, host); ExtractCertVerifyResult(get_env(), result, status, is_issued_by_known_root, verified_chain); + env->DeleteLocalRef(result); } static void jvm_add_test_root_certificate(const uint8_t* cert, size_t len) { @@ -1137,14 +1140,14 @@ static void jvm_clear_test_root_certificate() { extern "C" JNIEXPORT jobject JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibrary_callCertificateVerificationFromNative( - JNIEnv* env, jclass, jobjectArray certChain, jstring authType, jstring jhost) { + JNIEnv* env, jclass, jobjectArray certChain, jbyteArray jauthType, jbyteArray jhost) { std::vector cert_chain; std::string auth_type; std::string host; JavaArrayOfByteArrayToStringVector(env, certChain, &cert_chain); - ConvertJavaStringToUTF8(env, authType, &auth_type); - ConvertJavaStringToUTF8(env, jhost, &host); + JavaArrayOfByteToString(env, jauthType, &auth_type); + JavaArrayOfByteToString(env, jhost, &host); return call_jvm_verify_x509_cert_chain(cert_chain, auth_type, host); } diff --git a/library/common/jni/jni_utility.cc b/library/common/jni/jni_utility.cc index 3f3218e34c..4a156927f5 100644 --- a/library/common/jni/jni_utility.cc +++ b/library/common/jni/jni_utility.cc @@ -7,7 +7,6 @@ #include "library/common/jni/jni_support.h" #include "library/common/jni/jni_version.h" -#include "library/common/strings/string_conversions.h" // NOLINT(namespace-envoy) @@ -257,14 +256,6 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries) { return native_map; } -jstring ConvertUTF8ToJavaString(JNIEnv* env, const std::string& str) { - // JNI's NewStringUTF expects "modified" UTF8 so instead convert the string to UTF16 and pass it - // to NewString. - std::u16string utf16 = Envoy::Strings::UTF8ToUTF16(str.data(), str.size()); - const jchar* jutf16 = reinterpret_cast(utf16.data()); - return env->NewString(jutf16, utf16.length()); -} - jobjectArray ToJavaArrayOfByteArray(JNIEnv* env, const std::vector& v) { jclass jcls_byte_array = env->FindClass("[B"); jobjectArray joa = env->NewObjectArray(v.size(), jcls_byte_array, nullptr); @@ -284,6 +275,11 @@ jbyteArray ToJavaByteArray(JNIEnv* env, const uint8_t* bytes, size_t len) { return byte_array; } +jbyteArray ToJavaByteArray(JNIEnv* env, const std::string& str) { + const uint8_t* str_bytes = reinterpret_cast(str.data()); + return ToJavaByteArray(env, str_bytes, str.size()); +} + void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, std::vector* out) { size_t len = env->GetArrayLength(array); @@ -303,6 +299,12 @@ void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, } } +void JavaArrayOfByteToString(JNIEnv* env, jbyteArray jbytes, std::string* out) { + std::vector bytes; + JavaArrayOfByteToBytesVector(env, jbytes, &bytes); + *out = std::string(bytes.begin(), bytes.end()); +} + void JavaArrayOfByteToBytesVector(JNIEnv* env, jbyteArray array, std::vector* out) { const size_t len = env->GetArrayLength(array); out->resize(len); @@ -315,15 +317,3 @@ void JavaArrayOfByteToBytesVector(JNIEnv* env, jbyteArray array, std::vectorReleaseByteArrayElements(array, jbytes, JNI_ABORT); } - -void ConvertJavaStringToUTF8(JNIEnv* env, jstring str, std::string* result) { - // JNI's GetStringUTFChars() returns strings in Java "modified" UTF8, so - // instead get the String in UTF16 and manually convert that to UTF8. - // We don't care whether the returned string is a copy or not as we're simply - // copying it into C++ owned memory. - const jchar* utf16 = env->GetStringChars(str, /*isCopy=*/nullptr); - size_t len = env->GetStringLength(str); - std::string utf8 = Envoy::Strings::UTF16ToUTF8(reinterpret_cast(utf16), len); - *result = utf8; - env->ReleaseStringChars(str, utf16); -} diff --git a/library/common/jni/jni_utility.h b/library/common/jni/jni_utility.h index 0895dbe6b8..3e1e4b0db0 100644 --- a/library/common/jni/jni_utility.h +++ b/library/common/jni/jni_utility.h @@ -71,15 +71,15 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries); * Utilities to translate C++ std library constructs to their Java counterpart. * The underlying data is always copied to disentangle C++ and Java objects lifetime. */ -jstring ConvertUTF8ToJavaString(JNIEnv* env, const std::string& str); - jobjectArray ToJavaArrayOfByteArray(JNIEnv* env, const std::vector& v); jbyteArray ToJavaByteArray(JNIEnv* env, const uint8_t* bytes, size_t len); +jbyteArray ToJavaByteArray(JNIEnv* env, const std::string& str); + void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, std::vector* out); void JavaArrayOfByteToBytesVector(JNIEnv* env, jbyteArray array, std::vector* out); -void ConvertJavaStringToUTF8(JNIEnv* env, jstring str, std::string* result); +void JavaArrayOfByteToString(JNIEnv* env, jbyteArray jbytes, std::string* out); diff --git a/library/common/strings/BUILD b/library/common/strings/BUILD deleted file mode 100644 index 4597914ffb..0000000000 --- a/library/common/strings/BUILD +++ /dev/null @@ -1,10 +0,0 @@ -load("@rules_cc//cc:defs.bzl", "cc_library") - -licenses(["notice"]) # Apache 2 - -cc_library( - name = "string_conversions_lib", - srcs = ["string_conversions.cc"], - hdrs = ["string_conversions.h"], - visibility = ["//visibility:public"], -) diff --git a/library/common/strings/string_conversions.cc b/library/common/strings/string_conversions.cc deleted file mode 100644 index 1861552416..0000000000 --- a/library/common/strings/string_conversions.cc +++ /dev/null @@ -1,20 +0,0 @@ -#include "library/common/strings/string_conversions.h" - -#include -#include - -namespace Envoy { -namespace Strings { - -std::string UTF16ToUTF8(const char16_t* utf16, size_t len) { - return std::wstring_convert, char16_t>{}.to_bytes(utf16, - utf16 + len); -} - -std::u16string UTF8ToUTF16(const char* utf8, size_t len) { - return std::wstring_convert, char16_t>{}.from_bytes(utf8, - utf8 + len); -} - -} // namespace Strings -} // namespace Envoy diff --git a/library/common/strings/string_conversions.h b/library/common/strings/string_conversions.h deleted file mode 100644 index 7cc0a83c20..0000000000 --- a/library/common/strings/string_conversions.h +++ /dev/null @@ -1,25 +0,0 @@ -#pragma once - -#include - -namespace Envoy { -namespace Strings { - -/** - * Converts a UTF-16 string to an UTF-8 one. - * @param utf16, the array of code units composing the UTF-16 string - * @param len, the number of code units - * @return the input string encoded in UTF-8 - */ -std::string UTF16ToUTF8(const char16_t* utf16, size_t len); - -/** - * Converts a UTF-8 string to an UTF-16 one. - * @param utf8, the array of code units composing the UTF-8 string - * @param len, the number of code units - * @return the input string encoded in UTF-16 - */ -std::u16string UTF8ToUTF16(const char* utf8, size_t len); - -} // namespace Strings -} // namespace Envoy diff --git a/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java b/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java index 55edd24e74..3a9ddbcc66 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java +++ b/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java @@ -342,7 +342,7 @@ protected static native int registerStringAccessor(String accessorName, * @return Android certificate verification result code. */ public static native Object callCertificateVerificationFromNative(byte[][] certChain, - String authType, String host); + byte[] authType, byte[] host); /** * Mimic a call to AndroidNetworkLibrary#addTestRootCertificate from native code. * To be used for testing only. diff --git a/library/java/org/chromium/net/AndroidNetworkLibrary.java b/library/java/org/chromium/net/AndroidNetworkLibrary.java index 256b644c37..d299f9a8ac 100644 --- a/library/java/org/chromium/net/AndroidNetworkLibrary.java +++ b/library/java/org/chromium/net/AndroidNetworkLibrary.java @@ -4,6 +4,8 @@ import java.security.NoSuchAlgorithmException; import java.security.cert.CertificateException; +import java.nio.charset.StandardCharsets; + /** * This class implements net utilities required by the net component. */ @@ -29,12 +31,15 @@ public final class AndroidNetworkLibrary { * This is called from native code. * * @param certChain The ASN.1 DER encoded bytes for certificates. - * @param authType The key exchange algorithm name (e.g. RSA). - * @param host The hostname of the server. + * @param authType Bytes representing the UTF-8 encoding of the key exchange algorithm name (e.g. + * RSA). + * @param host Bytes representing the UTF-8 encoding of the hostname of the server. * @return Android certificate verification result code. */ - public static AndroidCertVerifyResult verifyServerCertificates(byte[][] certChain, - String authType, String host) { + public static AndroidCertVerifyResult + verifyServerCertificates(byte[][] certChain, byte[] authTypeBytes, byte[] hostBytes) { + String authType = new String(authTypeBytes, StandardCharsets.UTF_8); + String host = new String(hostBytes, StandardCharsets.UTF_8); if (mUseFakeCertificateVerification) { return FakeX509Util.verifyServerCertificates(certChain, authType, host); } diff --git a/test/common/strings/BUILD b/test/common/strings/BUILD deleted file mode 100644 index dc445ddc7d..0000000000 --- a/test/common/strings/BUILD +++ /dev/null @@ -1,14 +0,0 @@ -load("@envoy//bazel:envoy_build_system.bzl", "envoy_cc_test", "envoy_package") - -licenses(["notice"]) # Apache 2 - -envoy_package() - -envoy_cc_test( - name = "string_conversions_test", - srcs = ["string_conversions_test.cc"], - repository = "@envoy", - deps = [ - "//library/common/strings:string_conversions_lib", - ], -) diff --git a/test/common/strings/string_conversions_test.cc b/test/common/strings/string_conversions_test.cc deleted file mode 100644 index cb6f9aa1fd..0000000000 --- a/test/common/strings/string_conversions_test.cc +++ /dev/null @@ -1,41 +0,0 @@ -#include -#include -#include - -#include "gtest/gtest.h" -#include "library/common/strings/string_conversions.h" - -namespace Envoy { -namespace Strings { - -namespace { - -TEST(StringConversions, UTF8ToUTF16) { - std::string utf8 = u8"zß水🍌"; - std::u16string utf16 = u"zß水🍌"; - - // Some sanity checking before proceeding, the strings should: - // - Have different code unit sizes when UTF-8 encoded vs UTF-16. - // - Refer to the same abstract unicode characters. - ASSERT_NE(utf8.size(), utf16.size()); - ASSERT_TRUE(std::lexicographical_compare(utf8.begin(), utf8.end(), utf16.begin(), utf16.end())); - - EXPECT_EQ(utf16, UTF8ToUTF16(utf8.data(), utf8.size())); -} - -TEST(StringConversions, UTF16ToUTF8) { - std::string utf8 = u8"zß水🍌"; - std::u16string utf16 = u"zß水🍌"; - - // Some sanity checking before proceeding, the strings should: - // - Have different code unit sizes when UTF-8 encoded vs UTF-16. - // - Refer to the same abstract unicode characters. - ASSERT_NE(utf8.size(), utf16.size()); - ASSERT_TRUE(std::lexicographical_compare(utf8.begin(), utf8.end(), utf16.begin(), utf16.end())); - - EXPECT_EQ(utf8, UTF16ToUTF8(utf16.data(), utf16.size())); -} - -} // namespace -} // namespace Strings -} // namespace Envoy diff --git a/test/java/org/chromium/net/CertificateVerificationTest.java b/test/java/org/chromium/net/CertificateVerificationTest.java index 906ba5d088..86575c97d5 100644 --- a/test/java/org/chromium/net/CertificateVerificationTest.java +++ b/test/java/org/chromium/net/CertificateVerificationTest.java @@ -5,6 +5,8 @@ import io.envoyproxy.envoymobile.engine.AndroidJniLibrary; import io.envoyproxy.envoymobile.engine.JniLibrary; +import java.nio.charset.StandardCharsets; + import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -24,8 +26,9 @@ public final class CertificateVerificationTest { JniLibrary.load(); } - private static final String authType = FakeX509Util.expectedAuthType; - private static final String host = FakeX509Util.expectedHost; + private static final byte[] host = FakeX509Util.expectedHost.getBytes(StandardCharsets.UTF_8); + private static final byte[] authType = + FakeX509Util.expectedAuthType.getBytes(StandardCharsets.UTF_8); @Before public void setUp() throws Exception { @@ -44,8 +47,8 @@ public void testChainWithNonRootCertificate() throws Exception { final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes()}; AndroidCertVerifyResult result = - (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, - authType, host); + (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, host, + authType); assertEquals(result.getStatus(), CertVerifyStatusAndroid.NO_TRUSTED_ROOT); } @@ -66,11 +69,12 @@ public void testChainWithRootCertificateWrongHostname() throws Exception { final String[] fakeCertChain = new String[] {"fake cert"}; final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes()}; final String host = "wrong host"; + final byte[] hostBytes = host.getBytes(StandardCharsets.UTF_8); JniLibrary.callAddTestRootCertificateFromNative(certChain[0]); AndroidCertVerifyResult result = - (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, - authType, host); + (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative( + certChain, authType, hostBytes); assertEquals(result.getStatus(), CertVerifyStatusAndroid.FAILED); } @@ -79,11 +83,12 @@ public void testChainWithRootCertificateWrongAuthType() throws Exception { final String[] fakeCertChain = new String[] {"fake cert"}; final byte[][] certChain = new byte[][] {fakeCertChain[0].getBytes()}; final String authType = "wrong auth type"; + final byte[] authTypeBytes = authType.getBytes(StandardCharsets.UTF_8); JniLibrary.callAddTestRootCertificateFromNative(certChain[0]); AndroidCertVerifyResult result = - (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative(certChain, - authType, host); + (AndroidCertVerifyResult)JniLibrary.callCertificateVerificationFromNative( + certChain, authTypeBytes, host); assertEquals(result.getStatus(), CertVerifyStatusAndroid.FAILED); } From 55dc25066ec18aac74f98462fe152893a4674d39 Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Wed, 18 May 2022 19:23:59 +0100 Subject: [PATCH 11/12] Fix compile error Signed-off-by: Stefano Duo --- library/common/jni/BUILD | 1 - library/common/jni/jni_interface.cc | 7 ++++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/library/common/jni/BUILD b/library/common/jni/BUILD index 906085e544..ac721be063 100644 --- a/library/common/jni/BUILD +++ b/library/common/jni/BUILD @@ -21,7 +21,6 @@ cc_library( ], deps = [ "//library/common/jni/import:jni_import_lib", - "//library/common/strings:string_conversions_lib", "//library/common/types:c_types_lib", "@envoy//source/common/common:assert_lib", ], diff --git a/library/common/jni/jni_interface.cc b/library/common/jni/jni_interface.cc index 451dd807a7..010c2d3f40 100644 --- a/library/common/jni/jni_interface.cc +++ b/library/common/jni/jni_interface.cc @@ -1080,10 +1080,10 @@ static void ExtractCertVerifyResult(JNIEnv* env, jobject result, envoy_cert_veri } // `auth_type` and `host` are expected to be UTF-8 encoded. -static jobject call_jvm_verify_x509_cert_chain(const std::vector& cert_chain, +static jobject call_jvm_verify_x509_cert_chain(JNIEnv* env, + const std::vector& cert_chain, std::string auth_type, std::string host) { jni_log("[Envoy]", "jvm_verify_x509_cert_chain"); - JNIEnv* env = get_env(); jclass jcls_AndroidNetworkLibrary = env->FindClass("org/chromium/net/AndroidNetworkLibrary"); jmethodID jmid_verifyServerCertificates = env->GetStaticMethodID(jcls_AndroidNetworkLibrary, "verifyServerCertificates", @@ -1109,7 +1109,8 @@ static void jvm_verify_x509_cert_chain(const std::vector& cert_chai envoy_cert_verify_status_t* status, bool* is_issued_by_known_root, std::vector* verified_chain) { - jobject result = call_jvm_verify_x509_cert_chain(cert_chain, auth_type, host); + JNIEnv* env = get_env(); + jobject result = call_jvm_verify_x509_cert_chain(env, cert_chain, auth_type, host); ExtractCertVerifyResult(get_env(), result, status, is_issued_by_known_root, verified_chain); env->DeleteLocalRef(result); } From 5c461edea49bbb27c7ef0f336d41645a04e6e7e6 Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Thu, 19 May 2022 10:49:24 +0100 Subject: [PATCH 12/12] Fix missing argument to call call_jvm_verify_x509_cert_chain Signed-off-by: Stefano Duo --- library/common/jni/jni_interface.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/common/jni/jni_interface.cc b/library/common/jni/jni_interface.cc index 010c2d3f40..4adea5c57f 100644 --- a/library/common/jni/jni_interface.cc +++ b/library/common/jni/jni_interface.cc @@ -1150,7 +1150,7 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_callCertificateVerificationFrom JavaArrayOfByteToString(env, jauthType, &auth_type); JavaArrayOfByteToString(env, jhost, &host); - return call_jvm_verify_x509_cert_chain(cert_chain, auth_type, host); + return call_jvm_verify_x509_cert_chain(env, cert_chain, auth_type, host); } extern "C" JNIEXPORT void JNICALL