From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AB66820837969 for ; Thu, 10 May 2018 05:36:03 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 31F3E7C6B0; Thu, 10 May 2018 12:36:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-122-93.rdu2.redhat.com [10.10.122.93]) by smtp.corp.redhat.com (Postfix) with ESMTP id 60F851134CA5; Thu, 10 May 2018 12:36:01 +0000 (UTC) To: James Bottomley , "edk2-devel@lists.01.org" Cc: Zhang Lubo References: <1525903747.5882.11.camel@HansenPartnership.com> From: Laszlo Ersek Message-ID: <1595cae2-f01d-57e2-6c2b-2cbb472d7a84@redhat.com> Date: Thu, 10 May 2018 14:36:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <1525903747.5882.11.camel@HansenPartnership.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Thu, 10 May 2018 12:36:02 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Thu, 10 May 2018 12:36:02 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH] SecurityPkg: fix sha256 signature check X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 May 2018 12:36:04 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/10/18 00:09, James Bottomley wrote: > commit c035e37335ae43229d7e68de74a65f2c01ebc0af > Author: Zhang Lubo > Date: Thu Jan 5 14:58:05 2017 +0800 > > SecurityPkg: enhance secure boot Config Dxe & Time Based AuthVariable. > > Added a check for sha256 being the ownly allowed signature hash. > Unfortuantely this commit assumed the form of the signature data was a > raw SignedData sequence. Most tools actually generate a ContentInfo > sequence instead which contains a header identifying the content as > pkcs7-SignedData. Fix this check to allow either format to work. > > This fix is needed at least for efitools because we generate signed > variable updates with the ContentInfo header. > > Signed-off-by: James Bottomley > --- > CryptoPkg/Library/OpensslLib/openssl | 2 +- > SecurityPkg/Library/AuthVariableLib/AuthService.c | 11 ++++++++++- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/CryptoPkg/Library/OpensslLib/openssl b/CryptoPkg/Library/OpensslLib/openssl > index b2758a2292..d4e4bd2a81 160000 > --- a/CryptoPkg/Library/OpensslLib/openssl > +++ b/CryptoPkg/Library/OpensslLib/openssl > @@ -1 +1 @@ > -Subproject commit b2758a2292aceda93e9f44c219b94fe21bb9a650 > +Subproject commit d4e4bd2a8163f355fa8a3884077eaec7adc75ff7 This hunk should not be necessary; please see edk2 commit b85b20fba42e ("CryptoPkg/OpensslLib: Update OpenSSL version to 1.1.0h", 2018-04-15). (I'll let the SecurityPkg maintainers review the rest.) Thanks, Laszlo > diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c > index 213a524f27..855ea3350a 100644 > --- a/SecurityPkg/Library/AuthVariableLib/AuthService.c > +++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c > @@ -1908,10 +1908,19 @@ VerifyTimeBasedPayload ( > // in VARIABLE_AUTHENTICATION_2 descriptor. > // This field has the fixed offset (+13) and be calculated based on two bytes of length encoding. > // > + // However the data may also begin > + // ContentInfo ::= SEQUENCE { > + // contentType ContentType, > + // content > + // [0] EXPLICIT ANY DEFINED BY contentType OPTIONAL } > + // > + // In which case the fixed offset is +32 > + // > 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)) { > + (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0 && > + CompareMem (SigData + 32, &mSha256OidValue, sizeof (mSha256OidValue)) != 0)) { > return EFI_SECURITY_VIOLATION; > } > } >