RE: [PATCH] tipc: crypto: require a NUL-terminated AEAD algorithm name

[Tung Quang Nguyen]

>Subject: [PATCH] tipc: crypto: require a NUL-terminated AEAD algorithm name
>
>struct tipc_aead_key carries alg_name in a fixed 32-byte field, but both the
>generic netlink validation path and the MSG_CRYPTO receive path pass that
>field straight to crypto_has_alg(), strcmp(), and
>crypto_alloc_aead() without first proving that it contains a terminating NUL.
>
This is not correct. TIPC guarantees the algorithm string is nul-terminated one.
>Reject locally supplied and received keys whose algorithm name fills the entire
>fixed-width field without a terminator.
>
>Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")
>Cc: stable@xxxxxxxxxxxxxxx
>
>Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
>---
> net/tipc/crypto.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
>diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c index
>6d3b6b89b1d1..60110ea0fe7c 100644
>--- a/net/tipc/crypto.c
>+++ b/net/tipc/crypto.c
>@@ -307,6 +307,11 @@ static void tipc_crypto_work_tx(struct work_struct
>*work);  static void tipc_crypto_work_rx(struct work_struct *work);  static int
>tipc_aead_key_generate(struct tipc_aead_key *skey);
>
>+static bool tipc_aead_alg_name_valid(const char *alg_name) {
>+	return strnlen(alg_name, TIPC_AEAD_ALG_NAME) <
>TIPC_AEAD_ALG_NAME; }
>+
This is not needed because TIPC only supports one algorithm name "gcm(aes)" which is 8-byte length.
> #define is_tx(crypto) (!(crypto)->node)  #define is_rx(crypto) (!is_tx(crypto))
>
>@@ -335,6 +340,11 @@ int tipc_aead_key_validate(struct tipc_aead_key
>*ukey, struct genl_info *info)  {
> 	int keylen;
>
>+	if (unlikely(!tipc_aead_alg_name_valid(ukey->alg_name))) {
>+		GENL_SET_ERR_MSG(info, "algorithm name is not NUL-
>terminated");
>+		return -EINVAL;
>+	}
>+
This is not needed because the system guarantees that the string passed from user-space is nul-terminated one.
> 	/* Check if algorithm exists */
> 	if (unlikely(!crypto_has_alg(ukey->alg_name, 0, 0))) {
> 		GENL_SET_ERR_MSG(info, "unable to load the algorithm
>(module existed?)"); @@ -2298,6 +2308,10 @@ static bool
>tipc_crypto_key_rcv(struct tipc_crypto *rx, struct tipc_msg *hdr)
> 		pr_debug("%s: invalid MSG_CRYPTO key size\n", rx->name);
> 		goto exit;
> 	}
>+	if (unlikely(!tipc_aead_alg_name_valid(data))) {
>+		pr_debug("%s: invalid MSG_CRYPTO algorithm name\n", rx-
>>name);
>+		goto exit;
>+	}
This is not needed as explained above.
>
> 	spin_lock(&rx->lock);
> 	if (unlikely(rx->skey || (key_gen == rx->key_gen && rx->key.keys))) {
>--
>2.50.1 (Apple Git-155)
>