[nf,4/5] netfilter: nfnl_cthelper: fix memory leak when do update

Submitted by Liping Zhang on March 19, 2017, 2:36 p.m.

Details

Message ID 1489934162-7415-5-git-send-email-zlpnobody@163.com
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Liping Zhang March 19, 2017, 2:36 p.m.
From: Liping Zhang <zlpnobody@gmail.com>

When invoke nfnl_cthelper_update, we will malloc a new expect_policy,
then only point the helper->expect_policy to the new one but ignore
the old one, so it will be leaked forever.

Another issue is that the user can modify the expect_class_max to a
new value, for example, decrease the expect_class_max from 3 to 0.
Then, exp->class created by ctnetlink_alloc_expect may become invalid,
and out of bound access will happen.

So keep it simple, when update the cthelper, reject to modify the
expect_class_max, and we can modify the struct nf_conntrack_expect_policy
directly instead of allocing a new object, then memory leak does not
exist anymore.

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 net/netfilter/nfnetlink_cthelper.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

Comments

Pablo Neira March 21, 2017, 10:32 a.m.
On Sun, Mar 19, 2017 at 10:36:01PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> When invoke nfnl_cthelper_update, we will malloc a new expect_policy,
> then only point the helper->expect_policy to the new one but ignore
> the old one, so it will be leaked forever.
> 
> Another issue is that the user can modify the expect_class_max to a
> new value, for example, decrease the expect_class_max from 3 to 0.

If the code is allowing this, we should fix it since this is not
valid. We cannot change the number of classes once the helper has been
created.

Users may update the maximum number of expectations and its timeout
per policy, but not the number of classes once this has been created.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira March 21, 2017, 1:23 p.m.
On Tue, Mar 21, 2017 at 11:32:08AM +0100, Pablo Neira Ayuso wrote:
> On Sun, Mar 19, 2017 at 10:36:01PM +0800, Liping Zhang wrote:
> > From: Liping Zhang <zlpnobody@gmail.com>
> > 
> > When invoke nfnl_cthelper_update, we will malloc a new expect_policy,
> > then only point the helper->expect_policy to the new one but ignore
> > the old one, so it will be leaked forever.
> > 
> > Another issue is that the user can modify the expect_class_max to a
> > new value, for example, decrease the expect_class_max from 3 to 0.
> 
> If the code is allowing this, we should fix it since this is not
> valid. We cannot change the number of classes once the helper has been
> created.
> 
> Users may update the maximum number of expectations and its timeout
> per policy, but not the number of classes once this has been created.

Just sent a patch to sort out this.

You can rebase on top of nf.git as soon as I get those patches pushed
out, will wait a bit to wait for review and give it some testing here.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index cc70dd5..fc4733f 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -156,7 +156,7 @@  nfnl_cthelper_expect_policy_set[NFCTH_POLICY_SET_MAX+1] = {
 
 static int
 nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
-				  const struct nlattr *attr)
+				  const struct nlattr *attr, bool update)
 {
 	int i, ret;
 	struct nf_conntrack_expect_policy *expect_policy;
@@ -172,15 +172,23 @@  nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
 		return -EINVAL;
 
 	class_max = ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
-	if (class_max == 0)
-		return -EINVAL;
-	if (class_max > NF_CT_MAX_EXPECT_CLASSES)
-		return -EOVERFLOW;
 
-	expect_policy = kzalloc(sizeof(struct nf_conntrack_expect_policy) *
-				class_max, GFP_KERNEL);
-	if (expect_policy == NULL)
-		return -ENOMEM;
+	if (update) {
+		if (helper->expect_class_max + 1 != class_max)
+			return -EINVAL;
+
+		expect_policy = helper->expect_policy;
+	} else {
+		if (class_max == 0)
+			return -EINVAL;
+		if (class_max > NF_CT_MAX_EXPECT_CLASSES)
+			return -EOVERFLOW;
+
+		expect_policy = kcalloc(class_max, sizeof(*expect_policy),
+					GFP_KERNEL);
+		if (expect_policy == NULL)
+			return -ENOMEM;
+	}
 
 	for (i = 0; i < class_max; i++) {
 		if (!tb[NFCTH_POLICY_SET+i])
@@ -196,7 +204,8 @@  nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
 	helper->expect_policy = expect_policy;
 	return 0;
 err:
-	kfree(expect_policy);
+	if (!update)
+		kfree(expect_policy);
 	return -EINVAL;
 }
 
@@ -214,7 +223,8 @@  nfnl_cthelper_create(const struct nlattr * const tb[],
 	if (helper == NULL)
 		return -ENOMEM;
 
-	ret = nfnl_cthelper_parse_expect_policy(helper, tb[NFCTH_POLICY]);
+	ret = nfnl_cthelper_parse_expect_policy(helper, tb[NFCTH_POLICY],
+						false);
 	if (ret < 0)
 		goto err1;
 
@@ -269,7 +279,8 @@  nfnl_cthelper_update(const struct nlattr * const tb[],
 
 	if (tb[NFCTH_POLICY]) {
 		ret = nfnl_cthelper_parse_expect_policy(helper,
-							tb[NFCTH_POLICY]);
+							tb[NFCTH_POLICY],
+							true);
 		if (ret < 0)
 			return ret;
 	}