Proper fix for compilation issue caused by deprecated API in Mojave #30815

maryamariyan · 2018-07-03T16:50:04Z

Proper fix for compilation issue caused by deprecated API in macOS Mojave by

using dlsym to call available API rather than suppressing deprecation warnings.

Fixes: #30599

…jave by using dlsym to call available API rather than suppressing deprecation warnings. Fixes: #30599

maryamariyan · 2018-07-03T16:52:31Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_x509.c

+    if (!checked)
+    {
+        secCertificateCopyKey = (SecKeyRef (*)(SecCertificateRef))dlsym(RTLD_DEFAULT, "SecCertificateCopyKey");
+        secCertificateCopyPublicKey = (OSStatus (*)(SecCertificateRef, SecKeyRef*))dlsym(RTLD_DEFAULT, "SecCertificateCopyPublicKey");


@janvorli is there a way we could call dlsym only once in this code snippet to improve the diff?

From @bartonjs on Jul 3, 2018, 8:35 AM PDT

In C# I'd do this as Volatile.Write(ref checked, 1), or declare checked as volatile. Is declaring checked volatile here the right way to prevent a reordering race condition, @janvorli?

maryamariyan#25 (comment)

From @bartonjs on Jul 3, 2018, 8:37 AM PDT

(Or is my static local-fu weak, and we don't need to do a lazy initializer manually, because static locals guarantee that they only run their initializer once? (and therefore the two dlsym calls should move up to variable initializers))

maryamariyan#25 (comment)

@jkotas will also know for sure

Right, this code is wrong. It can be broken even on x64, depending on the compiler optimizations.

The other places in the corefx shims use volatile, double-checked locking and pthread_mutex_lock (look for pthread_mutex_lock under src/native for examples). You can use the same pattern.

Actually, I would just use pthread_once here. It does exactly what you need here.

@jkotas, in my case here, I would need to call either of SecCertificateCopyKey or SecCertificateCopyPublicKey once, and both have arguments and neither returns void.

While looking at the doc here, it seems that init_routine() which is the second argument passed to pthread_once, apparently should have no arguments and returns void.

Please let me know if I misunderstood, but I think because of this observation I don't think I can use pthread_once for my case here.

I just pushed up a commit using the alternative pthread_mutex_lock pattern you recommended as seen in other places in corefx.

jkotas · 2018-07-05T11:42:30Z

I think because of this observation I don't think I can use pthread_once for my case here.

I think it would work just fine. I do not think you need any arguments for the init routine. The init routine would be just:

static void InitCertificateCopy()
{
    secCertificateCopyKey = (SecKeyRef (*)(SecCertificateRef))dlsym(RTLD_DEFAULT, "SecCertificateCopyKey");
    secCertificateCopyPublicKey = (OSStatus (*)(SecCertificateRef, SecKeyRef*))dlsym(RTLD_DEFAULT, "SecCertificateCopyPublicKey");
}

And you would not need the checked variable at all.

jkotas · 2018-07-05T11:43:47Z

The implementation with mutex is functionally correct, but it is slower than it needs to be. Implementation using pthread_once is going to be faster.

…entation

jkotas

LGTM otherwise

jkotas · 2018-07-05T12:55:15Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_x509.c

+    }
+    else
+    {
+        assert(secCertificateCopyPublicKey != NULL);


This can be null when things go wrong. It may be better to return error instead of asserting.

maryamariyan · 2018-07-05T19:35:10Z

@dotnet-bot test Windows x64 Debug Build please

maryamariyan · 2018-07-05T19:35:50Z

@dotnet-bot test Packaging All Configurations x64 Debug Build please

maryamariyan · 2018-07-05T19:36:29Z

@dotnet-bot test OSX x64 Debug Build please

danmoseley · 2018-07-05T20:23:26Z

Any other feedback @jkotas?

jkotas · 2018-07-05T20:29:26Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_x509.c

+    }
+    else
+    {
+        return kErrorBadInput;


This is not really capturing the error (there is nothing wrong with the input), but I see we are ignoring the error code on the managed side in release builds anyway, so it does not really matter which error code we are going to return here. It may be worth a comment.

It's not completely ignored, but anything that isn't 1 or 0 is all a generic CryptographicException.

Here are the other options:

corefx/src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_seckey.h

Lines 12 to 19 in 9908dee

// Unless another interpretation is "obvious", pal_seckey functions return 1 on success.

// functions which represent a boolean return 0 on "successful false"

// otherwise functions will return one of the following return values:

static const int32_t kErrorBadInput = -1;

static const int32_t kErrorSeeError = -2;

static const int32_t kErrorUnknownAlgorithm = -3;

static const int32_t kErrorUnknownState = -4;

"Unknown state" certainly sounds like what we'd be in if neither method exists.

maryamariyan · 2018-07-09T20:39:04Z

@dotnet-bot test Packaging All Configurations x64 Debug Build please

maryamariyan · 2018-07-09T21:44:06Z

@dotnet-bot test OSX x64 Debug Build please

filipnavara · 2018-07-16T15:32:59Z

This broke a plenty of unit tests on my macOS Mojave DP2 and DP3 machines. I reverted commit 92b4826 and the unit test failures disappeared. I guess we need to revisit this.

maryamariyan · 2018-07-16T15:37:30Z

what version macOS Mojave are you using? which tests are failing for you?

filipnavara · 2018-07-16T15:42:36Z

About 8+ tests in System.Security.Cryptography.X509Certificates. It's actually hard to tell because there're some failure that cause assert to be hit before the tests finish:

  Discovering: System.Security.Cryptography.X509Certificates.Tests
  Discovered:  System.Security.Cryptography.X509Certificates.Tests
  Starting:    System.Security.Cryptography.X509Certificates.Tests
  Warning: unable to build chain to self-signed root for signer "(null)"   System.Security.Cryptography.X509Certificates.Tests.PublicKeyTests.TestECDsaPublicKey_BrainpoolP160r1_ValidatesSignature(curveData: [45, 45, 45, 45, 45, ...], existingSignature: [145, 69, 199, 157, 212, ...]) [FAIL]
        System.ArgumentException : Cannot open an invalid handle.
        Parameter name: publicKey
        Stack Trace:
  Assertion Failed
  Unknown keysize (0)
  
     at System.Security.Cryptography.EccSecurityTransforms.GetKeySize(SecKeyPair newKeys) in /Users/filipnavara/Documents/corefx/src/Common/src/System/Security/Cryptography/EccSecurityTransforms.cs:line 175
     at System.Security.Cryptography.EccSecurityTransforms.SetKeyAndGetSize(SecKeyPair keyPair) in /Users/filipnavara/Documents/corefx/src/Common/src/System/Security/Cryptography/EccSecurityTransforms.cs:line 81
     at System.Security.Cryptography.ECDsaImplementation.ECDsaSecurityTransforms..ctor(SafeSecKeyRefHandle publicKey) in /Users/filipnavara/Documents/corefx/src/Common/src/System/Security/Cryptography/ECDsaSecurityTransforms.cs:line 61
     at Internal.Cryptography.Pal.X509Pal.AppleX509Pal.DecodePublicKey(Oid oid, Byte[] encodedKeyValue, Byte[] encodedParameters, ICertificatePal certificatePal) in /Users/filipnavara/Documents/corefx/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/X509Pal.cs:line 43
     at Internal.Cryptography.Pal.CertificateExtensionsCommon.GetPublicKey[T](X509Certificate2 certificate, Predicate`1 matchesConstraints) in /Users/filipnavara/Documents/corefx/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/CertificateExtensionsCommon.cs:line 34
     at System.Security.Cryptography.X509Certificates.ECDsaCertificateExtensions.GetECDsaPublicKey(X509Certificate2 certificate) in /Users/filipnavara/Documents/corefx/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/ECDsaCertificateExtensions.cs:line 22
     at System.Security.Cryptography.X509Certificates.Tests.PublicKeyTests.TestECDsa224PublicKey() in /Users/filipnavara/Documents/corefx/src/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs:line 456
     at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
     at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) in /Users/buildagent/agent/_work/482/s/src/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs:line 485

Basically all the tests that fail either report this assert or that there's invalid handle related to public key.

filipnavara · 2018-07-16T15:43:30Z

macOS Mojave DP2 or DP3, basically any up-to-date developer preview hits it. Reproduced on two separate machines.

bartonjs · 2018-07-16T15:49:55Z

Sounds like Apple is adding support for ECC other than secp256r1, secp384r1, and secp521r1 now. If forward fixing is sensible that's the option I'd prefer (over going back to "ignore deprecation warnings"), but that might turn out to be a somewhat significant change.

filipnavara · 2018-07-16T15:56:46Z

@bartonjs That's probably not the case. Much likely there are some key copy operations that fail and the error is not checked for. Then hell breaks loose later... I am in process of pinning down the issue, but wanted to give a heads up.

bartonjs · 2018-07-16T16:13:41Z

The callstack says this is in a test for ECDSA-224 (secp224r1).

https://github.com/dotnet/corefx/blob/master/src/Common/src/System/Security/Cryptography/ECDsaSecurityTransforms.cs#L59-L62 called SecKeyPair.PublicOnly(publicKey). That would throw if publicKey was null / representing the null pointer (https://github.com/dotnet/corefx/blob/master/src/Common/src/System/Security/Cryptography/SecKeyPair.cs#L38-L44).

Then, we made it into

corefx/src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ecc.c

Lines 55 to 98 in 9908dee

    
           uint64_t AppleCryptoNative_EccGetKeySizeInBits(SecKeyRef publicKey) 
        
           { 
        
               if (publicKey == NULL) 
        
               { 
        
                   return 0; 
        
               } 
        
               size_t blockSize = SecKeyGetBlockSize(publicKey); 
        
               // This seems to be the expected size of an ECDSA signature for this key. 
        
               // But since Apple uses the DER SEQUENCE(r, s) format the signature size isn't 
        
               // fixed. It might be trying to encode the biggest the DER value could be: 
        
               // 
        
               // 256: r is 32 bytes, but maybe one padding byte, so 33. 
        
               //      s is 32 bytes, but maybe one padding byte, so 33. 
        
               //      each of those values gets one tag and one length byte 
        
               //      35 * 2 is 70 payload bytes for the sequence, so one length byte 
        
               //      and one tag byte, makes 72. 
        
               // 
        
               // 384: r,s are 48 bytes, plus padding, length, and tag: 51 
        
               //      2 * 51 = 102, requires one length byte and one tag byte, 104. 
        
               // 
        
               // 521: neither r nor s can have the high bit set, no padding. 66 content bytes 
        
               //      plus tag and length is 68. 
        
               //      2 * 68 is 136, since it's greater than 127 it takes 2 length bytes 
        
               //      so 136 + 2 + 1 = 139. Looks like they accounted for padding bytes anyways. 
        
               // 
        
               // This completely needs to be revisited if Apple adds support for "generic" ECC. 
        
               // 
        
               // Word of caution: While seeking meaning in these numbers I ran across a snippet of code 
        
               // which suggests that on iOS (vs macOS) they use a different set of reasoning and produce 
        
               // different numbers (they used (8 + 2*thisValue) on iOS for "signature length"). 
        
               switch (blockSize) 
        
               { 
        
                   case 72: 
        
                       return 256; 
        
                   case 104: 
        
                       return 384; 
        
                   case 141: 
        
                       return 521; 
        
               } 
        
               return 0; 
        
           }

.

We know that the pointer isn't NULL, so we skip the first return 0. Then we make it past the switch statement, none of those hit, and we hit the trailing return 0. Why? Apple has never supported secp224r1 before.

If that shim method is made to allow secp224r1 to flow through then now the caller will still assert (that it got back an answer that wasn't 256, 384, or 521), and then maybe many more places will have similar "we don't know what to do here, because there's no obvious path forward from Apple's current design" guards.

If the test passes with this change rolled back, it means that asking for the public key failed, which the test considers acceptable. And that means that Apple has added support for generic ECC (or, at least, "more curves"), but only when using the new function.

filipnavara · 2018-07-16T16:15:59Z

Ok, that sounds like valid reasoning, although it doesn't necessarily explain all the failures I am seeing. Btw, the new values I see in blockSize are 32 (for the stack trace above) and 66.

danmoseley · 2018-07-16T16:27:14Z

@filipnavara I suggest opening a new issue if a follow up change is needed.

filipnavara · 2018-07-16T16:30:47Z

@danmosemsft I will. Just gathering enough details to describe what the issue is... :-)

bartonjs · 2018-07-16T16:34:34Z

32?!. Using their previous formula it would be (((ceil(224 / 8) + 3) * 2) + 2) => 64. Gotta love consistency.

filipnavara · 2018-07-16T16:48:54Z

@bartonjs Eh, or maybe 28... on this machine. And 48 for 384 (TestECDsaPublicKey). And 66 for 521. ~~The values don't really make any sense whatsoever.~~ The values seem to be actual key size in bytes now.

…otnet/corefx#30815) * Proper fix for compilation issue caused by deprecated API in macOS Mojave by using dlsym to call available API rather than suppressing deprecation warnings. Fixes: dotnet/corefx#30599 Commit migrated from dotnet/corefx@92b4826

Proper fix for compilation issue caused by deprecated API in macOS Mo…

948114f

…jave by using dlsym to call available API rather than suppressing deprecation warnings. Fixes: #30599

maryamariyan added the os-mac-os-x OS-X aka Mac OS label Jul 3, 2018

maryamariyan self-assigned this Jul 3, 2018

maryamariyan requested review from bartonjs and janvorli July 3, 2018 16:50

maryamariyan mentioned this pull request Jul 3, 2018

Drafted fix maryamariyan/corefx#25

Closed

maryamariyan commented Jul 3, 2018

View reviewed changes

danmoseley mentioned this pull request Jul 3, 2018

Cherry picking Mojave fix (#30716) to release 2.1 #30744

Merged

using pthread_mutex_t to prevent race condition

62357ac

maryamariyan force-pushed the up-mojave branch from 91a3743 to 62357ac Compare July 5, 2018 11:41

using pthread_once instead of pthread_mutex_lock to get faster implem…

b67bfdf

…entation

jkotas reviewed Jul 5, 2018

View reviewed changes

bartonjs approved these changes Jul 5, 2018

View reviewed changes

maryamariyan force-pushed the up-mojave branch from b4e71b8 to 0f054c9 Compare July 5, 2018 17:04

jkotas approved these changes Jul 5, 2018

View reviewed changes

karelz added this to the 3.0 milestone Jul 8, 2018

karelz added the area-Infrastructure-libraries label Jul 8, 2018

Returns error code rather than asserting

9366919

maryamariyan force-pushed the up-mojave branch from 0f054c9 to 9366919 Compare July 9, 2018 17:33

stephentoub mentioned this pull request Jul 10, 2018

Enable compilation with -Wincompatible-pointer-types-discards-qualifiers for native code. #30890

Merged

maryamariyan merged commit 92b4826 into dotnet:master Jul 10, 2018

maryamariyan deleted the up-mojave branch July 10, 2018 16:11

filipnavara mentioned this pull request Jan 31, 2020

System.Security.Cryptography.X509Certificates tests fail on macOS Mojave dotnet/runtime#26836

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper fix for compilation issue caused by deprecated API in Mojave #30815

Proper fix for compilation issue caused by deprecated API in Mojave #30815

maryamariyan commented Jul 3, 2018

maryamariyan Jul 3, 2018 •

edited

Loading

maryamariyan Jul 3, 2018

maryamariyan Jul 3, 2018

danmoseley Jul 3, 2018

jkotas Jul 3, 2018

jkotas Jul 3, 2018

maryamariyan Jul 5, 2018

jkotas commented Jul 5, 2018

jkotas commented Jul 5, 2018

jkotas left a comment

jkotas Jul 5, 2018

maryamariyan commented Jul 5, 2018

maryamariyan commented Jul 5, 2018

maryamariyan commented Jul 5, 2018

danmoseley commented Jul 5, 2018

jkotas Jul 5, 2018

bartonjs Jul 5, 2018

maryamariyan Jul 9, 2018 •

edited

Loading

bartonjs Jul 9, 2018

maryamariyan commented Jul 9, 2018

maryamariyan commented Jul 9, 2018

filipnavara commented Jul 16, 2018

maryamariyan commented Jul 16, 2018

filipnavara commented Jul 16, 2018

filipnavara commented Jul 16, 2018

bartonjs commented Jul 16, 2018

filipnavara commented Jul 16, 2018

bartonjs commented Jul 16, 2018

filipnavara commented Jul 16, 2018

danmoseley commented Jul 16, 2018

filipnavara commented Jul 16, 2018

bartonjs commented Jul 16, 2018

filipnavara commented Jul 16, 2018 •

edited

Loading

	// Unless another interpretation is "obvious", pal_seckey functions return 1 on success.
	// functions which represent a boolean return 0 on "successful false"
	// otherwise functions will return one of the following return values:
	static const int32_t kErrorBadInput = -1;
	static const int32_t kErrorSeeError = -2;
	static const int32_t kErrorUnknownAlgorithm = -3;
	static const int32_t kErrorUnknownState = -4;

Proper fix for compilation issue caused by deprecated API in Mojave #30815

Proper fix for compilation issue caused by deprecated API in Mojave #30815

Conversation

maryamariyan commented Jul 3, 2018

maryamariyan Jul 3, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Jul 5, 2018

jkotas commented Jul 5, 2018

jkotas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryamariyan commented Jul 5, 2018

maryamariyan commented Jul 5, 2018

maryamariyan commented Jul 5, 2018

danmoseley commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryamariyan Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryamariyan commented Jul 9, 2018

maryamariyan commented Jul 9, 2018

filipnavara commented Jul 16, 2018

maryamariyan commented Jul 16, 2018

filipnavara commented Jul 16, 2018

filipnavara commented Jul 16, 2018

bartonjs commented Jul 16, 2018

filipnavara commented Jul 16, 2018

bartonjs commented Jul 16, 2018

filipnavara commented Jul 16, 2018

danmoseley commented Jul 16, 2018

filipnavara commented Jul 16, 2018

bartonjs commented Jul 16, 2018

filipnavara commented Jul 16, 2018 • edited Loading

maryamariyan Jul 3, 2018 •

edited

Loading

maryamariyan Jul 9, 2018 •

edited

Loading

filipnavara commented Jul 16, 2018 •

edited

Loading