diff mbox series

[v7,18/21] KEYS: trusted: Add session encryption protection to the seal/unseal path

Message ID 20240213171334.30479-19-James.Bottomley@HansenPartnership.com (mailing list archive)
State New, archived
Headers show
Series add integrity and security to TPM2 transactions | expand

Commit Message

James Bottomley Feb. 13, 2024, 5:13 p.m. UTC 
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>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

---
v2: fix unseal with policy and password
v3: fix session memory leak
v7: add review
---
 security/keys/trusted-keys/trusted_tpm2.c | 88 ++++++++++++++++-------
 1 file changed, 61 insertions(+), 27 deletions(-)

Comments

Jarkko Sakkinen Feb. 23, 2024, 5:11 p.m. UTC | #1 
On Tue Feb 13, 2024 at 7:13 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>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> ---
> v2: fix unseal with policy and password
> v3: fix session memory leak
> v7: add review
> ---
>  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;
>  


ditto

BR, Jarkko
diff mbox series

Patch

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;