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 <[email protected]>:
> 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);
>
>
