Add an initial implementation of parsing event types #108

mjg59 · 2019-09-20T21:35:01Z

Raw events aren't enormously helpful. Add an initial implementation of
some of the basic event types - this is largely a scratch proposal so we
can figure out API design.

ericchiang · 2019-09-20T21:45:26Z

Can you please put this in a different package for a bit? This would double the amount of exported API and we've worked hard to get the godoc to be digestible (#63): https://godoc.org/github.com/google/go-attestation/attest

With that in mind, I think we can avoid making the user type cast every event back to the type we want. Tom and I were discussing having Parse methods that parsed the event body. e.g.

type MicrocodeEvent struct {
    // ...
}
func ParseMicrocodeEvent(eventData []byte) (*MicrocodeEvent, error) {
    // ...
}

So the user can iterate through the events and use the corresponding Parse method

for _, e := range events {
    switch e.Type {
    case eventlog.TypeCPUMicrocode:
        mcEvent, err := eventlog.ParseMicrocodeEvent(e.Data)
    // ... 
    }
}

mjg59 · 2019-09-20T21:55:57Z

Can you please put this in a different package for a bit? This would double the amount of exported API and we've worked hard to get the godoc to be digestible (#63): https://godoc.org/github.com/google/go-attestation/attest

Sure, though that'll mean exporting rawEvent?

With that in mind, I think we can avoid making the user type cast every event back to the type we want. Tom and I were discussing having Parse methods that parsed the event body. e.g.

for _, e := range events {
switch e.Type {
case eventlog.TypeCPUMicrocode:
mcEvent, err := eventlog.ParseMicrocodeEvent(e.Data)

Is this really easier than

switch e.Type {
  case eventlog.TypeCPUMicrocode:
     mcEvent, ok := e.(CPUMicrocodeEvent)

? Splitting out the individual parse methods means a lot of basically identical functions, which feels a little less clean than basically doing the same in a switch statement (although for more complex types, we should probably also be exporting information about whether we succeeded in parsing the event)

ericchiang · 2019-09-20T22:12:14Z

You can use the exported Event type :) https://godoc.org/github.com/google/go-attestation/attest#Event

I think both strategies are similar amounts of code for the caller. For example to get a single event:

events, err := eventlog.ParseEvents(verifiedEvents)
if err != nil {
    // ...
}
for _, e := range events {
    if pbce, ok := e.(eventlog.PrebootCertEvent); ok {
        // use parsed event
    }
}

vs.

for _, e := range events {
    if e.Type == eventlog.TypePrebootCertEvent {
        pbce, err := eventlog.ParsePrebootCertEvent(e.Data)
        if err != nil {
            // ...
        }
        // use parsed event
    }
}

I suggested the latter because there's a clearer place for users to parse event types that we don't provide a parser for.

Either way, I don't think there's a huge difference. Feel free to go with either API

mjg59 · 2019-09-20T22:25:11Z

You can use the exported Event type :) https://godoc.org/github.com/google/go-attestation/attest#Event

But EventLog contains an array of rawEvent rather than Event?

ericchiang · 2019-09-20T22:27:51Z

Part of the API design is to require replaying the event log against PCRs before moving forward https://godoc.org/github.com/google/go-attestation/attest#EventLog.Verify

We want users of the API to be safe by default, and not mistakenly use something that's not been verified.

mjg59 · 2019-09-20T22:29:31Z

Oh, yes, I see what you mean now.

events/events.go

mjg59 · 2019-10-11T05:02:16Z

Ok, added support for Windows event types and tests against a bunch of sample event logs.

ericchiang

I left some comments, but this PR got a lot bigger before addressing the previous review. To avoid a moving target, can we get the original code merged first? :)

Reviewed the existing files, and will start looking at the events/windows.go later. Sorry I know the timezone difference isn't helping here.

ericchiang · 2019-10-11T17:53:34Z

events/events.go

+		return ""
+	}
+	uuid := uuid.UUID{}
+	if err := uuid.UnmarshalBinary(buf.Bytes()); err != nil {


Instead of importing a uuid package, please do:

func (d efiGUID) String() string { data1 := [4]byte{} data2 := [2]byte{} data3 := [2]byte{} binary.BigEndian.PutUint32(data1[:], d.Data1) binary.BigEndian.PutUint16(data2[:], d.Data2) binary.BigEndian.PutUint16(data3[:], d.Data3) return fmt.Sprintf("%x-%x-%x-%x", data1, data2, data3, d.Data4) }

ericchiang · 2019-10-11T17:54:51Z

events/events.go

+	}
+
+	if !bootOption.MatchString(parsedEvent.VariableName) {
+		return nil


Why not an error here?

ericchiang · 2019-10-11T17:57:14Z

events/events.go

+		return fmt.Errorf("malformed boot variable")
+	}
+
+	parsedEvent.DevicePathRaw = make([]byte, dplength)


Note, this could be a dos vector like #126 and #128. At the point where we're parsing the event, we don't actually trust the device.

ericchiang · 2019-10-11T17:58:00Z

events/events.go

