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

Add swtpm example #57

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add swtpm example #57

wants to merge 2 commits into from

Conversation

dimakuv
Copy link

@dimakuv dimakuv commented Mar 3, 2023

Requires a PR in Gramine: gramineproject/gramine#1210. UPDATE: That PR was merged already.


This change is Reviewable

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

a discussion (no related file):
@stefanberger Would you be interested in checking this PR?

I'm particularly interested in the security guarantees for swtpm with Gramine-SGX. Like, did I encrypt all files that swtpm creates, and whether I need to add some other tools inside Gramine-SGX.


swtpm/README.md Outdated

This configuration means:
- run `swtpm` in TPM2 mode,
- save all TPM state under `/myvtpm2/` dir (encrypted under Gramine with SGX),

Choose a reason for hiding this comment

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

Does Gramine with SGX transparently encrypt file contents? If not you could pass a state encryption key here (though have to keep the encryption key then in a safe place).

@stefanberger
Copy link

@stefanberger Would you be interested in checking this PR?

I'm particularly interested in the security guarantees for swtpm with Gramine-SGX. Like, did I encrypt all files that swtpm creates, and whether I need to add some other tools inside Gramine-SGX.

If Gramine-SGX transparently encrypts files, then obviously you are safe. If not then the state at least can be encrypted with a key passed via command line to swtpm. If swtpm was made to log, then this log will not be encrypted and there's no possibility to pass a key, but you are not logging.

Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

If swtpm was made to log, then this log will not be encrypted and there's no possibility to pass a key, but you are not logging.

Yes, I am not logging.

Thanks for this quick review, looks like this example is Ok from the security perspective? (I may play with migration keys supported in swtpm in the future, but I'm not using this now.)

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @stefanberger)


swtpm/Makefile line 10 at r2 (raw file):

ifeq ($(SGX),1)
GRAMINE_ENCRYPTION_KEY = "_sgx_mrenclave"

@stefanberger This is where we choose the SGX-special _sgx_mrenclave encryption key for files.


swtpm/Makefile line 24 at r2 (raw file):

	gramine-manifest \
		-Dlog_level=$(GRAMINE_LOG_LEVEL) \
		-Dencryption_key=$(GRAMINE_ENCRYPTION_KEY) \

@stefanberger And this is where we propagate the chosen encryption key into the manifest file.


swtpm/README.md line 33 at r2 (raw file):

Does Gramine with SGX transparently encrypt file contents?

Yes, see my other comments.


swtpm/swtpm.manifest.template line 28 at r2 (raw file):


  { path = "{{ entrypoint }}", uri = "file:{{ entrypoint }}" },
  { type = "encrypted", path = "/myvtpm2/", uri = "file:myvtpm2/", key_name = "{{ encryption_key }}" },

@stefanberger Please see this line -- here we ask Gramine to encrypt (using a special crypto protocol) all files under the path /myvtpm2/, and we use the encryption key {{ encryption_key }}. This encryption key variable resolves into "_sgx_mrenclave", which is an SGX-specific encryption key that ties encryption to the platform + to this particular SGX enclave.

@stefanberger
Copy link

If swtpm was made to log, then this log will not be encrypted and there's no possibility to pass a key, but you are not logging.

Yes, I am not logging.

Thanks for this quick review, looks like this example is Ok from the security perspective? (I may play with migration keys supported in swtpm in the future, but I'm not using this now.)

I think so, yes.

	gramine-manifest \
		-Dlog_level=$(GRAMINE_LOG_LEVEL) \
		-Dencryption_key=$(GRAMINE_ENCRYPTION_KEY) \

@stefanberger And this is where we propagate the chosen encryption key into the manifest file.

Ok. I don't even have SGX ... just portS of Gramine :-)

swtpm/README.md line 33 at r2 (raw file):

Does Gramine with SGX transparently encrypt file contents?

Yes, see my other comments.

Even better, then you could probably log as well and have that log encrypted.

swtpm/swtpm.manifest.template line 28 at r2 (raw file):


  { path = "{{ entrypoint }}", uri = "file:{{ entrypoint }}" },
  { type = "encrypted", path = "/myvtpm2/", uri = "file:myvtpm2/", key_name = "{{ encryption_key }}" },

@stefanberger Please see this line -- here we ask Gramine to encrypt (using a special crypto protocol) all files under the path /myvtpm2/, and we use the encryption key {{ encryption_key }}. This encryption key variable resolves into "_sgx_mrenclave", which is an SGX-specific encryption key that ties encryption to the platform + to this particular SGX enclave.

Looks good to me.

Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Even better, then you could probably log as well and have that log encrypted.

Yes, this could be also done.

Again, @stefanberger thanks for the review!

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)


swtpm/Makefile line 52 at r2 (raw file):

ifeq ($(SGX),)
GRAMINE = gramine-direct

Is this used somewhere?

Code quote:

GRAMINE

swtpm/README.md line 33 at r2 (raw file):

This configuration means:
- run `swtpm` in TPM2 mode,
- save all TPM state under `/myvtpm2/` dir (encrypted under Gramine with SGX),

It's actually encrypted anyway (though w/ a dummy hard-coded key w/o SGX).

Code quote:

encrypted under Gramine with SGX

swtpm/README.md line 109 at r2 (raw file):

##   0000060 d6 0d 01 a4 c4 37 3c f2 8a db 56 c9 b4 54

## 5 step: check TPM Established flag (must be 1)

Don't we check TPM Established flag equals 0 before hashing?

Code quote:

(must be 1)

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)


swtpm/Makefile line 52 at r2 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Is this used somewhere?

Done, it was a leftover from a previous version.


swtpm/README.md line 33 at r2 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

It's actually encrypted anyway (though w/ a dummy hard-coded key w/o SGX).

Done. Good catch.


swtpm/README.md line 109 at r2 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Don't we check TPM Established flag equals 0 before hashing?

Done. I didn't feel like it's important (though I tested for it), but since you're asking, I added this step.

Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)

a discussion (no related file):
FYI: Verified on the latest master branch of Gramine, everything works.


Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 5 files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners

Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners

a discussion (no related file):
There is no protection again rollback attacks (/myvtpm2/ files are encrypted, but the attacker could silently replace the new files with the old files, and the newly restarted swtpm app inside the SGX enclave won't notice the difference).

So two options:

  1. Either I just add a remark in the README, and make it somebody else's problem.
  2. Or I come up with at least some check for the trusted monotonic counter.
  3. (Or we can just have an ephemeral files, and simply claim that swtpm state is all lost if it is terminated.)

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

a discussion (no related file):

  1. Either I just add a remark in the README, and make it somebody else's problem.
  2. Or I come up with at least some check for the trusted monotonic counter.
  3. (Or we can just have an ephemeral files, and simply claim that swtpm state is all lost if it is terminated.)

If we consider examples in this repo: https://github.com/gramineproject/examples to be production ready, then I'd prefer opt#2. Otherwise if it's only a demonstation of Gramine's capability to support this kind of workloads, then opt#1 or opt#3 or (opt#1+opt#3) should be fine (we can provide some potential solutions together w/ the remark).


Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)

a discussion (no related file):

Previously, kailun-qin (Kailun Qin) wrote…
  1. Either I just add a remark in the README, and make it somebody else's problem.
  2. Or I come up with at least some check for the trusted monotonic counter.
  3. (Or we can just have an ephemeral files, and simply claim that swtpm state is all lost if it is terminated.)

If we consider examples in this repo: https://github.com/gramineproject/examples to be production ready, then I'd prefer opt#2. Otherwise if it's only a demonstation of Gramine's capability to support this kind of workloads, then opt#1 or opt#3 or (opt#1+opt#3) should be fine (we can provide some potential solutions together w/ the remark).

I didn't come up with something simple. So I'll go with option 2.


a discussion (no related file):
I need to test swtpm_setup, that simulates manufacturing of a TPM. See man swtpm_setup, there is EXAMPLE USAGE at the end.

This is important because we'd like to at least claim that the initial TPM state can be remotely provisioned from the trusted party (which in this case serves as the manufacturer/CA for this TPM). Otherwise e.g. Endorsement Keys (EKs) are not really known (or not securely provisioned inside the TPM).


Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)

a discussion (no related file):
Add the explicit BSD-3 license, see #90


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.

3 participants