On Mon, Apr 6, 2015 at 6:49 PM, Daniel De Graaf <dgde...@tycho.nsa.gov>
wrote:
> On 04/05/2015 07:09 AM, Emil Condrea wrote:
>
>> Enables deep quote execution for vtpmmgr which can not be started
>> using locality 2. The VTPM_ORD_GET_QUOTE command is backwards
>> compatible. When receives flags=0 from vTPM it extends additional
>> information into the PCRs as it did before.
>>
>
> Even without extending values into the resettable PCRs, the PCR
> selection for the quote should not be ignored: otherwise, the
> measurement of the hypervisor (stored in either PCR 4-5 or PCR 18-19)
> cannot be included in a deep quote. Allowing a guest to attest to the
> measurement of its hypervisor is one of the major reasons for allowing a
> deep quote and is likely to be mandatory in a vTPM provisioning quote.
>
very good point, I missed that someone would include additional PCRs in the
request.
>
> The backwards compatibility added here has some caveats that need to be
> treated carefully. In the current version, it is not possible to
> distinguish between a request that has the new externData structure and
> a request that uses flags=0 where the vTPM itself has constructed a
> requestData value that looks like a valid externData.
>
> The simplest solution is to always use the structure prepared by the
> vTPM manager for externData and remove backwards compatibility from the
> series. Since the new quote format would be introduced with a new
> version of the vTPM Manager and (likely) a new hypervisor, any support
> software that makes use of the quotes can be updated at the same time.
>
> If backwards compatibility is needed, then there must be a way to
> distinguish a new-format quote from an old-format quote. If the reset
> of the modified PCRs is moved to the end of the quote operation (instead
> of the beginning, as is currently done), the value of PCR 20 will be
> at its reset value (either 00..00 or ff..ff) when the new-format quote
> is requested. Including the value of PCR 20 in all new-format quotes
> would then be sufficient evidence that the externData value in the quote
> was generated by the vTPM Manager.
>
> In addition, it would be useful to add a command-line switch to the vTPM
> manager that disables this backwards compatibility mode if a given
> system will not be using it: this would avoid issues if the users of
> that system have not read this discussion and decide to use quotes that
> do not include PCR 20-23 (which is normally a sensible thing to do).
>
> I think it is reasonable to specify in the documentation the changes and
include only the new version.
> Flags are interpreted as a bitmask of:
>> * VTPM_QUOTE_FLAGS_HASH_UUID
>> * VTPM_QUOTE_FLAGS_VTPM_MEASUREMENTS
>> * VTPM_QUOTE_FLAGS_GROUP_INFO
>> When using flags!=0, vtpmmgr ignores quoteSelect and TPM_Quote is
>> executed with:
>> externData = SHA1(
>> flags,
>> requestData,
>> [UUIDs if requested],
>> [vTPM measurements if requested],
>> [vTPM group update policy if requested]
>> )
>>
>
> This specification does not match what you actually do below. There are
> some advantages to doing it the way you have coded, but the method for
> calculating the hash needs to be fully documented.
>
> I will update the documentation: sha1(UUIDs) instead of UUIDs and so on.
> Signed-off-by: Emil Condrea <emilcond...@gmail.com>
>> ---
>>
> [...]
>
> diff --git a/stubdom/vtpmmgr/mgmt_authority.c
>> b/stubdom/vtpmmgr/mgmt_authority.c
>> index 0526a12..0e4aac7 100644
>> --- a/stubdom/vtpmmgr/mgmt_authority.c
>> +++ b/stubdom/vtpmmgr/mgmt_authority.c
>> @@ -128,6 +128,49 @@ static int do_load_aik(struct mem_group *group,
>> TPM_HANDLE *handle)
>> return TPM_LoadKey(TPM_SRK_KEYHANDLE, &key, handle,
>> (void*)&vtpm_globals.srk_auth, &vtpm_globals.oiap);
>> }
>>
>> +static void do_vtpminfo_hash(uint32_t extra_info_flags,struct mem_group
>> *group,
>> + const void* uuid, const uint8_t* kern_hash,unsigned char**
>> calc_hashes)
>> +{
>> + int i;
>> + sha1_context ctx;
>> + if(extra_info_flags & VTPM_QUOTE_FLAGS_HASH_UUID){
>> + printk("hashing for FLAGS_HASH_UUID: ");
>> + if(uuid){
>> + printk("true");
>> + sha1_starts(&ctx);
>> + sha1_update(&ctx, (void*)uuid, 16);
>> + sha1_finish(&ctx, *calc_hashes);
>> + *calc_hashes = *calc_hashes + 20;
>> + }
>> + printk("\n");
>> + }
>> + if(extra_info_flags & VTPM_QUOTE_FLAGS_VTPM_MEASUREMENTS){
>> + printk("hashing for VTPM_QUOTE_FLAGS_VTPM_MEASUREMENTS:
>> ");
>> + if(kern_hash){
>> + printk("true");
>> + sha1_starts(&ctx);
>> + sha1_update(&ctx, (void*)kern_hash, 20);
>> + sha1_finish(&ctx, *calc_hashes);
>> + *calc_hashes = *calc_hashes + 20;
>> + }
>> + printk("\n");
>> + }
>>
>
> If exactly one of (uuid) and (kern_hash) is NULL and both were requested
> in extra_info_flags, it is not easy to determine which one was used. A
> better method would be to only place the sha1_update calls inside the
> NULL check and always calculating the SHA1. This makes interpreting the
> result easier: for a given value of extra_info_flags, the size of the
> externData structure is of constant size. The presence of the SHA1 of
> the empty string is a sentinel value indicating that the information is
> not available.
>
Indeed. Constant size for externData is a must.
>
> + if(extra_info_flags & VTPM_QUOTE_FLAGS_GROUP_INFO){
>> + printk("hashing for VTPM_QUOTE_FLAGS_GROUP_INFO: true");
>> + sha1_starts(&ctx);
>> + sha1_update(&ctx, (void*)&group->id_data.saa_pubkey,
>> sizeof(group->id_data.saa_pubkey));
>> + sha1_update(&ctx, (void*)&group->details.cfg_seq, 8);
>> + sha1_update(&ctx, (void*)&group->seal_bits.nr_cfgs, 4);
>> + for(i=0; i < group->nr_seals; i++)
>> + sha1_update(&ctx,
>> (void*)&group->seals[i].digest_release, 20);
>> + sha1_update(&ctx, (void*)&group->seal_bits.nr_kerns, 4);
>> + sha1_update(&ctx, (void*)&group->seal_bits.kernels, 20 *
>> be32_native(group->seal_bits.nr_kerns));
>> + sha1_finish(&ctx, *calc_hashes);
>> + *calc_hashes = *calc_hashes + 20;
>> + printk("\n");
>> + }
>> +}
>>
>
> You should return an error if an unknown bit in extra_info_flags is set;
> this will make it easier to extend the flags later.
>
will do.
>
> One additional flag that would be useful (I can add it later if you don't
> want to add it in the first version):
>
> VTPM_QUOTE_FLAGS_GROUP_PUBKEY - value is SHA1(group->id_data.saa_pubkey).
> This value does not change when the configuration of a group is
> updated, making it a less informative but more easily verified version of
> VTPM_QUOTE_FLAGS_GROUP_INFO.
I will include it in the next patch series.
>
>
> /*
>> * Sets up resettable PCRs for a vTPM deep quote request
>> */
>> @@ -273,18 +316,44 @@ int group_do_activate(struct mem_group *group,
>> void* blob, int blobSize,
>>
>> int vtpm_do_quote(struct mem_group *group, const uuid_t uuid,
>> const uint8_t* kern_hash, const struct tpm_authdata *data,
>> TPM_PCR_SELECTION *sel,
>> - void* pcr_out, uint32_t *pcr_size, void* sig_out)
>> + uint32_t extra_info_flags, void* pcr_out, uint32_t *pcr_size,
>> void* sig_out)
>> {
>> TPM_HANDLE handle;
>> TPM_AUTH_SESSION oiap = TPM_AUTH_SESSION_INIT;
>> TPM_PCR_COMPOSITE pcrs;
>> BYTE* sig;
>> UINT32 size;
>> - int rc;
>> + sha1_context ctx;
>> + TPM_DIGEST externData;
>> + const void* data_to_quote = data;
>> + UINT32 pcrSelSize = sel->sizeOfSelect;
>> + unsigned char* ppcr_out = (unsigned char*)pcr_out;
>> + unsigned char** pcr_outv = (unsigned char**)&ppcr_out;
>>
>> - rc = do_pcr_setup(group, uuid, kern_hash);
>> - if (rc)
>> - return rc;
>> + int rc;
>> + if(extra_info_flags == 0){
>> + printk("Extra Info Flags == 0, hope you are on locality
>> 2\n");
>> + rc = do_pcr_setup(group, uuid, kern_hash);
>> + if (rc)
>> + return rc;
>> + }
>> + else{
>> + printk("Extra Info Flags =0x%x\n",extra_info_flags);
>> + //resetting pcr selection
>> + if(sel->sizeOfSelect > sizeof(sel->pcrSelect))
>> + pcrSelSize = sizeof(sel->pcrSelect);
>> + memset(sel->pcrSelect,0,pcrSelSize);
>> +
>> + sha1_starts(&ctx);
>> + sha1_update(&ctx, (void*)&extra_info_flags, 4);
>> + sha1_update(&ctx, (void*)data, 20);
>> + do_vtpminfo_hash(extra_info_flags,group, uuid, kern_hash,
>> pcr_outv);
>> + *pcr_size = *pcr_outv - (unsigned char*)pcr_out;
>>
>
> The values of pcr_out and pcr_size will be NULL when called from
> GroupRegister.
>
I will use an additional pointer if pcr_out is NULL in order to include in
externData
the hash for VTPM_QUOTE_FLAGS_GROUP_INFO if requested.
>
> + if(*pcr_size > 0)
>> + sha1_update(&ctx, pcr_out, *pcr_size);
>> + sha1_finish(&ctx, externData.digest);
>> + data_to_quote = (void*)externData.digest;
>> + }
>>
>> rc = do_load_aik(group, &handle);
>> if (rc)
>> @@ -296,8 +365,7 @@ int vtpm_do_quote(struct mem_group *group, const
>> uuid_t uuid,
>> return rc;
>> }
>>
>> - rc = TPM_Quote(handle, (void*)data, sel,
>> (void*)&group->aik_authdata, &oiap, &pcrs, &sig, &size);
>> - printk("TPM_Quote: %d\n", rc);
>> + rc = TPM_Quote(handle, data_to_quote, sel,
>> (void*)&group->aik_authdata, &oiap, &pcrs, &sig, &size);
>>
>> TPM_TerminateHandle(oiap.AuthHandle);
>> TPM_FlushSpecific(handle, TPM_RT_KEY);
>> @@ -310,8 +378,11 @@ int vtpm_do_quote(struct mem_group *group, const
>> uuid_t uuid,
>> }
>>
>> if (pcr_out) {
>> - *pcr_size = pcrs.valueSize;
>> - memcpy(pcr_out, pcrs.pcrValue, *pcr_size);
>> + /*hashes already copied when flags!=0 by
>> do_vtpminfo_hash*/
>> + if(extra_info_flags == 0){
>> + *pcr_size = pcrs.valueSize;
>> + memcpy(pcr_out, pcrs.pcrValue, *pcr_size);
>> + }
>> }
>>
>
> I think it would be useful to append the PCR values to the externData
> values,
> as long as the entire set of hashes doesn't risk becoming too long.
>
Right now the hashes used for externData are written in pcr_out. Should we
limit
the pcr_out size to a certain value?
If there will be a limit for pcr_out, the domU executing the quote will be
able to
read the PCRs for physical TPM using tag TPM_TAG_RQU_COMMAND and
ord TPM_ORD_PcrRead using passthrough, right?
If domU executing the deep quote requests 10 PCRs to be included in the
quote
it will receive in pcr_out just the hashes used for externalData and the
PCR values
should be obtained later.
Now, the maximum hashes number for calculating the externData is 3
( 4 after including VTPM_QUOTE_FLAGS_GROUP_PUBKEY). In the future if
there will be much more hashes we can implement something like
vtpmmgr_GetBootHash for domU clients, so the validation data can be
obtained
with additional requests to vtpmmgr. Is it a good idea?
Also, I have an implementation for requesting a deep quote for trousers and
tpm tools. Do you think it is worth to send patches to those repositories
also, since
TPM_ORD_DeepQuote is not included in the TPM 1.2 specification ?
Thanks for the review!
Emil Condrea
>
> --
> Daniel De Graaf
> National Security Agency
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
