Skip to content

Commit 2555c38

Browse files
alonblalonbl
committed
util: fix deserialize buffer overflow
Missing check for source file increment. Reported-by: Aarnav Bos <[email protected]>
1 parent 5e13f52 commit 2555c38

File tree

5 files changed

+229
-56
lines changed

5 files changed

+229
-56
lines changed

‎ChangeLog‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ Copyright (c) 2005-2022 Alon Bar-Lev <[email protected]>
44
????-??-?? - Version 1.31.0
55
* threading: fix mutex handling for cond_wait, thanks to Gleb Popov.
66
* mbed: initialize certificate early using mbedtls_x509_crt_init.
7+
* util: fix deserialize buffer overflow. thanks to Aarnav Bos.
78

89
2023-12-01 - Version 1.30.0
910
* core: add dynamic loader provider attribute, thanks to Marc Becker.

‎lib/pkcs11h-serialization.c‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,11 @@ pkcs11h_certificate_deserializeCertificateId (
415415

416416
certificate_id->attrCKA_ID_size = strlen (p)/2;
417417

418+
if (certificate_id->attrCKA_ID_size == 0) {
419+
rv = CKR_ATTRIBUTE_VALUE_INVALID;
420+
goto cleanup;
421+
}
422+
418423
if (
419424
(rv = _pkcs11h_mem_malloc (
420425
(void *)&certificate_id->attrCKA_ID,

‎lib/pkcs11h-util.c‎

Lines changed: 85 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,20 @@
5151
#include "common.h"
5252
#include "_pkcs11h-util.h"
5353

54+
static
55+
int
56+
_ctoi(const char c) {
57+
if ('0' <= c && c <= '9') {
58+
return c - '0';
59+
} else if ('A' <= c && c <= 'F') {
60+
return c - 'A' + 10;
61+
} else if ('a' <= c && c <= 'f') {
62+
return c - 'a' + 10;
63+
} else {
64+
return -1;
65+
}
66+
}
67+
5468
void
5569
_pkcs11h_util_fixupFixedString (
5670
OUT char * const target, /* MUST BE >= length+1 */
@@ -78,6 +92,7 @@ _pkcs11h_util_hexToBinary (
7892
IN const char * const source,
7993
IN OUT size_t * const p_target_size
8094
) {
95+
CK_RV ret = CKR_ATTRIBUTE_VALUE_INVALID;
8196
size_t target_max_size;
8297
const char *p;
8398
char buf[3] = {'\0', '\0', '\0'};
@@ -92,29 +107,31 @@ _pkcs11h_util_hexToBinary (
92107
*p_target_size = 0;
93108

94109
while (*p != '\x0' && *p_target_size < target_max_size) {
95-
if (isxdigit ((unsigned char)*p)) {
96-
buf[i%2] = *p;
110+
int b1, b2;
97111

98-
if ((i%2) == 1) {
99-
unsigned v;
100-
if (sscanf (buf, "%x", &v) != 1) {
101-
v = 0;
102-
}
103-
target[*p_target_size] = (char)(v & 0xff);
104-
(*p_target_size)++;
105-
}
112+
if ((b1 = _ctoi(*p)) == -1) {
113+
goto cleanup;
114+
}
115+
p++;
106116

107-
i++;
117+
if ((b2 = _ctoi(*p)) == -1) {
118+
goto cleanup;
108119
}
109120
p++;
121+
122+
target[*p_target_size] = (char)((b1 << 4) | b2);
123+
(*p_target_size)++;
110124
}
111125

112126
if (*p != '\x0') {
113-
return CKR_ATTRIBUTE_VALUE_INVALID;
114-
}
115-
else {
116-
return CKR_OK;
127+
goto cleanup;
117128
}
129+
130+
ret = CKR_OK;
131+
132+
cleanup:
133+
134+
return ret;
118135
}
119136

120137
CK_RV
@@ -224,66 +241,78 @@ _pkcs11h_util_unescapeString (
224241
CK_RV rv = CKR_FUNCTION_FAILED;
225242
const char *s = source;
226243
char *t = target;
244+
size_t m = *max;
227245
size_t n = 0;
228246

229247
/*_PKCS11H_ASSERT (target!=NULL); Not required*/
230248
_PKCS11H_ASSERT (source!=NULL);
231249
_PKCS11H_ASSERT (max!=NULL);
232250

251+
#define __get_source(b) \
252+
do { \
253+
if (*s == '\0') { \
254+
rv = CKR_ATTRIBUTE_VALUE_INVALID; \
255+
goto cleanup; \
256+
} \
257+
b = *s; \
258+
s++; \
259+
} while(0)
260+
261+
#define __add_target(c) \
262+
do { \
263+
if (t != NULL) { \
264+
if (n >= m) { \
265+
rv = CKR_ATTRIBUTE_VALUE_INVALID; \
266+
goto cleanup; \
267+
} \
268+
*t = (c); \
269+
t++; \
270+
} \
271+
n++; \
272+
} while(0)
273+
233274
while (*s != '\x0') {
234275
if (*s == '\\') {
235-
if (t != NULL) {
236-
if (n+1 > *max) {
237-
rv = CKR_ATTRIBUTE_VALUE_INVALID;
238-
goto cleanup;
239-
}
240-
else {
241-
char b[3];
242-
unsigned u;
243-
b[0] = s[2];
244-
b[1] = s[3];
245-
b[2] = '\x0';
246-
sscanf (b, "%08x", &u);
247-
*t = (char)(u & 0xff);
248-
t++;
249-
}
250-
}
251-
s+=4;
252-
}
253-
else {
254-
if (t != NULL) {
255-
if (n+1 > *max) {
256-
rv = CKR_ATTRIBUTE_VALUE_INVALID;
257-
goto cleanup;
258-
}
259-
else {
260-
*t = *s;
261-
t++;
262-
}
276+
int bin;
277+
int b1, b2;
278+
279+
__get_source(bin);
280+
281+
__get_source(bin);
282+
if (bin != 'x') {
283+
rv = CKR_ATTRIBUTE_VALUE_INVALID;
284+
goto cleanup;
263285
}
264-
s++;
265-
}
266286

267-
n+=1;
268-
}
287+
__get_source(bin);
288+
if ((b1 = _ctoi(bin)) == -1) {
289+
rv = CKR_ATTRIBUTE_VALUE_INVALID;
290+
goto cleanup;
291+
}
269292

270-
if (t != NULL) {
271-
if (n+1 > *max) {
272-
rv = CKR_ATTRIBUTE_VALUE_INVALID;
273-
goto cleanup;
274-
}
275-
else {
276-
*t = '\x0';
277-
t++;
293+
__get_source(bin);
294+
if ((b2 = _ctoi(bin)) == -1) {
295+
rv = CKR_ATTRIBUTE_VALUE_INVALID;
296+
goto cleanup;
297+
}
298+
__add_target((b1 << 4) | b2);
299+
} else {
300+
int bin;
301+
__get_source(bin);
302+
__add_target(bin);
278303
}
279304
}
280-
n++;
305+
306+
__add_target('\0');
281307

282308
*max = n;
283309
rv = CKR_OK;
284310

285311
cleanup:
286312

287313
return rv;
314+
315+
#undef __get_source
316+
#undef __add_target
288317
}
289318

‎tests/test-basic/Makefile.am‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ MAINTAINERCLEANFILES=$(srcdir)/Makefile.in
5353
MY_TESTS = \
5454
test-basic \
5555
test-basic2 \
56+
test-serialize \
5657
$(NULL)
5758

5859
TESTS=$(MY_TESTS)

‎tests/test-basic/test-serialize.c‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
#include <stdio.h>
2+
#include <stdlib.h>
3+
#include <string.h>
4+
#include "../../config.h"
5+
#include <pkcs11-helper-1.0/pkcs11h-core.h>
6+
#include <pkcs11-helper-1.0/pkcs11h-certificate.h>
7+
8+
static
9+
void
10+
fatal (const char * const m, CK_RV rv) {
11+
fprintf (stderr, "%s - %08lu - %s\n", m, rv, pkcs11h_getMessage (rv));
12+
exit (1);
13+
}
14+
15+
static
16+
void
17+
_pkcs11h_hooks_log (
18+
IN void * const global_data,
19+
IN unsigned flags,
20+
IN const char * const format,
21+
IN va_list args
22+
) {
23+
vfprintf (stdout, format, args);
24+
fprintf (stdout, "\n");
25+
fflush (stdout);
26+
}
27+
28+
static
29+
CK_RV
30+
_deserialize (
31+
const char * const str,
32+
const PKCS11H_BOOL should_success
33+
) {
34+
pkcs11h_certificate_id_t id = NULL;
35+
char *p = NULL;
36+
size_t max;
37+
CK_RV ret;
38+
CK_RV rv;
39+
40+
ret = pkcs11h_certificate_deserializeCertificateId (
41+
&id,
42+
str
43+
);
44+
45+
if (should_success && ret != CKR_OK) {
46+
fprintf(stderr, "String: %s\n", str);
47+
fatal("Should succeed", rv);
48+
}
49+
if (!should_success && ret == CKR_OK) {
50+
fprintf(stderr, "String: %s\n", str);
51+
fatal("Should fail", rv);
52+
}
53+
54+
if (id != NULL) {
55+
if ((rv = pkcs11h_certificate_serializeCertificateId (
56+
NULL,
57+
&max,
58+
id
59+
)) != CKR_OK) {
60+
fatal("pkcs11h_certificate_serializeCertificateId", rv);
61+
}
62+
63+
if ((p = malloc(max)) == NULL) {
64+
fatal("malloc", CKR_HOST_MEMORY);
65+
}
66+
67+
if ((rv = pkcs11h_certificate_serializeCertificateId (
68+
p,
69+
&max,
70+
id
71+
)) != CKR_OK) {
72+
fatal("pkcs11h_certificate_serializeCertificateId(2)", rv);
73+
}
74+
75+
if (strcmp(p, str)) {
76+
fprintf(stderr, "in: '%s' construct: '%s'\n", str, p);
77+
fatal("Fail serialize compare", CKR_FUNCTION_FAILED);
78+
}
79+
}
80+
81+
if (p != NULL) {
82+
free(p);
83+
}
84+
85+
if (id != NULL) {
86+
pkcs11h_certificate_freeCertificateId(id);
87+
}
88+
89+
return ret;
90+
}
91+
92+
int main () {
93+
CK_RV rv;
94+
int i;
95+
96+
printf ("Initializing pkcs11-helper\n");
97+
98+
if ((rv = pkcs11h_initialize ()) != CKR_OK) {
99+
fatal ("pkcs11h_initialize failed", rv);
100+
}
101+
102+
printf ("Registering pkcs11-helper hooks\n");
103+
104+
if ((rv = pkcs11h_setLogHook (_pkcs11h_hooks_log, NULL)) != CKR_OK) {
105+
fatal ("pkcs11h_setLogHook failed", rv);
106+
}
107+
108+
pkcs11h_setLogLevel (TEST_LOG_LEVEL);
109+
110+
printf ("Sanity\n");
111+
112+
_deserialize("manufacturer/model/serial/label/02", TRUE);
113+
114+
_deserialize("manufacturer/model/serial/label/0z", FALSE);
115+
116+
_deserialize("manufa\\xggcturer/model/serial/label/02", FALSE);
117+
118+
printf ("Components\n");
119+
120+
const char *failure_string = "manu\\xff/model/serial/label\xee/0";
121+
char buffer[1024];
122+
123+
for (i = 0; i < strlen(failure_string); i++) {
124+
strcpy(buffer, failure_string);
125+
buffer[i] = '\0';
126+
_deserialize(buffer, FALSE);
127+
}
128+
129+
printf ("Terminating pkcs11-helper\n");
130+
131+
if ((rv = pkcs11h_terminate ()) != CKR_OK) {
132+
fatal ("pkcs11h_terminate failed", rv);
133+
}
134+
135+
exit (0);
136+
return 0;
137+
}

0 commit comments

Comments
 (0)