Android's certificate verification JNI layer #2251

StefanoDuo · 2022-05-05T17:33:01Z

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).

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).

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.

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 <stefanoduo@google.com>

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 <stefanoduo@google.com>

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 <stefanoduo@google.com>

StefanoDuo · 2022-05-05T17:38:10Z

@RyanTheOptimist I'm not sure who's going to review this code, sending over to you for the time being, feel free to redirect.

Note: this is still rough around the edges (I still need to look into the lifetime of JNI objects) but I wanted to touch base to make sure we don't have are on the same page. My idea is that once these JNI calls are in place, the async C++ cert verifier (yet to be built) will simply synchronously call into this.

Main type of feedback I'm looking for is: general shape of things and where we want the code to live. The PR is composed by 3 commits which you should be able to look at independently (I can't create child PR afaik).

RyanTheOptimist

Exciting! This approach looks great. A few comments... In particular, up to you but I would consider sending out the changes to jni_utility.h/cc in a distinct PR as they are so self-contained.

library/common/jni/jni_utility.h

library/common/jni/jni_utility.cc

library/common/jni/jni_interface.cc

library/common/types/c_types.h

library/java/org/chromium/net/FakeX509Util.java

test/java/org/chromium/net/CertificateVerificationTest.java

Signed-off-by: Stefano Duo <stefanoduo@google.com>

StefanoDuo

In particular, up to you but I would consider sending out the changes to jni_utility.h/cc in a distinct PR as they are so self-contained.

That is/was my intention, but AFAIK there is no way to create child PR on github and I wanted to parallelize the review process (ideally the 3 initial commit would have been 3 different CLs).

P.S. What is the accepted way to {D}CHECK inside EM?

Also, I've noticed that in the jni code we're not checking for Java exceptions?

library/common/types/c_types.h

library/common/jni/jni_interface.cc

library/common/jni/jni_utility.h

test/java/org/chromium/net/CertificateVerificationTest.java

library/java/org/chromium/net/FakeX509Util.java

library/common/jni/jni_utility.cc

Signed-off-by: Stefano Duo <stefanoduo@google.com>

RyanTheOptimist

I think this looks great. Just a few comments.

test/java/org/chromium/net/CertificateVerificationTest.java

library/java/org/chromium/net/FakeX509Util.java

library/common/jni/jni_utility.cc

library/common/jni/jni_interface.cc

Signed-off-by: Stefano Duo <stefanoduo@google.com>

* 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 <stefanoduo@google.com>

StefanoDuo

@RyanTheOptimist: at this point, if the only bits worth discussing are the utilities function, I can split them in a different CL if it's going to make your life easier. Let me know what you prefer.

RyanTheOptimist

Looks great! Just a couple of comments. Feel free to leave the new utility functions in, or break them out. Either way.

library/common/strings/string_conversions.h

test/common/strings/string_conversions_test.cc

RyanTheOptimist · 2022-05-09T18:15:46Z

library/common/jni/jni_utility.cc

@@ -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<std::codecvt_utf8_utf16<char16_t>, char16_t>{}.from_bytes(str);
+  // JNI's NewStringUTF expects "modified" UTF8 so instead convert the string to UTF16 and pass it


OOC, what is "modified UTF8"?

AFAIU it's what is used by the JVM under the hood https://docs.oracle.com/javase/1.5.0/docs/guide/jni/spec/types.html#wp16542.

Thanks Wowwwww! That is wacky! I didn't realize "Modified UTF-8" was term of art. Wikipedia also has a good summary.

Modified UTF-8 (MUTF-8) originated in the Java programming language. In Modified UTF-8, the null character (U+0000) uses the two-byte overlong encoding 11000000 10000000 (hexadecimal C0 80), instead of 00000000 (hexadecimal 00).[79] Modified UTF-8 strings never contain any actual null bytes but can contain all Unicode code points including U+0000,[80] which allows such strings (with a null byte appended) to be processed by traditional null-terminated string functions.

To avoid surprises due to the weirdness of Modified UTF-8, the convention we've adopted a this layer is to pass true UTF-8 byte arrays across the JNI and then encode/decode them to java.lang.Strings on the Java side. For simplicity's sake (and perhaps to avoid the risk of extra logic manipulating buffers) it would be nice to maintain that convention, if possible, rather than introduce Modified UTF-8 handling in C++.

Oh I wasn't aware of that, my bad. Can you point me to a specific example so I can more closely follow your approach?

