[PATCH 1/1] SecurityPkg/AuthVariableLib: Check SHA-256 OID with ContentInfo present


Jan Bobek
 

Based on whether the DER-encoded ContentInfo structure is present in
authenticated SetVariable payload or not, the SHA-256 OID can be
located at different places.

UEFI specification explicitly states the driver shall support both
cases, but the old code assumed ContentInfo was not present and
incorrectly rejected authenticated variable updates when it were
present.

Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Min Xu <min.m.xu@...>
Signed-off-by: Jan Bobek <jbobek@...>
---
.../Library/AuthVariableLib/AuthService.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index 054ee4d1d988..de8baccab410 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -1933,15 +1933,19 @@ VerifyTimeBasedPayload (
// .... }
// The DigestAlgorithmIdentifiers can be used to determine the hash algorithm
// in VARIABLE_AUTHENTICATION_2 descriptor.
- // This field has the fixed offset (+13) and be calculated based on two bytes of length encoding.
+ // This field has the fixed offset (+13) or (+32) based on whether the DER-encoded
+ // ContentInfo structure is present or not, and can be calculated based on two
+ // bytes of length encoding.
//
if ((Attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) != 0) {
- if (SigDataSize >= (13 + sizeof (mSha256OidValue))) {
- if (((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE) ||
- (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0))
- {
- return EFI_SECURITY_VIOLATION;
- }
+ if ( ( (SigDataSize >= (13 + sizeof (mSha256OidValue)))
+ && ( ((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE)
+ || (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0)))
+ && ( (SigDataSize >= (32 + sizeof (mSha256OidValue)))
+ && ( ((*(SigData + 20) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE)
+ || (CompareMem (SigData + 32, &mSha256OidValue, sizeof (mSha256OidValue)) != 0))))
+ {
+ return EFI_SECURITY_VIOLATION;
}
}

--
2.30.2


Jan Bobek
 

Ping. Can I get a review and/or some comments on this patch, please?

Thanks,
-Jan

Jan Bobek writes:

Based on whether the DER-encoded ContentInfo structure is present in
authenticated SetVariable payload or not, the SHA-256 OID can be
located at different places.

UEFI specification explicitly states the driver shall support both
cases, but the old code assumed ContentInfo was not present and
incorrectly rejected authenticated variable updates when it were
present.

Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Min Xu <min.m.xu@...>
Signed-off-by: Jan Bobek <jbobek@...>
---
.../Library/AuthVariableLib/AuthService.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index 054ee4d1d988..de8baccab410 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -1933,15 +1933,19 @@ VerifyTimeBasedPayload (
// .... }
// The DigestAlgorithmIdentifiers can be used to determine the hash algorithm
// in VARIABLE_AUTHENTICATION_2 descriptor.
- // This field has the fixed offset (+13) and be calculated based on two bytes of length encoding.
+ // This field has the fixed offset (+13) or (+32) based on whether the DER-encoded
+ // ContentInfo structure is present or not, and can be calculated based on two
+ // bytes of length encoding.
//
if ((Attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) != 0) {
- if (SigDataSize >= (13 + sizeof (mSha256OidValue))) {
- if (((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE) ||
- (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0))
- {
- return EFI_SECURITY_VIOLATION;
- }
+ if ( ( (SigDataSize >= (13 + sizeof (mSha256OidValue)))
+ && ( ((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE)
+ || (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0)))
+ && ( (SigDataSize >= (32 + sizeof (mSha256OidValue)))
+ && ( ((*(SigData + 20) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE)
+ || (CompareMem (SigData + 32, &mSha256OidValue, sizeof (mSha256OidValue)) != 0))))
+ {
+ return EFI_SECURITY_VIOLATION;
}
}


Jan Bobek
 

Anothing ping. Comments/reviews/merge highly appreciated.

Thank you,
-Jan

Jan Bobek writes:

Ping. Can I get a review and/or some comments on this patch, please?

Thanks,
-Jan

Jan Bobek writes:

Based on whether the DER-encoded ContentInfo structure is present in
authenticated SetVariable payload or not, the SHA-256 OID can be
located at different places.

UEFI specification explicitly states the driver shall support both
cases, but the old code assumed ContentInfo was not present and
incorrectly rejected authenticated variable updates when it were
present.

Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Min Xu <min.m.xu@...>
Signed-off-by: Jan Bobek <jbobek@...>
---
.../Library/AuthVariableLib/AuthService.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index 054ee4d1d988..de8baccab410 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -1933,15 +1933,19 @@ VerifyTimeBasedPayload (
// .... }
// The DigestAlgorithmIdentifiers can be used to determine the hash algorithm
// in VARIABLE_AUTHENTICATION_2 descriptor.
- // This field has the fixed offset (+13) and be calculated based on two bytes of length encoding.
+ // This field has the fixed offset (+13) or (+32) based on whether the DER-encoded
+ // ContentInfo structure is present or not, and can be calculated based on two
+ // bytes of length encoding.
//
if ((Attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) != 0) {
- if (SigDataSize >= (13 + sizeof (mSha256OidValue))) {
- if (((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE) ||
- (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0))
- {
- return EFI_SECURITY_VIOLATION;
- }
+ if ( ( (SigDataSize >= (13 + sizeof (mSha256OidValue)))
+ && ( ((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE)
+ || (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0)))
+ && ( (SigDataSize >= (32 + sizeof (mSha256OidValue)))
+ && ( ((*(SigData + 20) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE)
+ || (CompareMem (SigData + 32, &mSha256OidValue, sizeof (mSha256OidValue)) != 0))))
+ {
+ return EFI_SECURITY_VIOLATION;
}
}


Yao, Jiewen
 

Hi
That is good catch!
My apology to miss it before.

1) Please file a bugzilla (https://bugzilla.tianocore.org/) to record the issue and associate to the patch.

2) Would you please share with us that how you discover the issue?
For example, any real use case to include ContentInfo? If yes, please share a URL.
Or this is just a purely spec compliance fix ?

3) Please describe how you validate the fix.
If possible, would you please share your test case?

4) Since the new code is handling ContentInfo structure is present, I believe we need also check if the ContentInfo structure is valid.
For example:
============
c SignedData.contentInfo.contentType shall be set to id-data
d SignedData.contentInfo.content shall be absent
============
What do you think?

Thank you
Yao, Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jan
Bobek via groups.io
Sent: Wednesday, January 4, 2023 6:30 AM
To: devel@edk2.groups.io
Cc: Jeff Brasen <jbrasen@...>; Girish Mahadevan
<gmahadevan@...>; Jan Bobek <jbobek@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>; Xu, Min M
<min.m.xu@...>
Subject: Re: [edk2-devel] [PATCH 1/1] SecurityPkg/AuthVariableLib: Check
SHA-256 OID with ContentInfo present

Anothing ping. Comments/reviews/merge highly appreciated.

Thank you,
-Jan

Jan Bobek writes:

Ping. Can I get a review and/or some comments on this patch, please?

Thanks,
-Jan

Jan Bobek writes:

Based on whether the DER-encoded ContentInfo structure is present in
authenticated SetVariable payload or not, the SHA-256 OID can be
located at different places.

UEFI specification explicitly states the driver shall support both
cases, but the old code assumed ContentInfo was not present and
incorrectly rejected authenticated variable updates when it were
present.

Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Min Xu <min.m.xu@...>
Signed-off-by: Jan Bobek <jbobek@...>
---
.../Library/AuthVariableLib/AuthService.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c
b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index 054ee4d1d988..de8baccab410 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -1933,15 +1933,19 @@ VerifyTimeBasedPayload (
// .... }
// The DigestAlgorithmIdentifiers can be used to determine the hash
algorithm
// in VARIABLE_AUTHENTICATION_2 descriptor.
- // This field has the fixed offset (+13) and be calculated based on two
bytes of length encoding.
+ // This field has the fixed offset (+13) or (+32) based on whether the
DER-encoded
+ // ContentInfo structure is present or not, and can be calculated
based on two
+ // bytes of length encoding.
//
if ((Attributes &
EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) != 0) {
- if (SigDataSize >= (13 + sizeof (mSha256OidValue))) {
- if (((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE) ||
- (CompareMem (SigData + 13, &mSha256OidValue, sizeof
(mSha256OidValue)) != 0))
- {
- return EFI_SECURITY_VIOLATION;
- }
+ if ( ( (SigDataSize >= (13 + sizeof (mSha256OidValue)))
+ && ( ((*(SigData + 1) & TWO_BYTE_ENCODE) !=
TWO_BYTE_ENCODE)
+ || (CompareMem (SigData + 13, &mSha256OidValue, sizeof
(mSha256OidValue)) != 0)))
+ && ( (SigDataSize >= (32 + sizeof (mSha256OidValue)))
+ && ( ((*(SigData + 20) & TWO_BYTE_ENCODE) !=
TWO_BYTE_ENCODE)
+ || (CompareMem (SigData + 32, &mSha256OidValue, sizeof
(mSha256OidValue)) != 0))))
+ {
+ return EFI_SECURITY_VIOLATION;
}
}



Jan Bobek
 

Hi
That is good catch!
My apology to miss it before.

1) Please file a bugzilla (https://bugzilla.tianocore.org/) to record the issue and associate to the patch.
Filed bug 4305 [1]. Sorry for the delay, I didn't get my bugzilla
credentials until late last week.

2) Would you please share with us that how you discover the issue?
For example, any real use case to include ContentInfo? If yes, please share a URL.
Or this is just a purely spec compliance fix ?

3) Please describe how you validate the fix.
If possible, would you please share your test case?
I believe both of these answered / included in the bug description.

4) Since the new code is handling ContentInfo structure is present, I believe we need also check if the ContentInfo structure is valid.
For example:
============
c SignedData.contentInfo.contentType shall be set to id-data
d SignedData.contentInfo.content shall be absent
============
What do you think?
I think you're talking about the ContentInfo structure that's part of
the SignedData structure, but the real problem is with ContentInfo
structure that _wraps_ the SignedData structure. More info in the bug
description.

Also, is it customary to continue the discussion here on edk2-devel or
in the bug comments on bugzilla?

-Jan

References:
1. https://bugzilla.tianocore.org/show_bug.cgi?id=4305


Yao, Jiewen
 

I linked email with Bugzilla. Either email or Bugzilla is OK for the discussion.

Personally, I don't understand one thing.
If EDKII causes such failure, how the archlinux validates the correctness of the tool and document in [3] ?

Or are they using a different UEFI implementation?

Thank you
Yao, Jiewen

-----Original Message-----
From: Jan Bobek <jbobek@...>
Sent: Tuesday, January 17, 2023 6:30 AM
To: Yao, Jiewen <jiewen.yao@...>
Cc: devel@edk2.groups.io; Jeff Brasen <jbrasen@...>; Girish
Mahadevan <gmahadevan@...>; Wang, Jian J
<jian.j.wang@...>; Xu, Min M <min.m.xu@...>
Subject: Re: [edk2-devel] [PATCH 1/1] SecurityPkg/AuthVariableLib: Check
SHA-256 OID with ContentInfo present

Hi
That is good catch!
My apology to miss it before.

1) Please file a bugzilla (https://bugzilla.tianocore.org/) to record the issue
and associate to the patch.

Filed bug 4305 [1]. Sorry for the delay, I didn't get my bugzilla
credentials until late last week.

2) Would you please share with us that how you discover the issue?
For example, any real use case to include ContentInfo? If yes, please share
a URL.
Or this is just a purely spec compliance fix ?

3) Please describe how you validate the fix.
If possible, would you please share your test case?
I believe both of these answered / included in the bug description.

4) Since the new code is handling ContentInfo structure is present, I believe
we need also check if the ContentInfo structure is valid.
For example:
============
c SignedData.contentInfo.contentType shall be set to id-data
d SignedData.contentInfo.content shall be absent
============
What do you think?
I think you're talking about the ContentInfo structure that's part of
the SignedData structure, but the real problem is with ContentInfo
structure that _wraps_ the SignedData structure. More info in the bug
description.

Also, is it customary to continue the discussion here on edk2-devel or
in the bug comments on bugzilla?

-Jan

References:
1. https://bugzilla.tianocore.org/show_bug.cgi?id=4305


Jan Bobek
 

I linked email with Bugzilla. Either email or Bugzilla is OK for the
discussion.
Sounds good.

Personally, I don't understand one thing.
If EDKII causes such failure, how the archlinux validates the correctness of the tool and document in [3] ?

Or are they using a different UEFI implementation?
My understanding is that Archlinux assumes a standard-compliant UEFI
implementation. A Linux distribution doesn't typically provide UEFI
implementation; it's up to your platform vendor (e.g. laptop
manufacturer) to provide one. If the vendor wanted to use EDK2 as a
basis for their (typically proprietary) UEFI implementation, they would
need to address this issue one way or the other on their own.

-Jan

-----Original Message-----
From: Jan Bobek <jbobek@...>
Sent: Tuesday, January 17, 2023 6:30 AM
To: Yao, Jiewen <jiewen.yao@...>
Cc: devel@edk2.groups.io; Jeff Brasen <jbrasen@...>; Girish
Mahadevan <gmahadevan@...>; Wang, Jian J
<jian.j.wang@...>; Xu, Min M <min.m.xu@...>
Subject: Re: [edk2-devel] [PATCH 1/1] SecurityPkg/AuthVariableLib: Check
SHA-256 OID with ContentInfo present

Hi
That is good catch!
My apology to miss it before.

1) Please file a bugzilla (https://bugzilla.tianocore.org/) to record the issue
and associate to the patch.

Filed bug 4305 [1]. Sorry for the delay, I didn't get my bugzilla
credentials until late last week.

2) Would you please share with us that how you discover the issue?
For example, any real use case to include ContentInfo? If yes, please share
a URL.
Or this is just a purely spec compliance fix ?

3) Please describe how you validate the fix.
If possible, would you please share your test case?
I believe both of these answered / included in the bug description.

4) Since the new code is handling ContentInfo structure is present, I believe
we need also check if the ContentInfo structure is valid.
For example:
============
c SignedData.contentInfo.contentType shall be set to id-data
d SignedData.contentInfo.content shall be absent
============
What do you think?
I think you're talking about the ContentInfo structure that's part of
the SignedData structure, but the real problem is with ContentInfo
structure that _wraps_ the SignedData structure. More info in the bug
description.

Also, is it customary to continue the discussion here on edk2-devel or
in the bug comments on bugzilla?

-Jan

References:
1. https://bugzilla.tianocore.org/show_bug.cgi?id=4305