LinuxLists.cc - Kexec_file_load failed with "Missing required AuthAttr"

2016-01-07 05:25:07

Subject: Kexec_file_load failed with "Missing required AuthAttr"

Hi,

I saw the warning "Missing required AuthAttr" when testing kexec, known issue?
Idea about how to fix it?

The kernel is latest linus tree plus sevral patches from Toshi to cleanup io resource structure.

in function pkcs7_sig_note_set_of_authattrs():
if (!test_bit(sinfo_has_content_type, &sinfo->aa_set) ||
!test_bit(sinfo_has_message_digest, &sinfo->aa_set) ||
(ctx->msg->data_type == OID_msIndirectData &&
!test_bit(sinfo_has_ms_opus_info, &sinfo->aa_set))) {
pr_warn("Missing required AuthAttr\n");
return -EBADMSG;
}

The third condition below is true:
(ctx->msg->data_type == OID_msIndirectData &&
!test_bit(sinfo_has_ms_opus_info, &sinfo->aa_set))

I signed the kernel with redhat test key like below:
pesign -c 'Red Hat Test Certificate' -i arch/x86/boot/bzImage -o /boot/vmlinuz-4.4.0-rc8+ -s --force

Thanks
Dave

2016-01-15 08:13:04

by Dave Young

[permalink] [raw]

Subject: Re: Kexec_file_load failed with "Missing required AuthAttr"

Ccing Peter Jones for pesign possible issues.

On 01/07/16 at 01:25pm, Dave Young wrote:
> Hi,
>
> I saw the warning "Missing required AuthAttr" when testing kexec, known issue?
> Idea about how to fix it?
>
> The kernel is latest linus tree plus sevral patches from Toshi to cleanup io resource structure.
>
> in function pkcs7_sig_note_set_of_authattrs():
> if (!test_bit(sinfo_has_content_type, &sinfo->aa_set) ||
> !test_bit(sinfo_has_message_digest, &sinfo->aa_set) ||
> (ctx->msg->data_type == OID_msIndirectData &&
> !test_bit(sinfo_has_ms_opus_info, &sinfo->aa_set))) {
> pr_warn("Missing required AuthAttr\n");
> return -EBADMSG;
> }
>
> The third condition below is true:
> (ctx->msg->data_type == OID_msIndirectData &&
> !test_bit(sinfo_has_ms_opus_info, &sinfo->aa_set))
>
> I signed the kernel with redhat test key like below:
> pesign -c 'Red Hat Test Certificate' -i arch/x86/boot/bzImage -o /boot/vmlinuz-4.4.0-rc8+ -s --force
>
> Thanks
> Dave

2016-01-18 15:49:39

by Peter Jones

[permalink] [raw]

Subject: [PATCH] Don't require SpcSpOpusInfo in Authenticode pkcs7 signatures

Dave Young reported:
> Hi,
>
> I saw the warning "Missing required AuthAttr" when testing kexec,
> known issue? Idea about how to fix it?
>
> The kernel is latest linus tree plus sevral patches from Toshi to
> cleanup io resource structure.
>
> in function pkcs7_sig_note_set_of_authattrs():
> if (!test_bit(sinfo_has_content_type, &sinfo->aa_set) ||
> !test_bit(sinfo_has_message_digest, &sinfo->aa_set) ||
> (ctx->msg->data_type == OID_msIndirectData &&
> !test_bit(sinfo_has_ms_opus_info, &sinfo->aa_set))) {
> pr_warn("Missing required AuthAttr\n");
> return -EBADMSG;
> }
>
> The third condition below is true:
> (ctx->msg->data_type == OID_msIndirectData &&
> !test_bit(sinfo_has_ms_opus_info, &sinfo->aa_set))
>
> I signed the kernel with redhat test key like below:
> pesign -c 'Red Hat Test Certificate' -i arch/x86/boot/bzImage -o /boot/vmlinuz-4.4.0-rc8+ -s --force

And right he is! The Authenticode specification is a paragon amongst
technical documents, and has this pearl of wisdom to offer:

---------------------------------
Authenticode-Specific SignerInfo UnauthenticatedAttributes Structures

The following Authenticode-specific data structures are present in
SignerInfo authenticated attributes.

SpcSpOpusInfo
SpcSpOpusInfo is identified by SPC_SP_OPUS_INFO_OBJID
(1.3.6.1.4.1.311.2.1.12) and is defined as follows:
SpcSpOpusInfo ::= SEQUENCE {
programName [0] EXPLICIT SpcString OPTIONAL,
moreInfo [1] EXPLICIT SpcLink OPTIONAL,
} --#public--

SpcSpOpusInfo has two fields:
programName
This field contains the program description:
If publisher chooses not to specify a description, the SpcString
structure contains a zero-length program name.
If the publisher chooses to specify a
description, the SpcString structure contains a Unicode string.
moreInfo
This field is set to an SPCLink structure that contains a URL for
a Web site with more information about the signer. The URL is an
ASCII string.
---------------------------------

Which is to say that this is an optional *unauthenticated* field which
may be present in the Authenticated Attribute list. This is not how
pkcs7 is supposed to work, so when David implemented this, he didn't
appreciate the subtlety the original spec author was working with, and
missed the part of the sublime prose that says this Authenticated
Attribute is an Unauthenticated Attribute. As a result, the code in
question simply takes as given that the Authenticated Attributes should
be authenticated.

But this one should not, individually. Because it says it's not
authenticated.

It still has to hash right so the TBS digest is correct. So it is both
authenticated and unauthenticated, all at once. Truly, a wonder of
technical accomplishment.

Additionally, pesign's implementation has always attempted to be
compatible with the signatures emitted from contemporary versions of
Microsoft's signtool.exe. During the initial implementation, Microsoft
signatures always produced the same values for SpcSpOpusInfo -
{U"Microsoft Windows", "http://www.microsoft.com"} - without regard to
who the signer was.

Sometime between Windows 8 and Windows 8.1 they stopped including the
field in their signatures altogether, and as such pesign stopped
producing them in commits c0c4da6 and d79cb0c, sometime around June of
2012. The theory here is that anything that breaks with
pesign signatures would also be breaking with signtool.exe sigs as well,
and that'll be a more noticed problem for firmwares parsing it, so it'll
get fixed. The fact that we've done exactly this bug in Linux code is
first class, grade A irony.

So anyway, we should not be checking this field for presence or any
particular value: if the field exists, it should be at the right place,
but aside from that, as long as the hash matches the field is good.

