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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel