Skip to content

Commit aae6125

Browse files
mpgmpg
authored
Merge pull request #6883 from valeriosetti/issue6843
Improve X.509 cert writing serial number management
2 parents e28397a + 18b9b03 commit aae6125

12 files changed

+368
-50
lines changed

‎ChangeLog.d/improve_x509_cert_writing_serial_number_management.txt

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
Bugfix
2+
* mbedtls_x509write_crt_set_serial() now explicitly rejects serial numbers
3+
whose binary representation is longer than 20 bytes. This was already
4+
forbidden by the standard (RFC5280 - section 4.1.2.2) and now it's being
5+
enforced also at code level.
6+
7+
New deprecations
8+
* mbedtls_x509write_crt_set_serial() is now being deprecated in favor of
9+
mbedtls_x509write_crt_set_serial_raw(). The goal here is to remove any
10+
direct dependency of X509 on BIGNUM_C.
11+
12+
Changes
13+
* programs/x509/cert_write:
14+
- now it accepts the serial number in 2 different formats: decimal and
15+
hex. They cannot be used simultaneously
16+
- "serial" is used for the decimal format and it's limted in size to
17+
unsigned long long int
18+
- "serial_hex" is used for the hex format; max length here is
19+
MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN*2

‎include/mbedtls/x509_crt.h

+32-3
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ mbedtls_x509_crt_profile;
197197
#define MBEDTLS_X509_CRT_VERSION_2 1
198198
#define MBEDTLS_X509_CRT_VERSION_3 2
199199

200-
#define MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN 32
200+
#define MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN 20
201201
#define MBEDTLS_X509_RFC5280_UTC_TIME_LEN 15
202202

203203
#if !defined(MBEDTLS_X509_MAX_FILE_PATH_LEN)
@@ -277,7 +277,8 @@ mbedtls_x509_crt_profile;
277277
*/
278278
typedef struct mbedtls_x509write_cert {
279279
int MBEDTLS_PRIVATE(version);
280-
mbedtls_mpi MBEDTLS_PRIVATE(serial);
280+
unsigned char MBEDTLS_PRIVATE(serial)[MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN];
281+
size_t MBEDTLS_PRIVATE(serial_len);
281282
mbedtls_pk_context *MBEDTLS_PRIVATE(subject_key);
282283
mbedtls_pk_context *MBEDTLS_PRIVATE(issuer_key);
283284
mbedtls_asn1_named_data *MBEDTLS_PRIVATE(subject);
@@ -986,15 +987,43 @@ void mbedtls_x509write_crt_init(mbedtls_x509write_cert *ctx);
986987
*/
987988
void mbedtls_x509write_crt_set_version(mbedtls_x509write_cert *ctx, int version);
988989

990+
#if defined(MBEDTLS_BIGNUM_C) && !defined(MBEDTLS_DEPRECATED_REMOVED)
989991
/**
990992
* \brief Set the serial number for a Certificate.
991993
*
994+
* \deprecated This function is deprecated and will be removed in a
995+
* future version of the library. Please use
996+
* mbedtls_x509write_crt_set_serial_raw() instead.
997+
*
998+
* \note Even though the MBEDTLS_BIGNUM_C guard looks redundant since
999+
* X509 depends on PK and PK depends on BIGNUM, this emphasizes
1000+
* a direct dependency between X509 and BIGNUM which is going
1001+
* to be deprecated in the future.
1002+
*
9921003
* \param ctx CRT context to use
9931004
* \param serial serial number to set
9941005
*
9951006
* \return 0 if successful
9961007
*/
997-
int mbedtls_x509write_crt_set_serial(mbedtls_x509write_cert *ctx, const mbedtls_mpi *serial);
1008+
int MBEDTLS_DEPRECATED mbedtls_x509write_crt_set_serial(
1009+
mbedtls_x509write_cert *ctx, const mbedtls_mpi *serial);
1010+
#endif // MBEDTLS_BIGNUM_C && !MBEDTLS_DEPRECATED_REMOVED
1011+
1012+
/**
1013+
* \brief Set the serial number for a Certificate.
1014+
*
1015+
* \param ctx CRT context to use
1016+
* \param serial A raw array of bytes containing the serial number in big
1017+
* endian format
1018+
* \param serial_len Length of valid bytes (expressed in bytes) in \p serial
1019+
* input buffer
1020+
*
1021+
* \return 0 if successful, or
1022+
* MBEDTLS_ERR_X509_BAD_INPUT_DATA if the provided input buffer
1023+
* is too big (longer than MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN)
1024+
*/
1025+
int mbedtls_x509write_crt_set_serial_raw(mbedtls_x509write_cert *ctx,
1026+
unsigned char *serial, size_t serial_len);
9981027

9991028
/**
10001029
* \brief Set the validity period for a Certificate

‎library/x509write_crt.c

+49-7
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,11 @@ void mbedtls_x509write_crt_init(mbedtls_x509write_cert *ctx)
5252
{
5353
memset(ctx, 0, sizeof(mbedtls_x509write_cert));
5454

55-
mbedtls_mpi_init(&ctx->serial);
5655
ctx->version = MBEDTLS_X509_CRT_VERSION_3;
5756
}
5857

5958
void mbedtls_x509write_crt_free(mbedtls_x509write_cert *ctx)
6059
{
61-
mbedtls_mpi_free(&ctx->serial);
62-
6360
mbedtls_asn1_free_named_data_list(&ctx->subject);
6461
mbedtls_asn1_free_named_data_list(&ctx->issuer);
6562
mbedtls_asn1_free_named_data_list(&ctx->extensions);
@@ -103,17 +100,42 @@ int mbedtls_x509write_crt_set_issuer_name(mbedtls_x509write_cert *ctx,
103100
return mbedtls_x509_string_to_names(&ctx->issuer, issuer_name);
104101
}
105102

103+
#if defined(MBEDTLS_BIGNUM_C) && !defined(MBEDTLS_DEPRECATED_REMOVED)
106104
int mbedtls_x509write_crt_set_serial(mbedtls_x509write_cert *ctx,
107105
const mbedtls_mpi *serial)
108106
{
109-
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
107+
int ret;
108+
size_t tmp_len;
109+
110+
/* Ensure that the MPI value fits into the buffer */
111+
tmp_len = mbedtls_mpi_size(serial);
112+
if (tmp_len > MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN) {
113+
return MBEDTLS_ERR_X509_BAD_INPUT_DATA;
114+
}
110115

111-
if ((ret = mbedtls_mpi_copy(&ctx->serial, serial)) != 0) {
116+
ctx->serial_len = tmp_len;
117+
118+
ret = mbedtls_mpi_write_binary(serial, ctx->serial, tmp_len);
119+
if (ret < 0) {
112120
return ret;
113121
}
114122

115123
return 0;
116124
}
125+
#endif // MBEDTLS_BIGNUM_C && !MBEDTLS_DEPRECATED_REMOVED
126+
127+
int mbedtls_x509write_crt_set_serial_raw(mbedtls_x509write_cert *ctx,
128+
unsigned char *serial, size_t serial_len)
129+
{
130+
if (serial_len > MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN) {
131+
return MBEDTLS_ERR_X509_BAD_INPUT_DATA;
132+
}
133+
134+
ctx->serial_len = serial_len;
135+
memcpy(ctx->serial, serial, serial_len);
136+
137+
return 0;
138+
}
117139

118140
int mbedtls_x509write_crt_set_validity(mbedtls_x509write_cert *ctx,
119141
const char *not_before,
@@ -510,9 +532,29 @@ int mbedtls_x509write_crt_der(mbedtls_x509write_cert *ctx,
510532

511533
/*
512534
* Serial ::= INTEGER
535+
*
536+
* Written data is:
537+
* - "ctx->serial_len" bytes for the raw serial buffer
538+
* - if MSb of "serial" is 1, then prepend an extra 0x00 byte
539+
* - 1 byte for the length
540+
* - 1 byte for the TAG
513541
*/
514-
MBEDTLS_ASN1_CHK_ADD(len, mbedtls_asn1_write_mpi(&c, buf,
515-
&ctx->serial));
542+
MBEDTLS_ASN1_CHK_ADD(len, mbedtls_asn1_write_raw_buffer(&c, buf,
543+
ctx->serial, ctx->serial_len));
544+
if (*c & 0x80) {
545+
if (c - buf < 1) {
546+
return MBEDTLS_ERR_X509_BUFFER_TOO_SMALL;
547+
}
548+
*(--c) = 0x0;
549+
len++;
550+
MBEDTLS_ASN1_CHK_ADD(len, mbedtls_asn1_write_len(&c, buf,
551+
ctx->serial_len + 1));
552+
} else {
553+
MBEDTLS_ASN1_CHK_ADD(len, mbedtls_asn1_write_len(&c, buf,
554+
ctx->serial_len));
555+
}
556+
MBEDTLS_ASN1_CHK_ADD(len, mbedtls_asn1_write_tag(&c, buf,
557+
MBEDTLS_ASN1_INTEGER));
516558

517559
/*
518560
* Version ::= INTEGER { v1(0), v2(1), v3(2) }

‎programs/x509/cert_write.c

+84-10
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,12 @@ int main(void)
4343
#include "mbedtls/ctr_drbg.h"
4444
#include "mbedtls/md.h"
4545
#include "mbedtls/error.h"
46+
#include "test/helpers.h"
4647

4748
#include <stdio.h>
4849
#include <stdlib.h>
4950
#include <string.h>
51+
#include <errno.h>
5052

5153
#define SET_OID(x, oid) \
5254
do { x.len = MBEDTLS_OID_SIZE(oid); x.p = (unsigned char *) oid; } while (0)
@@ -75,6 +77,7 @@ int main(void)
7577
#define DFL_NOT_BEFORE "20010101000000"
7678
#define DFL_NOT_AFTER "20301231235959"
7779
#define DFL_SERIAL "1"
80+
#define DFL_SERIAL_HEX "1"
7881
#define DFL_SELFSIGN 0
7982
#define DFL_IS_CA 0
8083
#define DFL_MAX_PATHLEN -1
@@ -110,6 +113,13 @@ int main(void)
110113
" issuer_pwd=%%s default: (empty)\n" \
111114
" output_file=%%s default: cert.crt\n" \
112115
" serial=%%s default: 1\n" \
116+
" In decimal format; it can be used as\n" \
117+
" alternative to serial_hex, but it's\n" \
118+
" limited in max length to\n" \
119+
" unsigned long long int\n" \
120+
" serial_hex=%%s default: 1\n" \
121+
" In hex format; it can be used as\n" \
122+
" alternative to serial\n" \
113123
" not_before=%%s default: 20010101000000\n" \
114124
" not_after=%%s default: 20301231235959\n" \
115125
" is_ca=%%d default: 0 (disabled)\n" \
@@ -159,6 +169,11 @@ int main(void)
159169
" format=pem|der default: pem\n" \
160170
"\n"
161171

172+
typedef enum {
173+
SERIAL_FRMT_UNSPEC,
174+
SERIAL_FRMT_DEC,
175+
SERIAL_FRMT_HEX
176+
} serial_format_t;
162177

163178
/*
164179
* global options
@@ -175,7 +190,8 @@ struct options {
175190
const char *issuer_name; /* issuer name for certificate */
176191
const char *not_before; /* validity period not before */
177192
const char *not_after; /* validity period not after */
178-
const char *serial; /* serial number string */
193+
const char *serial; /* serial number string (decimal) */
194+
const char *serial_hex; /* serial number string (hex) */
179195
int selfsign; /* selfsign the certificate */
180196
int is_ca; /* is a CA certificate */
181197
int max_pathlen; /* maximum CA path length */
@@ -235,6 +251,44 @@ int write_certificate(mbedtls_x509write_cert *crt, const char *output_file,
235251
return 0;
236252
}
237253

254+
int parse_serial_decimal_format(unsigned char *obuf, size_t obufmax,
255+
const char *ibuf, size_t *len)
256+
{
257+
unsigned long long int dec;
258+
unsigned int remaining_bytes = sizeof(dec);
259+
unsigned char *p = obuf;
260+
unsigned char val;
261+
char *end_ptr = NULL;
262+
263+
errno = 0;
264+
dec = strtoull(ibuf, &end_ptr, 10);
265+
266+
if ((errno != 0) || (end_ptr == ibuf)) {
267+
return -1;
268+
}
269+
270+
*len = 0;
271+
272+
while (remaining_bytes > 0) {
273+
if (obufmax < (*len + 1)) {
274+
return -1;
275+
}
276+
277+
val = (dec >> ((remaining_bytes - 1) * 8)) & 0xFF;
278+
279+
/* Skip leading zeros */
280+
if ((val != 0) || (*len != 0)) {
281+
*p = val;
282+
(*len)++;
283+
p++;
284+
}
285+
286+
remaining_bytes--;
287+
}
288+
289+
return 0;
290+
}
291+
238292
int main(int argc, char *argv[])
239293
{
240294
int ret = 1;
@@ -252,7 +306,9 @@ int main(int argc, char *argv[])
252306
mbedtls_x509_csr csr;
253307
#endif
254308
mbedtls_x509write_cert crt;
255-
mbedtls_mpi serial;
309+
serial_format_t serial_frmt = SERIAL_FRMT_UNSPEC;
310+
unsigned char serial[MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN];
311+
size_t serial_len;
256312
mbedtls_asn1_sequence *ext_key_usage;
257313
mbedtls_entropy_context entropy;
258314
mbedtls_ctr_drbg_context ctr_drbg;
@@ -264,14 +320,14 @@ int main(int argc, char *argv[])
264320
mbedtls_x509write_crt_init(&crt);
265321
mbedtls_pk_init(&loaded_issuer_key);
266322
mbedtls_pk_init(&loaded_subject_key);
267-
mbedtls_mpi_init(&serial);
268323
mbedtls_ctr_drbg_init(&ctr_drbg);
269324
mbedtls_entropy_init(&entropy);
270325
#if defined(MBEDTLS_X509_CSR_PARSE_C)
271326
mbedtls_x509_csr_init(&csr);
272327
#endif
273328
mbedtls_x509_crt_init(&issuer_crt);
274329
memset(buf, 0, sizeof(buf));
330+
memset(serial, 0, sizeof(serial));
275331

276332
if (argc == 0) {
277333
usage:
@@ -291,6 +347,7 @@ int main(int argc, char *argv[])
291347
opt.not_before = DFL_NOT_BEFORE;
292348
opt.not_after = DFL_NOT_AFTER;
293349
opt.serial = DFL_SERIAL;
350+
opt.serial_hex = DFL_SERIAL_HEX;
294351
opt.selfsign = DFL_SELFSIGN;
295352
opt.is_ca = DFL_IS_CA;
296353
opt.max_pathlen = DFL_MAX_PATHLEN;
@@ -335,7 +392,19 @@ int main(int argc, char *argv[])
335392
} else if (strcmp(p, "not_after") == 0) {
336393
opt.not_after = q;
337394
} else if (strcmp(p, "serial") == 0) {
395+
if (serial_frmt != SERIAL_FRMT_UNSPEC) {
396+
mbedtls_printf("Invalid attempt to set the serial more than once\n");
397+
goto usage;
398+
}
399+
serial_frmt = SERIAL_FRMT_DEC;
338400
opt.serial = q;
401+
} else if (strcmp(p, "serial_hex") == 0) {
402+
if (serial_frmt != SERIAL_FRMT_UNSPEC) {
403+
mbedtls_printf("Invalid attempt to set the serial more than once\n");
404+
goto usage;
405+
}
406+
serial_frmt = SERIAL_FRMT_HEX;
407+
opt.serial_hex = q;
339408
} else if (strcmp(p, "authority_identifier") == 0) {
340409
opt.authority_identifier = atoi(q);
341410
if (opt.authority_identifier != 0 &&
@@ -514,10 +583,16 @@ int main(int argc, char *argv[])
514583
mbedtls_printf(" . Reading serial number...");
515584
fflush(stdout);
516585

517-
if ((ret = mbedtls_mpi_read_string(&serial, 10, opt.serial)) != 0) {
518-
mbedtls_strerror(ret, buf, sizeof(buf));
519-
mbedtls_printf(" failed\n ! mbedtls_mpi_read_string "
520-
"returned -0x%04x - %s\n\n", (unsigned int) -ret, buf);
586+
if (serial_frmt == SERIAL_FRMT_HEX) {
587+
ret = mbedtls_test_unhexify(serial, sizeof(serial),
588+
opt.serial_hex, &serial_len);
589+
} else { // SERIAL_FRMT_DEC || SERIAL_FRMT_UNSPEC
590+
ret = parse_serial_decimal_format(serial, sizeof(serial),
591+
opt.serial, &serial_len);
592+
}
593+
594+
if (ret != 0) {
595+
mbedtls_printf(" failed\n ! Unable to parse serial\n");
521596
goto exit;
522597
}
523598

@@ -661,10 +736,10 @@ int main(int argc, char *argv[])
661736
mbedtls_x509write_crt_set_version(&crt, opt.version);
662737
mbedtls_x509write_crt_set_md_alg(&crt, opt.md);
663738

664-
ret = mbedtls_x509write_crt_set_serial(&crt, &serial);
739+
ret = mbedtls_x509write_crt_set_serial_raw(&crt, serial, serial_len);
665740
if (ret != 0) {
666741
mbedtls_strerror(ret, buf, sizeof(buf));
667-
mbedtls_printf(" failed\n ! mbedtls_x509write_crt_set_serial "
742+
mbedtls_printf(" failed\n ! mbedtls_x509write_crt_set_serial_raw "
668743
"returned -0x%04x - %s\n\n", (unsigned int) -ret, buf);
669744
goto exit;
670745
}
@@ -807,7 +882,6 @@ int main(int argc, char *argv[])
807882
mbedtls_x509write_crt_free(&crt);
808883
mbedtls_pk_free(&loaded_subject_key);
809884
mbedtls_pk_free(&loaded_issuer_key);
810-
mbedtls_mpi_free(&serial);
811885
mbedtls_ctr_drbg_free(&ctr_drbg);
812886
mbedtls_entropy_free(&entropy);
813887

‎tests/data_files/Makefile

+9
Original file line numberDiff line numberDiff line change
@@ -972,6 +972,15 @@ test_ca_server1_config_file = test-ca.server1.opensslconf
972972

973973
server1.crt: server1.key server1.req.sha256 $(test_ca_crt) $(test_ca_key_file_rsa)
974974
$(MBEDTLS_CERT_WRITE) request_file=server1.req.sha256 issuer_crt=$(test_ca_crt) issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) version=1 not_before=20190210144406 not_after=20290210144406 md=SHA1 version=3 output_file=$@
975+
server1.long_serial.crt: server1.key server1.req.sha256 $(test_ca_crt) $(test_ca_key_file_rsa)
976+
echo "112233445566778899aabbccddeeff0011223344" > test-ca.server1.tmp.serial
977+
$(OPENSSL) ca -in server1.req.sha256 -key PolarSSLTest -config test-ca.server1.test_serial.opensslconf -notext -batch -out $@
978+
server1.80serial.crt: server1.key server1.req.sha256 $(test_ca_crt) $(test_ca_key_file_rsa)
979+
echo "8011223344" > test-ca.server1.tmp.serial
980+
$(OPENSSL) ca -in server1.req.sha256 -key PolarSSLTest -config test-ca.server1.test_serial.opensslconf -notext -batch -out $@
981+
server1.long_serial_FF.crt: server1.key server1.req.sha256 $(test_ca_crt) $(test_ca_key_file_rsa)
982+
echo "ffffffffffffffffffffffffffffffff" > test-ca.server1.tmp.serial
983+
$(OPENSSL) ca -in server1.req.sha256 -key PolarSSLTest -config test-ca.server1.test_serial.opensslconf -notext -batch -out $@
975984
server1.noauthid.crt: server1.key server1.req.sha256 $(test_ca_crt) $(test_ca_key_file_rsa)
976985
$(MBEDTLS_CERT_WRITE) request_file=server1.req.sha256 issuer_crt=$(test_ca_crt) issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20190210144406 not_after=20290210144406 md=SHA1 authority_identifier=0 version=3 output_file=$@
977986
server1.crt.der: server1.crt

0 commit comments

Comments
 (0)