Re: missing malloc failure check at /src/lib/libcrypto/asn1/bio_ndef.c

[Theo Buehler]

On Sun, May 14, 2023 at 05:51:16PM +0200, Илья Шипицин wrote:
> patch attached.

Thank you. While we could add these malloc checks, I do not think it is
enough. For example, derlen could be <= 0 after the first call and the
second call to ASN1_item_ndef_i2d() is not guaranteed to succeed and to
return the same derlen. All this should be checked.

We have started cleaning up the NDEF/SMIME code which will be helped by
the fact that we hid the general code from the public API surface. There
is a lot of work still to do. One of the very next steps should be to
add decent test coverage.

I think the below is closer to a step in the right direction. The idea
is that if p = NULL, ASN1_item_ndef_i2d(..., &p, ...) will do the
allocation internally and check that the derlen is consistent in the two
calls.

However, I would rather have better test coverage before changing this
code. Ownership in particular is extremely tricky here.

Index: asn1/bio_ndef.c
===================================================================
RCS file: /cvs/src/lib/libcrypto/asn1/bio_ndef.c,v
retrieving revision 1.22
diff -u -p -r1.22 bio_ndef.c
--- asn1/bio_ndef.c     25 Apr 2023 19:08:30 -0000      1.22
+++ asn1/bio_ndef.c     16 May 2023 12:39:13 -0000
@@ -171,7 +171,7 @@ static int
 ndef_prefix(BIO *b, unsigned char **pbuf, int *plen, void *parg)
 {
        NDEF_SUPPORT *ndef_aux;
-       unsigned char *p;
+       unsigned char *p = NULL;
        int derlen;
 
        if (!parg)
@@ -179,13 +179,13 @@ ndef_prefix(BIO *b, unsigned char **pbuf
 
        ndef_aux = *(NDEF_SUPPORT **)parg;
 
-       derlen = ASN1_item_ndef_i2d(ndef_aux->val, NULL, ndef_aux->it);
-       p = malloc(derlen);
+       if ((derlen = ASN1_item_ndef_i2d(ndef_aux->val, &p, ndef_aux->it)) <= 0)
+               return 0;
+
        ndef_aux->derbuf = p;
        *pbuf = p;
-       derlen = ASN1_item_ndef_i2d(ndef_aux->val, &p, ndef_aux->it);
 
-       if (!*ndef_aux->boundary)
+       if (*ndef_aux->boundary == NULL)
                return 0;
 
        *plen = *ndef_aux->boundary - *pbuf;
@@ -231,7 +231,7 @@ static int
 ndef_suffix(BIO *b, unsigned char **pbuf, int *plen, void *parg)
 {
        NDEF_SUPPORT *ndef_aux;
-       unsigned char *p;
+       unsigned char *p = NULL;
        int derlen;
        const ASN1_AUX *aux;
        ASN1_STREAM_ARG sarg;
@@ -251,14 +251,15 @@ ndef_suffix(BIO *b, unsigned char **pbuf
            &ndef_aux->val, ndef_aux->it, &sarg) <= 0)
                return 0;
 
-       derlen = ASN1_item_ndef_i2d(ndef_aux->val, NULL, ndef_aux->it);
-       p = malloc(derlen);
+       if ((derlen = ASN1_item_ndef_i2d(ndef_aux->val, &p, ndef_aux->it)) <= 0)
+               return 0;
+
        ndef_aux->derbuf = p;
        *pbuf = p;
-       derlen = ASN1_item_ndef_i2d(ndef_aux->val, &p, ndef_aux->it);
 
-       if (!*ndef_aux->boundary)
+       if (*ndef_aux->boundary == NULL)
                return 0;
+
        *pbuf = *ndef_aux->boundary;
        *plen = derlen - (*ndef_aux->boundary - ndef_aux->derbuf);