This is a simple example that shows the JNI side (this supports an implementation of a simple extension to retrieve runtime-defined platform Strings):

envoy-mobile/library/common/jni/jni_interface.cc

Line 819 in aa0beee

static envoy_data jvm_get_string(const void* context) {

And here is the Java side:

https://github.com/envoyproxy/envoy-mobile/blob/aa0beeea7079dbc6c9021f9bd6fd4801dbb1381a/library/java/io/envoyproxy/envoymobile/engine/JvmStringAccessorContext.java

I've updated the code to pass true UTF-8 byte arrays across the JNI interface as you suggested.

I don't think I need to wrap stuff in an accessor for my use case (the strings are only needed for the duration of the JNI call). Let me know if this looks okay to you!

Signed-off-by: Stefano Duo <stefanoduo@google.com>

goaway · 2022-05-12T11:59:23Z

It's late for me right now, but I'll do a full pass on this tomorrow during my a.m.

RyanTheOptimist · 2022-05-11T18:08:58Z

library/common/jni/jni_utility.cc

@@ -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<std::codecvt_utf8_utf16<char16_t>, char16_t>{}.from_bytes(str);
+  // JNI's NewStringUTF expects "modified" UTF8 so instead convert the string to UTF16 and pass it


Thanks Wowwwww! That is wacky! I didn't realize "Modified UTF-8" was term of art. Wikipedia also has a good summary.

Modified UTF-8 (MUTF-8) originated in the Java programming language. In Modified UTF-8, the null character (U+0000) uses the two-byte overlong encoding 11000000 10000000 (hexadecimal C0 80), instead of 00000000 (hexadecimal 00).[79] Modified UTF-8 strings never contain any actual null bytes but can contain all Unicode code points including U+0000,[80] which allows such strings (with a null byte appended) to be processed by traditional null-terminated string functions.

goaway

Thanks for the PR @StefanoDuo! Sorry it took me a while to catch it on my radar, and a few more days for me to actually do the full review.

I left some comments below.

Am I correct in my assumption that this code is intended to eventually support a platform-implementable cert validator? If so, I'd propose packaging the API into a struct with a matching Java/Kotlin interface (it looks like FakeX509Util might be the implementation of that interface). This is similar to what we've done with other platform-implementable extensions. Then a trivial Test implementation (FakeX509Util) could drive your tests. A similar approach can be seen in my currently open PR for registering a platform-implementable key-value store:
#2134

(Note there are substantial improvements that can be made to how we manage calling into these externally-implemented extensions; the current mechanism is a minimally-viable placeholder.)

library/common/jni/jni_interface.cc

goaway · 2022-05-16T07:31:23Z

library/common/jni/jni_utility.cc

@@ -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<std::codecvt_utf8_utf16<char16_t>, char16_t>{}.from_bytes(str);
+  // JNI's NewStringUTF expects "modified" UTF8 so instead convert the string to UTF16 and pass it


To avoid surprises due to the weirdness of Modified UTF-8, the convention we've adopted a this layer is to pass true UTF-8 byte arrays across the JNI and then encode/decode them to java.lang.Strings on the Java side. For simplicity's sake (and perhaps to avoid the risk of extra logic manipulating buffers) it would be nice to maintain that convention, if possible, rather than introduce Modified UTF-8 handling in C++.

goaway · 2022-05-16T07:47:06Z

library/common/types/c_types.h

+  // 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;


Can this enum be made generic such that it could be utilized by other platforms (currently iOS and C++)?

I've removed the "android qualifier". I'm not entirely sure if other platform semantics will/do differ, but I think this should be a reasonable first iteration, thoughts?

Signed-off-by: Stefano Duo <stefanoduo@google.com>

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 <stefanoduo@google.com>

Signed-off-by: Stefano Duo <stefanoduo@google.com>

goaway

Looks good. Thanks for making the updates @StefanoDuo!

* origin/main: (32 commits) Compress xcframework release asset (#2324) bazel: update rules_apple (#2326) Merge `android_dist` with `android_dist_ci` (#2323) kotlin: fix flaky receive error test (#2317) kotlin: fix flaky grpc test (#2316) build(deps): bump pyjwt from 2.1.0 to 2.4.0 in /.github/actions/pr_notifier (#2314) test: making C++ integration test more authentic (#2315) reverting override to override_request_timeout_by_gateway_timeout (#2296) Update Envoy (#2309) release: use `CREDENTIALS_GITHUB_RELEASE_DEPLOY_KEY` Use `CREDENTIALS_GITHUB_PUSH_TOKEN` Push to branch before tagging the release Cronvoy: preparation to unittest certificate verification JNI (#2251) Corrected typo in documentation for Android building requirements (#2313) instrumentation: add timers and warnings to platform callbacks (#2300) Bump Lyft Support Rotation (#2310) Revert "android: use local addresses as opposed to prefix (#2081)" (#2307) Fix release GitHub workflow (#2306) mobile: moving the c++ integration test to use default config (#2293) config: cleaning up deprecated configs (#2295) ... Signed-off-by: JP Simard <jp@jpsim.com>

StefanoDuo · 2022-06-17T12:04:08Z

(I forgot to mention the associated bug)

StefanoDuo added 3 commits May 5, 2022 18:05

RyanTheOptimist self-assigned this May 5, 2022

RyanTheOptimist reviewed May 5, 2022

View reviewed changes

Address comments.

fda54b8

Signed-off-by: Stefano Duo <stefanoduo@google.com>

StefanoDuo commented May 6, 2022

View reviewed changes

Typo

4f12064

Signed-off-by: Stefano Duo <stefanoduo@google.com>

StefanoDuo marked this pull request as ready for review May 6, 2022 15:37

RyanTheOptimist reviewed May 6, 2022

View reviewed changes

StefanoDuo added 2 commits May 6, 2022 18:10

Address comments.

15d9bc7

Signed-off-by: Stefano Duo <stefanoduo@google.com>

Address comments

68f24df

* 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 <stefanoduo@google.com>

StefanoDuo commented May 9, 2022

View reviewed changes

RyanTheOptimist reviewed May 9, 2022

View reviewed changes

Address comments

f8a6a46

Signed-off-by: Stefano Duo <stefanoduo@google.com>

goaway self-requested a review May 12, 2022 12:01

goaway self-assigned this May 12, 2022

RyanTheOptimist previously approved these changes May 12, 2022

View reviewed changes

goaway requested changes May 16, 2022

View reviewed changes

Generalize cert verify result enum and delete local refs

aa0beee

Signed-off-by: Stefano Duo <stefanoduo@google.com>

StefanoDuo dismissed RyanTheOptimist’s stale review via aa0beee May 16, 2022 14:18

StefanoDuo added 3 commits May 18, 2022 18:23

Fix compile error

55dc250

Signed-off-by: Stefano Duo <stefanoduo@google.com>

Fix missing argument to call call_jvm_verify_x509_cert_chain

5c461ed

Signed-off-by: Stefano Duo <stefanoduo@google.com>

StefanoDuo requested a review from goaway May 19, 2022 12:44

goaway approved these changes May 24, 2022

View reviewed changes

goaway merged commit b67f4fc into envoyproxy:main May 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android's certificate verification JNI layer #2251

Android's certificate verification JNI layer #2251

StefanoDuo commented May 5, 2022

StefanoDuo commented May 5, 2022

RyanTheOptimist left a comment

StefanoDuo left a comment

RyanTheOptimist left a comment

StefanoDuo left a comment

RyanTheOptimist left a comment

RyanTheOptimist May 9, 2022

StefanoDuo May 11, 2022

RyanTheOptimist May 11, 2022

goaway May 16, 2022

StefanoDuo May 16, 2022

goaway May 16, 2022

StefanoDuo May 18, 2022

goaway commented May 12, 2022

RyanTheOptimist May 11, 2022

goaway left a comment •

edited

Loading

goaway May 16, 2022

goaway May 16, 2022

StefanoDuo May 16, 2022

goaway left a comment

StefanoDuo commented Jun 17, 2022

Android's certificate verification JNI layer #2251

Android's certificate verification JNI layer #2251

Conversation

StefanoDuo commented May 5, 2022

Cronvoy: preparation to unittest certificate verification JNI

Add utility functions to translate between C++ and Java std constructs

Implement JNI layer for Android's certificate verification

StefanoDuo commented May 5, 2022

RyanTheOptimist left a comment

Choose a reason for hiding this comment

StefanoDuo left a comment

Choose a reason for hiding this comment

RyanTheOptimist left a comment

Choose a reason for hiding this comment

StefanoDuo left a comment

Choose a reason for hiding this comment

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goaway commented May 12, 2022

Choose a reason for hiding this comment

goaway left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goaway left a comment

Choose a reason for hiding this comment

StefanoDuo commented Jun 17, 2022

goaway left a comment •

edited

Loading