Re: [PATCH v6 18/20] KEYS: trusted: Add session encryption protection to the seal/unseal path

["Jarkko Sakkinen" <jarkko@kernel.org>]

On Tue Jan 2, 2024 at 7:04 PM EET, James Bottomley wrote:
> If some entity is snooping the TPM bus, the can see the data going in
> to be sealed and the data coming out as it is unsealed.  Add parameter
> and response encryption to these cases to ensure that no secrets are
> leaked even if the bus is snooped.
>
> As part of doing this conversion it was discovered that policy
> sessions can't work with HMAC protected authority because of missing
> pieces (the tpm Nonce).  I've added code to work the same way as
> before, which will result in potential authority exposure (while still
> adding security for the command and the returned blob), and a fixme to
> redo the API to get rid of this security hole.
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>
> ---
>
> v2: fix unseal with policy and password
> v3: fix session memory leak
> ---
>  security/keys/trusted-keys/trusted_tpm2.c | 88 ++++++++++++++++-------
>  1 file changed, 61 insertions(+), 27 deletions(-)
>
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 97b1dfca2dba..dfeec06301ce 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -253,26 +253,26 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
>  	if (rc)
>  		return rc;
>  
> +	rc = tpm2_start_auth_session(chip);
> +	if (rc)
> +		goto out_put;
> +
>  	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
>  	if (rc) {
> -		tpm_put_ops(chip);
> -		return rc;
> +		tpm2_end_auth_session(chip);
> +		goto out_put;
>  	}
>  
>  	rc = tpm_buf_init_sized(&sized);
>  	if (rc) {
>  		tpm_buf_destroy(&buf);
> -		tpm_put_ops(chip);
> -		return rc;
> +		tpm2_end_auth_session(chip);
> +		goto out_put;
>  	}
>  
> -	tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
> -	tpm_buf_append_u32(&buf, options->keyhandle);
> -	tpm2_buf_append_auth(&buf, TPM2_RS_PW,
> -			     NULL /* nonce */, 0,
> -			     0 /* session_attributes */,
> -			     options->keyauth /* hmac */,
> -			     TPM_DIGEST_SIZE);
> +	tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
> +	tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_DECRYPT,
> +				    options->keyauth, TPM_DIGEST_SIZE);
>  
>  	/* sensitive */
>  	tpm_buf_append_u16(&sized, options->blobauth_len);
> @@ -314,10 +314,13 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
>  
>  	if (buf.flags & TPM_BUF_OVERFLOW) {
>  		rc = -E2BIG;
> +		tpm2_end_auth_session(chip);
>  		goto out;
>  	}
>  
> +	tpm_buf_fill_hmac_session(chip, &buf);
>  	rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
> +	rc = tpm_buf_check_hmac_response(chip, &buf, rc);
>  	if (rc)
>  		goto out;
>  
> @@ -348,6 +351,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
>  	else
>  		payload->blob_len = blob_len;
>  
> +out_put:
>  	tpm_put_ops(chip);
>  	return rc;
>  }
> @@ -417,25 +421,31 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
>  	if (blob_len > payload->blob_len)
>  		return -E2BIG;
>  
> -	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD);
> +	rc = tpm2_start_auth_session(chip);
>  	if (rc)
>  		return rc;
>  
> -	tpm_buf_append_u32(&buf, options->keyhandle);
> -	tpm2_buf_append_auth(&buf, TPM2_RS_PW,
> -			     NULL /* nonce */, 0,
> -			     0 /* session_attributes */,
> -			     options->keyauth /* hmac */,
> -			     TPM_DIGEST_SIZE);
> +	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD);
> +	if (rc) {
> +		tpm2_end_auth_session(chip);
> +		return rc;
> +	}
> +
> +	tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
> +	tpm_buf_append_hmac_session(chip, &buf, 0, options->keyauth,
> +				    TPM_DIGEST_SIZE);
>  
>  	tpm_buf_append(&buf, blob, blob_len);
>  
>  	if (buf.flags & TPM_BUF_OVERFLOW) {
>  		rc = -E2BIG;
> +		tpm2_end_auth_session(chip);
>  		goto out;
>  	}
>  
> +	tpm_buf_fill_hmac_session(chip, &buf);
>  	rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
> +	rc = tpm_buf_check_hmac_response(chip, &buf, rc);
>  	if (!rc)
>  		*blob_handle = be32_to_cpup(
>  			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
> @@ -473,20 +483,44 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
>  	u8 *data;
>  	int rc;
>  
> -	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_UNSEAL);
> +	rc = tpm2_start_auth_session(chip);
>  	if (rc)
>  		return rc;
>  
> -	tpm_buf_append_u32(&buf, blob_handle);
> -	tpm2_buf_append_auth(&buf,
> -			     options->policyhandle ?
> -			     options->policyhandle : TPM2_RS_PW,
> -			     NULL /* nonce */, 0,
> -			     TPM2_SA_CONTINUE_SESSION,
> -			     options->blobauth /* hmac */,
> -			     options->blobauth_len);
> +	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_UNSEAL);
> +	if (rc) {
> +		tpm2_end_auth_session(chip);
> +		return rc;
> +	}
> +
> +	tpm_buf_append_name(chip, &buf, blob_handle, NULL);
> +
> +	if (!options->policyhandle) {
> +		tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT,
> +					    options->blobauth,
> +					    options->blobauth_len);
> +	} else {
> +		/*
> +		 * FIXME: The policy session was generated outside the
> +		 * kernel so we don't known the nonce and thus can't
> +		 * calculate a HMAC on it.  Therefore, the user can
> +		 * only really use TPM2_PolicyPassword and we must
> +		 * send down the plain text password, which could be
> +		 * intercepted.  We can still encrypt the returned
> +		 * key, but that's small comfort since the interposer
> +		 * could repeat our actions with the exfiltrated
> +		 * password.
> +		 */
> +		tpm2_buf_append_auth(&buf, options->policyhandle,
> +				     NULL /* nonce */, 0, 0,
> +				     options->blobauth, options->blobauth_len);
> +		tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT,
> +						NULL, 0);
> +	}
>  
> +	tpm_buf_fill_hmac_session(chip, &buf);
>  	rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
> +	rc = tpm_buf_check_hmac_response(chip, &buf, rc);
>  	if (rc > 0)
>  		rc = -EPERM;
>  

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko