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

tpm2: add support for a trusted SRK #26185

Merged
merged 1 commit into from Apr 3, 2023

Conversation

williamcroberts
Copy link
Contributor

@williamcroberts williamcroberts commented Jan 24, 2023

Prevent attackers from spoofing the tpmKey portion of the AuthSession by adding a trusted key to the LUKS header metadata. Also, use a persistent object rather than a transient object.

This provides the following benifits:

  1. No way to MITM the tpmKey portion of the session, see [1] for details.

  2. Strengthens the encrypted sessions, note that the bindKey could be dropped now.

  3. Speed, once it's created we just use it.

This is a "first to set" model, in where the first person to set the key in the LUKS header wins. Thus, setup should be done in a known good state. If an SRK, which is a primary key at a special persistent address, is found, it will use whatever is there. If not, it creates an SRK. The SRK follows the convetions used through the tpm2-software organization code on GitHub [2], however, a split has occured between Windows and Linux with respect to SRK templates. The Linux SRK is generated with the unique field size set to 0, in Windows, it properly sets the size to key size in bytes and the unique data to all 0's of that size. Note the proper templates for SRKs is covered in spec [3]. However, the most important thing, is that both SRKs are passwordless, and thus they should be interchangable. If Windows is the first to make the SRK, systemd will gladly accept it and vice-versa.

  1. Without the bindKey being utilized, an attacker was able to intercept this and fake a key, thus being able to decrypt and encrypt traffic as needed. Introduction of the bindKey strengthened this, but allows for the attacker to brute force AES128CFB using pin guesses. Introduction of the salt increases the difficulty of this attack as well as DA attacks on the TPM objects itself.

  2. https://github.com/tpm2-software

  3. https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-v2.0-Provisioning-Guidance-Published-v1r1.pdf

Signed-off-by: William Roberts william.c.roberts@intel.com

@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Jan 24, 2023
@williamcroberts williamcroberts force-pushed the tpm2-add-srk branch 2 times, most recently from 3ba39d4 to 2bd133f Compare January 24, 2023 21:55
src/shared/tpm2-util.c Fixed Show fixed Hide fixed
@williamcroberts williamcroberts force-pushed the tpm2-add-srk branch 2 times, most recently from d8eab81 to 55d33a2 Compare January 24, 2023 23:34
@williamcroberts
Copy link
Contributor Author

It seems as though UBSAN doesn't like the definition of TPM2_PERSISTENT_FIRST. Which expands to:
((TPM2_HC) (((TPM2_HC) (((TPM2_HT) 0x81) << ((TPM2_HC) 24))) + 0))

@williamcroberts
Copy link
Contributor Author

I wonder if it's because TPM2_HT is a UINT8 which is being leftshifted.... looks like that's it, if we promote the UINT8 to a handle size it corrects this behavior. This is a bug in our code, which we can just work around here.

@keszybz keszybz removed the please-review PR is ready for (re-)review by a maintainer label Jan 25, 2023
src/test/test-tpm2.c Outdated Show resolved Hide resolved
src/shared/tpm2-util.c Outdated Show resolved Hide resolved
@poettering
Copy link
Member

/cc @ddstreet

src/test/test-tpm2.c Outdated Show resolved Hide resolved
src/cryptenroll/cryptenroll-tpm2.c Outdated Show resolved Hide resolved
src/shared/creds-util.c Outdated Show resolved Hide resolved
src/shared/tpm2-util.c Outdated Show resolved Hide resolved
src/shared/tpm2-util.c Outdated Show resolved Hide resolved
src/shared/tpm2-util.c Outdated Show resolved Hide resolved
src/shared/tpm2-util.c Outdated Show resolved Hide resolved
src/shared/tpm2-util.c Outdated Show resolved Hide resolved
src/shared/tpm2-util.c Outdated Show resolved Hide resolved
@poettering
Copy link
Member

hmm, so what's the argument for persisting the primary key as SRK? that's merely about speed? or anything else?

@poettering
Copy link
Member

so this only pins the srk in the json luks2 header. but we could also pin the transient primary key if we have it, couldn't we? it's also derived from the seed key, hence should remain fixed for the same tpm2 chip, no?

@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks cryptsetup tpm2 labels Jan 25, 2023
@poettering
Copy link
Member

i wonder if we should also do the SRK initialization independently of cryptsetup in some new tiny binary systemd-provision-tpm2 or so. For two reasons: this is slow, right? so we could do it super early, long before the disks have popped up. And thus when they do pop up we don't have to wait for that step anymore.

But more interestingly: it would be independent from disk encryption, i.e. would be usable for the creds stuff or any other tpm2 user the same way.

such a mini service would just trivially call the code you just added.

does that make sense? (just asking generally, not suggesting you add this to this PR)

@poettering
Copy link
Member

(somewhat related #20668)

@williamcroberts
Copy link
Contributor Author

hmm, so what's the argument for persisting the primary key as SRK? that's merely about speed? or anything else?

  1. it follows the spec https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-v2.0-Provisioning-Guidance-Published-v1r1.pdf
  • This is how it should be done. This will allow all the projects to interoperate.
  1. We get a constant known trusted key in the TPM
    • Yes make primary returns a consistent object, but why do it every call.
  2. speed

@williamcroberts
Copy link
Contributor Author

williamcroberts commented Jan 25, 2023

i wonder if we should also do the SRK initialization independently of cryptsetup in some new tiny binary systemd-provision-tpm2 or so. For two reasons: this is slow, right?

Creating keys is slow, (ecc is faster) but the evictcontrol is usually fast.

so we could do it super early, long before the disks have popped up. And thus when they do pop up we don't have to wait for that step anymore.

Yes you could.

But more interestingly: it would be independent from disk encryption, i.e. would be usable for the creds stuff or any other tpm2 user the same way.

such a mini service would just trivially call the code you just added.

does that make sense?

Yes, and in theory that's possible now. I could create the SRK with tpm2-tools or Windows, etc, upgrade my systemd, cryptenroll a new device and have it pick up that SRK.

(just asking generally, not suggesting you add this to this PR)

Thank goodness, I don't know if I have the systemd chops to turn that around in a reasonable timeframe.

@williamcroberts
Copy link
Contributor Author

so this only pins the srk in the json luks2 header. but we could also pin the transient primary key if we have it, couldn't we? it's also derived from the seed key, hence should remain fixed for the same tpm2 chip, no?

Yes you could pin it as well.

@williamcroberts
Copy link
Contributor Author

so this only pins the srk in the json luks2 header. but we could also pin the transient primary key if we have it, couldn't we? it's also derived from the seed key, hence should remain fixed for the same tpm2 chip, no?

Yes you could pin it as well.

The code is not as simple, because an ESYS_TR is combination of a name + handle, so you'd have to just store the name and manually do a name check where it matters since it's not a stable handle. Right now, but just serializing to the SRK and deserialzing the ESYS_TR, ESAPI automatically can perform name checking without client code being responsible for it.

@williamcroberts
Copy link
Contributor Author

williamcroberts commented Jan 25, 2023

hmm, so what's the argument for persisting the primary key as SRK? that's merely about speed? or anything else?

  1. it follows the spec https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-v2.0-Provisioning-Guidance-Published-v1r1.pdf
  • This is how it should be done. This will allow all the projects to interoperate.
  1. We get a constant known trusted key in the TPM

    • Yes make primary returns a consistent object, but why do it every call.
  2. speed

@poettering I forgot the most important reason, only someone with owner authorization can create a key in the owner hierarchy, all this code is currently broken, we need to prompt for owner password if it's set (getcap call can get this information). So any current OWNER password set TPMs, this all breaks IIUC.

With this patch, their's at least a workaround, create the SRK first with tpm2-tools and then perform cryptenroll.

@williamcroberts
Copy link
Contributor Author

I've done a bad job explaining this all in one cohesive thought. The idea is that the TPM Owner might not be the computer owner/admin. I.E. the person performing cryptenroll might not have owner auth. In the current model, the owner auth has to be empty, which means that if I as a regular user want to do something, I would need to clear owner auth and re-establish it after the task is done. This is the WHY the SRK exists. The SRK exists to be a scratch space for regular users, which is why the password is emptyauth. This patch at least gets it to a point where the TPM Owner can create the SRK and walk away and users can cryptenroll to their hearts content.

src/shared/tpm2-util.h Outdated Show resolved Hide resolved
@poettering
Copy link
Member

PR looks good codewise to me. if this makes sense to @ddstreet too, I am all onboard.

if installation of an srk should be optional we could certainly add a setting for it, that you can specify when enrolling. but generally i'd rather have less settings and more "good defaults" that just work for everyone.

@poettering
Copy link
Member

does this also close #22129?

@williamcroberts
Copy link
Contributor Author

does this also close #22129?

Maybe. Yes if you're OK saying for password protected owner hierarchies you must create the SRK externally if it doesn't exist, ie via tpm2-tools , a one shot service, etc. If not, then we need to plumb owner auth prompt if required. I'd like to avoid that because it could be a policy protecting the hierarchy and not a password.

@ddstreet
Copy link
Contributor

PR looks good codewise to me. if this makes sense to @ddstreet too, I am all onboard.

if installation of an srk should be optional we could certainly add a setting for it, that you can specify when enrolling. but generally i'd rather have less settings and more "good defaults" that just work for everyone.

Yep we had a chat, I'm all good with this PR; for my PRs I'll work with him to get them working with serialized handles. Thanks!

@williamcroberts
Copy link
Contributor Author

@poettering ping?

Prevent attackers from spoofing the tpmKey portion of the AuthSession by
adding a trusted key to the LUKS header metadata. Also, use a persistent
object rather than a transient object.

This provides the following benifits:
1. No way to MITM the tpmKey portion of the session, see [1] for
details.

2. Strengthens the encrypted sessions, note that the bindKey could be
   dropped now.

3. Speed, once it's created we just use it.

4. Owner Auth is needed to call create primary, so using the SRK
   creates a scratch space for normal users.

This is a "first to set" model, in where the first person to set the key
in the LUKS header wins. Thus, setup should be done in a known good
state. If an SRK, which is a primary key at a special persistent
address, is found, it will use whatever is there. If not, it creates an
SRK. The SRK follows the convetions used through the tpm2-software
organization code on GitHub [2], however, a split has occured between
Windows and Linux with respect to SRK templates. The Linux SRK is
generated with the unique field size set to 0, in Windows, it properly
sets the size to key size in bytes and the unique data to all 0's of that
size. Note the proper templates for SRKs is covered in spec [3].
However, the most important thing, is that both SRKs are passwordless,
and thus they should be interchangable. If Windows is the first to make
the SRK, systemd will gladly accept it and vice-versa.

1. Without the bindKey being utilized, an attacker was able to intercept
this and fake a key, thus being able to decrypt and encrypt traffic as
needed. Introduction of the bindKey strengthened this, but allows for
the attacker to brute force AES128CFB using pin guesses. Introduction of
the salt increases the difficulty of this attack as well as DA attacks
on the TPM objects itself.

2. https://github.com/tpm2-software

3. https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-v2.0-Provisioning-Guidance-Published-v1r1.pdf

Fixes: systemd#20668
Fixes: systemd#22637

Signed-off-by: William Roberts <william.c.roberts@intel.com>
@williamcroberts
Copy link
Contributor Author

Rebased on current main and fixed conflicts, however I was unable to test as mkosi is failing again to build the test image, sent an email to the list.

@poettering any love on this PR yet :-p

@williamcroberts
Copy link
Contributor Author

@ddstreet any way you can ping @poettering , I've been trying for weeks with no luck.

@williamcroberts
Copy link
Contributor Author

@ddstreet any way you can ping @poettering , I've been trying for weeks with no luck.

Nevermind, @poettering just emailed me. Thanks again folks.

@poettering
Copy link
Member

lgtm, my nits don#t really matter. let's merge. they cna be fixed later, or not, doesn#t matter

@poettering poettering merged commit acbb504 into systemd:main Apr 3, 2023
39 checks passed
@github-actions github-actions bot removed the please-review PR is ready for (re-)review by a maintainer label Apr 3, 2023
@williamcroberts
Copy link
Contributor Author

lgtm, my nits don#t really matter. let's merge. they cna be fixed later, or not, doesn#t matter

I can update these nits next week, more travel, then back. Thanks @poettering and @ddstreet

williamcroberts pushed a commit to williamcroberts/systemd that referenced this pull request Apr 4, 2023
Fixes:
  - Comment style
  - Alignment style
  - cleanup macro usage
  - incorrect error message[1]

1. Thanks to tempusfugit991@gmail.com for pointing out the error
message mistake.

Signed-off-by: William Roberts <william.c.roberts@intel.com>
@williamcroberts
Copy link
Contributor Author

@poettering nits updated on #27138

bluca pushed a commit that referenced this pull request Apr 4, 2023
Fixes:
  - Comment style
  - Alignment style
  - cleanup macro usage
  - incorrect error message[1]

1. Thanks to tempusfugit991@gmail.com for pointing out the error
message mistake.

Signed-off-by: William Roberts <william.c.roberts@intel.com>
@@ -703,7 +703,9 @@ int encrypt_credential_and_warn(
&tpm2_blob, &tpm2_blob_size,
&tpm2_policy_hash, &tpm2_policy_hash_size,
&tpm2_pcr_bank,
&tpm2_primary_alg);
&tpm2_primary_alg,
/* ret_srk_buf= */ NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reason why for systemd-creds we're not storing the sealing key under the SRK? It would benefit from the same benefits AFAIK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't recall any specific reason, but I didn't fully study those code paths and just kept them at their current state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

TPM2 primary key should not have dictionary attack protection enabled
9 participants