Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2
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".
>
Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2
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.
> >
> > *** 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))
> > #d
Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2
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/decod
Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2
On Thu, Apr 04, 2024 at 11:03:47PM +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: > > [...] > > Any progress on this? You are blocking another patch set which depends > on some code which you introduce. If there is no progress here I will > ask an author of the other patch set to resume the work and queue your > patch set as a second one to merge. > I was waiting for upstream fix for CID 435762(*). It can be fixed by tweaking an if statement slightly but upstream prefers code refactoring to remove the loops. Anyway, the only potentially vulnerable call path is disabled in my patch set, so we can choose to leave the issue for the later update or just apply a quick fix. Cheers, Gary Lin (*) https://gitlab.com/gnutls/libtasn1/-/issues/49 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2
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: [...] Any progress on this? You are blocking another patch set which depends on some code which you introduce. If there is no progress here I will ask an author of the other patch set to resume the work and queue your patch set as a second one to merge. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2
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_
Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2
On Wed, Feb 21, 2024 at 04:10:14PM +0800, Gary Lin via Grub-devel wrote: > On Thu, Feb 08, 2024 at 08:58:43PM +0100, Daniel Kiper wrote: > > Hey, > > > > Adding Patrick... > > > > On Mon, Feb 05, 2024 at 03:39:33PM +0800, Gary Lin wrote: > > > GIT repo for v9: https://github.com/lcp/grub2/tree/tpm2-unlock-v9 > > > > > > This patch series is based on "Automatic TPM Disk Unlock"(*1) posted by > > > Hernan Gatta to introduce the key protector framework and TPM2 stack > > > to GRUB2, and this could be a useful feature for the systems to > > > implement full disk encryption. > > > > Sadly this patch set have many issues... > > > > The git complains in the following way... > > > > Applying: asn1_test: test module for libtasn1 > > .git/rebase-apply/patch:1374: new blank line at EOF. > > warning: 1 line adds whitespace errors. > Will be fixed in v10. > > > Applying: libtasn1: Add the documentation > > .git/rebase-apply/patch:90: trailing whitespace. > > .git/rebase-apply/patch:92: trailing whitespace. > > .git/rebase-apply/patch:99: trailing whitespace. > > .git/rebase-apply/patch:102: trailing whitespace. > > .git/rebase-apply/patch:108: trailing whitespace. > > warning: squelched 80 whitespace errors > > warning: 85 lines add whitespace errors. > > > Those whitespaces naturally exist in the two 'patch' files added in this > commit, so it's hard to avoid the warning. OK, please ignore my comment then... Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2
On Thu, Feb 08, 2024 at 08:58:43PM +0100, Daniel Kiper wrote:
> Hey,
>
> Adding Patrick...
>
> On Mon, Feb 05, 2024 at 03:39:33PM +0800, Gary Lin wrote:
> > GIT repo for v9: https://github.com/lcp/grub2/tree/tpm2-unlock-v9
> >
> > This patch series is based on "Automatic TPM Disk Unlock"(*1) posted by
> > Hernan Gatta to introduce the key protector framework and TPM2 stack
> > to GRUB2, and this could be a useful feature for the systems to
> > implement full disk encryption.
>
> Sadly this patch set have many issues...
>
> The git complains in the following way...
>
> Applying: asn1_test: test module for libtasn1
> .git/rebase-apply/patch:1374: new blank line at EOF.
> warning: 1 line adds whitespace errors.
Will be fixed in v10.
> Applying: libtasn1: Add the documentation
> .git/rebase-apply/patch:90: trailing whitespace.
> .git/rebase-apply/patch:92: trailing whitespace.
> .git/rebase-apply/patch:99: trailing whitespace.
> .git/rebase-apply/patch:102: trailing whitespace.
> .git/rebase-apply/patch:108: trailing whitespace.
> warning: squelched 80 whitespace errors
> warning: 85 lines add whitespace errors.
>
Those whitespaces naturally exist in the two 'patch' files added in this
commit, so it's hard to avoid the warning.
> The developers manual does not build due to following errors...
>
> grub-dev.texi:616: misplaced {
> grub-dev.texi:616: misplaced }
> grub-dev.texi:617: misplaced {
> grub-dev.texi:617: misplaced }
> grub-dev.texi:580: warning: node `libtasn1' is next for `minilzo' in
> sectioning but not in menu
> grub-dev.texi:599: warning: unreferenced node `libtasn1'
> grub-dev.texi:599: warning: node `minilzo' is prev for `libtasn1' in
> sectioning but not in menu
> grub-dev.texi:599: warning: node `Updating External Code' is up for
> `libtasn1' in sectioning but not in menu
> grub-dev.texi:499: node `Updating External Code' lacks menu item for
> `libtasn1' despite being its Up target
>
Will be fixed in v10.
> 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'm working on that and will fix them in v10.
> Please do not forget to check code which you add adhere to GRUB coding
> style [1]. Good example is in grub-core/kern/efi/sb.c too. Of course
> this requirement does not apply to the libs which you import.
>
> Additionally, please CC patrick.c...@oracle.com next time. He is
> interested in this patch set development.
>
Added Patrick to my patch sending script.
Thanks,
Gary Lin
> Daniel
>
> [1]
> https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Coding-style
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2
Hey,
Adding Patrick...
On Mon, Feb 05, 2024 at 03:39:33PM +0800, Gary Lin wrote:
> GIT repo for v9: https://github.com/lcp/grub2/tree/tpm2-unlock-v9
>
> This patch series is based on "Automatic TPM Disk Unlock"(*1) posted by
> Hernan Gatta to introduce the key protector framework and TPM2 stack
> to GRUB2, and this could be a useful feature for the systems to
> implement full disk encryption.
Sadly this patch set have many issues...
The git complains in the following way...
Applying: asn1_test: test module for libtasn1
.git/rebase-apply/patch:1374: new blank line at EOF.
warning: 1 line adds whitespace errors.
Applying: libtasn1: Add the documentation
.git/rebase-apply/patch:90: trailing whitespace.
.git/rebase-apply/patch:92: trailing whitespace.
.git/rebase-apply/patch:99: trailing whitespace.
.git/rebase-apply/patch:102: trailing whitespace.
.git/rebase-apply/patch:108: trailing whitespace.
warning: squelched 80 whitespace errors
warning: 85 lines add whitespace errors.
The developers manual does not build due to following errors...
grub-dev.texi:616: misplaced {
grub-dev.texi:616: misplaced }
grub-dev.texi:617: misplaced {
grub-dev.texi:617: misplaced }
grub-dev.texi:580: warning: node `libtasn1' is next for `minilzo' in
sectioning but not in menu
grub-dev.texi:599: warning: unreferenced node `libtasn1'
grub-dev.texi:599: warning: node `minilzo' is prev for `libtasn1' in
sectioning but not in menu
grub-dev.texi:599: warning: node `Updating External Code' is up for
`libtasn1' in sectioning but not in menu
grub-dev.texi:499: node `Updating External Code' lacks menu item for
`libtasn1' despite being its Up target
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.
Please do not forget to check code which you add adhere to GRUB coding
style [1]. Good example is in grub-core/kern/efi/sb.c too. Of course
this requirement does not apply to the libs which you import.
Additionally, please CC patrick.c...@oracle.com next time. He is
interested in this patch set development.
Daniel
[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Coding-style
Hi,
Please find the latest report on new defect(s) introduced to GRUB found with
Coverity Scan.
15 new defect(s) introduced to GRUB found with Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 15 of 15 defect(s)
** CID 435775: Resource leaks (RESOURCE_LEAK)
/util/grub-protect.c: 982 in grub_protect_tpm2_add()
*** CID 435775: Resource leaks (RESOURCE_LEAK)
/util/grub-protect.c: 982 in grub_protect_tpm2_add()
976
977 if (key_size > TPM_MAX_SYM_DATA)
978 {
979 fprintf (stderr,
980 _("Input key is too long, maximum allowed size is %u
bytes.\n"),
981 TPM_MAX_SYM_DATA);
>>> CID 435775: Resource leaks (RESOURCE_LEAK)
>>> Variable "key" going out of scope leaks the storage it points to.
982 return GRUB_ERR_OUT_OF_RANGE;
983 }
984
985 err = grub_protect_tpm2_get_srk (args, &srk);
986 if (err != GRUB_ERR_NONE)
987 goto exit2;
** CID 435774: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
/grub-core/lib/libtasn1/lib/decoding.c: 481 in asn1_get_object_id_der()
*** 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
** CID 435773: Integer handling issues (NO_EFFECT)
/grub-core/lib/libtasn1/lib/decoding.c: 439 in asn1_get_object_id_der()
*** 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:
[PATCH v9 00/22] Automatic Disk Unlock with TPM2
GIT repo for v9: https://github.com/lcp/grub2/tree/tpm2-unlock-v9 This patch series is based on "Automatic TPM Disk Unlock"(*1) posted by Hernan Gatta to introduce the key protector framework and TPM2 stack to GRUB2, and this could be a useful feature for the systems to implement full disk encryption. To support TPM 2.0 Key File format(*2), patch 1~6 are grabbed from Daniel Axtens's "appended signature secure boot support" (*3) to import libtasn1 into grub2. Besides, the libtasn1 version is upgraded to 4.19.0 instead of 4.16.0 in the original patch. Patch 7 adds the document for libtasn1 and the steps to upgrade the library. Patch 8~12 are Hernan Gatta's patches with the follow-up fixes and improvements: - Converting 8 spaces into 1 tab - Merging the minor build fix from Michael Chang - Replacing "lu" with "PRIuGRUB_SIZE" for grub_dprintf - Adding "enable = efi" to the tpm2 module in grub-core/Makefile.core.def - Rebasing "cryptodisk: Support key protectors" to the git master - Removing the measurement on the sealed key - Based on the patch from Olaf Kirch - Adjusting the input parameters of TPM2_EvictControl to match the order in "TCG TPM2 Part3 Commands" - Declaring the input arguments of TPM2 functions as const - Resending TPM2 commands on TPM_RC_RETRY - Adding checks for the parameters of TPM2 commands - Packing the missing authorization command for TPM2_PCR_Read - Tweaking the TPM2 command functions to allow some parameters to be NULL so that we don't have to declare empty variables - Only enabling grub-protect for "efi" since the TPM2 stack currently relies on the EFI TCG2 protocol to send TPM2 commands - Using grub_cpu_to_be*() in the TPM2 stack instead of grub_swap_bytes*() which may cause problems in big-indian machines - Changing the short name of "--protector" of "cryptomount" from "-k" to "-P" to avoid the conflict with "--key-file" - Supporting TPM 2.0 Key File Format besides the raw sealed key - Adding the external libtasn1 dependency to grub-protect to write the TPM 2.0 Key files Patch 13~16 implement the authorized policy support. Patch 17 implements the missing NV index mode. (Thanks to Patrick Colp) Patch 18 improves the 'cryptomount' command to fall back to the passphrase mode when the key protector fails to unlock the encrypted partition. (Another patch from Patrick Colp) Patch 19~20 fix the potential security issues spotted by Fabian Vogt. Patch 21~22 add the TPM key unsealing testcase. To utilize the TPM2 key protector to unlock the encrypted partition (sdb1), here are the sample steps: 1. Add an extra random key for LUKS (luks-key) $ dd if=/dev/urandom of=luks-key bs=1 count=32 $ sudo cryptsetup luksAddKey /dev/sdb1 luks-key --pbkdf=pbkdf2 2. Seal the key $ sudo grub-protect --action=add \ --protector=tpm2 \ --tpm2key \ --tpm2-keyfile=luks-key \ --tpm2-outfile=/boot/efi/boot/grub2/sealed.tpm 3. Unseal the key with the proper commands in grub.cfg: tpm2_key_protector_init --tpm2key=(hd0,gpt1)/boot/grub2/sealed.tpm cryptomount -u -P tpm2 (*1) https://lists.gnu.org/archive/html/grub-devel/2022-02/msg6.html (*2) https://www.hansenpartnership.com/draft-bottomley-tpm2-keys.html (*3) https://lists.gnu.org/archive/html/grub-devel/2021-06/msg00044.html v9: - Introducing c-ctype.h to posix_wrap and implementing strncat - Adding the descriptive comments to the disabled code in libtasn1 - Replacing strcat with the bound-checked _asn1_str_cat in libtasn1 and including c-ctype.h directly - Integrating the asn1 testcases into "functional_test" - Updating the libtasn1 patches mentioned in the documentation - Moving the key protector to a module - Amending configure.ac to enable/disable grub-protect - Fixing an timeout issue in the tpm2_test script by feeding the config through stdin v8: - https://lists.gnu.org/archive/html/grub-devel/2024-01/msg00013.html - GIT repo: https://github.com/lcp/grub2/tree/tpm2-unlock-v8 - Introducing TPM device support to grub-emu and adding the TPM key unsealing testcase v7: - https://lists.gnu.org/archive/html/grub-devel/2023-11/msg00127.html - GIT repo: https://github.com/lcp/grub2/tree/tpm2-unlock-v7 - Stopping reading SRK from the well-known persistent handle (TPM2_SRK_HANDLE, i.e. 0x8101) by default since the persistent handle may be created by other OS and causes unsealing failure due to SRK mismatching - The user now has to specify the persistent handle with "--srk" explicitly. - Utilizing grub_error() to print more error messages - Unifying the format of the error messages from TPM2 commands v6: - https://lists.gnu.org/archive/html/grub-devel/2023-10/msg00026.html - GIT repo: https://github.com/lcp/grub2/tree/tpm2-unlock-v6 - Supporting more SRK types than RSA2048 and ECC_NIST_P256 - Documenting SHA512 as the supported PCR bank type in the tpm2 protector - Removing the redundant error message
