Skip to content

Commit 3331001

Browse files
embhornembhorn
committed
Fix wolfSSL_sk_X509_OBJECT_deep_copy to check bounds
1 parent 178e10e commit 3331001

File tree

3 files changed

+209
-7
lines changed

3 files changed

+209
-7
lines changed

‎src/x509.c‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11703,23 +11703,38 @@ WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* wolfSSL_sk_X509_OBJECT_deep_copy(
1170311703
}
1170411704

1170511705
if (ret == WOLFSSL_SUCCESS) {
11706-
#if defined(OPENSSL_ALL)
11707-
int idx;
11708-
#endif
11709-
1171011706
cert->version = req->version;
1171111707
cert->isCA = req->isCa;
1171211708
cert->basicConstSet = req->basicConstSet;
1171311709
#ifdef WOLFSSL_CERT_EXT
1171411710
if (req->subjKeyIdSz != 0) {
11715-
XMEMCPY(cert->skid, req->subjKeyId, req->subjKeyIdSz);
11716-
cert->skidSz = (int)req->subjKeyIdSz;
11711+
if (req->subjKeyIdSz > CTC_MAX_SKID_SIZE) {
11712+
WOLFSSL_MSG("Subject Key ID too large");
11713+
WOLFSSL_ERROR_VERBOSE(BUFFER_E);
11714+
ret = WOLFSSL_FAILURE;
11715+
}
11716+
else if (req->subjKeyId == NULL) {
11717+
WOLFSSL_MSG("Subject Key ID missing");
11718+
WOLFSSL_ERROR_VERBOSE(BAD_FUNC_ARG);
11719+
ret = WOLFSSL_FAILURE;
11720+
}
11721+
else {
11722+
XMEMCPY(cert->skid, req->subjKeyId,
11723+
req->subjKeyIdSz);
11724+
cert->skidSz = (int)req->subjKeyIdSz;
11725+
}
1171711726
}
1171811727
if (req->keyUsageSet)
1171911728
cert->keyUsage = req->keyUsage;
1172011729

1172111730
cert->extKeyUsage = req->extKeyUsage;
1172211731
#endif
11732+
}
11733+
11734+
if (ret == WOLFSSL_SUCCESS) {
11735+
#if defined(OPENSSL_ALL)
11736+
int idx;
11737+
#endif
1172311738

1172411739
XMEMCPY(cert->challengePw, req->challengePw, CTC_NAME_SIZE);
1172511740
cert->challengePwPrintableString = req->challengePw[0] != 0;

‎tests/api/test_x509.c‎

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -878,3 +878,186 @@ int test_x509_CertFromX509_akid_overflow(void)
878878
#endif
879879
return EXPECT_RESULT();
880880
}
881+
882+
/* Test that ReqCertFromX509() rejects a CSR with an oversized
883+
* SubjectKeyIdentifier (> CTC_MAX_SKID_SIZE = 32 bytes). Before the fix
884+
* this would cause a heap buffer overflow into cert->skid[32]. */
885+
int test_x509_ReqCertFromX509_skid_overflow(void)
886+
{
887+
EXPECT_DECLS;
888+
#if defined(WOLFSSL_CERT_REQ) && defined(WOLFSSL_CERT_GEN) && \
889+
defined(WOLFSSL_CERT_EXT) && !defined(NO_BIO) && \
890+
(defined(OPENSSL_EXTRA) || defined(OPENSSL_ALL)) && \
891+
defined(HAVE_ECC)
892+
893+
/* Minimal DER-encoded CSR (PKCS#10) containing a SubjectKeyIdentifier
894+
* extension with a 64-byte value -- double the 32-byte CTC_MAX_SKID_SIZE
895+
* destination buffer.
896+
*
897+
* Structure:
898+
* SEQUENCE {
899+
* CertificationRequestInfo SEQUENCE {
900+
* version INTEGER 0
901+
* subject: CN=Test
902+
* subjectPKInfo: EC P-256 (generator point)
903+
* attributes [0] {
904+
* SEQUENCE {
905+
* OID 1.2.840.113549.1.9.14 (extensionRequest)
906+
* SET { SEQUENCE { -- Extensions
907+
* SEQUENCE { -- Extension
908+
* OID 2.5.29.14 (subjectKeyIdentifier)
909+
* OCTET STRING { OCTET STRING (64 bytes of 0x41) }
910+
* }
911+
* }}
912+
* }
913+
* }
914+
* }
915+
* signatureAlgorithm: ecdsa-with-SHA256
916+
* signatureValue: dummy
917+
* }
918+
*/
919+
static const unsigned char crafted_csr_der[] = {
920+
0x30, 0x81, 0xE7, 0x30, 0x81, 0xCD, 0x02, 0x01,
921+
0x00, 0x30, 0x0F, 0x31, 0x0D, 0x30, 0x0B, 0x06,
922+
0x03, 0x55, 0x04, 0x03, 0x0C, 0x04, 0x54, 0x65,
923+
0x73, 0x74, 0x30, 0x59, 0x30, 0x13, 0x06, 0x07,
924+
0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x02, 0x01, 0x06,
925+
0x08, 0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x03, 0x01,
926+
0x07, 0x03, 0x42, 0x00, 0x04, 0x6B, 0x17, 0xD1,
927+
0xF2, 0xE1, 0x2C, 0x42, 0x47, 0xF8, 0xBC, 0xE6,
928+
0xE5, 0x63, 0xA4, 0x40, 0xF2, 0x77, 0x03, 0x7D,
929+
0x81, 0x2D, 0xEB, 0x33, 0xA0, 0xF4, 0xA1, 0x39,
930+
0x45, 0xD8, 0x98, 0xC2, 0x96, 0x4F, 0xE3, 0x42,
931+
0xE2, 0xFE, 0x1A, 0x7F, 0x9B, 0x8E, 0xE7, 0xEB,
932+
0x4A, 0x7C, 0x0F, 0x9E, 0x16, 0x2B, 0xCE, 0x33,
933+
0x57, 0x6B, 0x31, 0x5E, 0xCE, 0xCB, 0xB6, 0x40,
934+
0x68, 0x37, 0xBF, 0x51, 0xF5, 0xA0, 0x5C, 0x30,
935+
0x5A, 0x06, 0x09, 0x2A, 0x86, 0x48, 0x86, 0xF7,
936+
0x0D, 0x01, 0x09, 0x0E, 0x31, 0x4D, 0x30, 0x4B,
937+
0x30, 0x49, 0x06, 0x03, 0x55, 0x1D, 0x0E, 0x04,
938+
0x42, 0x04, 0x40,
939+
/* 64 bytes of 0x41 -- oversized SKID value */
940+
0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41,
941+
0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41,
942+
0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41,
943+
0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41,
944+
0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41,
945+
0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41,
946+
0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41,
947+
0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41,
948+
/* end of SKID payload */
949+
0x30, 0x0A, 0x06, 0x08, 0x2A, 0x86, 0x48, 0xCE,
950+
0x3D, 0x04, 0x03, 0x02, 0x03, 0x09, 0x00, 0x30,
951+
0x06, 0x02, 0x01, 0x01, 0x02, 0x01, 0x01
952+
};
953+
954+
WOLFSSL_X509* req = NULL;
955+
WOLFSSL_BIO* bio = NULL;
956+
957+
/* Step 1: Parse the crafted CSR -- this should succeed (the parser
958+
* dynamically allocates subjKeyId to the parsed size). */
959+
req = wolfSSL_X509_REQ_d2i(NULL, crafted_csr_der,
960+
(int)sizeof(crafted_csr_der));
961+
ExpectNotNull(req);
962+
963+
/* Step 2: Attempt to re-encode. Before the fix, this triggered a
964+
* heap buffer overflow in ReqCertFromX509() writing 64 bytes into
965+
* cert->skid[32]. With the fix, it must return failure. */
966+
bio = wolfSSL_BIO_new(wolfSSL_BIO_s_mem());
967+
ExpectNotNull(bio);
968+
ExpectIntNE(wolfSSL_i2d_X509_REQ_bio(bio, req), WOLFSSL_SUCCESS);
969+
970+
wolfSSL_BIO_free(bio);
971+
wolfSSL_X509_free(req);
972+
#endif
973+
return EXPECT_RESULT();
974+
}
975+
976+
/* Positive / boundary companion to test_x509_ReqCertFromX509_skid_overflow.
977+
* Verifies that a CSR with a SubjectKeyIdentifier of exactly
978+
* CTC_MAX_SKID_SIZE (32) bytes -- the boundary of the bounds check in
979+
* ReqCertFromX509() -- is accepted and that the SKID round-trips with the
980+
* correct length and bytes through the sign / re-encode / re-parse path.
981+
* This kills boundary mutations (">" -> ">=") and copy-size mutations on
982+
* the XMEMCPY into cert->skid that the negative test alone cannot catch. */
983+
int test_x509_ReqCertFromX509_skid_boundary(void)
984+
{
985+
EXPECT_DECLS;
986+
#if defined(WOLFSSL_CERT_REQ) && defined(WOLFSSL_CERT_GEN) && \
987+
defined(WOLFSSL_CERT_EXT) && !defined(NO_BIO) && \
988+
(defined(OPENSSL_EXTRA) || defined(OPENSSL_ALL)) && \
989+
defined(HAVE_ECC) && defined(USE_CERT_BUFFERS_256)
990+
991+
WOLFSSL_EVP_PKEY* priv = NULL;
992+
WOLFSSL_EVP_PKEY* pub = NULL;
993+
WOLFSSL_X509* req = NULL;
994+
WOLFSSL_X509* parsed = NULL;
995+
WOLFSSL_X509_NAME* name = NULL;
996+
unsigned char* der = NULL;
997+
int derSz;
998+
const unsigned char* ecPriv = ecc_clikey_der_256;
999+
const unsigned char* ecPub = ecc_clikeypub_der_256;
1000+
unsigned char expected_skid[CTC_MAX_SKID_SIZE];
1001+
1002+
XMEMSET(expected_skid, 0x41, sizeof(expected_skid));
1003+
1004+
/* Load a real ECC keypair so that the CSR can actually be signed
1005+
* (ReqCertFromX509() is invoked from the signing path). */
1006+
ExpectNotNull(priv = wolfSSL_d2i_PrivateKey(EVP_PKEY_EC, NULL, &ecPriv,
1007+
(long)sizeof_ecc_clikey_der_256));
1008+
ExpectNotNull(pub = wolfSSL_d2i_PUBKEY(NULL, &ecPub,
1009+
(long)sizeof_ecc_clikeypub_der_256));
1010+
1011+
ExpectNotNull(req = wolfSSL_X509_REQ_new());
1012+
ExpectNotNull(name = wolfSSL_X509_NAME_new());
1013+
ExpectIntEQ(wolfSSL_X509_NAME_add_entry_by_txt(name, "commonName",
1014+
MBSTRING_UTF8, (const byte*)"Test", 4, -1, 0),
1015+
WOLFSSL_SUCCESS);
1016+
ExpectIntEQ(wolfSSL_X509_REQ_set_subject_name(req, name), WOLFSSL_SUCCESS);
1017+
ExpectIntEQ(wolfSSL_X509_REQ_set_pubkey(req, pub), WOLFSSL_SUCCESS);
1018+
1019+
/* Inject exactly CTC_MAX_SKID_SIZE bytes of SKID directly on the req
1020+
* (tests/api/test_x509.c already includes <wolfssl/internal.h>). This
1021+
* is the boundary value of the bounds check in ReqCertFromX509(). */
1022+
if (EXPECT_SUCCESS() && req != NULL) {
1023+
req->subjKeyId = (byte*)XMALLOC(sizeof(expected_skid), NULL,
1024+
DYNAMIC_TYPE_X509_EXT);
1025+
ExpectNotNull(req->subjKeyId);
1026+
if (req->subjKeyId != NULL) {
1027+
XMEMCPY(req->subjKeyId, expected_skid, sizeof(expected_skid));
1028+
req->subjKeyIdSz = (word32)sizeof(expected_skid);
1029+
}
1030+
}
1031+
1032+
/* Signing invokes wolfssl_x509_make_der() -> ReqCertFromX509(), which
1033+
* executes the bounds check and the XMEMCPY into cert->skid. A
1034+
* ">=" boundary mutation would make this step fail for a 32-byte
1035+
* SKID. */
1036+
ExpectIntEQ(wolfSSL_X509_REQ_sign(req, priv, wolfSSL_EVP_sha256()),
1037+
WOLFSSL_SUCCESS);
1038+
1039+
/* Re-encode the now-signed CSR and parse it back to verify the SKID
1040+
* round-trips with the correct length and bytes. This verifies the
1041+
* output of the XMEMCPY in ReqCertFromX509(). */
1042+
ExpectIntGT((derSz = wolfSSL_i2d_X509_REQ(req, &der)), 0);
1043+
ExpectNotNull(der);
1044+
1045+
ExpectNotNull(parsed = wolfSSL_X509_REQ_d2i(NULL, der, derSz));
1046+
if (parsed != NULL) {
1047+
ExpectIntEQ((int)parsed->subjKeyIdSz, CTC_MAX_SKID_SIZE);
1048+
ExpectNotNull(parsed->subjKeyId);
1049+
if (parsed->subjKeyId != NULL) {
1050+
ExpectIntEQ(XMEMCMP(parsed->subjKeyId, expected_skid,
1051+
CTC_MAX_SKID_SIZE), 0);
1052+
}
1053+
}
1054+
1055+
wolfSSL_X509_free(parsed);
1056+
XFREE(der, NULL, DYNAMIC_TYPE_OPENSSL);
1057+
wolfSSL_X509_NAME_free(name);
1058+
wolfSSL_X509_free(req);
1059+
wolfSSL_EVP_PKEY_free(pub);
1060+
wolfSSL_EVP_PKEY_free(priv);
1061+
#endif
1062+
return EXPECT_RESULT();
1063+
}

‎tests/api/test_x509.h‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,17 @@ int test_x509_set_serialNumber(void);
2828
int test_x509_verify_cert_hostname_check(void);
2929
int test_x509_time_field_overread_via_tls(void);
3030
int test_x509_CertFromX509_akid_overflow(void);
31+
int test_x509_ReqCertFromX509_skid_overflow(void);
32+
int test_x509_ReqCertFromX509_skid_boundary(void);
3133

3234
#define TEST_X509_DECLS \
3335
TEST_DECL_GROUP("x509", test_x509_rfc2818_verification_callback), \
3436
TEST_DECL_GROUP("x509", test_x509_GetCAByAKID), \
3537
TEST_DECL_GROUP("x509", test_x509_set_serialNumber), \
3638
TEST_DECL_GROUP("x509", test_x509_verify_cert_hostname_check), \
3739
TEST_DECL_GROUP("x509", test_x509_time_field_overread_via_tls), \
38-
TEST_DECL_GROUP("x509", test_x509_CertFromX509_akid_overflow)
40+
TEST_DECL_GROUP("x509", test_x509_CertFromX509_akid_overflow), \
41+
TEST_DECL_GROUP("x509", test_x509_ReqCertFromX509_skid_overflow), \
42+
TEST_DECL_GROUP("x509", test_x509_ReqCertFromX509_skid_boundary)
3943

4044
#endif /* WOLFCRYPT_TEST_X509_H */

0 commit comments

Comments
 (0)