-
Notifications
You must be signed in to change notification settings - Fork 22
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
There was a problem hiding this 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.
8980e46
to
0731b2a
Compare
swtpm/README.md
Outdated
|
||
This configuration means: | ||
- run `swtpm` in TPM2 mode, | ||
- save all TPM state under `/myvtpm2/` dir (encrypted under Gramine with SGX), |
There was a problem hiding this comment.
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).
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. |
There was a problem hiding this 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.
I think so, yes.
Ok. I don't even have SGX ... just portS of Gramine :-)
Even better, then you could probably log as well and have that log encrypted.
Looks good to me. |
There was a problem hiding this 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)
There was a problem hiding this 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]>
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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, 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
There was a problem hiding this 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:
- Either I just add a remark in the README, and make it somebody else's problem.
- Or I come up with at least some check for the trusted monotonic counter.
- (Or we can just have an ephemeral files, and simply claim that
swtpm
state is all lost if it is terminated.)
There was a problem hiding this 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):
- Either I just add a remark in the README, and make it somebody else's problem.
- Or I come up with at least some check for the trusted monotonic counter.
- (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).
There was a problem hiding this 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…
- Either I just add a remark in the README, and make it somebody else's problem.
- Or I come up with at least some check for the trusted monotonic counter.
- (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).
There was a problem hiding this 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
Requires a PR in Gramine: gramineproject/gramine#1210. UPDATE: That PR was merged already.This change is