Handle more basic constraints chain statuses. #35347

vcsjones · 2020-04-23T15:45:35Z

macOS can return additional chain status strings for a certificate that
was issued by a certificate that violated its basic constraints.

If a leaf certificate is issued from a certificate with CA:FALSE,
the strings BasicConstraintsCA and BasicConstraintsPathLen are
reported. We map these the same for BasicConstraints.

Relates to #35238.

macOS can return additional chain status strings for a certificate that was issued by a certificate that violated its basic constraints. If a leaf certificate is issued from a certificate with CA:FALSE, the strings BasicConstraintsCA and BasicConstraintsPathLen are reported. We map these the same for BasicConstraints.

ghost · 2020-04-23T15:45:40Z

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
Notify danmosemsft if you want to be subscribed.

bartonjs · 2020-04-23T15:58:42Z

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

        *pStatus |= PAL_X509ChainUntrustedRoot;
-    else if (CFEqual(keyString, CFSTR("BasicConstraints")))
+    else if (CFEqual(keyString, CFSTR("BasicConstraints")) || CFEqual(keyString, CFSTR("BasicConstraintsCA")) ||
+             CFEqual(keyString, CFSTR("BasicConstraintsPathLen")))


Presumably this means that we need more tests for this status code. Can you add them in DynamicChainTests along with this PR?

vcsjones · 2020-04-23T17:04:20Z

@bartonjs done. Confirmed that without the fix, test fails with:

[stuff elided]
ib/src/System/Runtime/CompilerServices/AsyncMethodBuilderCore.cs:line 42
     at Xunit.Sdk.ExceptionAggregator.RunAsync[T](Func`1 code)
     at Xunit.Sdk.TestInvoker`1.RunAsync() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\Runners\TestInvoker.cs:line 189
     at Xunit.Sdk.XunitTestRunner.InvokeTestMethodAsync(ExceptionAggregator aggregator) in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\Runners\XunitTestRunner.cs:line 84
  Unknown Chain Status: BasicConstraintsCA
     at Xunit.Sdk.XunitTestRunner.InvokeTestAsync(ExceptionAggregator aggregator) in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\Runners\XunitTestRunner.cs:line 67
     at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine) in /Users/kjones/Projects/runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncMethodBuilderCore.cs:line 42
[stuff elided]

vcsjones · 2020-04-23T17:36:19Z

@bartonjs once this is green / merged, is this backport worthy? If so I can prepare PRs for current LTS release branches.

(My current feeling is "no", but, couldn't hurt to ask)

vcsjones · 2020-04-23T21:33:41Z

For what it's worth, I saw the test fail on 10.15.4. I don't know if this test passes for the right reason in CI if it is using an older version of macOS. It would appear that at some point a "BasicConstraints" status was split out to "BasicConstraintsCA" and "BasicConstraintsPathLen" on macOS. So perhaps that test I wrote is just exercising the "BasicConstraints" path and would pass anyway in CI.

bartonjs · 2020-04-23T22:22:50Z

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

+        {
+            byte[] serialNumber = new byte[9];
+            RandomNumberGenerator.Fill(serialNumber);
+            serialNumber[0] &= 0b01111111;


99% sure CertificateRequest handles this already, by inserting an 0x00 if the high bit was set.

bartonjs · 2020-04-23T22:24:57Z

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

+                        }
+                    }
+
+                    Assert.True(verifiedIntermediate, "verify leaf certificate");


Suggested change

Assert.True(verifiedIntermediate, "verify leaf certificate");

Assert.True(verifiedIntermediate, "Verified intermediate certificate");

bartonjs · 2020-04-23T22:28:00Z

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

+                    }
+
+                    Assert.True(verifiedIntermediate, "verify leaf certificate");
+                }


Do we already have a test for pathLen exceeded? I feel like the easy overload on CertificateRequest will check that and block it, but if you use the X509SignatureGenerator version (where it can't see the parent cert) then it obviously won't.

(Really the question here is is there an extra extra error that appears for pathlen exceeded)

Really the question here is is there an extra extra error that appears for pathlen exceeded

Yeah, It's reported as BasicConstraintsPathLen which this test handled both since Apple reports both in this case (It's a CA:FALSE and has a path length violation) - I will split this in to two separate tests.

X509SignatureGenerator

Oh that's much better than the weird intermediate swap-out I did. I completely missed Create.

bartonjs · 2020-04-23T22:32:13Z

is this backport worthy?

Yeah, the API claims it handles a bad basic constraints extension; it throwing "something went wrong" when it happens is bad UX... and is presumably caused by an OS update somewhere along the way... so it's probably something we should take.

Test CA:FALSE and path length violations independently.

bartonjs · 2020-04-24T15:17:51Z

Looks like Linux isn't reporting the invalid constraints on pathLen 0 test. Either we (I) mis-mapped a code somewhere or OpenSSL thinks that CA + pathLen 0 is "obviously they meant null"; and it needs to be

Root: CA, PathLen 1
Intermediate: CA, PathLen null
Leaf

Maybe?

Or maybe there's some other nuance, like it only bothers reporting it for trusted chains (the tests could use the new contextual roots feature if that's the case).

vcsjones · 2020-04-24T16:09:54Z