+	// Check whether there's any optional data
+	optionaldatalen := len(parsedEvent.VariableData) - dpoffset - int(dplength)
+	if optionaldatalen > 0 {
+		parsedEvent.OptionalData = make([]byte, optionaldatalen)


same dos vector as above

ericchiang · 2019-10-11T17:58:15Z

events/events.go

+	if err != nil {
+		return err
+	}
+	parsedEvent.DevicePathRaw = make([]byte, devicePathLength)


ericchiang · 2019-10-11T17:59:32Z

events/events.go

+	if err != nil {
+		return err
+	}
+	parsedEvent.Tables = make([]efiConfigurationTable, numTables)


ericchiang · 2019-10-11T18:01:01Z

events/events.go

+	if err != nil {
+		return err
+	}
+	unicodeName := make([]uint16, header.UnicodeNameLength)


ericchiang · 2019-10-11T18:01:54Z

events/devicepath.go

+	return fmt.Sprintf("%s(%d,%02x)", prefix, dp.SubType, data[:])
+}
+
+// efiDevicePath translates an EFI Device Path into the canonical string representation


got a link for where the canonical representation is specified?

There's currently no support for creating application keys on Windows systems. This patch transitions the Windows key type to specifically refer to attestation keys, and reuses the existing wrapped key support for application keys. This allows the creation of keys in the platform store, while still allowing said keys to be manipulated with existing TPM functionality rather than duplicating it.

When generating a new key using a Windows TPM, a `wrappedKey20` was returned, which couldn't be used for signing on Windows, as it's backed by a `windowsTPM`. The `wrappedKey20` seems to be a type specifically aimed at usage with a `wrappedTPM20`, which in turn seems to be used on Linux and for testing, but not when instantiating a TPM on Windows. This commit adds the `newWindowsKey20` function, which returns a key backed by a `windowsTPM`. The key is a `windowsAK20`, now also conforming to the `key` interface, so that it can be used for signing purposes.

My system is returning RCScheme if TPM_ALG_NULL is passed here. This should be causing the key's default scheme to be used, but for some reason it seems unhappy. Just explicitly set the scheme for now to avoid that.

This reverts commit 03018e6.

mjg59 force-pushed the master branch from 394f627 to 64b577d Compare September 20, 2019 22:42

ericchiang reviewed Sep 20, 2019

View reviewed changes

events/events.go Outdated Show resolved Hide resolved

events/events.go Outdated Show resolved Hide resolved

events/events.go Outdated Show resolved Hide resolved

mjg59 force-pushed the master branch 5 times, most recently from 4b93759 to 6f41e62 Compare October 11, 2019 05:01

mjg59 mentioned this pull request Oct 11, 2019

Any plans for a TCPA log parser? #9

Closed

ericchiang reviewed Oct 11, 2019

View reviewed changes

mjg59 force-pushed the master branch 3 times, most recently from d4551cc to fbfc4a7 Compare May 23, 2023 19:09

mjg59 and others added 4 commits May 24, 2023 12:16

Fix missing ECDSA curve when signing on Windows

23380a3

Explicitly set scheme for ECDSA signing

6e414e2

My system is returning RCScheme if TPM_ALG_NULL is passed here. This should be causing the key's default scheme to be used, but for some reason it seems unhappy. Just explicitly set the scheme for now to avoid that.

mjg59 force-pushed the master branch from fbfc4a7 to 6e414e2 Compare May 24, 2023 20:15

Revert "Remove certificate-transparency-go dependancy"

f28ecb7

This reverts commit 03018e6.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an initial implementation of parsing event types #108

Add an initial implementation of parsing event types #108

mjg59 commented Sep 20, 2019

ericchiang commented Sep 20, 2019

mjg59 commented Sep 20, 2019

ericchiang commented Sep 20, 2019

mjg59 commented Sep 20, 2019

ericchiang commented Sep 20, 2019

mjg59 commented Sep 20, 2019

mjg59 commented Oct 11, 2019

ericchiang left a comment

ericchiang Oct 11, 2019 •

edited

Loading

ericchiang Oct 11, 2019

ericchiang Oct 11, 2019

ericchiang Oct 11, 2019

ericchiang Oct 11, 2019

ericchiang Oct 11, 2019

ericchiang Oct 11, 2019

ericchiang Oct 11, 2019

Add an initial implementation of parsing event types #108

Are you sure you want to change the base?

Add an initial implementation of parsing event types #108

Conversation

mjg59 commented Sep 20, 2019

ericchiang commented Sep 20, 2019

mjg59 commented Sep 20, 2019

ericchiang commented Sep 20, 2019

mjg59 commented Sep 20, 2019

ericchiang commented Sep 20, 2019

mjg59 commented Sep 20, 2019

mjg59 commented Oct 11, 2019

ericchiang left a comment

Choose a reason for hiding this comment

ericchiang Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

ericchiang Oct 11, 2019

Choose a reason for hiding this comment

ericchiang Oct 11, 2019

Choose a reason for hiding this comment

ericchiang Oct 11, 2019

Choose a reason for hiding this comment

ericchiang Oct 11, 2019

Choose a reason for hiding this comment

ericchiang Oct 11, 2019

Choose a reason for hiding this comment

ericchiang Oct 11, 2019

Choose a reason for hiding this comment

ericchiang Oct 11, 2019

Choose a reason for hiding this comment

ericchiang Oct 11, 2019 •

edited

Loading