parallaxsecond / rust-tss-esapi Goto Github PK

We could implement a generic List structure to help us handle the multitude of different lists supported by the TPM interface (structures named TPML_...).

I was thinking of something like

struct List<T>(Vec<T>);

trait ListProperties {
  fn max_size() -> usize;

  fn min_size() -> usize;
}

impl<T> Deref for List<T> {...}

Currently methods in the crate that need to report an error do so by using TSS2 response codes. We should probably have our own means of flagging errors to reduce the confusion.

The method names on Context should be as close to the TSS function names to make it easy for developers to reference them from the specs or previous experience with the TPM stack.

At the moment context structures are built directly with a constructor method which makes some assumptions about some of the parameters inside, e.g. algorithms used in auth sessions.

This could be improved using some builders that parameterize the creation but also allow the use of default values.

Existing constructor methods should be deprecated, but this would imply a breaking change - thoughts @hug-dev ?

Currently it is not possible to create any policies(I think). The pull #63 adds some nice improvement to the creation of pcr selections that will be very useful for making it possible to create a policy from pcr values. I suggest that this issue will include the following.

1. Improve the return value of the pcr_read api.
2. Create a new api for creating a policy from pcr values (policy_pcr) that can use the improved return value from read_pcr as input.

During CI runs that were attempting to run multiple tests using the Mssim TCTI in parallel we found that this could lead to a deadlock (tests are now set to run in a single-thread).
We should do some investigation into whether the Device TCTI has an access broker behind it or if the same issues would arise there as well.

This also needs to be thoroughly documented to make it clear that using a TCTI that does not provide access brokering can encounter these kind of issues if another application is trying to access the TPM at the same time.

Some unit tests that spin up multiple contexts and attempt to use them alternatively would be also be interesting - perhaps we should have tests run against different TCTI types.

Resource handle types that have various variants (e.g. hierarchies) would benefit from better static enforcing through Rust enum wrappers.

Currently neither context APIs nor the necessary structs for handling non volatile storage exists. This needs to be added in order to support for the non volatile storage in the TPM.

TPM2_TestParms can be used to determine if the parameters for an object are supported by the underlying hardware. This would of great use to anyone trying to use the best/most secure available primitives without knowledge of the hardware capabilities a priori.

When produced by bindgen on certain platforms, TPMS_CONTEXT adds an empty padding field to the end of the structure. Currently we do not make sure that this is factored into any usage of the structure, causing compilation failures on said platforms.

An example so far is in the TryFrom<TpmsContext> implementation. The fix would be to simply add a ..Default::default() to TOTC of TPMS_CONTEXT.

Maybe this should be added to all bindgen-generated structs (or manually check the ones that could require padding on platforms with different word sizes)?

I am trying to get the name from the object handle returned by LoadExternal. Normally I would do this by using Esys_TR_GetName. But that interface needs an esys_context and the esys_contetx is a private member of the tss_esapi::Context struct.

So it would be nice if there could be a way that I could access it by usng the tss_esapi construct struct.

One suggestion could be create a functions like thiis one:

pub fn tr_get_name(&mut self, handle: ESYS_TR) -> Result<TPM2B_NAME> {
    let mut name = null_mut();
    let ret = unsafe { 
        Esys_TR_GetName(
            self.mut_context(), 
            handle, 
            &mut name) 
    };
    let ret = Error::from_tss_rc(ret);
    if ret.is_success() {
        let name = unsafe { MBox::<TPM2B_NAME>::from_raw(name) };
        Ok(*name)
    } else {
        error!("Error in getting name: {}.", ret);
        Err(ret)
    }
}

But there might be draw backs with this that I have not considered.

We currently link the TSS libraries at compile time, it would be nice to be able to load them at runtime, through a constructor option.
There could be a feature flag to choose one of the two options.

Hi @puiterwijk ,

Thanks a lot for your help improving the tss-esapi crate. Since you already made significant contributions, plan to do more and as discussed in the community meeting which minutes are going to be published and linked here soon, we would like to give the Write role.

The role will allow you to make your approvals count when reviewing PRs and to be able to merge them.

If you accept and if

• @ionut-arm
• @Superhepper

approve, let's do it 🚀 !

Hi!

I'm currently looking at this issue regarding adding private key support to the PsaImportKey operation. For public keys, Parsec uses the load_external_rsa_public_key to get the public key context. It looks like there is currently no analog for private keys (e.g. load_external_rsa_private_key or load_external_rsa_keypair). Is this something we need? 🙂

Hi @Superhepper ,

If @ionut-arm can confirm, I would like to propose to add you as a contributor and give you Triage right. As you contribute frequently to this crate, it will be a bit easier for you to manage issues and pull requests.

@Superhepper Would you be happy to become a contributor?

The
nv_define_space
and
nv_undefine_space
behaves correctly with regard to authorization because the take an NvAuthorization.

From specification:

TPM2_NV_DefineSpace Command:

Type	Name	Description
TPMI_RH_PROVISION	@authHandle	TPM_RH_OWNER or TPM_RH_PLATFORM+{PP} Auth Index: 1 Auth Role: USER

And the TPM2_NV_UndefineSpace looks similair.

The bug regards the authorization for commands such as nv_read and nv_write (and the remaining commands that will be implmented later). Currently they look like this:

    pub fn nv_read(
        &mut self,
        nv_authorization: NvAuthorization,
        nv_index_handle: NvIndexHandle,
        size: u16,
        offset: u16,
    ) -> Result<MaxNvBuffer>

and

    pub fn nv_write(
        &mut self,
        nv_authorization: NvAuthorization,
        nv_index_handle: NvIndexHandle,
        data: &MaxNvBuffer,
        offset: u16,
    ) -> Result<()>

Here the specification says something different:

Table 220 — TPM2_NV_Write Command

Type	Name	Description
TPMI_RH_NV_AUTH	@authHandle	handle indicating the source of the authorization value Auth Index: 1 Auth Role: USER

So for those commands the authorization is not limited to Platform or Owner and therefor should not take a NvAuthorization as argument.

In the Transient Object Context we do not allow the clients to set a custom public exponent, always defaulting to 0 (i.e. 65537).
While the specs mention that this parameter is optional for TPM implementations, this crate should ideally support customisation for any clients that want to use it.

In #120 Session was added. This now needs to be deployed.

Builds are broken for generating the docs for publishing on docs.rs due to the TSS libraries not being installed on the Docker image where the build takes place. Unfortunately there seems to be no quick way to fix this (e.g. by installing a system package).

