Fix NULL pointer dereference in asn1_ex_i2c(), crypto/asn1/tasn_enc.c

[AITsygunka]

When fuzzing SMIME_write_ASN1_ex in combination with PKCS7_encrypt using random ciphers, we get error:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==1808607==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x0000005563ee bp 0x7ffc420f4590 sp 0x7ffc420f3ee0 T0)
==1808607==The signal is caused by a READ memory access.
==1808607==Hint: address points to the zero page.
    #0 0x5563ee in asn1_ex_i2c /workspaces/openssl-master/crypto/asn1/tasn_enc.c:631:24
    #1 0x551a02 in asn1_i2d_ex_primitive /workspaces/openssl-master/crypto/asn1/tasn_enc.c:476:11
    #2 0x54cf37 in ASN1_item_ex_i2d /workspaces/openssl-master/crypto/asn1/tasn_enc.c:104:16
    #3 0x551504 in asn1_template_ex_i2d /workspaces/openssl-master/crypto/asn1/tasn_enc.c:369:11
    #4 0x54e289 in ASN1_item_ex_i2d /workspaces/openssl-master/crypto/asn1/tasn_enc.c:180:22
    #5 0x551504 in asn1_template_ex_i2d /workspaces/openssl-master/crypto/asn1/tasn_enc.c:369:11
    #6 0x54e289 in ASN1_item_ex_i2d /workspaces/openssl-master/crypto/asn1/tasn_enc.c:180:22
    #7 0x551504 in asn1_template_ex_i2d /workspaces/openssl-master/crypto/asn1/tasn_enc.c:369:11
    #8 0x54e289 in ASN1_item_ex_i2d /workspaces/openssl-master/crypto/asn1/tasn_enc.c:180:22
    #9 0x550f16 in asn1_template_ex_i2d /workspaces/openssl-master/crypto/asn1/tasn_enc.c:348:13
    #10 0x54e289 in ASN1_item_ex_i2d /workspaces/openssl-master/crypto/asn1/tasn_enc.c:180:22
    #11 0x54c043 in asn1_item_flags_i2d /workspaces/openssl-master/crypto/asn1/tasn_enc.c:62:15
    #12 0x54c4e4 in ASN1_item_i2d /workspaces/openssl-master/crypto/asn1/tasn_enc.c:45:12
    #13 0xebfcac in ASN1_item_i2d_bio /workspaces/openssl-master/crypto/asn1/a_i2d_fp.c:90:9
    #14 0x536551 in i2d_ASN1_bio_stream /workspaces/openssl-master/crypto/asn1/asn_mime.c:99:9
    #15 0x537ce7 in B64_write_ASN1 /workspaces/openssl-master/crypto/asn1/asn_mime.c:119:9
    #16 0x539517 in SMIME_write_ASN1_ex /workspaces/openssl-master/crypto/asn1/asn_mime.c:326:10
    #17 0x533658 in FuzzerTestOneInput_SMIME_write_PKCS7 /workspaces/openssl_fuzzing/smime_write_cms_pkcs7_fuzzer/smime_write_cms_pkcs7_fuzzer.c:274:10
    #18 0x5342b5 in FuzzerTestOneInput /workspaces/openssl_fuzzing/smime_write_cms_pkcs7_fuzzer/smime_write_cms_pkcs7_fuzzer.c:337:11
    #19 0x535c78 in main /workspaces/openssl_fuzzing/smime_write_cms_pkcs7_fuzzer/main.c:35:9
    #20 0x7fe7eb79d082 in __libc_start_main /build/glibc-LcI20x/glibc-2.31/csu/../csu/libc-start.c:308:16
    #21 0x48874d in _start (/workspaces/openssl_fuzzing/build/smime_write_cms_pkcs7_fuzzer+0x48874d)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /workspaces/openssl-master/crypto/asn1/tasn_enc.c:631:24 in asn1_ex_i2c
==1808607==ABORTING

This happens when the ASN1 structure contains an element of type ASN1_TYPE without initializing its value (default constructed): ASN1_TYPE::type == -1 and ASN1_TYPE::value.asn1_value == NULL.

A simple reproducer for this issue:

#include <openssl/asn1t.h>
#include <openssl/pem.h>
#include <openssl/err.h>

typedef struct __Object_st {
    ASN1_TYPE *at;
} Object_st;

DECLARE_ASN1_FUNCTIONS(Object_st)

ASN1_SEQUENCE_cb(Object_st, NULL) = {
    ASN1_SIMPLE(Object_st, at, ASN1_ANY),
} ASN1_SEQUENCE_END_cb(Object_st, Object_st)

IMPLEMENT_ASN1_FUNCTIONS(Object_st)

int main(void){
    BIO *b64, *tmp;
    int ret;

    Object_st *asn1 = Object_st_new();

    BIO *out = BIO_new_file("/tmp/test.out", "w");
    
    b64 = BIO_new(BIO_f_base64());

    tmp = BIO_push(b64, out);
    ret = ASN1_item_i2d_bio(ASN1_ITEM_rptr(Object_st), tmp, (ASN1_VALUE *)asn1);
    if (!ret)
        ERR_print_errors_fp(stderr);

    BIO_flush(tmp);
    BIO_pop(tmp);
    BIO_free(b64);

    BIO_flush(out);
    BIO_free(out);
    Object_st_free(asn1);

    return 0;
}

[t8m]

please use strtmp == NULL instead

[t8m]

OK with CLA: trivial

[paulidale]

agreed trivial

[openssl-machine]

This pull request is ready to merge

[bernd-edlinger]

But when you e.g. do this you get a completely different crash:

Breakpoint 1, main () at repro.c:21
21	    Object_st *asn1 = Object_st_new();
(gdb) n
23	    BIO *out = BIO_new_file("/tmp/test.out", "w");
(gdb) p asn1->at.type = 2
$1 = 2
(gdb) c
Continuing.

Program received signal SIGSEGV, Segmentation fault.
0x00005555555a9f1a in i2c_ASN1_INTEGER (a=0x0, pp=0x0)
    at crypto/asn1/a_int.c:203
203	    return i2c_ibuf(a->data, a->length, a->type & V_ASN1_NEG, pp);
(gdb) bt
#0  0x00005555555a9f1a in i2c_ASN1_INTEGER (a=0x0, pp=0x0)
    at crypto/asn1/a_int.c:203
#1  0x0000555555591f8c in asn1_ex_i2c (pval=0x5555558152c8, cout=0x0, 
    putype=0x7fffffffddb0, it=0x5555557e59e0 <ASN1_ANY_it>)
    at crypto/asn1/tasn_enc.c:585
#2  0x0000555555591c4e in asn1_i2d_ex_primitive (pval=0x5555558152a0, out=0x0, 
    it=0x5555557e59e0 <ASN1_ANY_it>, tag=-1, aclass=0)
    at crypto/asn1/tasn_enc.c:461
#3  0x0000555555591028 in ASN1_item_ex_i2d (pval=0x5555558152a0, out=0x0, 
    it=0x5555557e59e0 <ASN1_ANY_it>, tag=-1, aclass=0)
    at crypto/asn1/tasn_enc.c:103
#4  0x000055555559188d in asn1_template_ex_i2d (pval=0x5555558152a0, out=0x0, 
    tt=0x5555558058a0 <Object_st_seq_tt>, tag=-1, iclass=0)
    at crypto/asn1/tasn_enc.c:360
#5  0x000055555559133a in ASN1_item_ex_i2d (pval=0x7fffffffdfa8, out=0x0, 
    it=0x5555557e55a0 <Object_st_it>, tag=16, aclass=0)
    at crypto/asn1/tasn_enc.c:179
#6  0x0000555555590e3f in asn1_item_flags_i2d (val=0x5555558152a0, 
    out=0x7fffffffe038, it=0x5555557e55a0 <Object_st_it>, flags=0)
    at crypto/asn1/tasn_enc.c:61
#7  0x0000555555590deb in ASN1_item_i2d (val=0x5555558152a0, 
    out=0x7fffffffe038, it=0x5555557e55a0 <Object_st_it>)
    at crypto/asn1/tasn_enc.c:44
--Type <RET> for more, q to quit, c to continue without paging--Quit
(gdb) 

[bernd-edlinger]

Would something like this also work for you?

--- a/crypto/asn1/tasn_enc.c
+++ b/crypto/asn1/tasn_enc.c
@@ -565,6 +565,7 @@ static int asn1_ex_i2c(const ASN1_VALUE **pval, unsigned char *cout, int *putype
             return -1;
         break;
 
+    case V_ASN1_UNDEF:
     case V_ASN1_NULL:
         cont = NULL;
         len = 0;

[AITsygunka]

Would something like this also work for you?

--- a/crypto/asn1/tasn_enc.c
+++ b/crypto/asn1/tasn_enc.c
@@ -565,6 +565,7 @@ static int asn1_ex_i2c(const ASN1_VALUE **pval, unsigned char *cout, int *putype
             return -1;
         break;
 
+    case V_ASN1_UNDEF:
     case V_ASN1_NULL:
         cont = NULL;
         len = 0;

Thanks, this works fine for me. But maybe this variant would be more correct?

--- a/crypto/asn1/tasn_enc.c
+++ b/crypto/asn1/tasn_enc.c
@@ -565,6 +565,9 @@ static int asn1_ex_i2c(const ASN1_VALUE **pval, unsigned char *cout, int *putype
             return -1;
         break;
 
+    case V_ASN1_UNDEF:
+        return -2;
+
     case V_ASN1_NULL:
         cont = NULL;
         len = 0;

openssl/crypto/asn1/tasn_enc.c

Lines 493 to 498 in 9505105

	/* -2 return is special meaning use ndef */
	if (len == -2) {
	ndef = 2;
	len = 0;
	}

[bernd-edlinger]

Yeah, looks good.

[bernd-edlinger]

Thanks, could you please also squash the 3 commits into one, and maybe update the commit message to
e.g. say that this adds handling of V_ASN1_UNDEF to avoid NULL dereference ?

[bernd-edlinger]

Very nice, thanks!

[t8m]

Still OK with CLA: trivial

[openssl-machine]

This pull request is ready to merge

[t8m]

Merged to all the active branches. Thank you for your contribution.