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

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

[Pengpeng Hou]

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.

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@vger.kernel.org

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 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;
+}
+
 #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;
+	}
+
 	/* 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;
+	}
 
 	spin_lock(&rx->lock);
 	if (unlikely(rx->skey || (key_gen == rx->key_gen && rx->key.keys))) {
-- 
2.50.1 (Apple Git-155)

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@vger.kernel.org
>
>Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
>---
> 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)
>

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

[Pengpeng Hou]

Hi Tung,

Thanks for the correction.

Agreed, I missed the TIPC-specific guarantee here, including that the
supported algorithm name is fixed to "gcm(aes)" and is NUL-terminated.
I'll drop this patch.

Thanks,
Pengpeng