Skip to content

Commit 619c787

Browse files
embhornembhorn
committed
Fix from review
1 parent 8adcc53 commit 619c787

2 files changed

Lines changed: 149 additions & 21 deletions

File tree

‎tests/api/test_x509.c‎

Lines changed: 146 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@
4040
#include <wolfssl/wolfcrypt/asn.h>
4141
#include <wolfssl/wolfcrypt/asn_public.h>
4242

43+
/* Shared scratch buffer size for the in-test DER builders below. */
44+
#define CSR_BUF_SZ 16384
45+
4346
#if defined(OPENSSL_ALL) && \
4447
defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES)
4548
#define HAVE_TEST_X509_RFC2818_VERIFICATION_CALLBACK
@@ -652,25 +655,38 @@ int test_x509_CertFromX509_akid_overflow(void)
652655
#ifdef WOLFSSL_SMALL_STACK
653656
unsigned char* buf = NULL;
654657
#else
655-
unsigned char buf[16384];
658+
unsigned char buf[CSR_BUF_SZ];
656659
#endif
657660
size_t pos = 0;
658661
size_t akid_val_len;
659662
unsigned char* akid_val = NULL;
660663
WOLFSSL_X509* x = NULL;
661664
WOLFSSL_BIO* bio = NULL;
665+
int build_ok = 1;
662666

663667
#ifdef WOLFSSL_SMALL_STACK
664-
buf = (unsigned char*)XMALLOC(16384, NULL, DYNAMIC_TYPE_TMP_BUFFER);
668+
buf = (unsigned char*)XMALLOC(CSR_BUF_SZ, NULL, DYNAMIC_TYPE_TMP_BUFFER);
665669
ExpectNotNull(buf);
666670
if (buf == NULL)
667671
return EXPECT_RESULT();
668672
#endif
669673

670-
#define PUT1(b) do { buf[pos++] = (b); } while(0)
671-
#define PUTN(p, n) do { XMEMCPY(buf + pos, (p), (n)); pos += (n); } while(0)
674+
/* Bounds-checked writers: on overflow, set build_ok = 0 and skip the
675+
* write so we never run past the fixed CSR_BUF_SZ buffer. */
676+
#define PUT1(b) do { \
677+
if (pos >= (size_t)CSR_BUF_SZ) { build_ok = 0; } \
678+
else { buf[pos++] = (b); } \
679+
} while(0)
680+
#define PUTN(p, n) do { \
681+
if (pos > (size_t)CSR_BUF_SZ || \
682+
(size_t)(n) > (size_t)CSR_BUF_SZ - pos) { \
683+
build_ok = 0; \
684+
} \
685+
else { XMEMCPY(buf + pos, (p), (n)); pos += (n); } \
686+
} while(0)
672687

673-
/* Emit tag + definite-length header, return header size */
688+
/* Emit tag + definite-length header, return header size. Lengths
689+
* >= 0x10000 are not supported; flag overflow if encountered. */
674690
#define TLV_HDR(tag, n, out, hlen) do { \
675691
size_t _i = 0; \
676692
(out)[_i++] = (tag); \
@@ -680,17 +696,31 @@ int test_x509_CertFromX509_akid_overflow(void)
680696
else if ((n) < 0x10000u) { (out)[_i++] = 0x82; \
681697
(out)[_i++] = (unsigned char)((n)>>8); \
682698
(out)[_i++] = (unsigned char)(n); } \
699+
else { build_ok = 0; } \
683700
(hlen) = _i; \
684701
} while(0)
685702

