tpm2_unseal regression on 3.1.0

[martinezjavier]

The following commands used to work with tpm2-tools 3.0:

$ tpm2_createprimary -Q -H e -g sha256 -G ecc -C context.p_sha256_ecc
$ tpm2_create -Q -g sha256 -G keyedhash -u opu_sha256_keyedhash -r opr_sha256_keyedhash \
 -I- -c context.p_sha256_ecc -A 'sign|fixedtpm|fixedparent|sensitivedataorigin' <<< secret
$ tpm2_load -Q -c context.p_sha256_ecc -u opu_sha256_keyedhash -r opr_sha256_keyedhash -n \
name.load_sha256_ecc-sha256_keyedhash -C ctx_load_out_sha256_ecc-sha256_keyedhash
$ tpm2_unseal -c ctx_load_out_sha256_ecc-sha256_keyedhash
secret

But since 3.1.0 it fails with:

$ tpm2_unseal -c ctx_load_out_sha256_ecc-sha256_keyedhash
ERROR: Sys_Unseal failed. Error Code: 0x12f
ERROR: Unseal failed!
ERROR: Unable to run tpm2_unseal

$ tpm2_rc_decode 0x12f
error layer
  hex: 0x0
  identifier: TSS2_TPM_RC_LAYER
  description: Error produced by the TPM
format 0 error code
  hex: 0x2f
  name: TPM2_RC_AUTH_UNAVAILABLE
  description: authValue or authPolicy is not available for selected entity.

It works correctly if a PCR selection is provided:

$ tpm2_createprimary -Q -H e -g sha256 -G ecc -C context.p_sha256_ecc
$ tpm2_pcrlist -Q -L sha1:0,1,2,3 -o pcr.bin
$ tpm2_createpolicy -Q -P -L sha1:0,1,2,3 -F pcr.bin -f policy.data
$ tpm2_create -Q -g sha256 -G keyedhash -u opu_sha256_keyedhash -r opr_sha256_keyedhash -I- \
-c context.p_sha256_ecc -L policy.data -A 'sign|fixedtpm|fixedparent|sensitivedataorigin' \
<<< secret
$ tpm2_load -Q -c context.p_sha256_ecc -u opu_sha256_keyedhash -r opr_sha256_keyedhash - \
name.load_sha256_ecc-sha256_keyedhash -C ctx_load_out_sha256_ecc-sha256_keyedhash
$ tpm2_unseal -c ctx_load_out_sha256_ecc-sha256_keyedhash -L sha1:0,1,2,3 -F pcr.bin
secret

I haven't been following tpm2-{tss,tools} development too closely lately but I guess it has to be related to the tpm2-tss 2.0 SAPI library since I don't see a lot of differences in the tools.

@williamcroberts @joshuagl any ideas?

[williamcroberts]

@martinezjavier nothing off the top of my head. This seems like a weird regression, considering our integration test with an empty password: https://github.com/tpm2-software/tpm2-tools/blob/3.1.0/test/system/test_tpm2_unseal.sh#L79

[martinezjavier]

@williamcroberts I found the issue, it's caused by commit e103bbf5117 ("objectattrs: clear before or'ing in values"). Reverting that patch brings up the old behaviour from 3.0.4 and it works again. It wasn't stressed by the test you mention because it uses the default object attributes from tpm2_create.

[martinezjavier]

@williamcroberts I think we should revert that commit for 3.X, since unlike the tpm2-tss 2.0 porting, this is a user facing change for 3.X users.

[williamcroberts]

@joshuagl can you roll another 3.X real quick?

[martinezjavier]

closing since it was fixed by commit 352908f7470, it's annoying that Github only closes issues if the commit is merged in the master branch.

[joshuagl]

3.1.2-rc0 has been tagged.

[diabonas]

As a warning for future readers, note that while this commit appears to fix the problem, it probably does not achieve the intended effect at all: if you omit the PCR list from tpm2_unseal, it will not unlock the secret based on the state of the PCRs, but based on the secret password, which is empty by default, see https://github.com/tpm2-software/tpm2-tools/issues/1097#issuecomment-404591991 and https://github.com/tpm2-software/tpm2-tools/issues/1123#issuecomment-408408796!

So while the revert is probably justified as the commit changed the user interface, one really should use

tpm2_create -Q -g sha256 -G keyedhash -u opu_sha256_keyedhash -r opr_sha256_keyedhash -I- -c context.p_sha256_ecc -A 0x492

