From: Nakayama Kenjiro <nakayamakenjiro@gmail.com>
Subject: Re: [PATCH] LibreSSL: move INT_MAX check before memory allocation in asn1_item_sign()
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Mon, 14 Apr 2025 16:57:43 +0900

Thank you.
This is an issue I noticed while reading the code. It didn’t actually occur
in practice.
You're right that the chance of it happening is extremely low, and having
the check right before ASN1_STRING_set0 does make it clearer.
So I think it’s fine to reject my patch and leave it as it is.


On Mon, Apr 14, 2025 at 4:29 PM Theo Buehler <tb@theobuehler.org> wrote:

> On Mon, Apr 14, 2025 at 01:37:17PM +0900, Kenjiro Nakayama wrote:
> > Reordered the out_len > INT_MAX check in asn1_item_sign() to occur
> > before calling calloc(), ensuring that big size allocations are
> > avoided earlier.
> >
> > This change has no functional change in logic but only improved
> > ordering of error handling for efficiency.
>
> The reason we check right before ASN1_STRING_set0(..., int len) is that
> out_len is truncated to an there. So I would like to keep a check there.
>
> We could add a second check where you suggest it, but I think it is
> overdoing it. I don't think this check can realistically be hit - it
> means that we deal with ASN.1 item 'asn' that is somewhere in the
> vicinity of 2GB already.
>
> Did you actually hit this or is this from code reading?
>
> >
> > Signed-off-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com>
> > ---
> >  src/lib/libcrypto/asn1/asn1_item.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git src/lib/libcrypto/asn1/asn1_item.c
> src/lib/libcrypto/asn1/asn1_item.c
> > index 86c800e3a..c9176e65c 100644
> > --- src/lib/libcrypto/asn1/asn1_item.c
> > +++ src/lib/libcrypto/asn1/asn1_item.c
> > @@ -298,6 +298,10 @@ asn1_item_sign(EVP_MD_CTX *ctx, const ASN1_ITEM
> *it, void *asn,
> >               ASN1error(ERR_R_EVP_LIB);
> >               goto err;
> >       }
> > +     if (out_len > INT_MAX) {
> > +             ASN1error(ASN1_R_TOO_LONG);
> > +             goto err;
> > +     }
> >       if ((out = calloc(1, out_len)) == NULL) {
> >               ASN1error(ERR_R_MALLOC_FAILURE);
> >               goto err;
> > @@ -307,11 +311,6 @@ asn1_item_sign(EVP_MD_CTX *ctx, const ASN1_ITEM
> *it, void *asn,
> >               goto err;
> >       }
> >
> > -     if (out_len > INT_MAX) {
> > -             ASN1error(ASN1_R_TOO_LONG);
> > -             goto err;
> > -     }
> > -
> >       ASN1_STRING_set0(signature, out, out_len);
> >       out = NULL;
> >
> > --
> > 2.39.5 (Apple Git-154)
> >
>


-- 
Kenjiro NAKAYAMA <nakayamakenjiro@gmail.com>
GPG Key fingerprint = ED8F 049D E67A 727D 9A44  8E25 F44B E208 C946 5EB9