The proposed solution is to set up dynamic loading through libloading . This implies creating a new type of FFI binding which can only be hand-crafted, as bindgen does not currently support it (see rust-lang/rust-bindgen#1541 ).

Alternatively, the work for adding the support in bindgen for this case could be done.
Or, the TSS include files can be pulled as a submodule.

EDIT: Initial problem was fixed by adding static bindings ESAPI that are used only for creating documentation. Thus, an issue still exists around removing said bindings and one of the suggestions above should be used.

This is about having two different builds in CI. One with MIN_VERSION = 2.3.3 and one with latest compatible 2.x.x.

See #80 for reference.

Using enums as parameters for algorithm types would help enforcing correct usage of the values involved.

E.g. for the Tpm2bPublicBuilder, with_type and with_name_alg would benefit from stricter compilation checks.

We should split this repository between two crates:

• tss-esapi-sys which role is to find and/or compile the ESAPI libraries, provide the unsafe bindgen types and functions
• tss-esapi which contains higher-level safe Rust constructs

The principle is explained here.

Abstractions over the base Context should present no "raw" FFI-level types to the user - only ones implemented manually in the crate in order to provide a Rustier experience.

We should check if the ESAPI ABI is stable and if it is, commit inside the repository (or in tss-esapi-sys) the bindgen bindings that we currently generate everytime during compilation.

That has multiple advantages:

• make the compilation faster, without the bindgen step
• simplify the build script
• we would be able to use for docs.rs

There is a need to implement the interface types(TPMI). These types are used in a lot of the API:s and are important if we want others to be able to develop new api:s

Interface types(TPMI) are used to indicate what kind of permanent handles that can be used in certain API calls. In the NV-functions there is a 'hack'(NvAuthorization) that needs to be removed. One of these interface types that are used a lot is the Hierarchy.

One major thing with this issue is to figure out how the interface types relates to the handle types that have already been implemented.

something like:

impl Into<TPML_PCR_SELECTION> for PcrSelections {
    fn into(self) -> TPML_PCR_SELECTION {
       ...
    }
}

I was looking at the return code handling. I was trying to get the error number for a specific error

TSS2_ESYS_RC_BAD_VALUE:

but I was unable to find the error number corresponding to this.

I then looked through the code and to me it seamed to do things perfectly fine according to the TPM specification. But the return code we get from the calls to esys APIs is not a TPM2_RC it is in fact a TSS2_RC. The ESAPI specification does not seam to mention response codes in any detailed fashion.

So my question is could there be something we are missing as the TPM_RC travels through he different layers in the software stack?

There are currently no rustdocs in the crate. This should be fixed before we can upstream to crates.io

Most fields denoting sizes (e.g. of keys) are marked as usize at the moment. To make it less ambiguous, the ones representing sizes in bits should be set to u16, u32 or other types unrelated to word size on current platform (as the size itself is not related to the memory of the platform)

This top-level issue is to gather all things that need to be done before publishing a stable version, without the alpha postfix.
Feel free to edit the list.

• Only expose and use abstracted Rust-native types (no raw TSS types)
• Add more documentation/example on the API (#21)
• Perform the split with the -sys crate, check if the bindings can be committed (#85, #30)
• Ensure all functions return all returned TPM data (#151)

There are essentially no real unit tests that reliably cover the functionality we expose. Should be fixed before we can upstream.

At the moment our documentation is purely comment-based and should be enhanced with some working examples of how to use the functionality within.

We need to add support for the evict control api. I think I will do this while doing the interface types.

At the moment, our methods implemented on Context appear to be safe for clients of the crate to use, but it might not be so. We should decide what unsafe behaviour means for us and what functionality we report as (un)safe.

We should also look into our uses of unwrap and try to reduce them as much as possible.

I ask myself, if this Projekt want to be compliane with the TCG Feature API.

It is not released yet (https://trustedcomputinggroup.org/resource/tss-feature-api-specification/), but we could try it with the draft (first link).

Usage of the Context structure is fairly limited by the number of methods implemented on it. This restriction could be lifted if the Context could be unwrapped to expose the ESYS_CONTEXT beneath.

This should be a one-way call, as once this is done, we can no longer keep track of generated handles.

My suggestion would be to bundle up the Esys and TCTI contexts together with the handle set in a "shell" structure that is only used to access them. This would be incorporated in the Context to let it work as usual. Additionally, closing the contexts could be delegated to a custom implementation of Drop for the new structure.

Currently all data passed to/through the library is released as-is once it is dropped. We should ideally clean up all the objects we use and all input we get from clients.

See parallaxsecond/parsec#122 for more details.

There's a lot of usage of the TPM2_ALG_ID type which is essentially a glorified int. Having a Rust enum would allow much better enforcement of allowed values.

Methods for discovering various traits about the algorithms, as defined in Part 2, Section 6.3 of the specification, should also be implemented.

Hi @Superhepper ,

Since you helped us so much with this project and are now deeply involved in its contents, would you like to have Write role to help us into building the tss-esapi crate?

You can find a definition of the Write role here but basically that means that you will be able to approve/merge pull-requests and tinker with the CI.

We would be really happy for you to have this new role 🚀

Currently when sessions are used it has been enough to only provide the session handle (i.e. ESYS_TR). For some of the api:s that needs to be developed this information is not enough. So the session handle needs to be wrapped inside of our own session struct and be deployed. So that it will be easier to add more information regarding the session in the future.

Some of the strings we log for various errors are wrong (copy-paste "errors") and that should be fixed.

We should also do a lot more logging in the TransientKeyContext.

As per mitigation 4 of our Parsec Threat Model, commands sent and received to/from the TPM should be logged.

We can close this issue (and keep it for record) if the logging is already done by the TSS libraries (I think it is).

I am just putting this here to remind us. That tpm2-tss 2.4.0 and greater introduces some breaking changes. Because this is in alpha we have the possibility to just increase the required version of tpm2-tss. But I am not sure which path is the best.

The transient object context gets an Owner hierarchy auth value as a slice of bytes which may be convenient in some circumstances, however it puts the burden on the clients to bring the value in this shape.

A better solution is to create a generic wrapper for Auth values that can be created from strings or byte slices. For strings, these should allow parsing "TSS2 CLI tools"-style strings, i.e. checking for prefixes like str: for string auth values, or hex: for hex encoded binary values. E.g. https://www.mankier.com/1/tpm2_startauthsession#Authorization_Formatting

In the future this structure could be expanded to stand for policy and/or PCR-based authentication values.

The files in the library are getting way to big. It is time to try to do something about that. And while we are at it we should probably come up with some better name and structure. This is just something I go and think about while doing other stuff so I have no proposal yet how to do it. But I am putting here so that we can start a discussion about it.

Some things that annoys me:

• The name utils (it awalys feels like this is a name used as placeholder for something better .. that never comes hehe).

Things that I would like to see:

• One thing I can say that personally like is that lib and mod files does not contain any code. They just contain documentation, settings, clippy-settings, use statements and mod statements.
• Maybe try to have a structure with names that resembles the specification? Tags, Tickets, e.t.c.

An implementation for the std::error::Error should be available for our Error type. Not having that makes it complicated to wrap or box our errors.

Look into all context methods and decide where improvements can be made to their signatures - either by grouping parameters/return values into a structure or making parameters more explicit (e.g. enums instead of bools).

ECDSA support should be added to the TransientKeyContext; given that the root key can be kept as RSA, we only need to deal with EC signing keys.

The context must thus support creating unrestricted signing keys with any elliptic curve and any supported signing scheme.

I noticed that when dropping the Context after calling tr_from_tpm_public() with a PersistentTpmHandle instance, I would get these kinds of errors and warnings:

[2020-11-20T21:06:15Z INFO  tss_esapi::context] Closing context.
[2020-11-20T21:06:15Z INFO  tss_esapi::context] Flushing handle 4293479
WARNING:esys:src/tss2-esys/api/Esys_FlushContext.c:234:Esys_FlushContext_Finish() Received TPM Error 
ERROR:esys:src/tss2-esys/api/Esys_FlushContext.c:89:Esys_FlushContext() Esys Finish ErrorCode (0x000001c4) 
[2020-11-20T21:06:15Z ERROR tss_esapi::context] Error in flushing context: value is out of range or is not correct for the context (associated with parameter number 1).
[2020-11-20T21:06:15Z ERROR tss_esapi::context] Error when dropping the context: value is out of range or is not correct for the context (associated with parameter number 1).
[2020-11-20T21:06:15Z INFO  tss_esapi::context] Context closed.

It seems that tr_from_tpm_public() adds the object handle it returns to the open_handles set, to automatically flush it from TPM memory upon dropping the Context instance. The TPM 2.0 spec says this about the FlushContext command:

This command causes all context associated with a loaded object, sequence object, or session to be
removed from TPM memory.

This command may not be used to remove a persistent object from the TPM.

So perhaps tr_from_tpm_public() should not add the object handle to the open_handles set if the input handle type is Persistent? I'm not sure if filtering out just the Persistent handles would cover all object types that shouldn't be flushed. Probably not, considering all the variants of the TpmHandle enum (shown below), but I don't know exactly which variants map to the "persistent objects" referenced in the spec.

pub enum TpmHandle {
    Pcr(PcrTpmHandle),
    NvIndex(NvIndexTpmHandle),
    HmacSession(HmacSessionTpmHandle),
    LoadedSession(LoadedSessionTpmHandle),
    PolicySession(PolicySessionTpmHandle),
    SavedSession(SavedSessionTpmHandle),
    Permanent(PermanentTpmHandle),
    Transient(TransientTpmHandle),
    Persistent(PersistentTpmHandle),
    AttachedComponent(AttachedComponentTpmHandle),
}