Derivechild support #250

hpya93 · 2023-11-02T13:46:57Z

Hi @jhand2 and @sree-revoori1
Tests check for

Effect of DeriveChild on MultiTcb of CertifiedKey certificate - TCItype, TargetLoc, TCI_CUMULATIVE etc
Derive Child tests - Wrong Locality, Usage of different combination of flags, Privilege escalation checks etc.
Can you please review the derive child command changes?
This I recreated the PR to rebase and fix a bug that I found while initializing handle.

This snippet was added in commit 8a56b14#diff-613d35b2e9d525cc6cf3d0df2be4c4c5cd8ec0813b72b02871620bda867bce7f

	var ctx ContextHandle   // This handle is used
	if use_simulation {
		if d.GetSupport().Simulation {
			handle, err := client.InitializeContext(InitIsSimulation)  // This handle has no use, it must have been reassigned to ctx
			if err != nil {
				t.Fatal("The instance should be able to create a simulation context.")
			}
			// Could prove difficult to prove it is a cryptographically secure random.
			if *handle == ContextHandle([16]byte{0}) {
				t.Fatal("Incorrect simulation context handle.")
			}

			defer client.DestroyContext(handle, 0)
		} else {
			t.Errorf("[ERROR]:  DPE instance doesn't support simulation contexts.")
		}
	} else {
		//default context
		ctx = [16]byte{0}
	}

jhand2 · 2023-12-15T20:50:53Z

verification/verification.go

-	"CertifyKeySimulation", TestCertifyKeySimulation, []string{"AutoInit", "Simulation", "X509", "IsCA"},
+	"CertifyKeySimulation", TestCertifyKeySimulation, []string{"AutoInit", "Simulation", "X509", "IsCA", "RotateContext", "ExtendTci"},
+}
+var CertifyKeyWithoutExtendTciTestCase = TestCase{


nit: Can you make this test the base CertifyKey test, and then add one that is CertifyKeyWithExtendTci

Done, only DiceTcb validation withDerived child requires ExtendTci, so added separate testcase for it in normal/simulator mode

jhand2 · 2023-12-15T21:04:56Z

verification/deriveChild.go

+	// Privilege escalation of child
+	// Adding new privileges to child that parent does NOT possess will cause failure
+	_, err = c.DeriveChild(handle,
+		make([]byte, digestLen),
+		DeriveChildFlags(InputAllowX509|InputAllowCA),
+		0, 0)
+	if err == nil {
+		t.Errorf("[ERROR]: Should return %q, but returned no error", StatusInvalidArgument)
+	} else if !errors.Is(err, StatusInvalidArgument) {
+		t.Errorf("[ERROR]: Incorrect error type. Should return %q, but returned %q", StatusInvalidArgument, err)
+	}
+
+	// Privilege escalation for commands run in child context
+	// Similarly, when commands like CertifyKey try to make use of features that are unsupported
+	// by child context, it will fail.
+	if _, err = c.CertifyKey(handle, make([]byte, digestLen), CertifyKeyX509, CertifyAddIsCA); err == nil {
+		t.Errorf("[ERROR]: Should return %q, but returned no error", StatusInvalidArgument)
+	} else if !errors.Is(err, StatusInvalidArgument) {
+		t.Errorf("[ERROR]: Incorrect error type. Should return %q, but returned %q", StatusInvalidArgument, err)
+	}


Should the privilege escalation tests go in a separate test case?

jhand2 · 2023-12-15T21:06:03Z

verification/deriveChild.go

+	// Internal input flags
+	if resp, err = c.DeriveChild(handle, make([]byte, digestLen), InternalInputDice, 0, 0); err != nil {
+		t.Errorf("[Error]: Error while making child context handle as default handle: %s", err)
+	}
+	handle = &resp.NewContextHandle
+
+	if resp, err = c.DeriveChild(handle, make([]byte, digestLen), InternalInputInfo, 0, 0); err != nil {
+		t.Errorf("[Error]: Error while making child context handle as default handle: %s", err)
+	}
+	handle = &resp.NewContextHandle


Should put these in a separate test case since not all implementation will support internal inputs.

jhand2 · 2023-12-15T21:08:33Z

verification/deriveChild.go

+
+	// Finally, restore locality of handle for other tests
+	prevLocality := currentLocality
+	d.SetLocality(otherLocality)


Not every test target has external locality control. We should move anything that requires locality control to a different test case and skip it if !d.HasLocalityControl()

Have made it a separate test target. Added this check too.

if !d.HasLocalityControl() { t.Skip("WARNING: DPE profile does not have control over locality. Skipping this test...") }

verification/deriveChild.go

jhand2 · 2023-12-15T21:12:30Z

verification/deriveChild.go

+	// Child context handle returned will be the default context handle when MakeDefault flag is set
+	if resp, err = c.DeriveChild(handle, make([]byte, digestLen), MakeDefault, 0, 0); err != nil {
+		t.Errorf("[ERROR]: Error while creating child context handle: %s", err)
+	}
+	handle = &resp.NewContextHandle
+	if resp.NewContextHandle != DefaultContextHandle {
+		t.Fatalf("[FATAL]: Incorrect handle. Should return %v, but returned %v", DefaultContextHandle, *handle)
+	}


Anything that permanently modifies the DPE tree (requiring a reset) should be moved to a separate test case.

In this case, this will create a new retired node which can't be removed until next reset.

This is made a separate test case and is moved out of list of common testcases - i.e. "AllTestCases" list

Can you make a separate list (similar to AllTestCases) called something like IrreversibleTestCases. And add any test case to that list which permenantly modifies the DPE tree?

This will make it easier for a target (other than the simulator) with reset control to run these tests separately, resetting DPE in between each one.

Added the list, so far there are just two tests DeriveChild and DeriveChild in Simulation mode that alter DPE tree.

hpya93 · 2023-12-18T16:55:15Z

verification/certifyKey.go

+func TestDiceTcbInfoSimulation(d TestDPEInstance, c DPEClient, t *testing.T) {
+	testDiceTcbInfo(d, c, t, true)
+}
+


Made as a separate test case because, TestDiceTcbInfo requires derived context for various fields validation. Deriving child context requires to use MakeDefault, ChangeLocality(for Sim mode) when used as a part of "AllTestCases". So made it as separate testcase.

hpya93 · 2023-12-18T16:58:20Z

verification/certifyKey.go

 )