@bartonjs I think the issue is that it is always reporting PartialChain. I fixed linux using the CustomTrustStore, but now I broke OSX. Working on trying to make everyone happy.

vcsjones · 2020-04-24T17:33:09Z

@bartonjs

The two intermediates, canonically, should still be in ExtraStore.

That makes way more sense, I misunderstood how it works from a test. I assume you meant "extra instead of custom" since the intermediates should not at all be considered trust anchors?

bartonjs · 2020-04-24T19:06:25Z

I assume you meant "extra instead of custom" since the intermediates should not at all be considered trust anchors?

Yep. All of our underlying engines support doing arbitrary anchors (e.g. pin to one of these issuing CA), but we chose not to support it with the initial custom trust store. We could at some point add things like PinnedIssuer (directly issued by something in the store) or AnyAncestor (like CustomRoot but allows intermediates to function as anchors); which merely changes the way that we interpret the custom collection. So it's just nice to represent the intent correctly :)

vcsjones · 2020-04-24T20:24:01Z

:-( Looking at Windows... path length seems to be excessively platform-specific behavior...

vcsjones · 2020-04-25T19:41:35Z

@bartonjs the path length case has a few interesting behaviors from platform to platform, but I think I've found a case where they all agree. The current fails appear unrelated.

bartonjs · 2020-04-27T19:45:03Z

I agree the failure looks unrelated, but trying one more re-run just to feel better about myself. (I don't like when the failure is in a downstream component... even though the only source change is in macOS and the test failure was in Windows)

vcsjones · 2020-04-27T19:58:14Z

is this backport worthy?

Yeah

I've managed to shake out a few more of these... (#35463, #35492, #35533) and might find a few more. Should I hold off opening a PR to the LTS branches in case you think any of these should go in to the same PR as well? (35492 seems to be a similar criteria as this one)

bartonjs · 2020-04-27T21:13:26Z

Yeah, we should try to get a bunch into one change as simply improving reliability around those cases. Nice work on burning down the delta, BTW.

vcsjones · 2020-04-27T21:19:48Z

@bartonjs Just so I'm clear.. do you want them all in this PR (I already opened #35488 separately) or can we make multiple PRs in to master, and when porting to release/3.1,2.1 we can cherry-pick each change in master in to that one PR?

bartonjs · 2020-04-27T21:21:11Z

Multiple PRs in master with one PR to each of 3.1 and 2.1 is what I had in mind.

Dotnet-GitSync-Bot added the area-System.Security label Apr 23, 2020

bartonjs reviewed Apr 23, 2020

View reviewed changes

Add test for basic constraints.

2e90a06

bartonjs reviewed Apr 23, 2020

View reviewed changes

Improve tests.

1b818dc

Test CA:FALSE and path length violations independently.

vcsjones added 2 commits April 24, 2020 13:00

Try to make everyone happy

07ed58c

Add intermediates to extra store

aa22085

Windows should be happy, too

133b142

vcsjones mentioned this pull request Apr 25, 2020

BlackListedLeaf is not handled by X509 PAL on macOS #35463

Closed

jaredpar mentioned this pull request Apr 27, 2020

Test timeline issue jaredpar/runfo#8

Closed

bartonjs approved these changes Apr 27, 2020

View reviewed changes

bartonjs merged commit 92cd5cd into dotnet:master Apr 27, 2020

vcsjones deleted the macos-chainstatus branch April 27, 2020 21:27

jaredpar mentioned this pull request Apr 27, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

bartonjs mentioned this pull request Apr 28, 2020

Connecting to server using SSL certificate in MacOS fails with code that success on Windows #35238

Closed

This was referenced Apr 28, 2020

[release/3.1] Handle additional chain statuses for X509Chain on macOS dotnet/corefx#42914

Merged

[release/2.1] Handle additional chain statuses for X509Chain on macOS dotnet/corefx#42916

Closed

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

	Assert.True(verifiedIntermediate, "verify leaf certificate");
	Assert.True(verifiedIntermediate, "Verified intermediate certificate");

Handle more basic constraints chain statuses. #35347

Handle more basic constraints chain statuses. #35347

Uh oh!

Conversation

vcsjones commented Apr 23, 2020

Uh oh!

ghost commented Apr 23, 2020

Uh oh!

bartonjs Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

vcsjones commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vcsjones commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vcsjones commented Apr 23, 2020

Uh oh!

bartonjs Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

bartonjs Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

bartonjs Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

bartonjs Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

vcsjones Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

bartonjs commented Apr 23, 2020

Uh oh!

bartonjs commented Apr 24, 2020

Uh oh!

vcsjones commented Apr 24, 2020

Uh oh!

vcsjones commented Apr 24, 2020

Uh oh!

bartonjs commented Apr 24, 2020

Uh oh!

vcsjones commented Apr 24, 2020

Uh oh!

vcsjones commented Apr 25, 2020

Uh oh!

bartonjs commented Apr 27, 2020

Uh oh!

vcsjones commented Apr 27, 2020

Uh oh!

bartonjs commented Apr 27, 2020

Uh oh!

vcsjones commented Apr 27, 2020

Uh oh!

bartonjs commented Apr 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vcsjones commented Apr 23, 2020 •

edited

Loading

vcsjones commented Apr 23, 2020 •

edited

Loading