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

PCR policy authmodel #833

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

Conversation

SergiiDmytruk
Copy link

@SergiiDmytruk SergiiDmytruk commented Mar 21, 2023

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 to addkey, import and link subcommands. There is also a new subcommand called objpol, which is similar to objmod 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 to objmod 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 if tpm2-policy isn't present or --without-policy is passed to configure.

There are also a couple of trivial changes in separate commits.

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
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #833 (bccb707) into master (1b3aab9) will decrease coverage by 0.01%.
The diff coverage is 70.00%.

@@            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     
Impacted Files Coverage Δ
src/lib/attrs.c 72.00% <ø> (ø)
src/lib/tpm.c 71.41% <ø> (ø)
src/lib/sign.c 76.55% <70.00%> (-0.21%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@SergiiDmytruk SergiiDmytruk marked this pull request as ready for review March 28, 2023 18:18
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]>
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]>
@glance-
Copy link

glance- commented Jan 16, 2024

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.

@SergiiDmytruk
Copy link
Author

@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 (tpm-policy.json)

Policies are described in JSON format (specification), the case of a PCR policy is quite simple:

{
  "description": "PCR7 policy",
  "policy": [{
    "type": "pcr",
    "pcrs": [{
        "pcr": 7,
        "hashAlg": "sha256",
        "digest": "0000000000000000000000000000000000000000000000000000000000000000"
    }]
  }]
}

The description field is merely informational and is optional in the specification, but implementation in tpm2-tss requires it to be present.

Current value of a PCR can be obtained via:

tpm2_pcrread sha256:7

Use it

# initialize pkcs#11
tpm2_ptool init
# create token
tpm2_ptool addtoken --pid 1 --sopin mysopin --userpin myuserpin --label tpm20
# create a key protected by the policy
tpm2_ptool addkey --label=tpm20 --key-label=mykey --userpin=myuserpin \
                  --algorithm=rsa2048 --policy="$(cat tpm-policy.json)"

@glance-
Copy link

glance- commented Jan 25, 2024

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 tpm2_pcrextend 8:sha256=f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2

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?

@SergiiDmytruk
Copy link
Author

Not sure, but it might be related to the fact that policy is processed only for signing here.

@glance-
Copy link

glance- commented Jan 25, 2024

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.

@SergiiDmytruk
Copy link
Author

You might want to look into something like this key creation and its usage with a policy. This one is enforced by TPM.

@glance-
Copy link

glance- commented Feb 5, 2024

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 tpm2_ptool addkey ... --policy

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.

@SergiiDmytruk
Copy link
Author

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.

Copy link
Member

@williamcroberts williamcroberts left a 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
Copy link
Member

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);
Copy link
Member

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;
Copy link
Member

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? */
Copy link
Member

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):
Copy link
Member

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.

@williamcroberts williamcroberts added this to the next milestone Sep 5, 2024
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