686703
/* Wrap [start, pos) in-place with a TLV header */
687704
#define WRAP(start, tag) do { \
688-
size_t _len = pos - (start); \
705+
size_t _start = (start); \
706+
size_t _len; \
689707
unsigned char _hdr[6]; size_t _hlen; \
690-
TLV_HDR((tag), _len, _hdr, _hlen); \
691-
XMEMMOVE(buf + (start) + _hlen, buf + (start), _len); \
692-
XMEMCPY(buf + (start), _hdr, _hlen); \
693-
pos += _hlen; \
708+
if (_start > pos || pos > (size_t)CSR_BUF_SZ) { \
709+
build_ok = 0; \
710+
break; \
711+
} \
712+
_len = pos - _start; \
713+
TLV_HDR((tag), _len, _hdr, _hlen); \
714+
if (_hlen > (size_t)CSR_BUF_SZ - pos || \
715+
_hlen > (size_t)CSR_BUF_SZ - _start || \
716+
_len > (size_t)CSR_BUF_SZ - _start - _hlen) { \
717+
build_ok = 0; \
718+
} \
719+
else { \
720+
XMEMMOVE(buf + _start + _hlen, buf + _start, _len); \
721+
XMEMCPY(buf + _start, _hdr, _hlen); \
722+
pos += _hlen; \
723+
} \
694724
} while(0)
695725

696726
/* ---- Build AKID extension value ---- */
@@ -854,7 +884,8 @@ int test_x509_CertFromX509_akid_overflow(void)
854884
}
855885

856886
/* Parse the crafted certificate */
857-
x = wolfSSL_X509_d2i(NULL, buf, (int)pos);
887+
ExpectIntEQ(build_ok, 1);
888+
x = build_ok ? wolfSSL_X509_d2i(NULL, buf, (int)pos) : NULL;
858889
ExpectNotNull(x);
859890

860891
/* Attempt re-encode via i2d_X509_bio -- must fail gracefully, not
@@ -890,23 +921,36 @@ int test_x509_ReqCertFromX509_skid_overflow(void)
890921
#ifdef WOLFSSL_SMALL_STACK
891922
unsigned char* buf = NULL;
892923
#else
893-
unsigned char buf[16384];
924+
unsigned char buf[CSR_BUF_SZ];
894925
#endif
895926
size_t pos = 0;
896927
WOLFSSL_X509* req = NULL;
897928
WOLFSSL_BIO* bio = NULL;
898929
unsigned char* der = NULL;
930+
int build_ok = 1;
899931

900932
#ifdef WOLFSSL_SMALL_STACK
901-
buf = (unsigned char*)XMALLOC(16384, NULL, DYNAMIC_TYPE_TMP_BUFFER);
933+
buf = (unsigned char*)XMALLOC(CSR_BUF_SZ, NULL, DYNAMIC_TYPE_TMP_BUFFER);
902934
ExpectNotNull(buf);
903935
if (buf == NULL)
904936
return EXPECT_RESULT();
905937
#endif
906938

907-
#define PUT1(b) do { buf[pos++] = (b); } while(0)
908-
#define PUTN(p, n) do { XMEMCPY(buf + pos, (p), (n)); pos += (n); } while(0)
939+
/* Bounds-checked writers: on overflow, set build_ok = 0 and skip the
940+
* write so we never run past the fixed CSR_BUF_SZ buffer. */
941+
#define PUT1(b) do { \
942+
if (pos >= (size_t)CSR_BUF_SZ) { build_ok = 0; } \
943+
else { buf[pos++] = (b); } \
944+
} while(0)
945+
#define PUTN(p, n) do { \
946+
if (pos > (size_t)CSR_BUF_SZ || \
947+
(size_t)(n) > (size_t)CSR_BUF_SZ - pos) { \
948+
build_ok = 0; \
949+
} \
950+
else { XMEMCPY(buf + pos, (p), (n)); pos += (n); } \
951+
} while(0)
909952

953+
/* Lengths >= 0x10000 are not supported; flag overflow if encountered. */
910954
#define TLV_HDR(tag, n, out, hlen) do { \
911955
size_t _i = 0; \
912956
(out)[_i++] = (tag); \
@@ -916,16 +960,30 @@ int test_x509_ReqCertFromX509_skid_overflow(void)
916960
else if ((n) < 0x10000u) { (out)[_i++] = 0x82; \
917961
(out)[_i++] = (unsigned char)((n)>>8); \
918962
(out)[_i++] = (unsigned char)(n); } \
963+
else { build_ok = 0; } \
919964
(hlen) = _i; \
920965
} while(0)
921966

922967
#define WRAP(start, tag) do { \
923-
size_t _len = pos - (start); \
968+
size_t _start = (start); \
969+
size_t _len; \
924970
unsigned char _hdr[6]; size_t _hlen; \
971+
if (_start > pos || pos > (size_t)CSR_BUF_SZ) { \
972+
build_ok = 0; \
973+
break; \
974+
} \
975+
_len = pos - _start; \
925976
TLV_HDR((tag), _len, _hdr, _hlen); \
926-
XMEMMOVE(buf + (start) + _hlen, buf + (start), _len); \
927-
XMEMCPY(buf + (start), _hdr, _hlen); \
928-
pos += _hlen; \
977+
if (_hlen > (size_t)CSR_BUF_SZ - pos || \
978+
_hlen > (size_t)CSR_BUF_SZ - _start || \
979+
_len > (size_t)CSR_BUF_SZ - _start - _hlen) { \
980+
build_ok = 0; \
981+
} \
982+
else { \
983+
XMEMMOVE(buf + _start + _hlen, buf + _start, _len); \
984+
XMEMCPY(buf + _start, _hdr, _hlen); \
985+
pos += _hlen; \
986+
} \
929987
} while(0)
930988

931989
{
@@ -1029,7 +1087,8 @@ int test_x509_ReqCertFromX509_skid_overflow(void)
10291087
WRAP(req_start, 0x30);
10301088
}
10311089

1032-
req = wolfSSL_X509_REQ_d2i(NULL, buf, (int)pos);
1090+
ExpectIntEQ(build_ok, 1);
1091+
req = build_ok ? wolfSSL_X509_REQ_d2i(NULL, buf, (int)pos) : NULL;
10331092
ExpectNotNull(req);
10341093
if (req != NULL) {
10351094
ExpectIntEQ((int)req->subjKeyIdSz, CTC_MAX_SKID_SIZE + 1);
@@ -1057,3 +1116,70 @@ int test_x509_ReqCertFromX509_skid_overflow(void)
10571116
#endif
10581117
return EXPECT_RESULT();
10591118
}
1119+
1120+
/* Boundary complement to the overflow test: a SubjectKeyIdentifier of
1121+
* exactly CTC_MAX_SKID_SIZE bytes must round-trip through ReqCertFromX509.
1122+
* This catches a `>` -> `>=` mutation on the size guard at src/x509.c. */
1123+
int test_x509_ReqCertFromX509_skid_max(void)
1124+
{
1125+
EXPECT_DECLS;
1126+
#if defined(WOLFSSL_CERT_EXT) && defined(WOLFSSL_CERT_GEN) && \
1127+
defined(WOLFSSL_CERT_REQ) && !defined(NO_BIO) && defined(HAVE_ECC) && \
1128+
defined(USE_CERT_BUFFERS_256) && \
1129+
(defined(OPENSSL_EXTRA) || defined(OPENSSL_ALL))
1130+
X509* req = NULL;
1131+
X509_NAME* name = NULL;
1132+
EVP_PKEY* priv = NULL;
1133+
BIO* bio = NULL;
1134+
ASN1_OBJECT* skid_oid = NULL;
1135+
ASN1_OCTET_STRING* skid_data = NULL;
1136+
X509_EXTENSION* skid_ext = NULL;
1137+
STACK_OF(X509_EXTENSION)* exts = NULL;
1138+
const unsigned char* keyPtr = ecc_clikey_der_256;
1139+
unsigned char skid_bytes[CTC_MAX_SKID_SIZE];
1140+
int i;
1141+
1142+
for (i = 0; i < CTC_MAX_SKID_SIZE; i++) {
1143+
skid_bytes[i] = (unsigned char)(0x41 + (i % 16));
1144+
}
1145+
1146+
ExpectNotNull(req = X509_REQ_new());
1147+
ExpectNotNull(name = X509_NAME_new());
1148+
ExpectIntEQ(X509_NAME_add_entry_by_txt(name, "CN", MBSTRING_UTF8,
1149+
(const unsigned char*)"A", -1, -1, 0), WOLFSSL_SUCCESS);
1150+
ExpectIntEQ(X509_REQ_set_subject_name(req, name), WOLFSSL_SUCCESS);
1151+
1152+
ExpectNotNull(priv = d2i_PrivateKey(EVP_PKEY_EC, NULL, &keyPtr,
1153+
(long)sizeof_ecc_clikey_der_256));
1154+
ExpectIntEQ(X509_REQ_set_pubkey(req, priv), WOLFSSL_SUCCESS);
1155+
1156+
ExpectNotNull(skid_oid = OBJ_nid2obj(NID_subject_key_identifier));
1157+
ExpectNotNull(skid_data = ASN1_OCTET_STRING_new());
1158+
ExpectIntEQ(ASN1_OCTET_STRING_set(skid_data, skid_bytes,
1159+
(int)sizeof(skid_bytes)), 1);
1160+
ExpectNotNull(skid_ext = X509_EXTENSION_create_by_OBJ(NULL, skid_oid, 0,
1161+
skid_data));
1162+
ExpectNotNull(exts = sk_X509_EXTENSION_new_null());
1163+
ExpectIntEQ(sk_X509_EXTENSION_push(exts, skid_ext), 1);
1164+
if (EXPECT_FAIL()) {
1165+
X509_EXTENSION_free(skid_ext);
1166+
}
1167+
ExpectIntEQ(X509_REQ_add_extensions(req, exts), 1);
1168+
1169+
ExpectIntGT(X509_REQ_sign(req, priv, EVP_sha256()), 0);
1170+
1171+
/* Re-encode via the path that was previously vulnerable. With a SKID
1172+
* of exactly CTC_MAX_SKID_SIZE bytes this MUST succeed. */
1173+
ExpectNotNull(bio = BIO_new(BIO_s_mem()));
1174+
ExpectIntGT(i2d_X509_REQ_bio(bio, req), 0);
1175+
1176+
BIO_free(bio);
1177+
sk_X509_EXTENSION_pop_free(exts, X509_EXTENSION_free);
1178+
ASN1_OCTET_STRING_free(skid_data);
1179+
ASN1_OBJECT_free(skid_oid);
1180+
EVP_PKEY_free(priv);
1181+
X509_NAME_free(name);
1182+
X509_REQ_free(req);
1183+
#endif
1184+
return EXPECT_RESULT();
1185+
}

‎tests/api/test_x509.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ 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);
3131
int test_x509_ReqCertFromX509_skid_overflow(void);
32+
int test_x509_ReqCertFromX509_skid_max(void);
3233

3334
#define TEST_X509_DECLS \
3435
TEST_DECL_GROUP("x509", test_x509_rfc2818_verification_callback), \
@@ -37,6 +38,7 @@ int test_x509_ReqCertFromX509_skid_overflow(void);
3738
TEST_DECL_GROUP("x509", test_x509_verify_cert_hostname_check), \
3839
TEST_DECL_GROUP("x509", test_x509_time_field_overread_via_tls), \
3940
TEST_DECL_GROUP("x509", test_x509_CertFromX509_akid_overflow), \
40-
TEST_DECL_GROUP("x509", test_x509_ReqCertFromX509_skid_overflow)
41+
TEST_DECL_GROUP("x509", test_x509_ReqCertFromX509_skid_overflow), \
42+
TEST_DECL_GROUP("x509", test_x509_ReqCertFromX509_skid_max)
4143

4244
#endif /* WOLFCRYPT_TEST_X509_H */

0 commit comments

Comments
 (0)