[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2
From: |
Daniel Kiper |
Subject: |
Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2 |
Date: |
Tue, 25 Jun 2024 14:49:42 +0200 |
User-agent: |
NeoMutt/20170113 (1.7.2) |
On Tue, Jun 25, 2024 at 02:42:31PM +0800, Gary Lin wrote:
> On Mon, Jun 24, 2024 at 07:28:14PM +0200, Daniel Kiper wrote:
> > On Thu, Mar 07, 2024 at 04:59:05PM +0800, Gary Lin via Grub-devel wrote:
> > > On Thu, Feb 08, 2024 at 08:58:43PM +0100, Daniel Kiper wrote:
> > > > Hey,
> > > >
> > > --8<--
> > > >
> > > > And I have attached the Coverity report. All issues reported there have
> > > > to be fixed. If you cannot fix an issue you have to explain why you
> > > > cannot do that and what is potential impact on the code stability,
> > > > security, etc.
> > > >
> > > I have went through all the coverity issues. There are 6 issues in the
> > > TPM2 stack and the utility:
> > >
> > > CID 435775, CID 435771, CID 435770, CID 435769, CID 435767, CID 435761
> > >
> > > Those will be fixed in the next version.
> > >
> > > The 9 issues are from libtasn1 and gnulib.
> > >
> > > There are two memory corruption issue: CID 435762 and CID 435766, and
> > > I've filed the issues in libtasn1 upstream.
> > >
> > > - CID 435762
> > > https://gitlab.com/gnutls/libtasn1/-/issues/49
> > > This is a valid case. However, the only exploitable function,
> > > _asn1_insert_tag_der(), is disabled in grub2 patch, so the severity is
> > > low. I have a quick fix but upstream would like to fix it in another
> > > way.
> > > - CID 435766
> > > https://gitlab.com/gnutls/libtasn1/-/issues/50
> > > IMO, this is false positive, but I need libtasn1 upstream to confirm
> > > that.
> > >
> > > Then, the remaining 7 Integer handling issues are involved with the macros
> > > from gnulib, and I believe those are false positive.
> > >
> > > The following are my analyses:
> > >
> > > ________________________________________________________________________________________________________
> > > *** CID 435774: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
> > > /grub-core/lib/libtasn1/lib/decoding.c: 481 in asn1_get_object_id_der()
> > > 475 */
> > > 476 if (leading != 0 && der[len_len + k] == 0x80)
> > > 477 return ASN1_DER_ERROR;
> > > 478 leading = 0;
> > > 479
> > > 480 /* check for wrap around */
> > > >>> CID 435774: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
> > > >>> "val < ((((1 ? 0 : val) - 1 < 0) ? ~((((1 ? 0 : val) + 1 << 62UL
> > > >>> /* sizeof (+val) * 8 - 2 */) - 1) * 2 + 1) : ((1 ? 0 : val) + 0)) >>
> > > >>> 7)" is always false regardless of the values of its operands. This
> > > >>> occurs as the second operand of "?:".
> > > 481 if (INT_LEFT_SHIFT_OVERFLOW (val, 7))
> > > 482 return ASN1_DER_ERROR;
> > > 483
> > > 484 val = val << 7;
> > > 485 val |= der[len_len + k] & 0x7F;
> > > 486
> > >
> > > Here are the related macros:
> > >
> > > #define EXPR_SIGNED(e) (_GL_INT_NEGATE_CONVERT (e, 1) < 0)
> > >
> > > #define _GL_INT_CONVERT(e, v) ((1 ? 0 : (e)) + (v))
> > >
> > > #define _GL_SIGNED_INT_MAXIMUM(e) \
> > > (((_GL_INT_CONVERT (e, 1) << (TYPE_WIDTH (+ (e)) - 2)) - 1) * 2 + 1)
> > >
> > > #define _GL_INT_MINIMUM(e) \
> > > (EXPR_SIGNED (e) \
> > > ? ~ _GL_SIGNED_INT_MAXIMUM (e) \
> > > : _GL_INT_CONVERT (e, 0))
> > >
> > > #define INT_LEFT_SHIFT_OVERFLOW(a, b) \
> > > INT_LEFT_SHIFT_RANGE_OVERFLOW (a, b, \
> > > _GL_INT_MINIMUM (a), _GL_INT_MAXIMUM (a))
> > >
> > > #define INT_LEFT_SHIFT_RANGE_OVERFLOW(a, b, min, max) \
> > > ((a) < 0 \
> > > ? (a) < (min) >> (b) \
> > > : (max) >> (b) < (a))
> > >
> > > The statement in question is expanded "(a) < (min) >> (b)" of
> > > INT_LEFT_SHIFT_RANGE_OVERFLOW. Since 'val' is 'uint64_t', '(a) < 0' is
> > > always
> > > false, so the result of that statement doen't matter.
> >
> > Something is missing and/or requires clarification/expansion here.
> > AFAICT at least your description completely ignores "(max) >> (b) < (a)"
> > expression result.
> >
> Because Coverity only complained "(a) < (min) >> (b)".
> "(max) >> (b) < (a)" does the right thing to check whether 'a' is
> overflowed after the left shift.
OK, then something like above should be added to the CID description then.
> > > ________________________________________________________________________________________________________
> > > *** CID 435773: Integer handling issues (NO_EFFECT)
> > > /grub-core/lib/libtasn1/lib/decoding.c: 439 in asn1_get_object_id_der()
> > > 433 return ASN1_DER_ERROR;
> > > 434
> > > 435 val0 = 0;
> > > 436
> > > 437 for (k = 0; k < len; k++)
> > > 438 {
> > > >>> CID 435773: Integer handling issues (NO_EFFECT)
> > > >>> This less-than-zero comparison of an unsigned value is never
> > > >>> true. "(1 ? 0UL : val0) - 1UL < 0UL".
> > > 439 if (INT_LEFT_SHIFT_OVERFLOW (val0, 7))
> > > 440 return ASN1_DER_ERROR;
> > > 441
> > > 442 val0 <<= 7;
> > > 443 val0 |= der[len_len + k] & 0x7F;
> > > 444 if (!(der[len_len + k] & 0x80))
> > >
> > > Here are the related macros:
> > >
> > > #define _GL_INT_NEGATE_CONVERT(e, v) ((1 ? 0 : (e)) - (v))
> > > #define EXPR_SIGNED(e) (_GL_INT_NEGATE_CONVERT (e, 1) < 0)
> > >
> > > The statement in question is the expanded 'EXPR_SIGNED'. Since that macro
> > > is
> > > to test whether the variable is signed or not. For 'uint64_t val0', it's
> > > expected to be always false.
> > >
> > > ________________________________________________________________________________________________________
> > > *** CID 435772: Integer handling issues (NO_EFFECT)
> > > /grub-core/lib/libtasn1/lib/decoding.c: 204 in asn1_get_tag_der()
> > > 198 /* Long form */
> > > 199 punt = 1;
> > > 200 ris = 0;
> > > 201 while (punt < der_len && der[punt] & 128)
> > > 202 {
> > > 203
> > > >>> CID 435772: Integer handling issues (NO_EFFECT)
> > > >>> This less-than-zero comparison of an unsigned value is never
> > > >>> true. "(1 ? 0U : ((1 ? 0U : ris) + 128U)) - 1U < 0U".
> > > 204 if (INT_MULTIPLY_OVERFLOW (ris, 128))
> > > 205 return ASN1_DER_ERROR;
> > > 206 ris *= 128;
> > > 207
> > > 208 if (INT_ADD_OVERFLOW (ris, ((unsigned) (der[punt] & 0x7F))))
> > > 209 return ASN1_DER_ERROR;
> > >
> > > Here are the related macros:
> > >
> > > #define _GL_INT_CONVERT(e, v) ((1 ? 0 : (e)) + (v))
> > > #define _GL_INT_NEGATE_CONVERT(e, v) ((1 ? 0 : (e)) - (v))
> > > #define EXPR_SIGNED(e) (_GL_INT_NEGATE_CONVERT (e, 1) < 0)
> > >
> > > The statement in question is the expanded 'EXPR_SIGNED(_GL_INT_CONVERT
> > > (ris, 128))'.
> > > Since 'ris' is 'unsigned int', it's expected that 'EXPR_SIGNED' always
> > > returns false.
> >
> > At least INT_MULTIPLY_OVERFLOW definitions is missing.
> >
> I got to admit that lots of details were skipped since the expansion of
> INT_MULTIPLY_OVERFLOW is really lengthy.
>
> I'll add the details back in my v18 patchset cover letter.
Cool! Thanks!
Daniel