utils: add function to generate an EK #279

whooo · 2021-12-06T13:14:59Z

I have tested the standard ECC and RSA templates towards a real TPM, but the the NV nonce/template is written according to the specification/tpm2-tools so I'm not 100% they work in real life.

Draft until I add types to arguments and documentation.

Related to #203

codecov · 2021-12-06T13:25:28Z

Codecov Report

Merging #279 (950a461) into master (c0e9f65) will increase coverage by 0.02%.
The diff coverage is 96.39%.

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
+ Coverage   93.59%   93.61%   +0.02%     
==========================================
  Files          15       17       +2     
  Lines        4665     5212     +547     
  Branches      592      750     +158     
==========================================
+ Hits         4366     4879     +513     
- Misses        218      239      +21     
- Partials       81       94      +13

Impacted Files	Coverage Δ
tpm2_pytss/utils.py	`92.07% <94.87%> (+2.41%)`	⬆️
tpm2_pytss/internal/templates.py	`100.00% <100.00%> (ø)`
tpm2_pytss/internal/utils.py	`84.96% <0.00%> (-1.93%)`	⬇️
tpm2_pytss/internal/crypto.py	`92.36% <0.00%> (-1.77%)`	⬇️
tpm2_pytss/types.py	`95.31% <0.00%> (-0.12%)`	⬇️
tpm2_pytss/ESAPI.py	`98.10% <0.00%> (-0.01%)`	⬇️
tpm2_pytss/encoding.py	`95.31% <0.00%> (ø)`
tpm2_pytss/constants.py	`99.28% <0.00%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0e9f65...950a461. Read the comment docs.

williamcroberts

This is the spec for the EK:

https://trustedcomputinggroup.org/wp-content/uploads/TCG_IWG_EKCredentialProfile_v2p4_r3.pdf

williamcroberts · 2021-12-06T14:31:11Z

tpm2_pytss/utils.py

@@ -226,3 +227,117 @@ def unwrap(
        )

    return s
+
+
+def create_ek(ectx, ekalg=TPM2_ALG.ECC, session=ESYS_TR.NONE):


I would change this slightly to get_ek_template and be able to pass session1, session2, and session3 here so folks pass everything into the esys calls and then I would have the createprimary call be by the user. This way the session1, which is auth for the ENDORSEMENT hierarchy can be set by the caller to an HMAC session, audit session, etc.

Oh and add type hints to the parameters.

docstring with link to the generic spec landing page:

https://trustedcomputinggroup.org/resource/tcg-ek-credential-profile-for-tpm-family-2-0/

@idesai could you weigh in on this API design with your thoughts as well?

I agree. Something to consider... because it uses a TCG EK credential specific value for authorization policy and attributes get_tcg_ek_template might be a good name.

I kept only one session as the only usefulness I see of passing a session after removing create_primary is for audit and encrypt sessions.
As for the name, I prefer to keep it as close to tpm2_createek as reasonable, to keep it slightly familiar for tpm2-tools users, but I can go either way.

You can have separate audit and encrypt sessions. I was trying to future proof the API a bit since session1,2,3 would just be the defaults and only if folks are interested in that control will they use them.

OK, I'll change it to session1 and session2, but I'm worried about session3 as that wouldn't be passed to nv_read and would break expectations for session3

williamcroberts · 2021-12-07T00:29:52Z

tpm2_pytss/utils.py

+        mtempl = b""
+        while left > 0:
+            off = tpub.nvPublic.dataSize - left
+            size = 0x300 if left > 0x300 else left


What's this magic number from?

I thought that was the minimum allowed for TPM2_PT_NV_BUFFER_MAX, but when I checked again and it's apparently 512, not 768 for the PC client profile

This value should ideally be read from the capability structure

williamcroberts · 2021-12-07T00:32:42Z

tpm2_pytss/utils.py

+    while more:
+        more, data = ectx.get_capability(
+            TPM2_CAP.HANDLES,
+            TPM2_HT.NV_INDEX << 24,


Doesn't this one include the shift: TPM2_HC.HR_NV_INDEX

williamcroberts · 2021-12-07T00:37:24Z

tpm2_pytss/utils.py

+            nonce = nonce + bytes(data)
+            left = left - len(data)
+
+    if len(nonce) > 0 and template.publicArea.type == TPM2_ALG.RSA:


This padding is only done on RSA 2048 and ECC NIST P256 per the 2.2.1.6 "TPMT_PUBLIC Calculation"

https://trustedcomputinggroup.org/wp-content/uploads/TCG_IWG_EKCredentialProfile_v2p4_r3.pdf

Their are other keys possible, so I don't think we want to always do this AFAICT.

The code currently only supports RSA 2048 and NIST P256 keys (unless the TPM manufacturer has put a non-standard template in one of the NV template areas)

williamcroberts · 2021-12-07T03:28:04Z

tpm2_pytss/utils.py

+        session: (ESYS_TR): Optional session, for encrypt/decrypt and audit sessions, defaults to ESYS_TR.NONE
+
+    Returns:
+        The templase as a TPM2B_PUBLIC instance


Spelling, templase --> template

lgtm-com · 2021-12-07T09:52:09Z

This pull request introduces 1 alert when merging c5ee5eb into 0f48299 - view on LGTM.com

new alerts:

1 for Unused import

williamcroberts · 2021-12-15T19:09:04Z

So the overall design is that it will take a callback to read the NV index and return the template. This way folks can use it with any type of TPM context (ESAPI or FAPI) and a userdata pointer. Since this is python, with closure, we don't really need to worry about passing user data just a callable.

whooo · 2021-12-15T19:29:33Z

So the overall design is that it will take a callback to read the NV index and return the template. This way folks can use it with any type of TPM context (ESAPI or FAPI) and a userdata pointer. Since this is python, with closure, we don't really need to worry about passing user data just a callable.

So would it be like something like?:

def create_ek_template(ekalg, nv_read_cb):
    defaults_here
    nonce = nv_read_cb(nonce_index)
    template = nv_read_cb(template_index)
    do_rest_here

williamcroberts · 2021-12-15T19:36:44Z

ekalg

Yes, but I forgot to mention ekalg is a string identifier that maps to one of the static templates or one of the NV indices. Let me see if I can figure out what those will be. I don't have them offhand.

williamcroberts · 2021-12-16T17:58:32Z

Here are the strings:

string	template address	nonce
EK-RSA2048	0x01c00003	0x01c00004
EK-ECC256	0x01c0000b	0x01c0000c
EK-HIGH-RSA2048	0x01c00013
EK-HIGH-ECC256	0x01c00015
EK-HIGH-ECC384	0x01c00017
EK-HIGH-ECC521	0x01c00019
EK-HIGH-ECCSM2P521	0x01c0001b
EK-HIGH-RSA3072	0x01c0001d
EK-HIGH-RSA4096	0x01c0001f

This is the callback signature:

def nvread(index)

So you could something like:

class MyNVReader:
    def __init__(ectx):
        self.ectx = ectx

    def __call__(self, nvindex):
        return self.ectx.nvread(nvindex)

get_ek("EK-RSA2048", MyNVReader())

# or something like
ectx = ESAPI()
#.....
def my_nv_read(nvindex):
    return ectx.nv_read(nvindex)

get_ek("EK-RSA2048", my_nv_read)

williamcroberts · 2021-12-16T17:59:52Z

If it has a nonce, it needs to call the nv reader cb for the templ and the nonce. The calling framework is responsible for handling merging those and unmarshalling everything into the final TPMT_PUBLIC.

williamcroberts · 2021-12-16T18:30:32Z

I'm not sure what they are going to be for the default static templates. It's not clear to me in the spec if the static templates are the the same as the ones in the NV Index. So these could be mapped to unique strings or based on if callback is None.

See the templates in B.4.4 and later in:

https://trustedcomputinggroup.org/wp-content/uploads/TCG_IWG_EKCredentialProfile_v2p4_r3.pdf

whooo · 2021-12-16T18:38:28Z

is it clear by the specification what should happen if the NV index isn't defined (low template/nonce) on the TPM?
For example should the NV read callback just return an empty buffer in that case?

williamcroberts · 2021-12-16T20:14:08Z

is it clear by the specification what should happen if the NV index isn't defined (low template/nonce) on the TPM? For example should the NV read callback just return an empty buffer in that case?

Whatever nvread failing for that condition would be. The framework would then use the static nonce.

williamcroberts · 2021-12-16T20:20:39Z

Also look at section "2.2.1.3" in that EK document. It lays out a table with what is and is not allowed, and things that are discouraged.

williamcroberts · 2021-12-16T20:25:23Z

Then the sections on LOW (2.2.1.4) and HIGH (2.2.1.5) layout more of what can be expected. Essentially for the LOW's their SHOULD be an NV index set, however if their isn't it's a default template. For the low range their MAY be a nonce to deal with. For the HIGH range, they're MAY be an NV index for the template, however if not it's the default template.

lgtm-com · 2021-12-17T14:37:41Z

This pull request introduces 1 alert when merging 6aaa50a into 7cd4cc0 - view on LGTM.com

new alerts:

1 for Unused import

whooo · 2021-12-19T12:07:47Z

@williamcroberts, when you have time could you check if the code matches the the general idea?
There are still somethings I'm unsure about for the new function specification, for example is it up to the caller or the function to check if a high range certificate NV index is defined or not?

williamcroberts · 2021-12-21T14:30:47Z

@williamcroberts, when you have time could you check if the code matches the the general idea? There are still somethings I'm unsure about for the new function specification, for example is it up to the caller or the function to check if a high range certificate NV index is defined or not?

The callback function should check. We should probably have a specific case for this error to then go get the default template, all other errors propagate to the original caller. The framework function should never go to the TPM, only through the callback.

lgtm-com · 2022-01-03T02:59:03Z

This pull request introduces 1 alert when merging 41c57bc into f79d3be - view on LGTM.com

new alerts:

1 for Unused import

whooo · 2022-01-06T05:37:00Z

This is the best I could come up with my understanding to the specification.
I added the certificate to the return values as it must be read in many cases regardless

williamcroberts · 2022-02-03T15:51:57Z

tpm2_pytss/utils.py

+
+    def __call__(self, index: Union[int, TPM2_RH]) -> bytes:
+        if index not in self._indices:
+            raise NoSuchIndex(index)


What if the index was defined between the list population and now. Perhaps it's better to just try and read the NV index and look at the error code to see if it's not defined. Which also reduces the trips we need to make to the TPM. Which then, do we even need this exception class, we could just use TSS2 Exceptions that already exist.

I view the chance as low for this use case as new template/certificate areas shouldn't be created during runtime
The problem with letting the TPM throw an error is that it would break audit sessions (and possibly encrypted sessions) for the scenarios where create_ek_template looks for an index but shouldn't fail if there isn't one
We could have a constant of an TSS2_Exception instead, but something explicit and clear is needed, for example someone writes their own callback using FAPI instead which wouldn't throw the same exception

I view the chance as low for this use case as new template/certificate areas shouldn't be created during runtime

It's an unlikely race condition, but it's a TOCTOU issue.

The problem with letting the TPM throw an error is that it would break audit sessions (and possibly encrypted sessions) for

I'm not following, what exactly breaks? Does a failing command invalidate the session?

the scenarios where create_ek_template looks for an index but shouldn't fail if there isn't one We could have a constant of an TSS2_Exception instead, but something explicit and clear is needed, for example someone writes their own callback using FAPI instead which wouldn't throw the same exception

True, or if they use their own custom TSS.

I view the chance as low for this use case as new template/certificate areas shouldn't be created during runtime

It's an unlikely race condition, but it's a TOCTOU issue.

The problem with letting the TPM throw an error is that it would break audit sessions (and possibly encrypted sessions) for

I'm not following, what exactly breaks? Does a failing command invalidate the session?

I reread the specification and I was wrong, so that can be dropped, I'll rework the code to not check the index before trying to read and raise a NoSuchIndex exception if the TSS2_Exception matches the correct error, otherwise reraise the TSS2_Exception

williamcroberts · 2022-02-03T15:52:30Z

tpm2_pytss/utils.py

+        return f"NV index 0x{index:08x} does not exist"
+
+
+class nv_read_ek:


Shouldn't classes be capitalized?

lgtm-com · 2022-02-16T08:59:09Z

This pull request introduces 1 alert when merging e321a6d into 584dd8b - view on LGTM.com

new alerts:

1 for Unused import

Signed-off-by: Erik Larsson <who+github@cnackers.org>

williamcroberts requested changes Dec 6, 2021

View reviewed changes

whooo force-pushed the create_ek branch 2 times, most recently from d16411f to 5168cd5 Compare December 6, 2021 20:16

whooo marked this pull request as ready for review December 6, 2021 20:39

williamcroberts requested changes Dec 7, 2021

View reviewed changes

williamcroberts reviewed Dec 7, 2021

View reviewed changes

whooo force-pushed the create_ek branch from 5168cd5 to c5ee5eb Compare December 7, 2021 09:44

whooo force-pushed the create_ek branch from c5ee5eb to a364c5b Compare December 7, 2021 10:12

whooo marked this pull request as draft December 16, 2021 14:43

whooo force-pushed the create_ek branch from a364c5b to 6aaa50a Compare December 17, 2021 14:29

whooo force-pushed the create_ek branch 2 times, most recently from 74ffa5f to 7ec6d51 Compare December 19, 2021 11:38

whooo force-pushed the create_ek branch from 7ec6d51 to 41c57bc Compare January 3, 2022 02:53

whooo force-pushed the create_ek branch from 41c57bc to b83769f Compare January 3, 2022 03:02

whooo force-pushed the create_ek branch 3 times, most recently from 794556e to edde6a3 Compare January 6, 2022 03:27

whooo marked this pull request as ready for review January 6, 2022 04:53

whooo force-pushed the create_ek branch from edde6a3 to 2b1d60d Compare January 9, 2022 04:46

williamcroberts reviewed Feb 3, 2022

View reviewed changes

whooo force-pushed the create_ek branch from 2b1d60d to e321a6d Compare February 16, 2022 08:54

utils: add function to generate an EK template

950a461

Signed-off-by: Erik Larsson <who+github@cnackers.org>

whooo force-pushed the create_ek branch from e321a6d to 950a461 Compare February 16, 2022 09:19

williamcroberts approved these changes Feb 16, 2022

View reviewed changes

williamcroberts merged commit a274a90 into tpm2-software:master Feb 16, 2022

		return f"NV index 0x{index:08x} does not exist"


		class nv_read_ek:

utils: add function to generate an EK #279

utils: add function to generate an EK #279

Conversation

whooo commented Dec 6, 2021

codecov bot commented Dec 6, 2021 • edited Loading

Codecov Report

williamcroberts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamcroberts Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

whooo Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Dec 7, 2021

williamcroberts commented Dec 15, 2021

whooo commented Dec 15, 2021

williamcroberts commented Dec 15, 2021

williamcroberts commented Dec 16, 2021

williamcroberts commented Dec 16, 2021

williamcroberts commented Dec 16, 2021

whooo commented Dec 16, 2021

williamcroberts commented Dec 16, 2021

williamcroberts commented Dec 16, 2021

williamcroberts commented Dec 16, 2021

lgtm-com bot commented Dec 17, 2021

whooo commented Dec 19, 2021

williamcroberts commented Dec 21, 2021

lgtm-com bot commented Jan 3, 2022

whooo commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Feb 16, 2022

codecov bot commented Dec 6, 2021 •

edited

Loading

williamcroberts Dec 7, 2021 •

edited

Loading

whooo Dec 7, 2021 •

edited

Loading