Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be much stricter when processing Attested COS State #246

Merged
merged 4 commits into from
Sep 12, 2022
Merged

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented Sep 10, 2022

Currently in getVerifiedCosState we do two things that are bad from a security perspective:

  1. We do not require the CEL events to use PCR 13
  2. We skip events that we do not understand (record.Content.Type != cel.CosEventType)

(1) is an issue because after a container launchers, we assume that a breakout is possible. So a malicious user can:

  • Breakout of a container
  • Extend fake events to PCR 14
  • Create their own fake CEL eventlog matching the PCR 14 extensions
  • Verify an attestation using PCR 14 and this fake CEL eventlog
  • Get fake container state to appear in the MachineState.

(2) is an issue because the record.Content.Type hasn't (yet) been verified in the loop. This means that a malicious user can:

  • Breakout of a container
  • Modify the CEL log to change the content type
  • Our processing will then skip this valid event
  • This causes data to be omitted from the MachineState

This change:

  • Adds the concept of a "launch separator" event that is measured right before a container is launched
  • Only creates AttestedCosState from PCR 13 data
  • Fails processing if we encounter any CEL events we don't understand (COS or otherwise)
  • Will not accept any AttestedCosState after a separator event is seen.
  • Will not accept duplicate ImageRefType, ImageDigestType, or ImageIDType
    • We can't do this for other events, as having multiple events for the other types isn't an error.
    • For example: multiple EnvVarType events are present if multiple environment variables are overridden.

This fixes the worst of the issues described above. Note, this doesn't change the client-side launcher at all. #247 will change the launcher code to actually measure in the LaunchSeparator event. Until then, a container breakout can cause some of the AttestedCosState to be wrong, but it cannot modify image_reference, image_digest, or image_id.

I also had to modify our tests somewhat, including adding test.SkipForRealTPM(t) as we can only run some of our tests against PCR13, which cannot be reset on a Real TPM.

Signed-off-by: Joe Richey joerichey@google.com

Signed-off-by: Joe Richey <joerichey@google.com>
server/eventlog.go Outdated Show resolved Hide resolved
server/eventlog.go Show resolved Hide resolved
server/eventlog.go Outdated Show resolved Hide resolved
Signed-off-by: Joe Richey <joerichey@google.com>
"found more than one" is more accurate as the events don't actually have
to be duplicates to cause an issue.

Signed-off-by: Joe Richey <joerichey@google.com>
Copy link
Contributor

@jessieqliu jessieqliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some minor changes

server/eventlog.go Show resolved Hide resolved
server/eventlog_test.go Outdated Show resolved Hide resolved
Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr josephlr merged commit 659e615 into master Sep 12, 2022
@josephlr josephlr deleted the stricter branch September 12, 2022 20:43
alexmwu added a commit to alexmwu/go-tpm-tools that referenced this pull request Dec 16, 2022
Breaking Changes:

New Features:
Add IsHardened in launch spec: google#244
Add container logging redirect policy: google#249
Add SEV-SNP attestation support: google#240
Integrity-protect stateful partition on CS image: google#251
Retry launcher OIDC token refresh with backoff: google#261
Change restart policy behavior to reboot: google#260
Add ability to GetGCEInstanceInfo from a certificate: google#267

Bug Fixes:
COS event log: require CEL events to use PCR13, add a launch separator, and don't skip unknown events: google#246
Measure LaunchSeparator event: google#247
Skip unallocated PCR selections when reading all PCRs: google#258
Remove gRPC client and use of insecure credentials: google#262
Fix server.VerifyAttestation proto merging(google#263) and defer of os.Exit(google#264): google#265

Other Changes:
Add fake verifier client: google#234
Update CI Go Version to 1.19: google#241
Add launcher integration testing support: google#255
Test multi-writer PD creation disabled: google#256
Update go-sev-guest dependency to v0.2.6: google#259
Change OIDC retry policy to hourly and add jitter to refresh time: google#266
Add wrapper cloudbuild workflow to trigger image build and testing: google#269
@alexmwu alexmwu mentioned this pull request Dec 16, 2022
alexmwu added a commit that referenced this pull request Dec 16, 2022
Breaking Changes:

New Features:
Add IsHardened in launch spec: #244
Add container logging redirect policy: #249
Add SEV-SNP attestation support: #240
Integrity-protect stateful partition on CS image: #251
Retry launcher OIDC token refresh with backoff: #261
Change restart policy behavior to reboot: #260
Add ability to GetGCEInstanceInfo from a certificate: #267

Bug Fixes:
COS event log: require CEL events to use PCR13, add a launch separator, and don't skip unknown events: #246
Measure LaunchSeparator event: #247
Skip unallocated PCR selections when reading all PCRs: #258
Remove gRPC client and use of insecure credentials: #262
Fix server.VerifyAttestation proto merging(#263) and defer of os.Exit(#264): #265

Other Changes:
Add fake verifier client: #234
Update CI Go Version to 1.19: #241
Add launcher integration testing support: #255
Test multi-writer PD creation disabled: #256
Update go-sev-guest dependency to v0.2.6: #259
Change OIDC retry policy to hourly and add jitter to refresh time: #266
Add wrapper cloudbuild workflow to trigger image build and testing: #269
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants