Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2

[Daniel Kiper]

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.

> ________________________________________________________________________________________________________
> *** 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.

Please double check other analysis too. If you provide correct analysis
for all non-fixable CIDs I will mark them as ignored. It would be nice
if updated analysis is a part of latest version of the patch set.

Daniel