I'm totally fine with your approach. I tried to find "missing malloc null check" using the following coccinelle script (easy to run from within CI)
malloc.cocci: // find calls to malloc @call@ expression ptr; position p; @@ ptr@p = malloc(...); // find ok calls to malloc @ok@ expression ptr; position call.p; @@ ptr@p = malloc(...); ... when != ptr ( (ptr == NULL || ...) | (ptr != NULL || ...) ) // fix bad calls to malloc @depends on !ok@ expression ptr; position call.p; @@ ptr@p = malloc(...); + if (ptr == NULL) return; ~/portable/libressl-3.8.0$ spatch --sp-file malloc.cocci --dir . ... HANDLING: ./apps/openssl/apps.c diff = diff -u -p a/apps/openssl/apps.c b/apps/openssl/apps.c --- a/apps/openssl/apps.c +++ b/apps/openssl/apps.c @@ -379,6 +379,8 @@ password_callback(char *buf, int bufsiz, PW_MIN_LENGTH, bufsiz - 1); if (ok >= 0 && verify) { buff = malloc(bufsiz); +if (buff == NULL) +return; ok = UI_add_verify_string(ui, prompt, ui_flags, buff, PW_MIN_LENGTH, bufsiz - 1, buf); } ... HANDLING: ./crypto/asn1/bio_ndef.c diff = diff -u -p a/crypto/asn1/bio_ndef.c b/crypto/asn1/bio_ndef.c --- a/crypto/asn1/bio_ndef.c +++ b/crypto/asn1/bio_ndef.c @@ -181,6 +181,8 @@ ndef_prefix(BIO *b, unsigned char **pbuf derlen = ASN1_item_ndef_i2d(ndef_aux->val, NULL, ndef_aux->it); p = malloc(derlen); +if (p == NULL) +return; ndef_aux->derbuf = p; *pbuf = p; derlen = ASN1_item_ndef_i2d(ndef_aux->val, &p, ndef_aux->it); @@ -253,6 +255,8 @@ ndef_suffix(BIO *b, unsigned char **pbuf derlen = ASN1_item_ndef_i2d(ndef_aux->val, NULL, ndef_aux->it); p = malloc(derlen); +if (p == NULL) +return; ndef_aux->derbuf = p; *pbuf = p; derlen = ASN1_item_ndef_i2d(ndef_aux->val, &p, ndef_aux->it); ... HANDLING: ./tests/ecdhtest.c diff = diff -u -p a/tests/ecdhtest.c b/tests/ecdhtest.c --- a/tests/ecdhtest.c +++ b/tests/ecdhtest.c @@ -147,6 +147,8 @@ test_ecdh_curve(int nid, const char *tex alen = KDF1_SHA1_len; abuf = malloc(alen); +if (abuf == NULL) +return; aout = ECDH_compute_key(abuf, alen, EC_KEY_get0_public_key(b), a, KDF1_SHA1); @@ -155,6 +157,8 @@ test_ecdh_curve(int nid, const char *tex blen = KDF1_SHA1_len; bbuf = malloc(blen); +if (bbuf == NULL) +return; bout = ECDH_compute_key(bbuf, blen, EC_KEY_get0_public_key(a), b, KDF1_SHA1); @@ -345,6 +349,8 @@ ecdh_kat(BIO *out, const char *cname, in if (Ztmplen != Zlen) goto err; Ztmp = malloc(Ztmplen); +if (Ztmp == NULL) +return; if (!ECDH_compute_key(Ztmp, Ztmplen, EC_KEY_get0_public_key(key2), key1, 0)) goto err; ..... HANDLING: ./ssl/d1_pkt.c diff = diff -u -p a/ssl/d1_pkt.c b/ssl/d1_pkt.c --- a/ssl/d1_pkt.c +++ b/ssl/d1_pkt.c @@ -214,6 +214,8 @@ dtls1_buffer_record(SSL *s, record_pqueu return 0; rdata = malloc(sizeof(DTLS1_RECORD_DATA_INTERNAL)); +if (rdata == NULL) +return; item = pitem_new(priority, rdata); if (rdata == NULL || item == NULL) goto init_err; @@ -260,6 +262,8 @@ dtls1_buffer_rcontent(SSL *s, rcontent_p return 0; rdata = malloc(sizeof(DTLS1_RCONTENT_DATA_INTERNAL)); +if (rdata == NULL) +return; item = pitem_new(priority, rdata); if (rdata == NULL || item == NULL) goto init_err; вт, 16 мая 2023 г. в 15:18, Theo Buehler <t...@theobuehler.org>: > 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); > >