Signed-off-by: Peter Jones <[email protected]>
---
crypto/asymmetric_keys/pkcs7_parser.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c
index 758acab..8f3056c 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypto/asymmetric_keys/pkcs7_parser.c
@@ -547,9 +547,7 @@ int pkcs7_sig_note_set_of_authattrs(void *context, size_t hdrlen,
struct pkcs7_signed_info *sinfo = ctx->sinfo;

if (!test_bit(sinfo_has_content_type, &sinfo->aa_set) ||
- !test_bit(sinfo_has_message_digest, &sinfo->aa_set) ||
- (ctx->msg->data_type == OID_msIndirectData &&
- !test_bit(sinfo_has_ms_opus_info, &sinfo->aa_set))) {
+ !test_bit(sinfo_has_message_digest, &sinfo->aa_set)) {
pr_warn("Missing required AuthAttr\n");
return -EBADMSG;
}
--
2.5.0

2016-01-18 15:50:00

by Peter Jones

[permalink] [raw]

Subject: [PATCH] Don't require SpcSpOpusInfo in Authenticode pkcs7 signatures

2016-01-19 01:08:58

by Dave Young

[permalink] [raw]

Subject: Re: [PATCH] Don't require SpcSpOpusInfo in Authenticode pkcs7 signatures

On 01/18/16 at 10:49am, Peter Jones wrote:
> Dave Young reported:
> > Hi,
> >
> > I saw the warning "Missing required AuthAttr" when testing kexec,
> > known issue? Idea about how to fix it?
> >
> > The kernel is latest linus tree plus sevral patches from Toshi to
> > cleanup io resource structure.
> >
> > in function pkcs7_sig_note_set_of_authattrs():
> > if (!test_bit(sinfo_has_content_type, &sinfo->aa_set) ||
> > !test_bit(sinfo_has_message_digest, &sinfo->aa_set) ||
> > (ctx->msg->data_type == OID_msIndirectData &&
> > !test_bit(sinfo_has_ms_opus_info, &sinfo->aa_set))) {
> > pr_warn("Missing required AuthAttr\n");
> > return -EBADMSG;
> > }
> >
> > The third condition below is true:
> > (ctx->msg->data_type == OID_msIndirectData &&
> > !test_bit(sinfo_has_ms_opus_info, &sinfo->aa_set))
> >
> > I signed the kernel with redhat test key like below:
> > pesign -c 'Red Hat Test Certificate' -i arch/x86/boot/bzImage -o /boot/vmlinuz-4.4.0-rc8+ -s --force
>
> And right he is! The Authenticode specification is a paragon amongst
> technical documents, and has this pearl of wisdom to offer:
>
> ---------------------------------
> Authenticode-Specific SignerInfo UnauthenticatedAttributes Structures
>
> The following Authenticode-specific data structures are present in
> SignerInfo authenticated attributes.
>
> SpcSpOpusInfo
> SpcSpOpusInfo is identified by SPC_SP_OPUS_INFO_OBJID
> (1.3.6.1.4.1.311.2.1.12) and is defined as follows:
> SpcSpOpusInfo ::= SEQUENCE {
> programName [0] EXPLICIT SpcString OPTIONAL,
> moreInfo [1] EXPLICIT SpcLink OPTIONAL,
> } --#public--
>
> SpcSpOpusInfo has two fields:
> programName
> This field contains the program description:
> If publisher chooses not to specify a description, the SpcString
> structure contains a zero-length program name.
> If the publisher chooses to specify a
> description, the SpcString structure contains a Unicode string.
> moreInfo
> This field is set to an SPCLink structure that contains a URL for
> a Web site with more information about the signer. The URL is an
> ASCII string.
> ---------------------------------
>
> Which is to say that this is an optional *unauthenticated* field which
> may be present in the Authenticated Attribute list. This is not how
> pkcs7 is supposed to work, so when David implemented this, he didn't
> appreciate the subtlety the original spec author was working with, and
> missed the part of the sublime prose that says this Authenticated
> Attribute is an Unauthenticated Attribute. As a result, the code in
> question simply takes as given that the Authenticated Attributes should
> be authenticated.
>
> But this one should not, individually. Because it says it's not
> authenticated.
>
> It still has to hash right so the TBS digest is correct. So it is both
> authenticated and unauthenticated, all at once. Truly, a wonder of
> technical accomplishment.
>
> Additionally, pesign's implementation has always attempted to be
> compatible with the signatures emitted from contemporary versions of
> Microsoft's signtool.exe. During the initial implementation, Microsoft
> signatures always produced the same values for SpcSpOpusInfo -
> {U"Microsoft Windows", "http://www.microsoft.com"} - without regard to
> who the signer was.
>
> Sometime between Windows 8 and Windows 8.1 they stopped including the
> field in their signatures altogether, and as such pesign stopped
> producing them in commits c0c4da6 and d79cb0c, sometime around June of
> 2012. The theory here is that anything that breaks with
> pesign signatures would also be breaking with signtool.exe sigs as well,
> and that'll be a more noticed problem for firmwares parsing it, so it'll
> get fixed. The fact that we've done exactly this bug in Linux code is
> first class, grade A irony.
>
> So anyway, we should not be checking this field for presence or any
> particular value: if the field exists, it should be at the right place,
> but aside from that, as long as the hash matches the field is good.
>
> Signed-off-by: Peter Jones <[email protected]>
> ---
> crypto/asymmetric_keys/pkcs7_parser.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c
> index 758acab..8f3056c 100644
> --- a/crypto/asymmetric_keys/pkcs7_parser.c
> +++ b/crypto/asymmetric_keys/pkcs7_parser.c
> @@ -547,9 +547,7 @@ int pkcs7_sig_note_set_of_authattrs(void *context, size_t hdrlen,
> struct pkcs7_signed_info *sinfo = ctx->sinfo;
>
> if (!test_bit(sinfo_has_content_type, &sinfo->aa_set) ||
> - !test_bit(sinfo_has_message_digest, &sinfo->aa_set) ||
> - (ctx->msg->data_type == OID_msIndirectData &&
> - !test_bit(sinfo_has_ms_opus_info, &sinfo->aa_set))) {
> + !test_bit(sinfo_has_message_digest, &sinfo->aa_set)) {
> pr_warn("Missing required AuthAttr\n");
> return -EBADMSG;
> }
> --
> 2.5.0
>

Tested-by: Dave Young <[email protected]>

Peter, thanks for the expanation. I was using such fix for several days but
not sure if it is right or not though it works for me.

Dave

2016-01-25 14:00:58

by Herbert Xu

[permalink] [raw]

Subject: Re: [PATCH] Don't require SpcSpOpusInfo in Authenticode pkcs7 signatures

Dave Young <[email protected]> wrote:
>
>> So anyway, we should not be checking this field for presence or any
>> particular value: if the field exists, it should be at the right place,
>> but aside from that, as long as the hash matches the field is good.
>>
>> Signed-off-by: Peter Jones <[email protected]>
>
> Tested-by: Dave Young <[email protected]>

Patch applied. Thanks!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt