Skip to content

Commit 772cda3

Browse files
embhornembhorn
committed
Fix CertFromX509 copy length check
1 parent 84f9b20 commit 772cda3

3 files changed

Lines changed: 237 additions & 16 deletions

File tree

‎src/x509.c‎

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -11870,25 +11870,28 @@ static int CertFromX509(Cert* cert, WOLFSSL_X509* x509)
1187011870
return WOLFSSL_FAILURE;
1187111871
}
1187211872

11873-
if (x509->authKeyIdSz < sizeof(cert->akid)) {
1187411873
#ifdef WOLFSSL_AKID_NAME
11875-
cert->rawAkid = 0;
11876-
if (x509->authKeyIdSrc) {
11877-
XMEMCPY(cert->akid, x509->authKeyIdSrc, x509->authKeyIdSrcSz);
11878-
cert->akidSz = (int)x509->authKeyIdSrcSz;
11879-
cert->rawAkid = 1;
11874+
cert->rawAkid = 0;
11875+
if (x509->authKeyIdSrc) {
11876+
if (x509->authKeyIdSrcSz > sizeof(cert->akid)) {
11877+
WOLFSSL_MSG("Auth Key ID too large");
11878+
WOLFSSL_ERROR_VERBOSE(BUFFER_E);
11879+
return WOLFSSL_FAILURE;
1188011880
}
11881-
else
11881+
XMEMCPY(cert->akid, x509->authKeyIdSrc, x509->authKeyIdSrcSz);
11882+
cert->akidSz = (int)x509->authKeyIdSrcSz;
11883+
cert->rawAkid = 1;
11884+
}
11885+
else
1188211886
#endif
11883-
if (x509->authKeyId) {
11884-
XMEMCPY(cert->akid, x509->authKeyId, x509->authKeyIdSz);
11885-
cert->akidSz = (int)x509->authKeyIdSz;
11887+
if (x509->authKeyId) {
11888+
if (x509->authKeyIdSz > sizeof(cert->akid)) {
11889+
WOLFSSL_MSG("Auth Key ID too large");
11890+
WOLFSSL_ERROR_VERBOSE(BUFFER_E);
11891+
return WOLFSSL_FAILURE;
1188611892
}
11887-
}
11888-
else {
11889-
WOLFSSL_MSG("Auth Key ID too large");
11890-
WOLFSSL_ERROR_VERBOSE(BUFFER_E);
11891-
return WOLFSSL_FAILURE;
11893+
XMEMCPY(cert->akid, x509->authKeyId, x509->authKeyIdSz);
11894+
cert->akidSz = (int)x509->authKeyIdSz;
1189211895
}
1189311896

1189411897
for (i = 0; i < x509->certPoliciesNb; i++) {

‎tests/api/test_x509.c‎

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838

3939
#include <wolfssl/internal.h>
4040
#include <wolfssl/wolfcrypt/asn.h>
41+
#include <wolfssl/wolfcrypt/asn_public.h>
4142

4243
#if defined(OPENSSL_ALL) && \
4344
defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES)
@@ -632,3 +633,218 @@ int test_x509_time_field_overread_via_tls(void)
632633
#endif /* compile guards */
633634
return EXPECT_RESULT();
634635
}
636+
637+
638+
/* Test that CertFromX509 rejects an oversized raw AuthorityKeyIdentifier
639+
* extension. Before the fix, the guard checked authKeyIdSz (the [0]
640+
* keyIdentifier sub-field) but the WOLFSSL_AKID_NAME branch copied
641+
* authKeyIdSrcSz (the full extension) bytes, causing a heap overflow. */
642+
int test_x509_CertFromX509_akid_overflow(void)
643+
{
644+
EXPECT_DECLS;
645+
#if defined(WOLFSSL_AKID_NAME) && defined(WOLFSSL_CERT_GEN) && \
646+
defined(WOLFSSL_CERT_EXT) && \
647+
(defined(OPENSSL_EXTRA) || defined(OPENSSL_ALL))
648+
/* DER builder helpers -- write into a flat buffer */
649+
unsigned char buf[16384];
650+
size_t pos = 0;
651+
size_t akid_val_len;
652+
unsigned char* akid_val = NULL;
653+
WOLFSSL_X509* x = NULL;
654+
WOLFSSL_BIO* bio = NULL;
655+
656+
#define PUT1(b) do { buf[pos++] = (b); } while(0)
657+
#define PUTN(p, n) do { XMEMCPY(buf + pos, (p), (n)); pos += (n); } while(0)
658+
659+
/* Emit tag + definite-length header, return header size */
660+
#define TLV_HDR(tag, n, out, hlen) do { \
661+
size_t _i = 0; \
662+
(out)[_i++] = (tag); \
663+
if ((n) < 0x80u) { (out)[_i++] = (unsigned char)(n); } \
664+
else if ((n) < 0x100u) { (out)[_i++] = 0x81; \
665+
(out)[_i++] = (unsigned char)(n); } \
666+
else if ((n) < 0x10000u) { (out)[_i++] = 0x82; \
667+
(out)[_i++] = (unsigned char)((n)>>8); \
668+
(out)[_i++] = (unsigned char)(n); } \
669+
(hlen) = _i; \
670+
} while(0)
671+
672+
/* Wrap [start, pos) in-place with a TLV header */
673+
#define WRAP(start, tag) do { \
674+
size_t _len = pos - (start); \
675+
unsigned char _hdr[6]; size_t _hlen; \
676+
TLV_HDR((tag), _len, _hdr, _hlen); \
677+
XMEMMOVE(buf + (start) + _hlen, buf + (start), _len); \
678+
XMEMCPY(buf + (start), _hdr, _hlen); \
679+
pos += _hlen; \
680+
} while(0)
681+
682+
/* ---- Build AKID extension value ---- */
683+
{
684+
size_t akid_start = pos;
685+
size_t s;
686+
int i;
687+
688+
/* [0] keyIdentifier: 20 bytes (small, passes old check) */
689+
s = pos;
690+
for (i = 0; i < 20; i++) PUT1(0x41);
691+
WRAP(s, 0x80);
692+
693+
/* [1] authorityCertIssuer: one URI of ~4000 bytes
694+
* This makes authKeyIdSrcSz >> sizeof(cert->akid) (~1628) */
695+
s = pos;
696+
{
697+
const char* pfx = "http://e/";
698+
PUTN(pfx, (size_t)XSTRLEN(pfx));
699+
for (i = 0; i < 4000; i++) PUT1('Z');
700+
}
701+
WRAP(s, 0x86); /* GeneralName [6] URI */
702+
WRAP(s, 0xA1); /* [1] IMPLICIT */
703+
704+
/* [2] authorityCertSerialNumber */
705+
s = pos;
706+
PUT1(0x01);
707+
WRAP(s, 0x82);
708+
709+
WRAP(akid_start, 0x30); /* SEQUENCE */
710+
akid_val_len = pos - akid_start;
711+
akid_val = (unsigned char*)XMALLOC(akid_val_len, NULL,
712+
DYNAMIC_TYPE_TMP_BUFFER);
713+
ExpectNotNull(akid_val);
714+
if (akid_val != NULL)
715+
XMEMCPY(akid_val, buf + akid_start, akid_val_len);
716+
}
717+
718+
/* ---- Build minimal self-signed v3 certificate ---- */
719+
pos = 0;
720+
{
721+
size_t tbs_start = pos;
722+
size_t s;
723+
int i;
724+
725+
/* version [0] EXPLICIT INTEGER 2 (v3) */
726+
PUT1(0xA0); PUT1(0x03); PUT1(0x02); PUT1(0x01); PUT1(0x02);
727+
728+
/* serialNumber INTEGER 1 */
729+
PUT1(0x02); PUT1(0x01); PUT1(0x01);
730+
731+
/* signature: ecdsa-with-SHA256 */
732+
s = pos;
733+
{
734+
unsigned char oid[] = {0x06,0x08,0x2A,0x86,0x48,0xCE,
735+
0x3D,0x04,0x03,0x02};
736+
PUTN(oid, sizeof(oid));
737+
}
738+
WRAP(s, 0x30);
739+
740+
/* issuer: CN=A */
741+
s = pos;
742+
{
743+
size_t rdn = pos, atv = pos;
744+
unsigned char cn[] = {0x06,0x03,0x55,0x04,0x03};
745+
PUTN(cn, sizeof(cn));
746+
PUT1(0x0C); PUT1(0x01); PUT1('A');
747+
WRAP(atv, 0x30); WRAP(rdn, 0x31); WRAP(s, 0x30);
748+
}
749+
750+
/* validity */
751+
s = pos;
752+
{
753+
unsigned char t1[] = {0x17,0x0D,'2','5','0','1','0','1',
754+
'0','0','0','0','0','0','Z'};
755+
unsigned char t2[] = {0x17,0x0D,'3','5','0','1','0','1',
756+
'0','0','0','0','0','0','Z'};
757+
PUTN(t1, sizeof(t1)); PUTN(t2, sizeof(t2));
758+
}
759+
WRAP(s, 0x30);
760+
761+
/* subject: CN=A */
762+
s = pos;
763+
{
764+
size_t rdn = pos, atv = pos;
765+
unsigned char cn[] = {0x06,0x03,0x55,0x04,0x03};
766+
PUTN(cn, sizeof(cn));
767+
PUT1(0x0C); PUT1(0x01); PUT1('A');
768+
WRAP(atv, 0x30); WRAP(rdn, 0x31); WRAP(s, 0x30);
769+
}
770+
771+
/* subjectPublicKeyInfo: EC P-256 with dummy point */
772+
s = pos;
773+
{
774+
size_t alg = pos, bs;
775+
unsigned char ecpk[] = {0x06,0x07,0x2A,0x86,0x48,0xCE,
776+
0x3D,0x02,0x01};
777+
unsigned char p256[] = {0x06,0x08,0x2A,0x86,0x48,0xCE,
778+
0x3D,0x03,0x01,0x07};
779+
PUTN(ecpk, sizeof(ecpk));
780+
PUTN(p256, sizeof(p256));
781+
WRAP(alg, 0x30);
782+
bs = pos;
783+
PUT1(0x00); PUT1(0x04);
784+
for (i = 0; i < 64; i++) PUT1(0x01);
785+
WRAP(bs, 0x03);
786+
}
787+
WRAP(s, 0x30);
788+
789+
/* extensions [3] */
790+
{
791+
size_t exts_outer = pos, exts_seq = pos, ext = pos, ev;
792+
unsigned char akid_oid[] = {0x06,0x03,0x55,0x1D,0x23};
793+
PUTN(akid_oid, sizeof(akid_oid));
794+
ev = pos;
795+
if (akid_val != NULL)
796+
PUTN(akid_val, akid_val_len);
797+
WRAP(ev, 0x04);
798+
WRAP(ext, 0x30);
799+
WRAP(exts_seq, 0x30);
800+
WRAP(exts_outer, 0xA3);
801+
}
802+
803+
WRAP(tbs_start, 0x30);
804+
805+
/* signatureAlgorithm */
806+
s = pos;
807+
{
808+
unsigned char oid[] = {0x06,0x08,0x2A,0x86,0x48,0xCE,
809+
0x3D,0x04,0x03,0x02};
810+
PUTN(oid, sizeof(oid));
811+
}
812+
WRAP(s, 0x30);
813+
814+
/* signatureValue: dummy */
815+
s = pos;
816+
{
817+
size_t sig;
818+
PUT1(0x00);
819+
sig = pos;
820+
PUT1(0x02); PUT1(0x01); PUT1(0x01);
821+
PUT1(0x02); PUT1(0x01); PUT1(0x01);
822+
WRAP(sig, 0x30);
823+
}
824+
WRAP(s, 0x03);
825+
826+
WRAP(0, 0x30); /* outer Certificate SEQUENCE */
827+
}
828+
829+
/* Parse the crafted certificate */
830+
x = wolfSSL_X509_d2i(NULL, buf, (int)pos);
831+
ExpectNotNull(x);
832+
833+
/* Attempt re-encode via i2d_X509_bio -- must fail gracefully, not
834+
* overflow. Before the fix this would write ~4000 bytes past the
835+
* end of cert->akid[]. */
836+
bio = wolfSSL_BIO_new(wolfSSL_BIO_s_mem());
837+
ExpectNotNull(bio);
838+
ExpectIntEQ(wolfSSL_i2d_X509_bio(bio, x), 0);
839+
840+
wolfSSL_BIO_free(bio);
841+
wolfSSL_X509_free(x);
842+
XFREE(akid_val, NULL, DYNAMIC_TYPE_TMP_BUFFER);
843+
844+
#undef PUT1
845+
#undef PUTN
846+
#undef TLV_HDR
847+
#undef WRAP
848+
#endif
849+
return EXPECT_RESULT();
850+
}

‎tests/api/test_x509.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,14 @@ int test_x509_GetCAByAKID(void);
2727
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);
30+
int test_x509_CertFromX509_akid_overflow(void);
3031

3132
#define TEST_X509_DECLS \
3233
TEST_DECL_GROUP("x509", test_x509_rfc2818_verification_callback), \
3334
TEST_DECL_GROUP("x509", test_x509_GetCAByAKID), \
3435
TEST_DECL_GROUP("x509", test_x509_set_serialNumber), \
3536
TEST_DECL_GROUP("x509", test_x509_verify_cert_hostname_check), \
36-
TEST_DECL_GROUP("x509", test_x509_time_field_overread_via_tls)
37+
TEST_DECL_GROUP("x509", test_x509_time_field_overread_via_tls), \
38+
TEST_DECL_GROUP("x509", test_x509_CertFromX509_akid_overflow)
3739

3840
#endif /* WOLFCRYPT_TEST_X509_H */

0 commit comments

Comments
 (0)