From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=supreeth.venkatesh@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id 27D5A21183ED7 for ; Fri, 30 Nov 2018 14:27:34 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0082EA78; Fri, 30 Nov 2018 14:27:34 -0800 (PST) Received: from supven01-VirtualBox (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id AFBF73F5AF; Fri, 30 Nov 2018 14:27:33 -0800 (PST) Message-ID: From: Supreeth Venkatesh To: Eric Jin , edk2-devel@lists.01.org Date: Fri, 30 Nov 2018 16:27:32 -0600 In-Reply-To: <20181129084602.116544-1-eric.jin@intel.com> References: <20181129084602.116544-1-eric.jin@intel.com> X-Mailer: Evolution 3.28.5-0ubuntu0.18.04.1 Mime-Version: 1.0 Subject: Re: [edk2-test][Patch 3/3] uefi-sct/SctPkg:Add VerifySignature() Conf Test X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Nov 2018 22:27:34 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Commit message to mention what this patch is imlementing. In addition, there is cleanup to remove #if 0 block below. Both of this should be in commit message. On Thu, 2018-11-29 at 16:46 +0800, Eric Jin wrote: > Cc: Supreeth Venkatesh > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Jin > --- > .../EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c | 5 +- > .../EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h | 16 ++ > .../BlackBoxTest/Pkcs7BBTestConformance.c | 303 > ++++++++++++++++++++- > .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestMain.c | 2 - > 4 files changed, 319 insertions(+), 7 deletions(-) > > diff --git a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c > b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c > index 4d433c3..8f6546a 100644 > --- a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c > +++ b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c > @@ -36,7 +36,10 @@ EFI_GUID gPkcs7BBTestConformanceAssertionGuid007 = > EFI_TEST_PKCS7BBTESTCONFORMAN > EFI_GUID gPkcs7BBTestConformanceAssertionGuid008 = > EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_008_GUID; > EFI_GUID gPkcs7BBTestConformanceAssertionGuid009 = > EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_009_GUID; > EFI_GUID gPkcs7BBTestConformanceAssertionGuid010 = > EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_010_GUID; > - > +EFI_GUID gPkcs7BBTestConformanceAssertionGuid011 = > EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_011_GUID; > +EFI_GUID gPkcs7BBTestConformanceAssertionGuid012 = > EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_012_GUID; > +EFI_GUID gPkcs7BBTestConformanceAssertionGuid013 = > EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_013_GUID; > +EFI_GUID gPkcs7BBTestConformanceAssertionGuid014 = > EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_014_GUID; > > EFI_GUID gPkcs7BBTestFunctionAssertionGuid001 = > EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_001_GUID; > EFI_GUID gPkcs7BBTestFunctionAssertionGuid002 = > EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_002_GUID; > diff --git a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h > b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h > index 32a00f6..f8d1b8f 100644 > --- a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h > +++ b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h > @@ -65,6 +65,22 @@ extern EFI_GUID > gPkcs7BBTestConformanceAssertionGuid009; > { 0xb136e016, 0x4f80, 0x44bd, {0xba, 0xb0, 0x1c, 0x34, 0x8a, 0x2d, > 0xa1, 0x8a }} > extern EFI_GUID gPkcs7BBTestConformanceAssertionGuid010; > > +#define EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_011_GUID \ > +{ 0xa58f3626, 0xf16e, 0x4cd5, { 0xba, 0x12, 0x7a, 0x9d, 0xd6, 0x9a, > 0x7a, 0x71 }} > +extern EFI_GUID gPkcs7BBTestConformanceAssertionGuid011; > + > +#define EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_012_GUID \ > +{ 0xbe4a0bf2, 0x2d46, 0x4d96, { 0xa6, 0x11, 0x21, 0x99, 0xa5, 0x5f, > 0xa4, 0xee }} > +extern EFI_GUID gPkcs7BBTestConformanceAssertionGuid012; > + > +#define EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_013_GUID \ > +{ 0xc0536a27, 0x304e, 0x497a, { 0xa5, 0xe3, 0x94, 0x11, 0x38, 0x53, > 0x6e, 0x40 }} > +extern EFI_GUID gPkcs7BBTestConformanceAssertionGuid013; > + > +#define EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_014_GUID \ > +{ 0x8c5aa0e8, 0x17ff, 0x49cd, { 0x8f, 0xec, 0x37, 0xc3, 0xfd, 0x5f, > 0x77, 0x0 }} > +extern EFI_GUID gPkcs7BBTestConformanceAssertionGuid014; > + > > #define EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_001_GUID \ > { 0x5c0eec50, 0xa6ea, 0x413c, {0x8a, 0x46, 0x4a, 0xd1, 0x4a, 0x77, > 0x76, 0xf1 }} > diff --git a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7B > BTestConformance.c b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7B > BTestConformance.c > index b7bc19b..ce7d5bb 100644 > --- a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7B > BTestConformance.c > +++ b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7B > BTestConformance.c > @@ -278,10 +278,6 @@ BBTestVerifyBufferConformanceTest ( > Status > ); > > - > - > - > - > // Signed data embedded in SignedData but InData is not NULL > AllowedDb[0] = DbEntry1; > RevokedDb[0] = NULL; > @@ -507,4 +503,303 @@ BBTestVerifyBufferConformanceTest ( > return EFI_SUCCESS; > } > > +EFI_STATUS > +BBTestVerifySignatureConformanceTest ( > + IN EFI_BB_TEST_PROTOCOL *This, > + IN VOID *ClientInterface, > + IN EFI_TEST_LEVEL TestLevel, > + IN EFI_HANDLE SupportHandle > + ) > +{ > + EFI_STANDARD_TEST_LIBRARY_PROTOCOL *StandardLib; > + EFI_STATUS Status; > + EFI_PKCS7_VERIFY_PROTOCOL *Pkcs7Verify; > + EFI_TEST_ASSERTION AssertionType; > > + Pkcs7Verify = (EFI_PKCS7_VERIFY_PROTOCOL*)ClientInterface; > + if (Pkcs7Verify == NULL) > + return EFI_UNSUPPORTED; > + > + // > + // Get the Standard Library Interface > + // > + Status = gtBS->HandleProtocol ( > + SupportHandle, > + &gEfiStandardTestLibraryGuid, > + (VOID **) &StandardLib > + ); > + if (EFI_ERROR(Status)) { > + return Status; > + } > + > + AllowedDb[0] = DbEntry1; > + > + // > + // Checkpoint 1 - EFI_INVALID_PARAMETER > + // > + > + // Signature is NULL > + Status = Pkcs7Verify->VerifySignature (Pkcs7Verify, NULL, > sizeof(P7TestSignature), TestInHash, sizeof(TestInHash), AllowedDb, > NULL, NULL); > + if (Status == EFI_INVALID_PARAMETER) > + AssertionType = EFI_TEST_ASSERTION_PASSED; > + else > + AssertionType = EFI_TEST_ASSERTION_FAILED; > + > + StandardLib->RecordAssertion ( > + StandardLib, > + AssertionType, > + gPkcs7BBTestConformanceAssertionGuid011, > + L"PKCS7_VERIFY_PROTOCOL.VerifySignature - > VerifySignature() should returns EFI_INVALID_PARAMETER when Signature > is NULL.", > + L"%a:%d: Status - %r\n", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + > + // SignatureSize is 0 > + Status = Pkcs7Verify->VerifySignature (Pkcs7Verify, > P7TestSignature, 0, TestInHash, sizeof(TestInHash), AllowedDb, NULL, > NULL); > + if (Status == EFI_INVALID_PARAMETER) > + AssertionType = EFI_TEST_ASSERTION_PASSED; > + else > + AssertionType = EFI_TEST_ASSERTION_FAILED; > + > + StandardLib->RecordAssertion ( > + StandardLib, > + AssertionType, > + gPkcs7BBTestConformanceAssertionGuid011, > + L"PKCS7_VERIFY_PROTOCOL.VerifySignature - > VerifySignature() should returns EFI_INVALID_PARAMETER when > SignatureSize is 0.", > + L"%a:%d: Status - %r\n", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + > + // InHash is NULL. > + Status = Pkcs7Verify->VerifySignature (Pkcs7Verify, > P7TestSignature, sizeof(P7TestSignature), NULL, sizeof(TestInHash), > AllowedDb, NULL, NULL); > + if (Status == EFI_INVALID_PARAMETER) > + AssertionType = EFI_TEST_ASSERTION_PASSED; > + else > + AssertionType = EFI_TEST_ASSERTION_FAILED; > + > + StandardLib->RecordAssertion ( > + StandardLib, > + AssertionType, > + gPkcs7BBTestConformanceAssertionGuid011, > + L"PKCS7_VERIFY_PROTOCOL.VerifySignature - > VerifySignature() should returns EFI_INVALID_PARAMETER when InHash is > NULL.", > + L"%a:%d: Status - %r\n", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + > + // InHashSize is 0. > + Status = Pkcs7Verify->VerifySignature (Pkcs7Verify, > P7TestSignature, sizeof(P7TestSignature), TestInHash, 0, AllowedDb, > NULL, NULL); > + if (Status == EFI_INVALID_PARAMETER) > + AssertionType = EFI_TEST_ASSERTION_PASSED; > + else > + AssertionType = EFI_TEST_ASSERTION_FAILED; > + > + StandardLib->RecordAssertion ( > + StandardLib, > + AssertionType, > + gPkcs7BBTestConformanceAssertionGuid011, > + L"PKCS7_VERIFY_PROTOCOL.VerifySignature - > VerifySignature() should returns EFI_INVALID_PARAMETER when > InHashSize is 0.", > + L"%a:%d: Status - %r\n", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + > + // AllowedDb is NULL > + Status = Pkcs7Verify->VerifySignature (Pkcs7Verify, > P7TestSignature, sizeof(P7TestSignature), TestInHash, > sizeof(TestInHash), NULL, NULL, NULL); > + if (Status == EFI_INVALID_PARAMETER) > + AssertionType = EFI_TEST_ASSERTION_PASSED; > + else > + AssertionType = EFI_TEST_ASSERTION_FAILED; > + > + StandardLib->RecordAssertion ( > + StandardLib, > + AssertionType, > + gPkcs7BBTestConformanceAssertionGuid011, > + L"PKCS7_VERIFY_PROTOCOL.VerifySignature - > VerifySignature() should returns EFI_INVALID_PARAMETER when AllowedDb > is NULL.", > + L"%a:%d: Status - %r\n", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + > + // > + // Checkpoint 2 - EFI_UNSUPPORTED > + // NOTE: It's hard to distinguish unsupported Signature or > security-violated signature for VerifySignature() interface > + // since we don't need to extra the attached content from > input signature as VerifyBuffer() interface. > + // So it's OK to bypass this check or just check the > EFI_SECURITY_VIOLATION result. > + // > + > + // > + // Modify the Data in the P7TestSignature to make it the invalid > one > + // > + P7TestSignature[0] = 0x40; > + > + Status = Pkcs7Verify->VerifySignature (Pkcs7Verify, > P7TestSignature, sizeof(P7TestSignature), TestInHash, > sizeof(TestInHash), AllowedDb, NULL, NULL); > + > + if ((Status == EFI_UNSUPPORTED) || (Status == > EFI_SECURITY_VIOLATION)) > + AssertionType = EFI_TEST_ASSERTION_PASSED; > + else > + AssertionType = EFI_TEST_ASSERTION_FAILED; > + > + StandardLib->RecordAssertion ( > + StandardLib, > + AssertionType, > + gPkcs7BBTestConformanceAssertionGuid012, > + L"PKCS7_VERIFY_PROTOCOL.VerifySignature - > VerifySignature() should returns EFI_UNSUPPORTED when Signature > buffer was not clrrectly formatted.", > + L"%a:%d: Status - %r\n", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + > + // > + // Restore the P7TestSignature to the valid one > + // > + P7TestSignature[0] = 0x30; > + > + // > + // Checkpoint 4 - EFI_COMPROMISED_DATA > + // NOTE: P7Verify leverage OpenSSL for pkcs7 signature > verification > + // It's hard for EFI_COMPROMISED_DATA check. Just use > EFI_SECURITY_VIOLATION for wrong InHash input. > + // > + > + // > + // Modify the Data in the InHash to make it the invalid one > + // > + TestInHash[0] = 0x3E; > + Status = Pkcs7Verify->VerifySignature (Pkcs7Verify, > P7TestSignature, sizeof(P7TestSignature), TestInHash, > sizeof(TestInHash), AllowedDb, RevokedDb, TimestampDb); > + if (Status == EFI_SECURITY_VIOLATION) > + AssertionType = EFI_TEST_ASSERTION_PASSED; > + else > + AssertionType = EFI_TEST_ASSERTION_FAILED; > + > + StandardLib->RecordAssertion ( > + StandardLib, > + AssertionType, > + gPkcs7BBTestConformanceAssertionGuid013, > + L"PKCS7_VERIFY_PROTOCOL.VerifySignature - > VerifySignature() should returns EFI_SECURITY_VIOLATION when hash > differs from signed hash.", > + L"%a:%d: Status - %r\n", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + // > + // Restore the InHash > + // > + TestInHash[0] = 0x3D; > + > + // > + // Input the invalid Hashsize > + // > + Status = Pkcs7Verify->VerifySignature (Pkcs7Verify, > P7TestSignature, sizeof(P7TestSignature), TestInHash, > sizeof(TestInHash)- 2, AllowedDb, RevokedDb, TimestampDb); > + if (Status == EFI_SECURITY_VIOLATION) > + AssertionType = EFI_TEST_ASSERTION_PASSED; > + else > + AssertionType = EFI_TEST_ASSERTION_FAILED; > + > + StandardLib->RecordAssertion ( > + StandardLib, > + AssertionType, > + gPkcs7BBTestConformanceAssertionGuid013, > + L"PKCS7_VERIFY_PROTOCOL.VerifySignature - > VerifySignature() should returns EFI_SECURITY_VIOLATION when > encrypted hash is different size.", > + L"%a:%d: Status - %r\n", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + > + // > + // Checkpoint 5 - EFI_SECURITY_VIOLATION > + // > + > + // > + // The SignedData buffer was correctly formatted but signer was in > RevokedDb > + // > + AllowedDb[0] = DbEntry1; > + RevokedDb[0] = DbEntry1; > + > + Status = Pkcs7Verify->VerifySignature (Pkcs7Verify, > P7TestSignature, sizeof(P7TestSignature), TestInHash, > sizeof(TestInHash), AllowedDb, RevokedDb, TimestampDb); > + > + AllowedDb[0] = NULL; > + RevokedDb[0] = NULL; > + > + if (Status == EFI_SECURITY_VIOLATION) > + AssertionType = EFI_TEST_ASSERTION_PASSED; > + else > + AssertionType = EFI_TEST_ASSERTION_FAILED; > + > + StandardLib->RecordAssertion ( > + StandardLib, > + AssertionType, > + gPkcs7BBTestConformanceAssertionGuid014, > + L"PKCS7_VERIFY_PROTOCOL.VerifySignature - > VerifySignature() should returns EFI_SECURITY_VIOLATION when the > SignedData buffer was correctly formatted but signer was in > RevokedDb.", > + L"%a:%d: Status - %r\n", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + > + // > + // The SignedData buffer was correctly formatted but signer was > not in AllowedDb > + // > + AllowedDb[0] = NULL; > + > + Status = Pkcs7Verify->VerifySignature (Pkcs7Verify, > P7TestSignature, sizeof(P7TestSignature), TestInHash, > sizeof(TestInHash), AllowedDb, RevokedDb, TimestampDb); > + > + if (Status == EFI_SECURITY_VIOLATION) > + AssertionType = EFI_TEST_ASSERTION_PASSED; > + else > + AssertionType = EFI_TEST_ASSERTION_FAILED; > + > + StandardLib->RecordAssertion ( > + StandardLib, > + AssertionType, > + gPkcs7BBTestConformanceAssertionGuid014, > + L"PKCS7_VERIFY_PROTOCOL.VerifySignature - > VerifySignature() should returns EFI_SECURITY_VIOLATION when the > SignedData buffer was correctly formatted but signer was not in > AllowedDb.", > + L"%a:%d: Status - %r\n", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + // > + // Matching content hash found in the RevokedDb > + // > + SctCopyMem ((UINT8 *)DbEntry4 + sizeof (EFI_SIGNATURE_LIST) + > sizeof(EFI_GUID), TestInHash, sizeof(TestInHash)); > + RevokedDb[0] = DbEntry4; > + AllowedDb[0] = DbEntry1; > + Status = Pkcs7Verify->VerifySignature (Pkcs7Verify, > P7TestSignature, sizeof(P7TestSignature), TestInHash, > sizeof(TestInHash), AllowedDb, RevokedDb, TimestampDb); > + > + RevokedDb[0] = NULL; > + AllowedDb[0] = NULL; > + > + if (Status == EFI_SECURITY_VIOLATION) > + AssertionType = EFI_TEST_ASSERTION_PASSED; > + else > + AssertionType = EFI_TEST_ASSERTION_FAILED; > + > + StandardLib->RecordAssertion ( > + StandardLib, > + AssertionType, > + gPkcs7BBTestConformanceAssertionGuid014, > + L"PKCS7_VERIFY_PROTOCOL.VerifySignature - > VerifySignature() should returns EFI_SECURITY_VIOLATION when matching > content hash found in the RevokedDb.", > + L"%a:%d: Status - %r\n", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + > + // > + // Clean the Data in all Dbs > + // > + AllowedDb[0] = NULL; > + RevokedDb[0] = NULL; > + TimestampDb[0] = NULL; > + > + return EFI_SUCCESS; > +} > diff --git a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7B > BTestMain.c b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7B > BTestMain.c > index 770bf33..a84cc69 100644 > --- a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7B > BTestMain.c > +++ b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7B > BTestMain.c > @@ -88,7 +88,6 @@ EFI_BB_TEST_ENTRY_FIELD gBBTestEntryField[] = { > EFI_TEST_CASE_AUTO, > BBTestVerifyBufferConformanceTest > }, > -#if 0 > { > EFI_PKCS7_VERIFY_PROTOCOL_TEST_ENTRY_GUID0202, > L"VerifySignatureConformance", > @@ -98,7 +97,6 @@ EFI_BB_TEST_ENTRY_FIELD gBBTestEntryField[] = { > EFI_TEST_CASE_AUTO, > BBTestVerifySignatureConformanceTest > }, > -#endif > 0 > }; >