TPM Provider does not persist generated keys accross reboot #504
Comments
|
From @paulhowardarm: Assuming that we can fix the TPM provider to persist the public/private key areas instead of the context, I can think of a couple of ways that we could integrate such a change into the service without breaking backwards-compatibility and without requiring a standalone migration script.
Example ideause serde::{Deserialize, Serialize};
use bincode::{Error, ErrorKind};
// Hack these typedefs to avoid actually pulling in any TPM stuff.
type TPMI_DH_CONTEXT = u32;
type TPMI_RH_HIERARCHY = u32;
#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct TpmsContext {
sequence: u64,
saved_handle: TPMI_DH_CONTEXT,
hierarchy: TPMI_RH_HIERARCHY,
context_blob: Vec<u8>,
}
// This is what the TPM provider stores in the KIM today.
#[derive(Debug, Serialize, Deserialize)]
pub struct PasswordContext {
pub context: TpmsContext,
/// This value is confidential and needs to be zeroized by its new owner.
pub auth_value: Vec<u8>,
}
// Dummy struct to represent needing to store the key in a new way.
#[derive(Debug, Serialize, Deserialize)]
pub struct ExtraBits {
private_key_area: Vec<u8>, // These would probably be some structured TPM type rather than
public_key_area: Vec<u8>, // just byte arrays.
}
#[derive(Debug, Serialize, Deserialize)]
// This is what we could store instead. We probably would call it something
// else, but "UpgradedPasswordContext" is good to demonstrate the idea.
pub struct UpgradedPasswordContext {
original: PasswordContext,
extra_bits: ExtraBits,
}
fn main() {
// Fake up a context to serialize
let tpmctx = TpmsContext {
sequence: 42,
saved_handle: 0xDEADBEEF,
hierarchy: 0xBAADF00D,
context_blob: vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10],
};
// Make the original/existing form of context. This is what the TPM provider is serializing
// in the KIM as of today.
let pwdctx = PasswordContext {
context: tpmctx,
auth_value: vec![101, 102, 103, 104, 105],
};
// This represents serializing to disk (in the original PasswordContext form)
let ondisk: Vec<u8> = bincode::serialize(&pwdctx).unwrap();
// Try to read back as UpgradedPasswordContext. This is guaranteed to give us a
// UnexpectedEof error, because UpgradedPasswordContext is a PasswordContext plus
// additional fields.
let upgraded = match bincode::deserialize::<UpgradedPasswordContext>(&ondisk) {
Ok(_) => panic!("Did not expect this to succeed."),
Err(e) => {
match *e {
ErrorKind::Io(io_error) => {
if io_error.kind() == std::io::ErrorKind::UnexpectedEof {
// In this case, we expect the UnexpectedEof!
// So we need to deserialize the original form instead
// This time, just unwrap() because this should be correct.
let oldctx: PasswordContext = bincode::deserialize(&ondisk).unwrap();
// This is where we would do TPM stuff to (hopefully) load the context
// and export the public/private areas. Just emulate the idea here.
let newctx = UpgradedPasswordContext {
original: oldctx,
extra_bits: ExtraBits {
private_key_area: vec![201, 202, 203, 204, 205, 206],
public_key_area: vec![211, 212, 213, 214, 215, 216],
}
};
let newondisk = bincode::serialize(&newctx).unwrap();
newondisk
}
else {
panic!("An IO error occurred other than UnexpectedEof.")
}
},
_ => panic!("Error other than Io::EndOfFile occurred."),
}
},
};
// We should now be able to deserialize the upgraded data
let upgrade_ctx: UpgradedPasswordContext = bincode::deserialize(&upgraded).unwrap();
// Just dump to the console.
println!("The upgraded context is {:#?}", &upgrade_ctx);
} |
|
I like the version 1 you describe and is probably something we should have added from the start. This is especially true for the TPM provider where we don't only serialise an integer but big data structures that can change. This is something that was also suggested by @MattDavis00 in his design. Concretely, we need to serialise the |
Is it known how we can recover these What about symmetric keys? I don't think the TPM provider supports those as yet, but I imagine it probably will at some point. Is that represented by just a |
|
We should probably begin by developing just the code changes needed to serialise the correct structures, without being concerned with upgrade or migration just yet. Possibly we should consider doing this on a separate branch so that stability and compatibility are preserved in the mainline. Once we have those changes committed and evaluated in their branch, we can decide what our approach to compatibility should be, and then implement that strategy in the branch before merging the entire fix to main. |
I am not sure of all possible ways and @ionut-arm , @Superhepper and @wiktor-k might help too but from a loaded context handle and looking at the ESYS spec I see that you could:
We seem to have a similar problem with imported keys (#505). If we wanted to fix that in the same time, we would also need to do the same there.
|
|
To serialise the C structures, |
|
I guess we are limited to the context methods we have implemented so far. But otherwise it could be possible to use |
The private part of the key is trivial to store because it's just a buffer with bytes (and we've already created native types for it). But I don't think we actually need to store the whole |
|
Ok, writing an update note just to keep track of what I've been working on and what issues I've hit. Refactoring the TPM stack to work as I've highlighted above wasn't that difficult and most of the changes fell into the Implementing the migration part (i.e. converting our old key context structures to A few problems persist and are next on my TODO list: some of the newer code bits in the I'll be updating this comment as work progresses. I'll also try to do a detailed description of the bugs I hit, as it is likely that in the future something like that could cause problems again. |
It was observed, first on Slack that keys generated in the TPM provider can not be used after a cold reboot of the system. When trying to use the key generated before (for example export the public part) the following error is seen:
After investigation on the Slack thread linked above, on the TPM Dev chat (logs) and this post, the root cause was found.
Although the Storage Primary Seed stays the same accross TPM Reset (which happens when there is a cold system reboot), the key contexts created from the Storage Primary Key are invalidated.
This is for example written at section 30.4 of the Part 1 (Architecture) of the specs:
Also in section 30.1:
As said in the TPM Dev chat, a better solution to save persistent keys in to store the result of the
tpm2_createoperation which are theTPM2B_PRIVATEandTPM2B_PUBLICstructure. When the key needs to be used again, those structures can be loaded in the TPM with atpm2_load. See here for what we currently do when creating a key.This is a bug as it means that anyone having keys stored in the TPM provider would lose them after a reboot.
This is a stability concern as by changing the way we represent TPM keys in memory we would invalidate old format.
This isssue is made discuss solutions and start a fix!
The text was updated successfully, but these errors were encountered: