-
Notifications
You must be signed in to change notification settings - Fork 104
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
PCR policy authmodel #833
base: master
Are you sure you want to change the base?
PCR policy authmodel #833
Conversation
This makes it easier to add new vendor attributes without accidentally reusing the same constant value and making implementations incompatible. Signed-off-by: Sergii Dmytruk <[email protected]>
To be used as a storage of TPM policy in JSON formed understood by tpm2-tss libpolicy library. Signed-off-by: Sergii Dmytruk <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #833 +/- ##
==========================================
- Coverage 72.43% 72.43% -0.01%
==========================================
Files 34 34
Lines 9773 9783 +10
==========================================
+ Hits 7079 7086 +7
- Misses 2694 2697 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
824688c
to
9667048
Compare
Policies are expected to be specified in JSON format. The command allows reading, writing or deleting policy set on an object. Signed-off-by: Sergii Dmytruk <[email protected]>
To specify the policy right on creation. Signed-off-by: Sergii Dmytruk <[email protected]>
It was added in tpm2-tss v4.0 and functionality that requires it will be no-op when it's not there or --without-policy is specified during configuration step. Signed-off-by: Sergii Dmytruk <[email protected]>
If tpm2-pkcs11 was built without tss2-policy, policy is ignored (a warning is logged about it). This might work for other kinds of policies if they don't require any callbacks for calculation or execution of a policy. Signed-off-by: Sergii Dmytruk <[email protected]>
Signed-off-by: Sergii Dmytruk <[email protected]>
It said "add" instead of "export". Signed-off-by: Sergii Dmytruk <[email protected]>
It's --with/without-fapi (or --with-fapi=yes/no). Signed-off-by: Sergii Dmytruk <[email protected]>
It's a calculation callback, but not yet calculated policy is automatically calculated before execution. Signed-off-by: Sergii Dmytruk <[email protected]>
e7a8ead
to
bccb707
Compare
I'd like to see some examples on how to use this, to for example lock a key to only be usable when a set of pcr's has a certain value. |
@glance-, we ended up not needing the changes (at least not yet), so haven't touched this stuff for awhile, but some example based on notes is provided below. Create a policy (
|
I think I'm missing something here. I've built a tpm2-pkcs11 with this pull-request merged ontop of master, tpm2-tss 4.0.1 and tpm2-pytss 2.2.0-rc0 which all where the latest when I started playing around with this. When I create a key with a policy just as you've described, only changed policy from pcr 7 to pcr 8 for my tests to be simpler. To create a simple self signed certificate with that key I've used openssl with -provider tpm as shown in #766 (comment) to create such a certificate: yaml_rsa0=$(tpm2_ptool export --label=tpm20 --key-label=mykey --userpin=myuserpin)
auth_rsa0=$(echo "$yaml_rsa0" | grep "object-auth" | cut -d' ' -f2-)
openssl req -new -x509 -provider tpm2 -provider base -key "mykey".pem -passin "pass:$auth_rsa0" -subj "/CN=$HOSTNAME" -out "$HOSTNAME.pem" -days 3650 -nodes -sha256 -addext 'basicConstraints=critical,CA:FALSE' -addext 'keyUsage=digitalSignature' All goes fine while: # tpm2_pcrread sha256:8
sha256:
8 : 0x0000000000000000000000000000000000000000000000000000000000000000 I can even use it via libtpm2_pkcs11.so with ex sbsign: sbsign --engine pkcs11 --key 'pkcs11:token=tpm20;pin-value=myuserpin' --cert "$HOSTNAME.pem" --output test-signed.efi /boot/vmlinuz-6.1.0-17-amd64 If I now "break" things by I can't run above sbsign command via the pkcs11 module, but above openssl -provider tpm2 runs just fine. How can the tpm still allow the openssl tpm provider to access to the key? |
Not sure, but it might be related to the fact that policy is processed only for signing here. |
But the whole idea is that the tpm should refuse to do anything with the key unless it knows the policy is satisfied. That means that even if I dig up the handles to the key some other way it should know that there's a policy on this key, and refuse to do anything unless it's satisfied. I just tried the following dirty hack: diff --git i/src/lib/sign.c w/src/lib/sign.c
index bd91a88..47a07f7 100644
--- i/src/lib/sign.c
+++ w/src/lib/sign.c
@@ -255,7 +255,8 @@ static CK_RV common_init(operation op, session_ctx *ctx, CK_MECHANISM_PTR mechan
if (op == operation_sign) {
rv = policy_is_satisfied(tok->tctx, tobj, tok->pobject.handle);
if (rv != CKR_OK) {
- return rv;
+ LOGE("policy_is_satisfied failed, continuing anyway");
+ rv = CKR_OK;
}
}
And now I get the libtpm2_pkcs11.so to ignore any policies and just sign things anyway, so something is definitely not the way it should be. |
You might want to look into something like this key creation and its usage with a policy. This one is enforced by TPM. |
I was just expecting the key to be setup in a way requiring the policy to be fulfilled for usage, when it was created with I'd guess any other user also would expect that to. But this concludes, that this part of this feature isn't implemented in this PR, which I'd guess would be a blocker for actually merging this. |
The implementation follows this comment. There is always a chance of misunderstanding, so maybe you're right and the implementation isn't correct. |
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.
Overall great work. However, most of my comments are nits. However, this is a big one here. You need to create the object with the proper policy for it to work, ie set the policy hash in the TPM2B_PUBLIC
that is fed to Esys_Create
. Also, you need to update the object attributes to remove userwithauth attribute which will enforce only policy. Right now it's essentially working still on passwords.
Sorry for the delay.
/* the policy is considered to be satisfied, but will warn about it */ | ||
LOGW("Found a policy, but support for enforcing it wasn't compiled in"); | ||
return CKR_OK; | ||
#else |
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.
I would create 2 versions of this function and ifdef between the two and avoid even looking for the policy on non-policy enabled builds.
if (rc != TSS2_RC_SUCCESS) { | ||
LOGE("Esys_PCR_Read: %s:", Tss2_RC_Decode(rc)); | ||
free(pcr_selection); | ||
free(pcr_values); |
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.
These should be ESYS_Free calls.
} | ||
|
||
*out_selection = *pcr_selection; | ||
*out_digest = *pcr_values; |
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.
Personally, I would have the failure Esys_PCR_Read goto an out label that Esys_Free's all the allocated pointers and here where you transfer ownership to out_selection
and out_digest
set the pcr_selection
and pcr_values
to NULL and still pass them to Esys_Free as NULL is ok to pass and then return an rv
variable that gets tripped to success before the out label and is initialized to CKR_GENERAL_ERROR
or whatever makes sense.
return CKR_GENERAL_ERROR; | ||
} | ||
|
||
/* XXX should we cache the session or running multiple policies is unlikely? */ |
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.
This session handle needs to be used to authorize the object for Esys_Sign
. You're essentially still on the password model (see my other comment in the review summary). I would modify the tpm_ctx
struct to have two session handles:
- auth_session: This is the session that will be either the HMAC session OR the policy session from execute policy.
- For HMAC session use, it can just live the lifetime of the token.
- For POLICY session use, it needs to be created for each command.
- You can use the attribute continue session and use policy reset command, but thats a performance thing that can be done later.
- enc_session: This is important, In the current code the HMAC session handle is also the encryption handle for protecting the traffic to and from the TPM, we need this to also be created for the lifetime of the token and passed to each command as the first unused session handle, which in most commands in session2, sometimes its session3.
@@ -551,6 +559,67 @@ def __call__(self, args): | |||
|
|||
ObjDel.delete(path, args['id']) | |||
|
|||
@commandlet("objpol") | |||
class ObjPol(Command): |
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.
I don't think we need this as policies are immutable (unless its like policynv or policysigned) and obj mod would let you do it in theory (you could add some helpers to obj mod so if it's that attribute validate and encode the input.
Hi.
As was mentioned in #301 about a month ago, we want to have the ability to bind use of a private key to an expected value of a PCR.
Currently the changes include a new attribute and related modifications to
tpm2_ptool
. Sending the PR just in case you can see an issue with the changes right away.I've added
--policy
parameter toaddkey
,import
andlink
subcommands. There is also a new subcommand calledobjpol
, which is similar toobjmod
but deals only with the policy.objpol
can be dropped if you don't see much value in it, but then it would be nice to add--raw
toobjmod
for printing value as is rather than as a YAML.Update. Fixed a bug in
tpm2_ptool
and added code for executing a policy during signing if it's present on a private key. The only callback set is "get PCR" which is enough for handling of PCR policies and their templates.I've noticed that there seems to be an effort to maintain compatibility with
tpm2-tss
v2.0, so handling of policies is optional and is disabled iftpm2-policy
isn't present or--without-policy
is passed toconfigure
.There are also a couple of trivial changes in separate commits.