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. ________________________________________________________________________________________________________ *** 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. ________________________________________________________________________________________________________ *** CID 435768: (CONSTANT_EXPRESSION_RESULT) /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 435768: (CONSTANT_EXPRESSION_RESULT) >>> "ris < (((1 ? 0 : ((1 ? 0 : ris) + 128)) - 1 < 0) ? ~((((1 ? 0 : ((1 ? >>> 0 : ris) + 128)) + 1 << 30UL /* sizeof (+((1 ? 0 : ris) + 128)) * 8 - 2 */) >>> - 1) * 2 + 1) : ((1 ? 0 : ((1 ? 0 : ris) + 128)) + 0)) / 128" is always >>> false regardless of the values of its operands. This occurs as the second >>> operand of "?:". 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; /grub-core/lib/libtasn1/lib/decoding.c: 217 in asn1_get_tag_der() 211 punt++; 212 } 213 214 if (punt >= der_len) 215 return ASN1_DER_ERROR; 216 >>> CID 435768: (CONSTANT_EXPRESSION_RESULT) >>> "ris < (((1 ? 0 : ((1 ? 0 : ris) + 128)) - 1 < 0) ? ~((((1 ? 0 : ((1 ? >>> 0 : ris) + 128)) + 1 << 30UL /* sizeof (+((1 ? 0 : ris) + 128)) * 8 - 2 */) >>> - 1) * 2 + 1) : ((1 ? 0 : ((1 ? 0 : ris) + 128)) + 0)) / 128" is always >>> false regardless of the values of its operands. This occurs as the second >>> operand of "?:". 217 if (INT_MULTIPLY_OVERFLOW (ris, 128)) 218 return ASN1_DER_ERROR; 219 ris *= 128; 220 221 if (INT_ADD_OVERFLOW (ris, ((unsigned) (der[punt] & 0x7F)))) 222 return ASN1_DER_ERROR; Here are the related macros: #define _GL_INT_CONVERT(e, v) ((1 ? 0 : (e)) + (v)) #define EXPR_SIGNED(e) (_GL_INT_NEGATE_CONVERT (e, 1) < 0) #define _GL_INT_MINIMUM(e) \ (EXPR_SIGNED (e) \ ? ~ _GL_SIGNED_INT_MAXIMUM (e) \ : _GL_INT_CONVERT (e, 0)) #define INT_MULTIPLY_RANGE_OVERFLOW(a, b, min, max) \ ((b) < 0 \ ? ((a) < 0 \ ? (a) < (max) / (b) \ : (b) == -1 \ ? 0 \ : (min) / (b) < (a)) \ : (b) == 0 \ ? 0 \ : ((a) < 0 \ ? (a) < (min) / (b) \ : (max) / (b) < (a))) # define _GL_MULTIPLY_OVERFLOW(a, b, min, max) \ (((min) == 0 && (((a) < 0 && 0 < (b)) || ((b) < 0 && 0 < (a)))) \ || INT_MULTIPLY_RANGE_OVERFLOW (a, b, min, max)) #define _GL_BINARY_OP_OVERFLOW(a, b, op_result_overflow) \ op_result_overflow (a, b, \ _GL_INT_MINIMUM (_GL_INT_CONVERT (a, b)), \ _GL_INT_MAXIMUM (_GL_INT_CONVERT (a, b))) #define INT_MULTIPLY_OVERFLOW(a, b) \ _GL_BINARY_OP_OVERFLOW (a, b, _GL_MULTIPLY_OVERFLOW) The statement in question is expanded from '(a) < (min) / (b)' in INT_MULTIPLY_RANGE_OVERFLOW. '(a) < (min) / (b)' => '(a) < (_GL_INT_MINIMUM (_GL_INT_CONVERT (a, b))) / (b)' => '(ris) < (_GL_INT_MINIMUM (_GL_INT_CONVERT (ris, 128))) / (128)' Since 'ris' is 'unsigned int', '(a) < 0' is always false, so the result of '(a) < (min) / (b)' doesn't matter. ________________________________________________________________________________________________________ *** CID 435765: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /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 435765: Integer handling issues (CONSTANT_EXPRESSION_RESULT) >>> "val0 < ((((1 ? 0 : val0) - 1 < 0) ? ~((((1 ? 0 : val0) + 1 << 62UL /* >>> sizeof (+val0) * 8 - 2 */) - 1) * 2 + 1) : ((1 ? 0 : val0) + 0)) >> 7)" is >>> always false regardless of the values of its operands. This occurs as the >>> second operand of "?:". 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 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 'val0' is 'uint64_t', '(a) < 0' is always false, so the result of that statement doesn't matter. ________________________________________________________________________________________________________ *** CID 435764: Integer handling issues (NO_EFFECT) /grub-core/lib/libtasn1/lib/decoding.c: 137 in asn1_get_length_der() 131 punt = 1; 132 if (k) 133 { /* definite length method */ 134 ans = 0; 135 while (punt <= k && punt < der_len) 136 { >>> CID 435764: Integer handling issues (NO_EFFECT) >>> This less-than-zero comparison of an unsigned value is never true. "(1 >>> ? 0U : ((1 ? 0U : ans) + 256U)) - 1U < 0U". 137 if (INT_MULTIPLY_OVERFLOW (ans, 256)) 138 return -2; 139 ans *= 256; 140 141 if (INT_ADD_OVERFLOW (ans, ((unsigned) der[punt]))) 142 return -2; 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) #define _GL_INT_MINIMUM(e) \ (EXPR_SIGNED (e) \ ? ~ _GL_SIGNED_INT_MAXIMUM (e) \ : _GL_INT_CONVERT (e, 0)) #define _GL_INT_MAXIMUM(e) \ (EXPR_SIGNED (e) \ ? _GL_SIGNED_INT_MAXIMUM (e) \ : _GL_INT_NEGATE_CONVERT (e, 1)) #define _GL_BINARY_OP_OVERFLOW(a, b, op_result_overflow) \ op_result_overflow (a, b, \ _GL_INT_MINIMUM (_GL_INT_CONVERT (a, b)), \ _GL_INT_MAXIMUM (_GL_INT_CONVERT (a, b))) The statement in question is EXPR_SIGNED from either _GL_INT_MINIMUM or _GL_INT_MAXIMUM. EXPR_SIGNED (_GL_INT_CONVERT (ans, 256)) => _GL_INT_NEGATE_CONVERT (_GL_INT_CONVERT (ans, 256), 1) < 0 The macro is used to test whether ans and 256 are signed or not, so it's expected to be always false. ________________________________________________________________________________________________________ *** CID 435763: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /grub-core/lib/libtasn1/lib/decoding.c: 137 in asn1_get_length_der() 131 punt = 1; 132 if (k) 133 { /* definite length method */ 134 ans = 0; 135 while (punt <= k && punt < der_len) 136 { >>> CID 435763: Integer handling issues (CONSTANT_EXPRESSION_RESULT) >>> "ans < (((1 ? 0 : ((1 ? 0 : ans) + 256)) - 1 < 0) ? ~((((1 ? 0 : ((1 ? >>> 0 : ans) + 256)) + 1 << 30UL /* sizeof (+((1 ? 0 : ans) + 256)) * 8 - 2 */) >>> - 1) * 2 + 1) : ((1 ? 0 : ((1 ? 0 : ans) + 256)) + 0)) / 256" is always >>> false regardless of the values of its operands. This occurs as the second >>> operand of "?:". 137 if (INT_MULTIPLY_OVERFLOW (ans, 256)) 138 return -2; 139 ans *= 256; 140 141 if (INT_ADD_OVERFLOW (ans, ((unsigned) der[punt]))) 142 return -2; Here are the related macros: #define _GL_INT_CONVERT(e, v) ((1 ? 0 : (e)) + (v)) #define EXPR_SIGNED(e) (_GL_INT_NEGATE_CONVERT (e, 1) < 0) #define _GL_INT_MINIMUM(e) \ (EXPR_SIGNED (e) \ ? ~ _GL_SIGNED_INT_MAXIMUM (e) \ : _GL_INT_CONVERT (e, 0)) #define INT_MULTIPLY_RANGE_OVERFLOW(a, b, min, max) \ ((b) < 0 \ ? ((a) < 0 \ ? (a) < (max) / (b) \ : (b) == -1 \ ? 0 \ : (min) / (b) < (a)) \ : (b) == 0 \ ? 0 \ : ((a) < 0 \ ? (a) < (min) / (b) \ : (max) / (b) < (a))) # define _GL_MULTIPLY_OVERFLOW(a, b, min, max) \ (((min) == 0 && (((a) < 0 && 0 < (b)) || ((b) < 0 && 0 < (a)))) \ || INT_MULTIPLY_RANGE_OVERFLOW (a, b, min, max)) #define _GL_BINARY_OP_OVERFLOW(a, b, op_result_overflow) \ op_result_overflow (a, b, \ _GL_INT_MINIMUM (_GL_INT_CONVERT (a, b)), \ _GL_INT_MAXIMUM (_GL_INT_CONVERT (a, b))) #define INT_MULTIPLY_OVERFLOW(a, b) \ _GL_BINARY_OP_OVERFLOW (a, b, _GL_MULTIPLY_OVERFLOW) The statement in question is expanded from '(a) < (min) / (b)' in INT_MULTIPLY_RANGE_OVERFLOW. '(a) < (min) / (b)' => '(a) < (_GL_INT_MINIMUM (_GL_INT_CONVERT (a, b))) / (b)' => '(ans) < (_GL_INT_MINIMUM (_GL_INT_CONVERT (ans, 256))) / (256)' Since 'ans' is 'unsigned int', '(a) < 0' is always false, so the result of '(a) < (min) / (b)' doesn't matter. _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel