Questions policy JSON encoding and callbacks with tss2-policy

[whooo]

Hi,

After working on the python bindings for tss2-policy I some questions, TPMT_PUBLIC, TPM2B_PUBLIC and TPM2B_NV_PUBLIC is encoded with the size and nvPublic/publicArea fields, but my understanding of the specification is it should be encoded as TPMT_PUBLIC and TPMS_NV_PUBLIC.
Would it be possible to change the encoding, or would it break backward compatibility for FAPI?

Also, the callback TSS2_POLICY_CB_NVPUBLIC doesn't work with TPMS_POLICYNV if nvIndex is used instead of nvPath

[williamcroberts]

AFAICT Fapi Callbacks don't mess w

Hi,

After working on the python bindings for tss2-policy I some questions, TPMT_PUBLIC, TPM2B_PUBLIC and TPM2B_NV_PUBLIC is encoded with the size and nvPublic/publicArea fields, but my understanding of the specification is it should be encoded as TPMT_PUBLIC and TPMS_NV_PUBLIC. Would it be possible to change the encoding, or would it break backward compatibility for FAPI?

AFAICT that is not exported publicly through FAPI Callbacks, so we can change that but we have to do it now.

Also, the callback TSS2_POLICY_CB_NVPUBLIC doesn't work with TPMS_POLICYNV if nvIndex is used instead of nvPath

Can you elaborate, what happens?

[williamcroberts]

Oh and TPMT_PUBLIC doesn't have a size, that's the data member in the TPM2B_PUBLIC. But I still get your point.

[whooo]

AFAICT that is not exported publicly through FAPI Callbacks, so we can change that but we have to do it now.

I was thinking of the on disk storage for FAPI and Fapi_ExportPolicy

Also, the callback TSS2_POLICY_CB_NVPUBLIC doesn't work with TPMS_POLICYNV if nvIndex is used instead of nvPath

Can you elaborate, what happens?
When executing a policy looking something like:

{
  "description": "my policy",
  "policy": {
    "type": "nv",
    "nvIndex": "0x1000000",
    "operandB": "00"
  }       
} 

It fails with ERROR:fapi:src/tss2-fapi/ifapi_policy_instantiate.c:314:ifapi_policyeval_instantiate_finish() ErrorCode (0x00060028) No path for policy PolicyNv.
I guess part of the problem is that there is no way for tss2-policy to get the matching TPMS_NV_PUBLIC with nvIndex as it can't be passed via TSS2_POLICY_CB_NVPUBLIC
I can always create a PR which replaces nv_public with a struct containing a selector and an union with TPM2B_NV_PUBLIC and a TPMI_RH_NV_INDEX

[JuergenReppSIT]

The current implementation did assume that the NV public info could be set in the json policy:
if (pol_element->element.PolicyNV.nvPublic.nvPublic.nvIndex) {
/* nvIndex is already set in policy. Path will not be needed */
But according to the Spec "JSON policy data types" only the NV index can be set.
The callback cbnvpublic should be extended with a parameter nv_index and call asynchronous Esys_NV_ReadPublic if the index is set and otherwise read the public info from the keystore.
@whooo, @williamcroberts Would it be ok for you to use:

typedef TSS2_RC (*TSS2_POLICY_CB_NVPUBLIC) (
    const char *path,
    nv_index TPMI_RH_NV_INDEX,
    TPM2B_NV_PUBLIC *nv_public,
    void *userdata);   /* e.g. for ESAPI_CONTEXT */

for the callback?

[whooo]

@JuergenReppSIT that would work for me

[williamcroberts]

@JuergenReppSIT Is it a if path is NULL then use index logic?

[williamcroberts]

AFAICT that is not exported publicly through FAPI Callbacks, so we can change that but we have to do it now.

I was thinking of the on disk storage for FAPI and Fapi_ExportPolicy

You shouldn't need to change that, just the value through the callback API.

[JuergenReppSIT]

@JuergenReppSIT Is it a if path is NULL then use index logic?

@williamcroberts Yes it's ensured by ifapi_json_TPMS_POLICYNV_deserialize that either the path or the nv index is set.

[whooo]

I was thinking of the on disk storage for FAPI and Fapi_ExportPolicy

You shouldn't need to change that, just the value through the callback API.

The JSON encoding/decoding doesn't seem to be affected by the callbacks, for example look at test_calc_public_callback( in https://github.com/tpm2-software/tpm2-pytss/pull/366/files#diff-dcf814a26ae9b973685a1574fa18af4dc8ad00fffc6c047d4556ba779653c23e
Fapi_ExportPolicy for example calls the serialization functions directly so I think it would break backwards compatibility changing how it's encoded/decoded

[williamcroberts]

TPM2B_NV_PUBLIC

@whooo you don't modify the serialization routines, you just use a temporary TPM2B_NV_PUBLIC populated there with the contents of TPMS_NV_PUBLIC like in the patch below. This way none of the on-disk format changes just the in-memory format through the callbacks.

diff --git a/include/tss2/tss2_policy.h b/include/tss2/tss2_policy.h
index a9e5e92d6bf2..e9cc7c071076 100644
--- a/include/tss2/tss2_policy.h
+++ b/include/tss2/tss2_policy.h
@@ -89,7 +89,7 @@ typedef TSS2_RC (*TSS2_POLICY_CB_PCR) (
 typedef TSS2_RC (*TSS2_POLICY_CB_NVPUBLIC) (
     const char *path,
     TPMI_RH_NV_INDEX nv_index,
-    TPM2B_NV_PUBLIC *nv_public,
+    TPMS_NV_PUBLIC *nv_public,
     void *userdata);   /* e.g. for ESAPI_CONTEXT */
 
 typedef struct TSS2_POLICY_CALC_CALLBACKS TSS2_POLICY_CALC_CALLBACKS;
@@ -137,7 +137,7 @@ typedef TSS2_RC (*TSS2_POLICY_CB_EXEC_POLAUTH) (
     void *userdata);
 
 typedef TSS2_RC (*TSS2_POLICY_CB_EXEC_POLAUTHNV) (
-    TPM2B_NV_PUBLIC *nv_public,
+    TPMS_NV_PUBLIC *nv_public,
     TPMI_ALG_HASH hash_alg,
     void *userdata);
 
diff --git a/src/tss2-fapi/ifapi_helpers.c b/src/tss2-fapi/ifapi_helpers.c
index 12dca7e290b6..f3e07e9fe773 100644
--- a/src/tss2-fapi/ifapi_helpers.c
+++ b/src/tss2-fapi/ifapi_helpers.c
@@ -1526,7 +1526,7 @@ ifapi_get_name(TPMT_PUBLIC *publicInfo, TPM2B_NAME *name)
  * @retval TSS2_SYS_RC_* for SAPI errors.
  */
 TSS2_RC
-ifapi_nv_get_name(TPM2B_NV_PUBLIC *publicInfo, TPM2B_NAME *name)
+ifapi_nv_get_name(TPMS_NV_PUBLIC *publicInfo, TPM2B_NAME *name)
 {
     BYTE buffer[sizeof(TPMS_NV_PUBLIC)];
     size_t offset = 0;
@@ -1534,18 +1534,18 @@ ifapi_nv_get_name(TPM2B_NV_PUBLIC *publicInfo, TPM2B_NAME *name)
     size_t len_alg_id = sizeof(TPMI_ALG_HASH);
     IFAPI_CRYPTO_CONTEXT_BLOB *cryptoContext;
 
-    if (publicInfo->nvPublic.nameAlg == TPM2_ALG_NULL) {
+    if (publicInfo->nameAlg == TPM2_ALG_NULL) {
         name->size = 0;
         return TSS2_RC_SUCCESS;
     }
     TSS2_RC r;
 
     /* Initialize the hash computation with the nameAlg. */
-    r = ifapi_crypto_hash_start(&cryptoContext, publicInfo->nvPublic.nameAlg);
+    r = ifapi_crypto_hash_start(&cryptoContext, publicInfo->nameAlg);
     return_if_error(r, "Crypto hash start");
 
     /* Get the marshaled data of the public area. */
-    r = Tss2_MU_TPMS_NV_PUBLIC_Marshal(&publicInfo->nvPublic,
+    r = Tss2_MU_TPMS_NV_PUBLIC_Marshal(publicInfo,
                                        &buffer[0], sizeof(TPMS_NV_PUBLIC),
                                        &offset);
     if (r) {
@@ -1572,7 +1572,7 @@ ifapi_nv_get_name(TPM2B_NV_PUBLIC *publicInfo, TPM2B_NAME *name)
 
     offset = 0;
     /* Store the nameAlg in the result. */
-    r = Tss2_MU_TPMI_ALG_HASH_Marshal(publicInfo->nvPublic.nameAlg,
+    r = Tss2_MU_TPMI_ALG_HASH_Marshal(publicInfo->nameAlg,
                                       &name->name[0], sizeof(TPMI_ALG_HASH),
                                       &offset);
     return_if_error(r, "Marshaling TPMI_ALG_HASH");
@@ -1609,7 +1609,7 @@ ifapi_object_cmp_name(IFAPI_OBJECT *object, void *name, bool *equal)
         obj_name = &object->misc.key.name;
         break;
     case IFAPI_NV_OBJ:
-        r = ifapi_nv_get_name(&object->misc.nv.public, &nv_name);
+        r = ifapi_nv_get_name(&object->misc.nv.public.nvPublic, &nv_name);
         return_if_error(r, "Get NV name.");
 
         obj_name = &nv_name;
diff --git a/src/tss2-fapi/ifapi_helpers.h b/src/tss2-fapi/ifapi_helpers.h
index 8c419e7e7125..94d82c40ff39 100644
--- a/src/tss2-fapi/ifapi_helpers.h
+++ b/src/tss2-fapi/ifapi_helpers.h
@@ -104,7 +104,7 @@ ifapi_get_name(
 
 TSS2_RC
 ifapi_nv_get_name(
-    TPM2B_NV_PUBLIC *publicInfo,
+    TPMS_NV_PUBLIC *publicInfo,
     TPM2B_NAME *name);
 
 TSS2_RC
diff --git a/src/tss2-fapi/ifapi_policy_calculate.c b/src/tss2-fapi/ifapi_policy_calculate.c
index 011c66874398..7bbd916f3a21 100644
--- a/src/tss2-fapi/ifapi_policy_calculate.c
+++ b/src/tss2-fapi/ifapi_policy_calculate.c
@@ -289,7 +289,7 @@ ifapi_calculate_policy_authorize_nv(
 
     /* Written flag has to be set for policy calculation, because during
        policy execution it will be set. */
-    policy->nvPublic.nvPublic.attributes |= TPMA_NV_WRITTEN;
+    policy->nvPublic.attributes |= TPMA_NV_WRITTEN;
 
     r = ifapi_nv_get_name(&policy->nvPublic, &nv_name);
     return_if_error(r, "Compute NV name");
@@ -1061,7 +1061,7 @@ ifapi_calculate_policy_nv(
 
     /* Written flag has to be set for policy calculation, because during
        policy execution it will be set. */
-    policy->nvPublic.nvPublic.attributes |= TPMA_NV_WRITTEN;
+    policy->nvPublic.attributes |= TPMA_NV_WRITTEN;
 
     /* Compute NV name from public info */
 
diff --git a/src/tss2-fapi/ifapi_policy_callbacks.c b/src/tss2-fapi/ifapi_policy_callbacks.c
index 8bc15b2120e2..3f0524b7ef47 100644
--- a/src/tss2-fapi/ifapi_policy_callbacks.c
+++ b/src/tss2-fapi/ifapi_policy_callbacks.c
@@ -190,7 +190,7 @@ ifapi_get_object_name(
                                (TPM2B_NAME *)name);
             break;
         case IFAPI_NV_OBJ:
-            r = ifapi_nv_get_name(&object.misc.nv.public, name);
+            r = ifapi_nv_get_name(&object.misc.nv.public.nvPublic, name);
             break;
         default:
             goto_error(r, TSS2_FAPI_RC_BAD_VALUE, "Invalid object %s.",
@@ -243,7 +243,7 @@ TSS2_RC
 ifapi_get_nv_public(
     const char *path,
     TPMI_RH_NV_INDEX nv_index,
-    TPM2B_NV_PUBLIC *nv_public,
+    TPMS_NV_PUBLIC *nv_public,
     void *ctx)
 {
     TSS2_RC r = TSS2_RC_SUCCESS;
@@ -274,7 +274,7 @@ ifapi_get_nv_public(
                                               NULL);
                 try_again_or_error_goto(r, "Error: nv read public finish", cleanup);
 
-                *nv_public = *nv_public_esys;
+                *nv_public = nv_public_esys->nvPublic;
                 SAFE_FREE(nv_public_esys);
                 context->io_state = IO_INIT;
                 break;
@@ -302,7 +302,7 @@ ifapi_get_nv_public(
                                cleanup, path);
                 }
 
-                *nv_public = object.misc.nv.public;
+                *nv_public = object.misc.nv.public.nvPublic;
                 context->io_state = IO_INIT;
                 break;
 
@@ -1331,7 +1331,7 @@ cleanup:
  */
 TSS2_RC
 ifapi_exec_auth_nv_policy(
-    TPM2B_NV_PUBLIC *nv_public,
+    TPMS_NV_PUBLIC *nv_public,
     TPMI_ALG_HASH hash_alg,
     void *userdata)
 {
@@ -1367,8 +1367,11 @@ ifapi_exec_auth_nv_policy(
     switch (cb_ctx->cb_state) {
         statecase(cb_ctx->cb_state, POL_CB_EXECUTE_INIT)
             /* Search a NV object with a certain NV indext stored in nv_public. */
+            /* TODO modify internal APIs to use TPMS_NV_INDEX over TPM2B variant */
+            TPM2B_NV_PUBLIC tmp = { 0 };
+            tmp.nvPublic = *nv_public;
             r = ifapi_keystore_search_nv_obj(&fapi_ctx->keystore, &fapi_ctx->io,
-                                             nv_public, &nv_path);
+                                             &tmp, &nv_path);
             FAPI_SYNC(r, "Search Object", cleanup);
 
             r = ifapi_keystore_load_async(&fapi_ctx->keystore, &fapi_ctx->io, nv_path);
diff --git a/src/tss2-fapi/ifapi_policy_callbacks.h b/src/tss2-fapi/ifapi_policy_callbacks.h
index 0fe6ffe6511b..4a8f14b984cf 100644
--- a/src/tss2-fapi/ifapi_policy_callbacks.h
+++ b/src/tss2-fapi/ifapi_policy_callbacks.h
@@ -54,7 +54,7 @@ TSS2_RC
 ifapi_get_nv_public(
     const char *path,
     TPMI_RH_NV_INDEX nv_index,
-    TPM2B_NV_PUBLIC *nv_public,
+    TPMS_NV_PUBLIC *nv_public,
     void *context);
 
 TSS2_RC
@@ -102,7 +102,7 @@ ifapi_exec_auth_policy(
 
 TSS2_RC
 ifapi_exec_auth_nv_policy(
-    TPM2B_NV_PUBLIC *nv_public,
+    TPMS_NV_PUBLIC *nv_public,
     TPMI_ALG_HASH hash_alg,
     void *userdata);
 
diff --git a/src/tss2-fapi/ifapi_policy_instantiate.c b/src/tss2-fapi/ifapi_policy_instantiate.c
index 8cc91667b51d..d31d9913bfd4 100644
--- a/src/tss2-fapi/ifapi_policy_instantiate.c
+++ b/src/tss2-fapi/ifapi_policy_instantiate.c
@@ -303,10 +303,10 @@ ifapi_policyeval_instantiate_finish(
             break;
 
         case POLICYNV:
-            if (pol_element->element.PolicyNV.nvPublic.nvPublic.nvIndex) {
+            if (pol_element->element.PolicyNV.nvPublic.nvIndex) {
                 /* nvIndex is already set in policy. Path will not be needed */
                 pol_element->element.PolicyNV.nvIndex
-                    = pol_element->element.PolicyNV.nvPublic.nvPublic.nvIndex;
+                    = pol_element->element.PolicyNV.nvPublic.nvIndex;
                 SAFE_FREE(pol_element->element.PolicyNV.nvPath);
                 break;
             }
@@ -321,7 +321,7 @@ ifapi_policyeval_instantiate_finish(
             return_if_error(r, "read_finish failed");
 
             pol_element->element.PolicyNV.nvIndex
-                = pol_element->element.PolicyNV.nvPublic.nvPublic.nvIndex;
+                = pol_element->element.PolicyNV.nvPublic.nvIndex;
 
             /* Clear NV path, only public data will be needed */
             SAFE_FREE(pol_element->element.PolicyNV.nvPath);
@@ -355,7 +355,7 @@ ifapi_policyeval_instantiate_finish(
             break;
 
         case POLICYAUTHORIZENV:
-            if (pol_element->element.PolicyAuthorizeNv.nvPublic.nvPublic.nvIndex) {
+            if (pol_element->element.PolicyAuthorizeNv.nvPublic.nvIndex) {
                 /* nvIndex is already set in policy. Path will not be needed */
                 SAFE_FREE(pol_element->element.PolicyAuthorizeNv.nvPath);
                 break;
diff --git a/src/tss2-fapi/ifapi_policy_json_deserialize.c b/src/tss2-fapi/ifapi_policy_json_deserialize.c
index 7c81b639bc33..659718491ef1 100644
--- a/src/tss2-fapi/ifapi_policy_json_deserialize.c
+++ b/src/tss2-fapi/ifapi_policy_json_deserialize.c
@@ -444,8 +444,10 @@ ifapi_json_TPMS_POLICYNV_deserialize(json_object *jso,  TPMS_POLICYNV *out)
     if (!ifapi_get_sub_object(jso, "nvPublic", &jso2)) {
         memset(&out->nvPublic, 0, sizeof(TPM2B_NV_PUBLIC));
     } else {
-        r = ifapi_json_TPM2B_NV_PUBLIC_deserialize(jso2, &out->nvPublic);
+        TPM2B_NV_PUBLIC tmp = { 0 };
+        r = ifapi_json_TPM2B_NV_PUBLIC_deserialize(jso2, &tmp);
         return_if_error(r, "Bad value for field \"nvPublic\".");
+        out->nvPublic = tmp.nvPublic;
     }
 
     if (!ifapi_get_sub_object(jso, "operandB", &jso2)) {
@@ -1110,8 +1112,10 @@ ifapi_json_TPMS_POLICYAUTHORIZENV_deserialize(json_object *jso,
         memset(&out->nvPublic, 0, sizeof(TPM2B_NV_PUBLIC));
     } else {
         cond_cnt++;
-        r = ifapi_json_TPM2B_NV_PUBLIC_deserialize(jso2, &out->nvPublic);
+        TPM2B_NV_PUBLIC tmp = { 0 };
+        r = ifapi_json_TPM2B_NV_PUBLIC_deserialize(jso2, &tmp);
         return_if_error(r, "Bad value for field \"nvPublic\".");
+        out->nvPublic = tmp.nvPublic;
     }
     /* Check whether only one condition field found in policy. */
     if (cond_cnt != 1) {
diff --git a/src/tss2-fapi/ifapi_policy_json_serialize.c b/src/tss2-fapi/ifapi_policy_json_serialize.c
index 34f5813fd4ab..c84d46b25b00 100644
--- a/src/tss2-fapi/ifapi_policy_json_serialize.c
+++ b/src/tss2-fapi/ifapi_policy_json_serialize.c
@@ -327,9 +327,11 @@ ifapi_json_TPMS_POLICYNV_serialize(const TPMS_POLICYNV *in, json_object **jso)
         json_object_object_add(*jso, "nvIndex", jso2);
     }
 
-    if (in->nvPublic.nvPublic.nvIndex) {
+    if (in->nvPublic.nvIndex) {
         jso2 = NULL;
-        r = ifapi_json_TPM2B_NV_PUBLIC_serialize(&in->nvPublic, &jso2);
+        TPM2B_NV_PUBLIC tmp = { 0 };
+        tmp.nvPublic = in->nvPublic;
+        r = ifapi_json_TPM2B_NV_PUBLIC_serialize(&tmp, &jso2);
         return_if_error(r, "Serialize TPM2B_NV_PUBLIC");
 
         json_object_object_add(*jso, "nvPublic", jso2);
@@ -848,10 +850,12 @@ ifapi_json_TPMS_POLICYAUTHORIZENV_serialize(const TPMS_POLICYAUTHORIZENV *in,
         json_object_object_add(*jso, "nvPath", jso2);
     }
     jso2 = NULL;
-    if (in->nvPublic.nvPublic.nvIndex > 0) {
+    if (in->nvPublic.nvIndex > 0) {
         cond_cnt++;
+        TPM2B_NV_PUBLIC tmp = { 0 };
+        tmp.nvPublic = in->nvPublic;
         /* Template already instantiated */
-        r = ifapi_json_TPM2B_NV_PUBLIC_serialize(&in->nvPublic, &jso2);
+        r = ifapi_json_TPM2B_NV_PUBLIC_serialize(&tmp, &jso2);
         return_if_error(r, "Serialize TPM2B_NV_PUBLIC");
 
         json_object_object_add(*jso, "nvPublic", jso2);
diff --git a/src/tss2-fapi/ifapi_policy_types.h b/src/tss2-fapi/ifapi_policy_types.h
index 99f160bd06f5..78d60b037631 100644
--- a/src/tss2-fapi/ifapi_policy_types.h
+++ b/src/tss2-fapi/ifapi_policy_types.h
@@ -71,7 +71,7 @@ typedef struct {
 typedef struct {
     char                                        *nvPath;    /**< None */
     TPMI_RH_NV_INDEX                            nvIndex;    /**< None */
-    TPM2B_NV_PUBLIC                            nvPublic;    /**< None */
+    TPMS_NV_PUBLIC                             nvPublic;    /**< None */
     TPMI_RH_NV_AUTH                          authHandle;    /**< This is determined by FAPI at runtime. */
     TPM2B_OPERAND                              operandB;    /**< None */
     UINT16                                       offset;    /**< Default value is 0 */
@@ -181,7 +181,7 @@ typedef struct {
  */
 typedef struct {
     char                                        *nvPath;    /**< None */
-    TPM2B_NV_PUBLIC                            nvPublic;    /**< None */
+    TPMS_NV_PUBLIC                             nvPublic;    /**< None */
     TPM2B_DIGEST                                 policy;    /**< Policy Digest */
     TPMT_HA                                   nv_policy;    /**< Policy stored in NV ram */
     uint8_t                               *policy_buffer;

[williamcroberts]

Ill finish this change up and submit it later today with the other callbacks.

[williamcroberts]

Actually I think this patch is about right, I looked into my TODO on changing the lookup routine and the IFAPI_NV union but it starts to cascade wildly into the FAPI API code.

[whooo]

sorry for the late reply. I was thinking of the actual JSON encoding/decoding, so for example encoding a TPM2B_PUBLIC (and TPMT_PUBLIC in at least one case) will output (and expect) something like:

"newParentPublic": {
  "size": 0, 
  "publicArea": {
    "type": "RSA",
    ...
  }
}

While my understanding of the specification is that it should encode (and decode) publicArea (and nvPublic) directly, skipping size and publicArea/nvPublic, so something like:

"newParentPublic": { 
  "type": "RSA",
  ....
}

[JuergenReppSIT]

@whooo Sorry I had the impression that you wanted to change the json encoding of POLIYCYAUTHORIZENV where the TPM2B encoding was used in the spec.
But yes in POLICYDUPLICATIONSELECT the spec uses TPMT_PUBLIC and the implementation uses TPM2B_PUBLIC.
This should be fixed. @williamcroberts To achieve backward compatibility perhaps the TPM2B version should also be allowed in deserialization?
@AndreasFuchsTPM the usage of TPM2B_PUBLIC in _POLICYAUTHORIZENV seems to be an error. Currently the implementation uses the TPM2B_NV_PUBLIC encoding. Perhaps here TPMS_NV_PUBLIC should be used in the spec and also the TPM2B version should be allowed to achieve backward compatibility?

[whooo]

@whooo Sorry I had the impression that you wanted to change the json encoding of POLIYCYAUTHORIZENV where the

TPM2B encoding was used in the spec. But yes in POLICYDUPLICATIONSELECT the spec uses TPMT_PUBLIC and the implementation uses TPM2B_PUBLIC.
I should have been clearer, I read the specification as that all complex TPM2B types should be encoded/decoded as their contained type, so TPM2B_PUBLIC should be encoded as a TPMT_PUBLIC and TPM2B_NV_PUBLIC should be encoded as a TPMS_NV_PUBLIC.
One reason I'm bringing this up is that it affects how the JSON encoder/decoder in tpm2-pytss should behave with complex TPM2B types

[JuergenReppSIT]

@whooo Thank you for explaining the issue.