nationalsecurityagency / kmyth Goto Github PK

Unit tests must be written and integrated into the kmyth unit testing framework for the kmyth PCR utility functionality implemented in tpm2/src/tpm/pcrs.c.

The kmyth sgx Makefile needs an install option that will install the shared objects and header alongside the tpm2 install. It also needs an uninstall to remove only the sgx portions.

When using the Microsoft TPM simulator, kmyth applications classify it as a hardware TPM. As the current check examines the vendor string and tests whether or not it is "IBM", this is not surprising. The check, however, should probably also include the Microsoft simulator (and any others, if possible).

Kmyth does not have a make associated with it's sgx source code, but it does provide a build in its test code. Instead of writing code that's partially a demo and partially unit tests, we should separate these into two distinct paths. The demo code should demonstrate how to use the enclave and build it alongside existing enclave code, and the test can focus on testing the local build.

Attempting to unseal a ski file gives an error:

$ ./kmyth-unseal -i out.ski -o out
kmyth-0.0.0 INFO - src/util/tpm2_config_tools.c(tpm2_init_connection:82) initialized connection to TPM 2.0 emulator
kmyth-0.0.0 ERROR - src/util/tpm2_kmyth_io.c(kmyth_getSkiBlock:929) unexpected delimiter ... exiting
kmyth-0.0.0 ERROR - src/util/tpm2_kmyth_io.c(tpm2_kmyth_read_ski_file:510) get PCR selection list error ... exiting
kmyth-0.0.0 ERROR - src/util/tpm2_kmyth_seal.c(tpm2_kmyth_unseal:402) error reading .ski file out.ski ... exiting
kmyth-0.0.0 ERROR - src/unseal/main.c(main:136) kmyth-unseal failed ... exiting

The marshalling functions have been moved from the formatting tools functions so the tests should also be moved.

The original version of kmyth-getkey demonstrated key retrieval from a generic TLS server. When a version of kmyth-getkey that incorporated libkmip library functionality to demonstrate a Kmyth client that retrieves keys from a KMIP server was developed, it replaced the original kmyth-getkey version.

As we would like to retain the functionality of both versions, this issue more specifically proposes modification of kmyth-getkey to support both (either of) "get_key_from_kmip_server()" and "get_key_from_tls_server()" options. Further, selection of which option to use should be via a required command line parameter.

Unit tests must be written and integrated into the kmyth unit testing framework for the kmyth data formatting functionality implemented in tpm2/src/tpm/formatting_tools.c.

As a step to make the code and build process more modular and portable, it will probably help to revamp the current single, top-level makefile approach by instead applying a hierarchical (recursive) makefile schema.

In the unit tests, create a test to verify that a Policy OR digest works with both TPM conditions.

Unit tests must be written and integrated into the kmyth unit testing framework for the kmyth implementation of the AES GCM cipher (encrypt/decrypt) in tpm2/src/cipher/aes_gcm.c.

Unit tests must be written and integrated into the kmyth unit testing framework for the TPM 2.0 based kmyth seal and unseal functionality implemented in tpm2/src/tpm/kmyth_seal_unseal_impl.c.

The function get_srk_handle() currently gets a list of all objects in TPM persistent storage. It then has a for loop that calls check_if_srk() on each of those objects. As soon as it comes across an object in that list that passes all of the criteria tested in check_if_srk() (i.e., check_if_srk() sets the isSRK boolean to true), it sets srk_handle to that handle value and breaks out of the for loop. Since srk_handle will be non-zero in this case, get_srk_handle() will just exit normally at this point with the srk_handle parameter available to the calling function. If the for loop exits after check_if_srk() has failed on all loaded objects, srk_handle will have never been changed from its zero initialization. In this case, derive_srk() will be called to re-derive the SRK from its seed and load it into persistent TPM storage. In this case derive_srk() provides the handle referencing the SRK in TPM storage to get_srk_handle() so that it can provide it to its calling function.

While the logic in get_srk_handle() seems fine, this issue attempts to make the function more readable and the code a little more modular. After discussion about this, a decision to simplify get_srk_handle() by abstracting it into the following two function calls, seemed the best way to achieve this:

• get_existing_srk_handle() gets the list of objects in TPM persistent storage and checks them all against the SRK criteria. Upon return from this function, the srk_handle parameter passed to it is either zero (none of the loaded objects is the SRK) or contains the handle used to reference the SRK.
• put_srk_into_tpm_persistent_storage() is a renamed derive_srk() function.

The new get_srk_handle() would call get_existing_srk_handle(). It would then check the srk_handle parameter resulting from this call and call put_srk_into_tpm_persistent_storage() if srk_handle is still zero (i.e., wasn't found by get_existing_srk_handle()).

Attempting to run kmyth-seal without starting tabrmd results in a seg fault.

$ ./bin/kmyth-seal -i test.txt -o test.ski

** (process:3110): CRITICAL **: 09:37:15.882: failed to allocate dbus proxy object: Error calling StartServiceByName for com.intel.tss2.Tabrmd: Timeout was reached
kmyth-0.0.0 ERROR - src/util/tpm2_config_tools.c(tpm2_init_tcti_abrmd:145) Tss2_Tcti_Tabrmd_Init(): rc = 0x000A0008, tcti:Fails to connect to next lower layer
kmyth-0.0.0 ERROR - src/util/tpm2_config_tools.c(tpm2_init_tcti_abrmd:148) failed to initialize resource manager TCTI context ... exiting
kmyth-0.0.0 ERROR - src/util/tpm2_config_tools.c(tpm2_init_connection:37) unable to initialize TCTI context ... exiting
kmyth-0.0.0 ERROR - src/util/tpm2_kmyth_seal.c(tpm2_kmyth_seal:44) unable to init conn to TPM2 res mgr, check that it's running
kmyth-0.0.0 ERROR - src/util/tpm2_info_tools.c(tpm2_get_properties:44) Tss2_Get_Capability(): rc = 0x00080005, sys:A pointer is NULL that isn't allowed to be NULL.
kmyth-0.0.0 ERROR - src/util/tpm2_info_tools.c(tpm2_get_properties:46) unable to get capability = 1, property = 33554432, count = 273 ... exiting
Segmentation fault (core dumped)

I have no clue what this repo is, I'm just browsing stuff bored but you're derefing a raw pointer in enclave.cpp twice without a null check.

The kmyth applications accept as command line parameters an authorization password (used to create a hash used as an authorization value in a TPM policy) and a password for the TPM owner (storage) hierarchy. They are currently initialized differently. The authString variable is initialized to a NULL pointer and the ownerAuthPasswd variable is initialized to an empty string. This issue proposes to standardize on the approach where we initialize to the NULL pointer and then use a check for NULL and set to a default value, where required.

The following was successfully executed even though it contains characters that should not be parseable:

$ kmyth-seal -i test.txt -p "0, 1panda, 2" -o test.ski

The following errored out correctly:

$ kmyth-seal -i test.txt -p "0, 1, 2, panda" -o test.ski

kmyth-0.0.0 ERROR - src/util/tpm2_pcrManagement.c(init_pcr_selection:113) error parsing PCR value ... exiting
kmyth-0.0.0 ERROR - src/util/tpm2_kmyth_seal.c(tpm2_kmyth_seal:112) error parsing PCR string: 0, 1, 2, panda ... exiting
kmyth-0.0.0 ERROR - src/util/tpm2_kmyth_seal.c(tpm2_kmyth_seal_file:446) Failed to kmyth-seal data ... exiting
kmyth-0.0.0 ERROR - src/main/seal.c(main:238) kmyth-seal error ... exiting

Unit tests must be written and integrated into the kmyth unit testing framework for the top-level (main()) implementation of the TPM 2.0 based kmyth-unseal application in tpm2/src/main/unseal.c.

Unit tests must be written and integrated into the kmyth unit testing framework for the kmyth implementation of the AES Key Wrap with padding (RFC 5649) cipher in tpm2/src/cipher/aes_keywrap_5649pad.c.

We will be re-naming our default branch from master to main. GitHub has published guidance on this topic.

New tools have been promised to make this more seamless, but, in our case, we should be in pretty good shape to go ahead and switch over. For instance, as of July 17, 2020, links to deleted branched will now automatically redirect to the default branch. The biggest challenge looks to be the current need to manually re-target open pull requests to the new default branch. For this reason, we may schedule the change for either a time in the near future when this is minimized (e.g., no or few open pull requests) or when the promised automated tooling becomes available to deal with this.

Unit tests must be written and integrated into the kmyth unit testing framework for the kmyth cipher-related utility functionality implemented in tpm2/src/cipher/cipher.c.

The current API for calling the 'retrieve-key' functionality from within the enclave needs modification to support its use beyond the current demo. The following initial issues have been identified:

• The 'key ID' parameter passed to the server is currently generated within the function. This needs to be exposed to the API to permit the caller of the enclave_retrieve_key() to specify this value
• The returned key object is not exposed to the caller. The API needs to provide a reference to this value to the caller so that it has access to it.
• The port value is specified as a string, should pass this as an integer rather than doing the string conversion as part of the function.

Unit tests must be written and integrated into the kmyth unit testing framework for the top-level (main()) implementation of the TPM 2.0 based kmyth-getkey application in tpm2/src/main/getkey.c.

As part of the Feature/add policy or for authorization pull request (PR #166), a kmyth-reseal application was added. While the intent was to simplify the workflow for resealing an existing kmyth-sealed result (.ski file) requiring either the original or a new TPM policy digest criteria (logical OR of the two), the kmyth-reseal application does not yet do this.

The version currently implemented is a redundant implementation of kmyth-seal with the -g option invoked, which simply prints out the policy digest that results from the authorization criteria specified using the current machine configuration. While this is not the complete functionality of the envisioned kmyth-reseal application, we decided not to address this as part of PR #166 and instead with a future revision.

The kmyth-reseal utility should instead:

1. kmyth-unseal a previously sealed .ski file result. The filename for this input should be specified as a required command line option.

2. kmyth-seal the data unsealed in the first step. The filename for the output result should be specified as a required command line option. The authorization policy applied should be a "policy-OR" criteria satisfying either of the following two conditions:

   • The criteria based on the command line options specified (e.g., PCRs, etc) and the current system state.
   • The criteria based on the command line options and system state that results in the policy digest specified as a required command line option (Note: the -g option for the kmyth-seal application can be used to obtain this required input policy digest parameter)

Currently, for the kmyth functionality implemented in tpm2/src/util/tls_util.c, the only unit tests that have been written are some simple checks for invalid inputs to tls_set_context(). Additional unit tests, therefore, must be written and integrated into the kmyth unit testing framework for the TLS utility functions found in tpm2/src/util/tls_util.c.

There are quite a few places in the code where memory isn't freed on failure, or where failure conditions aren't properly handled. We need to go through the entire codebase ensuring all errors are handling appropriately and all memory is freed appropriately.

As part of the Feature/add policy or for authorization pull request (PR #166), an option to print out (get) the policy digest was added. The application terminates normally after printing this result but also produces an empty .ski output file. In this case, it would be cleaner to perform this new functionality without producing the empty output file (.ski) result.

The last few error return cases in read_bytes_from_file() can return after memory has been allocated for for the output data buffer. It would be cleaner to not require the caller to free memory allocated by this function after an unsuccessful return.

Unit tests must be written and integrated into the kmyth unit testing framework for the miscellaneous set of kmyth utilities that facilitate interfacing with the TPM 2.0 for initialization and startup through maintaining the session and querying the TPM. More specifically, tests need to be added for the functionality implemented in tpm2/src/tpm/tpm2_interface.c.

In tpm2_kmyth_create_authVal the final log call uses the value authValOut[authValOut->size-1] when it should use authValOut->buffer[authValOut->size-1]

This function currently initializes the isSRK boolean parameter to true and then changes it to false if any of the SRK checks fail. Although early termination of this function should produce an error, it will also leave this variable in the state indicating that the SRK checks passed. It would be safer to avoid exiting the function with this parameter set to an improperly true result.

Unit tests must be written and integrated into the kmyth unit testing framework for the kmyth TPM object tool functionality implemented in tpm2/src/tpm/object_tools.c.

At the moment we have no way to recognize that a file has been SGX-sealed like we do with the .ski formatting for TPM-sealed files. We should add an additional file type (maybe .nkl) along with a header/footer to identify and help parse these SGX-sealed files.

The Feature/add policy or for authorization pull request (PR #166) added the capability to TPM seal data that can be TPM unsealed if the policy digest matches one of two possible criteria (policy-OR). When unsealing, however, the user must currently specify the -p (policy OR) flag. As the existence of a policy OR criteria should be easily detectable when parsing/reading the input .ski file, however, the user should not have to specify this as a command line option. Instead, kmyth-unseal should just apply whatever authorization criteria is specified in the .ski file, including a policy OR criteria.

With the new build structure in place the README and INSTALL files need to be updated to keep pace. In particular

• you no longer have to separately install the logger
• indent is EPEL for CentOS8, so we have to figure out what to do with that

There may be other inconsistencies, those are just the two I noticed.

When running kmyth-seal, the following error message is being observed:

kmyth-0.0.0 ERROR - src/util/tpm2_kmyth_object.c(tpm2_kmyth_create_object:353) Tss2_Sys_CreatePrimary(): rc = 0x00090006, mu:A buffer isn't large enough ... exiting

Enabling more detailed logging in TSS reveals the underlying marshaling error:

debug:marshal:src/tss2-mu/tpm2b-types.c:302:Tss2_MU_TPM2B_DIGEST_Marshal() buffer_size: 4096 with offset: 30 are insufficient for object of size 32769

The compute_ecdh_shared_secret() function in sgx/common/src/ecdh_utils.c uses OpenSSL's ECDH_compute_key() call to produce a 'shared secret' result. The last parameter seems to allow the specification of a KDF. This might, therefore, support direct use of this result as a "session key", instead of a two-step (compute ECDH shared secret and then use shared secret to compute session key) approach.

Unit tests must be written and integrated into the kmyth unit testing framework for the top-level (main()) implementation of the TPM 2.0 based kmyth-seal application in tpm2/src/main/seal.c.

The auth_string, pcrs_string, owner_auth_password, and cipher_string are passed in and out of several calls. These should be consolidated into a single struct to be passed in and out of tpm2_kmyth_seal and tpm2_kmyth_unseal

The duplicate enclave build commands in sgx/demo/Makefile and sgx/test/Makefile are annoying to keep up to date.
This issue recommends potentially merging them to avoid the issues with maintaining duplicated build specifications.

Kmyth includes functionality for retrieving a key from a remote server (e.g., a KMIP server) into the enclave. Although, for the connection to the the key server, TLS is appropriate, in order to avoid potential licensing issues with our open source code, we did not implement a TLS client within the enclave. Instead, we implemented a TLS proxy, in untrusted code, which might reside, say, in environment that provides some level of enhanced protection. To demonstrate that the data between the enclave and the proxy can be secured, we initially implemented a trivial protocol based on an ECDH key agreement scheme.

While this was always intended as demonstration, and never as production code, we plan to replace the existing protocol with a slightly less-trivial example to demonstrate that a more complex protocol can terminate inside the enclave:

1. The client sends a "client hello" message containing the client's identify and the client's ephemeral public key, signed by the client's long-term signing key.

2. The server sends a "server hello" message containing the server's identity, the server's ephemeral public key, and the client's ephemeral public key, signed by the server's long-term signing key.

3. Client and server derive the ECDH ephemeral key as a shared secret and run this through a key derivation function (KDF) using the two 'Hello' messages and the two public ephemerals as "additional info" to produce a 512 bit result.

4. The client sends a "request" message containing the currently implemented request, along with the servers ephemeral public key. This message is encrypted with the first 256 bits of the key derived in (3) and the ciphertext is signed by the client's long-term signing key.

5. The server sends the currently implemented "response" message, encrypted with the next 256 bits of the key derived in (3). The ciphertext is signed by the server's long-term signing key.

While it contains useful information, the prefix that precedes each log entry can be distracting when monitoring normal activity via stddest (stdout / stderr). If it were more configurable, the user would have the option to choose between the current full information view or a streamlined (less cluttered) log entry view.

The initial idea is to always include the prefix when entries are logged to file. When logged to screen, however, if the logging severity level is LOG_INFO or higher, however, logging to screen would omit the prefix. If the logging severity level is lower (e.g., LOG_DEBUG), however, the prefix would be included. The user would then, for normal operation, be presented simplified log entries, but would be able to obtain the full log entries (prefix included) by selecting the verbose logging option.

Trying to seal an empty input file results in:

touch test.in
./bin/kmyth-seal -i test.in -o test.ski -v

kmyth-0.0.0 DEBUG - src/util/tpm2_config_tools.c(tpm2_init_sapi:162) ABI version is 1.2.1.108
kmyth-0.0.0 DEBUG - src/util/tpm2_config_tools.c(tpm2_init_sapi:195) initialized SAPI context
.
.
.
kmyth-0.0.0 DEBUG - src/util/aes_gcm.c(aes_gcm_encrypt:23) AES/GCM encryption starting
kmyth-0.0.0 ERROR - src/util/aes_gcm.c(aes_gcm_encrypt:35) no input data ... exiting
kmyth-0.0.0 ERROR - src/util/tpm2_kmyth_seal.c(kmyth_encrypt_data:717) error encrypting data ... exiting
kmyth-0.0.0 ERROR - src/util/tpm2_kmyth_seal.c(kmyth_wrap_input:675) unable to encrypt (wrap) data ... exiting
kmyth-0.0.0 ERROR - src/util/tpm2_kmyth_seal.c(tpm2_kmyth_seal:246) kmyth_wrap_input() call failed ... exiting
double free or corruption (out)
Aborted (core dumped)

Generate the function and code logic needed to do enclave to enclave attestation.

The function create_sk() in src/tpm/storage_key_tools.c, takes a storage key handle (sk_handle) as a parameter, but then does not use this parameter anywhere within the function. This interface, therefore, should be simplified and cleaned up to remove this unnecessary parameter.

In pull request #55, we refactored the naming and organization of source and header files. As a follow-on, we should probably review, and refactor if/where appropriate, function names to be more consistent with these changes. As an example, we have a lot of function names that include tpm2 and/or kmyth in their naming. In some cases, this may be helpful or add clarity (e.g., to distinguish them from similarly named functionality in the TSS). In some cases, however, I think some cleanup may help with the readability of the code.

Unit tests must be written and integrated into the kmyth unit testing framework for the kmyth implementation of the AES Key Wrap with no padding (RFC 3394) cipher in tpm2/src/cipher/aes_keywrap_3394nopad.c.

Unit tests must be written and integrated into the kmyth unit testing framework for the kmyth utility functions that manage and interact with TPM keys (i.e., storage root key and storage keys). More specifically, add tests for the functionality implemented in tpm2/src/tpm/storage_key_tools.c.