| Message ID | 1453132198-19644-1-git-send-email-pjones@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
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 <pjones@redhat.com> > --- > 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 <dyoung@redhat.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dave Young <dyoung@redhat.com> 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 <pjones@redhat.com> > > Tested-by: Dave Young <dyoung@redhat.com> Patch applied. Thanks!
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; }