Closed martinezjavier closed 6 years ago
I've looked at this a little close, I've done the same on both versions and have the following files for each:
load.context obj.name obj.priv obj.pub pcr.bin pcr.policy primary.context
From those pcr.bin
and pcr.policy
are exactly the same and load.context
, primary.context
, obj.name
and ob.pub
are different of course but have the same size.
The only file that has a different size is obj.priv
:
$ ls -lh obj-*
-rw-rw-r--. 1 javier javier 103 Apr 10 13:38 obj-master.priv
-rw-rw-r--. 1 javier javier 101 Apr 10 13:38 obj-stable.priv
I don't see how a change in the version should matter, since these should just be a on-disk representation of a data structure returned by the TPM.
@williamcroberts @flihp thoughts?
Hmm did something change in the TSS code? Perhaps when they updated to use libmu they fixed an issue.
Between 1.x and the current master branch? Lots. If memory serves there were a number of places in the tools code that were dumping structures to disk using sizeof (type)
which I think caused a lot of 0 padding since the type itself had a size field and a buffer that was only partially filled. Using libtss2-mu
now means that the 0
padding will be gone which reduces the size of the structures. This doesn't explain what @martinezjavier is seeing though since the size of the "stable" object is smaller than the "master".
Something else that's bothering me: Depending on how the 3.x tools are saving structures to disk we may end up with a byte order issue. The libtss2-mu will transform the TPM2 types into network byte order and so I would expect that none of the files saved by the 3.x tools would be usable with the current master. In the experiment above it looks like some of them are being loaded and used without a problem so maybe I'm just inventing things to worry about?
I guess the best thing to do in this case would be to dump them out with a hex editor and see what they look like byte for byte.
@flihp @williamcroberts did you have time to look at this issue? I think it's pretty critical.
I wouldn't worry about the endianess of the data since everything that's serialized is data that came from the TPM anyways, we shouldn't serialize anything that's used by the tools or libraries (which may have different endianess depending on the architecture).
About dumping the structures instead of the buffers, I believe those issues that you mention were fixed before releasing the 3.X tools, at least I remember @williamcroberts and I fixing in a lot of places. For example in the tpm2_create tool it was fixed by commit 116763b85031 ("tpm2_create: fix serialization of structures to disk").
Actually, it's very suspicious that there's a 2 byte difference between the two versions and the TPM2B_PRIVATE .size field is UINT16. So yeah, I guess I'll have to look with a hex editor to see where is the difference...
Ok, so it's indeed a problem with libtss2-mu
serializing the whole TPM2B_PRIVATE and not just the TPM2B_PRIVATE .buffer field (which I believe is the correct thing to do). So when migrating to libtss2-mu
, the issue fixed by commit 116763b was re-introduced.
$ $ tpm2_load -c primary.context -u obj.pub -r obj.priv -n obj.name -C load.context
ERROR: Tss2_Sys_Load(0x1D5) - tpm:parameter(1):structure is the wrong size
ERROR: Unable to run tpm2_load
$ echo -n -e \\x00\\$(printf 'x%x' $(ls -lh obj.priv | cut -d ' ' -f5)) > TPM2B_PRIVATE.size
$ cat TPM2B_PRIVATE.size obj.priv > obj-with-size.priv
$ tpm2_load -c primary.context -u obj.pub -r obj-with-size.priv -n obj.name -C load.context
handle: 0x80fffffe
$ tpm2_unseal -c load.context -L sha1:0,1,2,3,4 -F pcr.bin
secret
@flihp @williamcroberts sigh, and the public portion is the same just because there's still a bug in the 3.X version and the TPM2B_PUBLIC size field is still serialized...
$ dd if=obj.pub of=TPM2B_PUBLIC.size bs=1 count=2
$ hexdump -v -C TPM2B_PUBLIC.size
00000000 00 4e |.N|
00000002
$ ls -lh obj.pub
-rw-rw-r--. 1 javier javier 80 Apr 26 14:41 obj.pub
Notice how the size is 0x4e (78 decimal) so the total size is 78 bytes of TPMT_PUBLIC
+ the 2 bytes of UINT16 size
.
So this has to be fixed in 3.X and make it consistent with what we have in tpm2-tss 2.0 / tpm2-tools 4.0.
It looks like you meant to say the size is 0x4e (from the hexdump output). So the total size is 80 bytes = 78 + 2 (sizeof UINT16).
@danintel right, that's what I tried to say. I'll edit the comment to fix the s/43/4e typo.
We need to worry about endian-ness, unless the serialization/marshaling never leaves the host.
The TPM itself is always big endian, Intel is little endian. I don't know how this works at all. But I guess it does work sometime. What if we want to migrate data to another system that's big endian? I know big endian is becoming more uncommon--maybe we just don't worry about it.
@danintel well, I thought that we were already serializing the bufffers that came from the TPM in the endianess used by the TPM (BE). If that's not the case and we are serializing using the host endianess, then yes it could be an issue.
Although I would worry less about that since the idea of the TPM is to bind created objects to a particular machine, so I don't think these created objets (i.e: keys) leaving the machine.
But I do worry about someone creating keys with the current tpm2-software version and using that to encrypt some data, just to find later than on an update the keys are no longer usable with the same machine and TPM.
Once this is fixed, I think a simple unserialize of some fixed test data should be added. And vice-versa with serialize.
@danintel agreed. There are some test cases in tpm2-tss already, the problem is that they only teste serializing/deserializing against the same tpm2-tss version. So as you said, having some fixed test data would be great to make sure that a new version remains compatible with the older ones.
I'm trying to figure out the various things in this thread.
So between 3.X and master, a 2 byte delta between TPM2B_PRIVATE and TPM2B_PUBLIC data types exist as saved to disk. This is because in 3.X were not serializing the length field?
I don't see where TPM2B_PUBLIC is messed up, but you mention it above. Is the issue only with TPM2B_PRIVATE?
Can we modify the load routines in 3.X and master so that if libmu fails, check to see if the size of the file is the sizeof the buffer within that type issue a warning, and convert the file in place?
Can we update the save routines in 3.X to not do this anymore.
I am not worried about the endianess of the files. If you need to share them to a BE machine, you can just convert the file formats.
SO lets try and find all the spots this is broken:
Tool | Data Type | Link |
---|---|---|
tpm2_create | TPM2B_PRIVATE | https://github.com/tpm2-software/tpm2-tools/blob/3.X/tools/tpm2_create.c#L223 |
@williamcroberts the endianess is a red herring, as mentioned in a previous comment the objects are tied to a TPM anyways, so it's unlikely that will be moved to another machine (not sure in the attestation case though).
The real problem for me is the mismatch between different version with regard to objects serialization/deserialization. I don't think we should try to workaround this but instead fix it properly.
So the first question is whether the size field for objects should be stored or not. If it shouldn't, then we should audit all the places were objects are stored and make sure that only the buffer specified by size is stored and fix libtss2-mu
to do the same.
If libtss2-mu
is doing the right thing, then we should fix all the places in the tools where only the buffer is serialized without its size.
The real problem for me is the mismatch between different version with regard to objects serialization/deserialization. I don't think we should try to workaround this but instead fix it properly.
Well the fix will include a work-around to deal with different file representations on disk depending On how we deal with the second half of the issue.
So the first question is whether the size field for objects should be stored or not. If it shouldn't, then we should audit all the places were objects are stored and make sure that only the buffer specified by size is stored and fix libtss2-mu to do the same.
This is the crux of the issue, do we mix and match are go full libmu where-ever we save data.
I'm thinking in cases where the data is just used internally to the tools, like public/private, we just use libmu to make a blob and save it.
In some cases, we don't care, like if it's a digest that folks use elsewhere, just save the digest buffer.
However, if we avoid the size, endianess issues go away. But we would have to deal with nested data types as well.
I know that we are breaking backward compatibility in the tools names, options, etc but I think that at the very least we should maintain backward compatibility for the created objects.
That is, keys created with a version of the tools should keep working with newer versions. If there's a bug on how the data is serialized in tpm2-tools 3.X, then I think we should fix that instead so is compatible with the upcoming 4.X release.
So I did this using the tpm2-tools 3.0.3 and tpm2-tss 1.4.0:
And then attempted to use the created objects with current tpm2-tools and tpm2-tss master branches:
Unsealing a loaded object works but loading didn't:
I tried to create a new primary context and derivate the PK from the seed, since the we only should care about the created public and private portions of the key:
But it didn't work either.