and specify the PCR list in tpm2_unseal, or else anybody will be able to unlock the secret no matter the PCR configuration.

[martinezjavier]

and specify the PCR list in tpm2_unseal, or else anybody will be able to unlock the secret no matter the PCR configuration.

@diabonas @williamcroberts @joshuagl so this is a big issue, probably then commit ("e103bbf5117 objectattrs: clear before or'ing in values") justified breaking the user-visible interface on 3.X since it was really about fixing a security vulnerability? What's semver policy on that case?

And instead of this revert, we should instead backport it to the 3.0.4 branch?

Since I don't think that users expect that once a policy was provided, the object could be unsealed with a different PCR state. What do you think?

[martinezjavier]

Another option is to clear the TPMA_OBJECT_USERWITHAUTH from the ctx.in_public.publicArea.objectAttributes in tpm2_create if a policy is provided. That will be a minimal user interface change, while making sure that a user must provide a -A userwithauth if wants to also unseal with a password.

But I wonder if we shouldn't set a TPMA_OBJECT_ADMINWITHPOLICY if a user provides a policy in that case. Opinions?

[diabonas]

@martinezjavier I like your last suggestion, as it is least likely to trip users up: I think the main security/usability problem is not 352908f7470338aba858a9ac20c1c5ec608b661c, but the fact that userwithauth is set by default for keys locked with a policy (so this currently is a problem even on master if no object attributes are explicitly specified for tpm2_create).

Adding to the default attributes instead of replacing them is still a bit inconvenient as there seems to be no way of unsetting an attribute besides using an explicit hexadecimal attribute specification, but with a more sensible default attribute list this should be less of a problem.

[martinezjavier]

@martinezjavier I like your last suggestion, as it is least likely to trip users up: I think the main security/usability problem is not 352908f, but the fact that userwithauth is set by default for keys locked with a policy (so this currently is a problem even on master if no object attributes are explicitly specified for tpm2_create).

Right, I wonder if for master we should just remove any defaults and ask users to explicitly set the attributes...

For master I don't think is a problem, because if you set a policy with -L you will need to replace the default attributes anyways since TPMA_OBJECT_ADMINWITHPOLICY is not set by default.

Which is another bug since by setting a policy I guess that user will expect it to be able to authenticate using the defined PCR policy session.

Adding to the default attributes instead of replacing them is still a bit inconvenient as there seems to be no way of unsetting an attribute besides using an explicit hexadecimal attribute specification, but with a more sensible default attribute list this should be less of a problem.

Yeah, I just wanted to keep the user visible changes to a minimum. I do agree that @williamcroberts change to set instead of append is the correct approach, specially since we don't have a way to clear the bits as you said.

Since you agree with my suggestion, I proposed PR #1126. What I didn't mentioned in my previous comment but realized when writing the patch is that userwithauth should be set if a user provided a password.

[martinezjavier]

@diabonas @havolarkis @williamcroberts @joshuagl fixed some bugs and also added test cases to the tpm2_unseal integration test to catch regressions. Please review!

[martinezjavier]

and also forward ported the commit to master in PR #1127, since it has the same issue. We should also backport to 3.0.X and do a 3.0.5 release with the fix.

[williamcroberts]

Theirs a lot of stuff in this thread, so let me work through it item by item.

adminWithPolicy I don't think we want to set this, at least not at this point in time, as it requires ADMIN role authorization to be done with a policy session versus an HMAC auth value. I think for now, not having this poses no significant threat.

No Default Attributes We want some set of sane defaults, it's quite tricky to specify these. If you ever stepped through the simulator code looking at how all the properties of an object are checked, its a nightmare. Certain attributes need to be there, etc. The new interface in create and createprimary helps to figure a working sane set of attributes, schemes, etc based on what is set. Having the user do it, everytime, can be quite confusing for new users.

Javier's Suggestion

Another option is to clear the TPMA_OBJECT_USERWITHAUTH from the ctx.in_public.publicArea.objectAttributes in tpm2_create if a policy is provided. That will be a minimal user interface change, while making sure that a user must provide a -A userwithauth if wants to also unseal with a password.

This seems OK to me.

Empty Passwords This is something that has always bugged me. Perhaps on tpm2_create, tpm2_createprimary and tpm2_import we require password or we prompt for the password. This way we require user interaction to specify a password; thoughts?

[joshuagl]

I absolutely like the idea of prompting the user not to use empty passwords. Perhaps we can add a flag that indicates "I know this is iffy". If the command is invoked with without a password or that flag then we error out?