-type TcgMultiTcbInfo = []DiceTcbInfo
-


Moved to certs.go to remove cluttering

hpya93 · 2023-12-18T17:00:10Z

verification/certifyKey.go

+//   - the "type" field must contain 4-byte tciType is provided by a client to DeriveChild.
+//   - the "fwid" field must contain cumulative TCI measurement.
+func testDiceTcbInfo(d TestDPEInstance, c DPEClient, t *testing.T, simulation bool) {
+	handle := getInitialContextHandle(d, c, t, simulation)


Do field validation for MultiTcb extension

hpya93 · 2023-12-18T17:00:32Z

verification/certifyKey.go

-	if err != nil {
-		t.Error(err)
+func checkPubKey(t *testing.T, p Profile, pubkey any, response CertifiedKey) {
+	var pubKeyInResponse ecdsa.PublicKey


Existing function

hpya93 · 2023-12-18T17:00:53Z

verification/certifyKey.go

-		t.Error(err)
+// Checks whether the context handle is unchanged after certifyKey command when default context handle is used.
+func checkCertifyKeyRespHandle(res CertifiedKey, t *testing.T, handle *ContextHandle) {
+	if *handle != DefaultContextHandle {


Existing function

verification/certifyKey.go

verification/certs.go

verification/deriveChild.go

verification/extendTCI.go

verification/getCertificateChain.go

verification/simulator.go

verification/deriveChild.go

jhand2 · 2023-12-19T18:06:41Z

verification/deriveChild.go

+	// Child context handle returned will be the default context handle when MakeDefault flag is set
+	if resp, err = c.DeriveChild(handle, make([]byte, digestLen), MakeDefault, 0, 0); err != nil {
+		t.Errorf("[ERROR]: Error while creating child context handle: %s", err)
+	}
+	handle = &resp.NewContextHandle
+	if resp.NewContextHandle != DefaultContextHandle {
+		t.Fatalf("[FATAL]: Incorrect handle. Should return %v, but returned %v", DefaultContextHandle, *handle)
+	}


Can you make a separate list (similar to AllTestCases) called something like IrreversibleTestCases. And add any test case to that list which permenantly modifies the DPE tree?

This will make it easier for a target (other than the simulator) with reset control to run these tests separately, resetting DPE in between each one.

jhand2 · 2023-12-19T18:08:01Z

verification/deriveChild.go

+// Validates DerivedChild command with ChangeLocality flag.
+func TestChangeLocality(d TestDPEInstance, c DPEClient, t *testing.T) {
+	if !d.HasLocalityControl() {
+		t.Skip("WARNING: DPE profile does not have control over locality. Skipping this test...")


nit:

Suggested change

t.Skip("WARNING: DPE profile does not have control over locality. Skipping this test...")

t.Skip("WARNING: DPE target does not have control over locality. Skipping this test...")

Changed this

sree-revoori1 · 2024-03-04T21:55:15Z

Done in #305

hpya93 and others added 10 commits October 31, 2023 21:31

rebase and add tpm policysigning changes

706fb63

rebase

c45596e

deriveChild commit with a bug fix

ff7e4b1

deriveChild commit with a bug fix

12380d0

deriveChild commit with a bug fix

58b6d39

Merge branch 'main' into derivechild_support

9795b8b

rebase

0847e27

rebase

aa5b38a

update

31dc764

update

da35b5c

sree-revoori1 mentioned this pull request Nov 28, 2023

Implement additional commands in DPE Go client #248

Closed

hpya93 and others added 6 commits November 28, 2023 23:33

rebased code

f689aee

Merge branch 'chipsalliance:main' into derivechild_support

12c2cf7

.

1bbfb04

Merge branch 'main' into derivechild_support

775ad30

.

c17fe54

Merge branch 'chipsalliance:main' into derivechild_support

a97ff49

jhand2 reviewed Dec 15, 2023

View reviewed changes

hpya93 added 2 commits December 16, 2023 19:27

.

04826b2

.

76fe4aa