[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2
|
From: |
Gary Lin |
|
Subject: |
Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2 |
|
Date: |
Thu, 7 Mar 2024 16:59:05 +0800 |
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.
| [Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2,
Gary Lin <=