[AndroidCrypto] Implement X509 chain building #49532

elinor-fung · 2021-03-12T08:36:25Z

There isn't a way to get rich status information (like what .NET APIs expose) or ignore certain validation around chain building on Android. In order to build a chain at all through the APIs available on Android, there must be a path with a valid time, signature, name constraints, policy constraints, and trusted root.

Attempt to build the chain without extra validation (e.g. revocation checking) through CertPathBuilder
- Per above, there is still a lot of validation that cannot be bypassed. if any of those can't be satisfied, building the chain simply fails with an exception - without anything more detailed than that a valid chain could not be constructed.
- In these failure cases, X509Chain.ChainElements is empty and X509Chain.ChainStatus has PartialChain with whatever the exception message was.
Do a validation pass over the built chain through CertPathValidator
Propagate exception message from Android cert path building APIs as chain status items
- If we have an index (failure was in additional validation after the path was built), associate it with the corresponding chain element. Otherwise, just add it to the overall status.
- If we have a failure reason (failure was in additional validation after the path was built, API level is 24+), map that to a corresponding status. Otherwise, report PartialChain.

Tests:

Update RevocationResponder to handle POST requests for OCSP
A bunch of the tests relied on AllowUnknownCertificateAuthority to validate other scenarios. On Android, you can't build a chain at all in that case.
- For those tests, I made them use a custom trust store with a self-issued cert on Android so that they could build a chain and do whatever other validation the test was targeting.
We don't have the detailed status information on Android, so a lot of the validation for invalid chains was changed to just check that it was invalid and had no elements.
Skip tests for AIA fetching and offline mode of revocation

This gets ChainTests, DynamicChainTests, and DynamicRevocationTests (outerloop) working. There is one failure in CertificateRequestChainTests:

CreateChain_RSAPSS - fails to build any chain (zero-element, PartialChain)

Platform not supported:

Ignoring verification that we can't bypass on Android: AllowUnknownCertificateAuthority, IgnoreInvalidName, IgnoreInvalidPolicy, IgnoreTimeNotValid
Using a custom trust store with no (self-issued) certs
Revocation check in Offline mode
- Treated as Online
Revocation check with EndCertificateOnly when newer Android APIs supporting it aren't available
- Treated as ExcludeRoot
Revocation check with EntireChain
- Treated as ExcludeRoot
AIA fetching: https://docs.oracle.com/javase/8/docs/technotes/guides/security/certpath/CertPathProgGuide.html#AIA
- This is disabled by default and requires setting a system property to enable, which we can't really do properly
- Would probably require doing the manual downloading like what the Unix/OpenSSL implementation does

cc @jkoritzinsky @steveisok @AaronRobinsonMSFT @bartonjs

TODO: - set custom trust root - handle revocation checking - propagate errors/status where possible

ghost · 2021-03-12T08:36:32Z

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

There isn't a way to get rich status information (like what .NET APIs expose) or ignore certain validation around chain building on Android. In order to build a chain at all through the APIs available on Android, there must be a path with a valid time, signature, name constraints, policy constraints, and trusted root.

Attempt to build the chain without extra validation (e.g. revocation checking) through CertPathBuilder
- Per above, there is still a lot of validation that cannot be bypassed. if any of those can't be satisfied, building the chain simply fails with an exception - without anything more detailed than that a valid chain could not be constructed.
- In these failure cases, X509Chain.ChainElements is empty and X509Chain.ChainStatus has PartialChain with whatever the exception message was.
- If any of the verification flags for ignoring this validation is set (and the chain couldn't be built), throws PlatformNotSupportedException
Do a validation pass over the built chain through CertPathValidator
Propagate exception message from Android cert path building APIs as chain status items
- If we have an index (failure was in additional validation after the path was built), associate it with the corresponding chain element. Otherwise, just add it to the overall status.
- If we have a failure reason (failure was in additional validation after the path was built, API level is 24+), map that to a corresponding status. Otherwise, report PartialChain.

Tests:

A bunch of the tests relied on AllowUnknownCertificateAuthority to validate other scenarios. On Android, you can't build a chain at all in that case.
- For those tests, I made them use a custom trust store with a self-issued cert on Android so that they could build a chain and do whatever other validation the test was targeting.
We don't have the detailed status information on Android, so a lot of the validation for invalid chains was changed to just check that it was invalid and had no elements.

This gets almost all the ChainTests, DynamicChainTests, and CertificateRequestChainTests working. There are still 7 failures in those where we expect to build a valid chain, but it is invalid on Android (some are related to AIA - see below).

The DynamicRevocationTests (outerloop) are still mostly failing. For all the cases with OCSP, we end up with unknown revocation status due to an exception trying to connect to the specified URI. The tests with just CRL seem to work. Some settings are also not implemented yet (see below).

Platform not supported:

Ignoring verification that we can't bypass on Android: AllowUnknownCertificateAuthority, IgnoreInvalidName, IgnoreInvalidPolicy, IgnoreTimeNotValid
Using a custom trust store with no (self-issued) certs
Revocation check with EndCertificateOnly when newer Android APIs supporting it aren't available

Not handled in this change:

Certain revocation settings - planning on doing this in a follow-up
- Checking the entire chain
- Online mode when newer Android APIs aren't available
AIA: https://docs.oracle.com/javase/8/docs/technotes/guides/security/certpath/CertPathProgGuide.html#AIA
- This is disabled by default and requires setting a system property to enable, which we can't really do properly
- Might require doing the manual downloading like what the Unix/OpenSSL implementation does

cc @jkoritzinsky @steveisok @AaronRobinsonMSFT @bartonjs

Author:	elinor-fung
Assignees:	-
Labels:	`area-System.Security`
Milestone:	-

jkoritzinsky

Not familiar enough with the cert side to fully grok if the logic is correct, but generally looks good to me. Just a few small nits from my first review pass.

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.c

src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c

...tem.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Android/ChainPal.cs

Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>

vcsjones · 2021-03-15T21:35:43Z

...tem.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Android/ChainPal.cs

@@ -35,7 +38,319 @@ public static bool ReleaseSafeX509ChainHandle(IntPtr handle)
            TimeSpan timeout,
            bool disableAia)
        {
-            throw new NotImplementedException(nameof(BuildChain));
+            var chainPal = new AndroidCertPath();


It doesn't look like we do anything with the disableAia parameter here. This maps to X509ChainPolicy.DisableCertificateDownloads from the public API.

Does Android have a way to prevent fetching intermediate certificates? If not, should a PNSE be thrown if someone sets it to true? Should we ignore it and document that it does not work in Android?

There is some precedent for ignoring it with macOS. In macOS, this property only works if revocation checking is disabled as well.

It is actually that we don't have a good way to enable fetching. This is part of my 'not handled in this change'. By default, the fetching is disabled and can only be enabled though a system property which gets read and stored once. I don't think we really have a good way of dealing with that - I think we'd have to do something like the Unix/OpenSSl implementation's CertificateAssetDownloader.

Until the need is proven otherwise, I'm OK with saying/believing that Android's use cases don't require AIA fetching.

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.h

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c

src/libraries/System.Security.Cryptography.X509Certificates/tests/DynamicChainTests.cs

src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs

...tem.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Android/ChainPal.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

...tem.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Android/ChainPal.cs

Validation in two passes - with / without revocation for better status

elinor-fung · 2021-03-22T19:15:30Z

Okay. PR description has been updated. With the latest updates, the chain-related tests should be passing except for CreateChain_RSAPSS.

bartonjs · 2021-03-22T19:27:48Z

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c

+        }
+    }
+
+    return PAL_X509ChainPartialChain;


Can we do better than this?

The main thing is that we never return true from chain.Build if we don't know why something failed, so this is sort of OK, but I know I'm about to ask why a PartialChain is popping up and I'm guessing it's this fallback here.

On macOS our details processing comes from weak string processing, and anytime we get an unknown value we throw an exception (since we don't know if unknown means fatal or innocuous).

A big source of PartialChain without any details would be from the initial build (instead of the second-pass validation). Unfortunately, all seem to come as a CertPathBuilderException with a generic message (unable to find valid certification path to requested target) and no wrapped exception, so I don't think there's any better we can do in that case.

In the case of the CertPathValidatorException without a reason (not set or on a version where it doesn't exist), I think we could do something like walk through any wrapped exceptions and check for specific exception types (e.g. the subclasses of CertificateException) to determine a better status.

Testing on a version that has the reason API available, I was seeing errors with it set such that we could map them to a status, so I think it is on older versions where trying additional metrics would be helpful. I'll add it as an item to #45741.

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c

...ystem.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs

src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs

...ystem.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs

bartonjs · 2021-03-22T22:06:11Z

Alrighty, I think we're good on the product code, but a few bits of evolutionary cleanup remain in the test code. Almost done!

elinor-fung · 2021-03-23T21:24:13Z

/backport to release/6.0-preview3

github-actions · 2021-03-23T21:25:09Z

Started backporting to release/6.0-preview3: https://github.com/dotnet/runtime/actions/runs/681018345

github-actions · 2021-03-23T21:27:22Z

@elinor-fung backporting to release/6.0-preview3 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Basic chain building
Using index info to reconstruct a base tree...
M	src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/CMakeLists.txt
M	src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.c
M	src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.h
M	src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.c
M	src/libraries/System.Security.Cryptography.X509Certificates/src/Resources/Strings.resx
M	src/libraries/System.Security.Cryptography.X509Certificates/src/System.Security.Cryptography.X509Certificates.csproj
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Security.Cryptography.X509Certificates/src/System.Security.Cryptography.X509Certificates.csproj
CONFLICT (content): Merge conflict in src/libraries/System.Security.Cryptography.X509Certificates/src/System.Security.Cryptography.X509Certificates.csproj
Auto-merging src/libraries/System.Security.Cryptography.X509Certificates/src/Resources/Strings.resx
Auto-merging src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.c
Auto-merging src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.h
CONFLICT (content): Merge conflict in src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.h
Auto-merging src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.c
CONFLICT (content): Merge conflict in src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.c
Auto-merging src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/CMakeLists.txt
CONFLICT (content): Merge conflict in src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/CMakeLists.txt
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Basic chain building
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

elinor-fung added 5 commits March 11, 2021 23:37

Basic chain building

28794a2

TODO: - set custom trust root - handle revocation checking - propagate errors/status where possible

Propagate error messages and set custom trust root certs

10c92d9

Share x509 enums

16751c4

Map failure reason to chain status when possible

ddb048d

Update tests to reflect limited support on Android

14ac707

dotnet-issue-labeler bot added the area-System.Security label Mar 12, 2021

elinor-fung added the os-android label Mar 12, 2021

elinor-fung added this to the 6.0.0 milestone Mar 12, 2021

Handle revocation when newer APIs are available

aa47315

elinor-fung force-pushed the chainPal-android branch from 9f84029 to aa47315 Compare March 12, 2021 08:46

jkoritzinsky requested a review from vcsjones March 12, 2021 22:30

jkoritzinsky reviewed Mar 12, 2021

View reviewed changes

elinor-fung commented Mar 12, 2021

View reviewed changes

...tem.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Android/ChainPal.cs Outdated Show resolved Hide resolved

marek-safar requested a review from bartonjs March 13, 2021 07:41

elinor-fung and others added 2 commits March 15, 2021 11:37

Apply suggestions from code review

ff5f4bd

Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>

Move INIT_LOCALS, RELEASE_LOCALS, RELEASE_LOCALS_ENV macros to pal_jni

ecb6936

vcsjones reviewed Mar 15, 2021

View reviewed changes

This was referenced Mar 16, 2021

Mono System.Text.Json.Tests on Windows timing out #42677

Open

ClientAndServer_OneOrBothUseDefault_Ok - fails with NullReferenceException #2353

Closed

Safe handle use after close in System.Net.Security.Tests #46770

Closed

bartonjs reviewed Mar 17, 2021

View reviewed changes

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.h Show resolved Hide resolved

bartonjs reviewed Mar 17, 2021

View reviewed changes

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c Show resolved Hide resolved

bartonjs reviewed Mar 17, 2021

View reviewed changes

src/libraries/System.Security.Cryptography.X509Certificates/tests/DynamicChainTests.cs Outdated Show resolved Hide resolved

bartonjs reviewed Mar 17, 2021

View reviewed changes

src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs Outdated Show resolved Hide resolved

bartonjs reviewed Mar 17, 2021

View reviewed changes

src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs Outdated Show resolved Hide resolved

bartonjs reviewed Mar 17, 2021

View reviewed changes

...tem.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Android/ChainPal.cs Outdated Show resolved Hide resolved

elinor-fung and others added 2 commits March 17, 2021 16:23

Apply suggestions from code review

3191130

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

Merge branch 'main' into chainPal-android

1885f8e

bartonjs reviewed Mar 18, 2021

View reviewed changes

...tem.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Android/ChainPal.cs Outdated Show resolved Hide resolved

elinor-fung added 2 commits March 22, 2021 10:55

Handle EndCertificateOnly revocation check with root-only chain

9532108

Validation in two passes - with / without revocation for better status

Update DynamicRevocationTests for Android behaviour

7d9f79b