Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

esys: remove trailing zeros in auth value. #2665

Merged
merged 1 commit into from
Jan 10, 2024
Merged

esys: remove trailing zeros in auth value. #2665

merged 1 commit into from
Jan 10, 2024

Conversation

JuergenReppSIT
Copy link
Member

@JuergenReppSIT JuergenReppSIT commented Jul 18, 2023 •
edited by cplappert
Loading

When TPM Calulates the HMAC trailing zeros are rmoved. Therfore the
trailing zeros are removed in Esys_TR_SetAuth, Esys_Create, Esys_CreateLoded,
Esys_NV_DefineSpace, and Esys_CreatePrimary.
The auth value is also adapted in the ChangeAuth programs

Addresses #2664

Signed-off-by: Juergen Repp juergen_repp@web.de

@JuergenReppSIT JuergenReppSIT force-pushed the esys-fix-trailing-zeroes-in-auth-value branch from 9ef067d to 1ba1f5b Compare July 19, 2023 00:00
@JuergenReppSIT JuergenReppSIT marked this pull request as draft July 19, 2023 00:27
@JuergenReppSIT JuergenReppSIT force-pushed the esys-fix-trailing-zeroes-in-auth-value branch from 1ba1f5b to 1cbf7a3 Compare July 19, 2023 12:54
@JuergenReppSIT JuergenReppSIT mentioned this pull request Jul 20, 2023
Closed
@williamcroberts
Copy link
Member

@JuergenReppSIT we updated the bug tracker #2660 with complete trigger examples, any way you could respin this to get it to fail? I backed your patch out and ran the test and couldn't get a failure. Perhaps I didn't do something properly locally. Don't we also need to handle the auth values when we create objects (Esys_Create, Esys_CreateLoaded, Esys_CreatePrimary, Esys_NV_DefineSpace) since they get remembered?

@JuergenReppSIT JuergenReppSIT force-pushed the esys-fix-trailing-zeroes-in-auth-value branch from 1cbf7a3 to 361a436 Compare July 20, 2023 14:50
@JuergenReppSIT
Copy link
Member Author

@williamcroberts sorry I forgot, that Esys_Create does not create an esys object. The object will be created by Esys_Load. So the call of Esys_TR_SetAuth is needed for keys.
I just added the zero stripping to the function which computes the hash for long password. The integration tests work.
I will remove the draft flag after the following additional changes:

  • The integration test is extended with the example which did produce the failure.
  • The functions Esys_ChangeAuth, Esys_NV_ChangeAuth , and Esys_ObjectChangeAuth. do not call the function which adapts long passwords.

@JuergenReppSIT JuergenReppSIT force-pushed the esys-fix-trailing-zeroes-in-auth-value branch 6 times, most recently from dea8a31 to 1badbb8 Compare July 25, 2023 22:18
@JuergenReppSIT JuergenReppSIT marked this pull request as ready for review July 25, 2023 23:31
williamcroberts
williamcroberts reviewed Jul 26, 2023
View reviewed changes
src/tss2-esys/api/Esys_HierarchyChangeAuth.c
store_input_parameters(esysContext, authHandle, newAuth);
iesys_strip_trailing_zeros(&esysContext->in.HierarchyChangeAuth.newAuth);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to not do this in store_input_parameters?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good hint. The spec states:

This command does not change the authorization of the TPM-resident object on which it operates.
Therefore, the old authValue (of the TPM-resident object) is used when generating the response HMAC
key if required.

So it makes no sense to store the auth value in the esys object. I will remove this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @williamcroberts wants the "iesys_strip_trailing_zeros" command to be implemented in the "store_input_parameters" function. This makes also sense to me.
@JuergenReppSIT: Could you do this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@williamcroberts @cplappert Sorry my comment was related to Esys_ObjectChangeAuth. I will do it in store_input_parameters.

src/tss2-esys/api/Esys_ObjectChangeAuth.c
/* Retrieve the metadata objects for provided handles */
r = esys_GetResourceObject(esysContext, objectHandle, &objectHandleNode);
return_state_if_error(r, _ESYS_STATE_INIT, "objectHandle unknown.");
r = esys_GetResourceObject(esysContext, parentHandle, &parentHandleNode);
return_state_if_error(r, _ESYS_STATE_INIT, "parentHandle unknown.");

if (objectHandleNode->rsrc.rsrcType == IESYSC_KEY_RSRC) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about NV objects?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec states:
NOTE 3 If an NV Index is to have a new authorization, it is done with TPM2_NV_ChangeAuth().

@JuergenReppSIT JuergenReppSIT force-pushed the esys-fix-trailing-zeroes-in-auth-value branch 2 times, most recently from 2ad640f to f32bda0 Compare July 26, 2023 19:20
@JuergenReppSIT JuergenReppSIT force-pushed the esys-fix-trailing-zeroes-in-auth-value branch 3 times, most recently from 0f4e2c8 to 26068c4 Compare August 15, 2023 11:39
@JuergenReppSIT JuergenReppSIT added this to the 4.0.2 milestone Aug 15, 2023
@JuergenReppSIT
esys: remove trailing zeros in auth value
d103e84 
When TPM Calulates the HMAC trailing zeros are rmoved.
Therfore the trailing zeros are removed in Esys_TR_SetAuth,
Esys_Create, Esys_CreateLoded, Esys_NV_DefineSpace,
and Esys_CreatePrimary.
The removing is added to the function iesys_hash_long_auth_value.
Therefore this function is renamed to iesys_adapt_auth_value.
An integration test which uses trailing zeros in auth values
is added.

The auth value handling in ChangeAuth programs is fixed:
* The trailing zeros are now removed in these programs.
* The new auth value now is stored in objects where the auth value
  is changed with Esys_ObjectChangeAuth.
* the integration test which checkd trailing zeros is extend.

Fixes: tpm2-software#2664

Signed-off-by: Juergen Repp <juergen_repp@web.de>
@JuergenReppSIT JuergenReppSIT force-pushed the esys-fix-trailing-zeroes-in-auth-value branch from 26068c4 to d103e84 Compare December 13, 2023 16:13
@codecov Codecov
Copy link

codecov bot commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fe83020) 82.57% compared to head (02ff859) 82.58%.
Report is 30 commits behind head on master.

❗ Current head 02ff859 differs from pull request most recent head d103e84. Consider uploading reports for the commit d103e84 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2665      +/-   ##
==========================================
+ Coverage   82.57%   82.58%   +0.01%     
==========================================
  Files         368      368              
  Lines       42985    43011      +26     
==========================================
+ Hits        35495    35522      +27     
+ Misses       7490     7489       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AndreasFuchsTPM AndreasFuchsTPM merged commit 1fca623 into tpm2-software:master Jan 10, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williamcroberts williamcroberts williamcroberts left review comments

@cplappert cplappert cplappert left review comments

@AndreasFuchsTPM AndreasFuchsTPM AndreasFuchsTPM approved these changes

Assignees

@AndreasFuchsTPM AndreasFuchsTPM

@williamcroberts williamcroberts

Labels
None yet
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@JuergenReppSIT @williamcroberts @AndreasFuchsTPM @cplappert