Skip to content

Commit 148623a

Browse files
bartonjsstephentoub
bartonjs
authored and
stephentoub
committed
Improve X509Chain handling of NotSignatureValid on Linux (dotnet#35801)
* Native changes for better NotSignatureValid support * clang-format -i the modified files * Tests and managed changes for better supporting NotSignatureValid chains * Chain the WorkingChain ctors * Fix tests on macOS (Classic) Sierra
1 parent b1c9885 commit 148623a

File tree

9 files changed

+651
-38
lines changed

9 files changed

+651
-38
lines changed

‎src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.X509StoreCtx.cs‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,29 @@ internal static void X509StoreCtxCommitToChain(SafeX509StoreCtxHandle ctx)
3333
}
3434
}
3535

36+
[DllImport(Libraries.CryptoNative)]
37+
private static extern int CryptoNative_X509StoreCtxResetForSignatureError(
38+
SafeX509StoreCtxHandle ctx,
39+
out SafeX509StoreHandle newStore);
40+
41+
internal static void X509StoreCtxResetForSignatureError(
42+
SafeX509StoreCtxHandle ctx,
43+
out SafeX509StoreHandle newStore)
44+
{
45+
if (CryptoNative_X509StoreCtxResetForSignatureError(ctx, out newStore) != 1)
46+
{
47+
newStore.Dispose();
48+
newStore = null;
49+
throw CreateOpenSslCryptographicException();
50+
}
51+
52+
if (newStore.IsInvalid)
53+
{
54+
newStore.Dispose();
55+
newStore = null;
56+
}
57+
}
58+
3659
[DllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_X509StoreCtxGetSharedUntrusted")]
3760
private static extern SafeSharedX509StackHandle X509StoreCtxGetSharedUntrusted_private(SafeX509StoreCtxHandle ctx);
3861

‎src/Native/Unix/System.Security.Cryptography.Native/opensslshim.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -523,6 +523,7 @@ void SSL_get0_alpn_selected(const SSL* ssl, const unsigned char** protocol, unsi
523523
FALLBACK_FUNCTION(X509_STORE_CTX_get0_store) \
524524
FALLBACK_FUNCTION(X509_STORE_CTX_get0_untrusted) \
525525
REQUIRED_FUNCTION(X509_STORE_CTX_get1_chain) \
526+
REQUIRED_FUNCTION(X509_STORE_CTX_get1_issuer) \
526527
REQUIRED_FUNCTION(X509_STORE_CTX_init) \
527528
REQUIRED_FUNCTION(X509_STORE_CTX_new) \
528529
REQUIRED_FUNCTION(X509_STORE_CTX_set_flags) \
@@ -906,6 +907,7 @@ FOR_ALL_OPENSSL_FUNCTIONS
906907
#define X509_STORE_CTX_get0_store X509_STORE_CTX_get0_store_ptr
907908
#define X509_STORE_CTX_get0_untrusted X509_STORE_CTX_get0_untrusted_ptr
908909
#define X509_STORE_CTX_get1_chain X509_STORE_CTX_get1_chain_ptr
910+
#define X509_STORE_CTX_get1_issuer X509_STORE_CTX_get1_issuer_ptr
909911
#define X509_STORE_CTX_get_error X509_STORE_CTX_get_error_ptr
910912
#define X509_STORE_CTX_get_error_depth X509_STORE_CTX_get_error_depth_ptr
911913
#define X509_STORE_CTX_init X509_STORE_CTX_init_ptr

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

Lines changed: 144 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,12 @@
44

55
#include "pal_x509.h"
66

7-
#include <stdbool.h>
7+
#include "../Common/pal_safecrt.h"
88
#include <assert.h>
99
#include <dirent.h>
10+
#include <stdbool.h>
1011
#include <string.h>
1112
#include <unistd.h>
12-
#include "../Common/pal_safecrt.h"
1313

1414
c_static_assert(PAL_X509_V_OK == X509_V_OK);
1515
c_static_assert(PAL_X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT);
@@ -521,7 +521,8 @@ X509_STORE* CryptoNative_X509ChainNew(X509Stack* systemTrust, const char* userTr
521521
if (!X509_STORE_add_cert(store, cert))
522522
{
523523
// cert refcount is still 1
524-
if (ERR_get_error() != ERR_PACK(ERR_LIB_X509, X509_F_X509_STORE_ADD_CERT, X509_R_CERT_ALREADY_IN_HASH_TABLE))
524+
if (ERR_get_error() !=
525+
ERR_PACK(ERR_LIB_X509, X509_F_X509_STORE_ADD_CERT, X509_R_CERT_ALREADY_IN_HASH_TABLE))
525526
{
526527
// cert refcount goes to 0
527528
X509_free(cert);
@@ -600,7 +601,6 @@ int32_t CryptoNative_X509StackAddDirectoryStore(X509Stack* stack, char* storePat
600601
// Just clear it all.
601602
ERR_clear_error();
602603
}
603-
604604
}
605605

606606
return clearError;
@@ -672,7 +672,13 @@ int32_t CryptoNative_X509StoreCtxCommitToChain(X509_STORE_CTX* storeCtx)
672672
// For a fully trusted chain this will add the trust root redundantly to the
673673
// untrusted lookup set, but the resulting extra work is small compared to the
674674
// risk of being wrong about promoting trust or losing the chain at this point.
675-
sk_X509_push(untrusted, cur);
675+
if (!sk_X509_push(untrusted, cur))
676+
{
677+
X509err(X509_F_X509_VERIFY_CERT, ERR_R_MALLOC_FAILURE);
678+
X509_free(cur);
679+
sk_X509_pop_free(chain, X509_free);
680+
return 0;
681+
}
676682
}
677683
}
678684

@@ -682,6 +688,126 @@ int32_t CryptoNative_X509StoreCtxCommitToChain(X509_STORE_CTX* storeCtx)
682688
return 1;
683689
}
684690

691+
int32_t CryptoNative_X509StoreCtxResetForSignatureError(X509_STORE_CTX* storeCtx, X509_STORE** newStore)
692+
{
693+
if (storeCtx == NULL || newStore == NULL)
694+
{
695+
return -1;
696+
}
697+
698+
*newStore = NULL;
699+
700+
int errorDepth = X509_STORE_CTX_get_error_depth(storeCtx);
701+
X509Stack* chain = X509_STORE_CTX_get0_chain(storeCtx);
702+
int chainLength = sk_X509_num(chain);
703+
X509_STORE* store = X509_STORE_CTX_get0_store(storeCtx);
704+
705+
// If the signature error was reported at the last element
706+
if (chainLength - 1 == errorDepth)
707+
{
708+
X509* root;
709+
X509* last = sk_X509_value(chain, errorDepth);
710+
711+
// If the last element is in the trust store we need to build a new trust store.
712+
if (X509_STORE_CTX_get1_issuer(&root, storeCtx, last))
713+
{
714+
if (root == last)
715+
{
716+
// We know it's a non-zero refcount after this because last has one, too.
717+
// So go ahead and undo the get1.
718+
X509_free(root);
719+
720+
X509_STORE* tmpNew = X509_STORE_new();
721+
722+
if (tmpNew == NULL)
723+
{
724+
return 0;
725+
}
726+
727+
X509* duplicate = X509_dup(last);
728+
729+
if (duplicate == NULL)
730+
{
731+
X509_STORE_free(tmpNew);
732+
return 0;
733+
}
734+
735+
if (!X509_STORE_add_cert(tmpNew, duplicate))
736+
{
737+
X509_free(duplicate);
738+
X509_STORE_free(tmpNew);
739+
return 0;
740+
}
741+
742+
*newStore = tmpNew;
743+
store = tmpNew;
744+
chainLength--;
745+
}
746+
else
747+
{
748+
// This really shouldn't happen, since if we could have resolved it now
749+
// it should have resolved during chain walk.
750+
//
751+
// But better safe than sorry.
752+
X509_free(root);
753+
}
754+
}
755+
}
756+
757+
X509Stack* untrusted = X509_STORE_CTX_get0_untrusted(storeCtx);
758+
X509* cur;
759+
760+
while ((cur = sk_X509_pop(untrusted)) != NULL)
761+
{
762+
X509_free(cur);
763+
}
764+
765+
for (int i = chainLength - 1; i > 0; --i)
766+
{
767+
cur = sk_X509_value(chain, i);
768+
769+
// errorDepth and lower need to be duplicated to avoid x->valid taint.
770+
if (i <= errorDepth)
771+
{
772+
X509* duplicate = X509_dup(cur);
773+
774+
if (duplicate == NULL)
775+
{
776+
return 0;
777+
}
778+
779+
if (!sk_X509_push(untrusted, duplicate))
780+
{
781+
X509err(X509_F_X509_VERIFY_CERT, ERR_R_MALLOC_FAILURE);
782+
X509_free(duplicate);
783+
return 0;
784+
}
785+
}
786+
else
787+
{
788+
if (sk_X509_push(untrusted, cur))
789+
{
790+
X509_up_ref(cur);
791+
}
792+
else
793+
{
794+
X509err(X509_F_X509_VERIFY_CERT, ERR_R_MALLOC_FAILURE);
795+
return 0;
796+
}
797+
}
798+
}
799+
800+
X509* leafDup = X509_dup(X509_STORE_CTX_get0_cert(storeCtx));
801+
802+
if (leafDup == NULL)
803+
{
804+
return 0;
805+
}
806+
807+
X509_STORE_CTX_cleanup(storeCtx);
808+
return CryptoNative_X509StoreCtxInit(storeCtx, store, leafDup, untrusted);
809+
}
810+
685811
static char* BuildOcspCacheFilename(char* cachePath, X509* subject)
686812
{
687813
assert(cachePath != NULL);
@@ -697,7 +823,8 @@ static char* BuildOcspCacheFilename(char* cachePath, X509* subject)
697823
unsigned long issuerHash = X509_issuer_name_hash(subject);
698824
unsigned long subjectHash = X509_subject_name_hash(subject);
699825

700-
size_t written = (size_t)snprintf(fullPath, allocSize, "%s/%08lx.%08lx.ocsp", cachePath, issuerHash, subjectHash);
826+
size_t written =
827+
(size_t)snprintf(fullPath, allocSize, "%s/%08lx.%08lx.ocsp", cachePath, issuerHash, subjectHash);
701828
assert(written == allocSize - 1);
702829
(void)written;
703830

@@ -719,14 +846,13 @@ static OCSP_CERTID* MakeCertId(X509* subject, X509* issuer)
719846
return OCSP_cert_to_id(EVP_sha1(), subject, issuer);
720847
}
721848

722-
static X509VerifyStatusCode CheckOcsp(
723-
OCSP_REQUEST* req,
724-
OCSP_RESPONSE* resp,
725-
X509* subject,
726-
X509* issuer,
727-
X509_STORE_CTX* storeCtx,
728-
ASN1_GENERALIZEDTIME** thisUpdate,
729-
ASN1_GENERALIZEDTIME** nextUpdate)
849+
static X509VerifyStatusCode CheckOcsp(OCSP_REQUEST* req,
850+
OCSP_RESPONSE* resp,
851+
X509* subject,
852+
X509* issuer,
853+
X509_STORE_CTX* storeCtx,
854+
ASN1_GENERALIZEDTIME** thisUpdate,
855+
ASN1_GENERALIZEDTIME** nextUpdate)
730856
{
731857
if (thisUpdate != NULL)
732858
{
@@ -887,9 +1013,7 @@ X509VerifyStatusCode CryptoNative_X509ChainGetCachedOcspStatus(X509_STORE_CTX* s
8871013
// if thisUpdate < oldest || nextUpdate < now, reject.
8881014
//
8891015
// Since X509_cmp(_current)_time returns 0 on error, do a <= 0 check.
890-
if (nextUpdate == NULL ||
891-
thisUpdate == NULL ||
892-
X509_cmp_current_time(nextUpdate) <= 0 ||
1016+
if (nextUpdate == NULL || thisUpdate == NULL || X509_cmp_current_time(nextUpdate) <= 0 ||
8931017
X509_cmp_time(thisUpdate, &oldest) <= 0)
8941018
{
8951019
ret = PAL_X509_V_ERR_UNABLE_TO_GET_CRL;
@@ -971,11 +1095,8 @@ OCSP_REQUEST* CryptoNative_X509ChainBuildOcspRequest(X509_STORE_CTX* storeCtx)
9711095
return req;
9721096
}
9731097

974-
X509VerifyStatusCode CryptoNative_X509ChainVerifyOcsp(
975-
X509_STORE_CTX* storeCtx,
976-
OCSP_REQUEST* req,
977-
OCSP_RESPONSE* resp,
978-
char* cachePath)
1098+
X509VerifyStatusCode
1099+
CryptoNative_X509ChainVerifyOcsp(X509_STORE_CTX* storeCtx, OCSP_REQUEST* req, OCSP_RESPONSE* resp, char* cachePath)
9791100
{
9801101
if (storeCtx == NULL || req == NULL || resp == NULL)
9811102
{
@@ -1018,9 +1139,7 @@ X509VerifyStatusCode CryptoNative_X509ChainVerifyOcsp(
10181139

10191140
// If the response is within our caching policy (which requires a nextUpdate value)
10201141
// then try to cache it.
1021-
if (nextUpdate != NULL &&
1022-
thisUpdate != NULL &&
1023-
X509_cmp_time(thisUpdate, &oldest) > 0)
1142+
if (nextUpdate != NULL && thisUpdate != NULL && X509_cmp_time(thisUpdate, &oldest) > 0)
10241143
{
10251144
char* fullPath = BuildOcspCacheFilename(cachePath, subject);
10261145

‎src/Native/Unix/System.Security.Cryptography.Native/pal_x509.h‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,14 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33
// See the LICENSE file in the project root for more information.
44

5-
#include "pal_crypto_types.h"
6-
#include "pal_compiler.h"
75
#include "opensslshim.h"
6+
#include "pal_compiler.h"
7+
#include "pal_crypto_types.h"
88

99
/*
1010
These values should be kept in sync with System.Security.Cryptography.X509Certificates.X509RevocationFlag.
1111
*/
12-
typedef enum
13-
{
12+
typedef enum {
1413
EndCertificateOnly = 0,
1514
EntireChain = 1,
1615
ExcludeRoot = 2,
@@ -21,8 +20,7 @@ The error codes used when verifying X509 certificate chains.
2120
2221
These values should be kept in sync with Interop.Crypto.X509VerifyStatusCode.
2322
*/
24-
typedef enum
25-
{
23+
typedef enum {
2624
PAL_X509_V_OK = 0,
2725
PAL_X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT = 2,
2826
PAL_X509_V_ERR_UNABLE_TO_GET_CRL = 3,
@@ -233,7 +231,10 @@ DLLEXPORT void CryptoNative_X509StoreCtxDestroy(X509_STORE_CTX* v);
233231
/*
234232
Shims the X509_STORE_CTX_init method.
235233
*/
236-
DLLEXPORT int32_t CryptoNative_X509StoreCtxInit(X509_STORE_CTX* ctx, X509_STORE* store, X509* x509, X509Stack* extraStore);
234+
DLLEXPORT int32_t CryptoNative_X509StoreCtxInit(X509_STORE_CTX* ctx,
235+
X509_STORE* store,
236+
X509* x509,
237+
X509Stack* extraStore);
237238

238239
/*
239240
Shims the X509_verify_cert method.
@@ -357,6 +358,14 @@ the current chain to make chain builds after Reset faster.
357358
*/
358359
DLLEXPORT int32_t CryptoNative_X509StoreCtxCommitToChain(X509_STORE_CTX* storeCtx);
359360

361+
/*
362+
Duplicates any certificate at or below the level where the error marker is.
363+
364+
Outputs a new store with a clone of the root, if necessary.
365+
The new store does not have any properties set other than the trust. (Mainly, CRLs are lost)
366+
*/
367+
DLLEXPORT int32_t CryptoNative_X509StoreCtxResetForSignatureError(X509_STORE_CTX* storeCtx, X509_STORE** newStore);
368+
360369
/*
361370
Look for a cached OCSP response appropriate to the end-entity certificate using the issuer as
362371
determined by the chain in storeCtx.
@@ -373,4 +382,7 @@ DLLEXPORT OCSP_REQUEST* CryptoNative_X509ChainBuildOcspRequest(X509_STORE_CTX* s
373382
Determine if the OCSP response is acceptable, and if acceptable report the status and
374383
cache the result (if appropriate)
375384
*/
376-
DLLEXPORT X509VerifyStatusCode CryptoNative_X509ChainVerifyOcsp(X509_STORE_CTX* storeCtx, OCSP_REQUEST* req, OCSP_RESPONSE* resp, char* cachePath);
385+
DLLEXPORT X509VerifyStatusCode CryptoNative_X509ChainVerifyOcsp(X509_STORE_CTX* storeCtx,
386+
OCSP_REQUEST* req,
387+
OCSP_RESPONSE* resp,
388+
char* cachePath);

0 commit comments

Comments
 (0)