Hi,
I have attached the follow up fix that checks for the proto index during conntrack creation.
Thanks, Will
On 07/31/2020, William Mcvicker wrote:
Hi Pablo,
Note that this code does not exist in the tree anymore. I'm not sure if this problem still exists upstream, this patch does not apply to nf.git. This fix should only go for -stable maintainers.
Right, the vulnerability has been fixed by the refactor commit fe2d0020994cd ("netfilter: nat: remove l4proto->in_range"), but this patch is a part of a full re-work of the code and doesn't backport very cleanly to the LTS branches. So this fix is only applicable to the 4.19, 4.14, 4.9, and 4.4 LTS branches. I missed the -stable email, but will re-add it to this thread with the re-worked patch.
Thanks, Will
On 07/31/2020, Pablo Neira Ayuso wrote:
Hi William,
On Fri, Jul 31, 2020 at 12:26:11AM +0000, William Mcvicker wrote:
Hi Pablo,
Yes, I believe this oops is only triggered by userspace when the user specifically passes in an invalid nf_nat_l3protos index. I'm happy to re-work the patch to check for this in ctnetlink_create_conntrack().
Great.
Note that this code does not exist in the tree anymore. I'm not sure if this problem still exists upstream, this patch does not apply to nf.git. This fix should only go for -stable maintainers.
BTW, do you have a Fixes: tag for this? This will be useful for -stable maintainer to pick up this fix.
Regarding the Fixes: tag, I don't have one offhand since this bug was reported to me, but I can search through the code history to find the commit that exposed this vulnerability.
That would be great.
Thank you.
Hi,
This patch is much smaller and if you confirm this is address the issue, then this is awesome.
On Mon, Aug 03, 2020 at 06:31:56PM +0000, William Mcvicker wrote: [...]
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c index 31fa94064a62..56d310f8b29a 100644 --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -1129,6 +1129,8 @@ ctnetlink_parse_tuple(const struct nlattr * const cda[], if (!tb[CTA_TUPLE_IP]) return -EINVAL;
- if (l3num >= NFPROTO_NUMPROTO)
return -EINVAL;
l3num can only be either NFPROTO_IPV4 or NFPROTO_IPV6.
Other than that, bail out with EOPNOTSUPP.
Thank you.
Hi Pablo,
This patch is much smaller and if you confirm this is address the issue, then this is awesome.
Yes, I can confirm the updated patch does fix the kernel panic. I have retested on the Pixel 4 XL with version 4.14.180. Please see the updated patchset v3.
Thanks, Will
Will McVicker (1): netfilter: nat: add a range check for l3/l4 protonum
net/netfilter/nf_conntrack_netlink.c | 2 ++ 1 file changed, 2 insertions(+)
The indexes to the nf_nat_l[34]protos arrays come from userspace. So check the tuple's family, e.g. l3num, when creating the conntrack in order to prevent an OOB memory access during setup. Here is an example kernel panic on 4.14.180 when userspace passes in an index greater than NFPROTO_NUMPROTO.
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP Modules linked in:... Process poc (pid: 5614, stack limit = 0x00000000a3933121) CPU: 4 PID: 5614 Comm: poc Tainted: G S W O 4.14.180-g051355490483 Hardware name: Qualcomm Technologies, Inc. SM8150 V2 PM8150 Google Inc. MSM task: 000000002a3dfffe task.stack: 00000000a3933121 pc : __cfi_check_fail+0x1c/0x24 lr : __cfi_check_fail+0x1c/0x24 ... Call trace: __cfi_check_fail+0x1c/0x24 name_to_dev_t+0x0/0x468 nfnetlink_parse_nat_setup+0x234/0x258 ctnetlink_parse_nat_setup+0x4c/0x228 ctnetlink_new_conntrack+0x590/0xc40 nfnetlink_rcv_msg+0x31c/0x4d4 netlink_rcv_skb+0x100/0x184 nfnetlink_rcv+0xf4/0x180 netlink_unicast+0x360/0x770 netlink_sendmsg+0x5a0/0x6a4 ___sys_sendmsg+0x314/0x46c SyS_sendmsg+0xb4/0x108 el0_svc_naked+0x34/0x38
Fixes: c1d10adb4a521 ("[NETFILTER]: Add ctnetlink port for nf_conntrack") Signed-off-by: Will McVicker willmcvicker@google.com --- net/netfilter/nf_conntrack_netlink.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c index 31fa94064a62..0b89609a6e9d 100644 --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -1129,6 +1129,8 @@ ctnetlink_parse_tuple(const struct nlattr * const cda[], if (!tb[CTA_TUPLE_IP]) return -EINVAL;
+ if (l3num != NFPROTO_IPV4 && l3num != NFPROTO_IPV6) + return -EOPNOTSUPP; tuple->src.l3num = l3num;
err = ctnetlink_parse_tuple_ip(tb[CTA_TUPLE_IP], tuple);
Hi Will,
Given this is for -stable maintainers only, I'd suggest:
1) Specify what -stable kernel versions this patch applies to. Explain that this problem is gone since what kernel version.
2) Maybe clarify that this is only for stable in the patch subject, e.g. [PATCH -stable v3] netfilter: nat: add a range check for l3/l4
Otherwise, this -stable maintainers might not identify this patch as something that is targetted to them.
Thanks.
On Mon, Aug 24, 2020 at 07:38:32PM +0000, Will McVicker wrote:
The indexes to the nf_nat_l[34]protos arrays come from userspace. So check the tuple's family, e.g. l3num, when creating the conntrack in order to prevent an OOB memory access during setup. Here is an example kernel panic on 4.14.180 when userspace passes in an index greater than NFPROTO_NUMPROTO.
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP Modules linked in:... Process poc (pid: 5614, stack limit = 0x00000000a3933121) CPU: 4 PID: 5614 Comm: poc Tainted: G S W O 4.14.180-g051355490483 Hardware name: Qualcomm Technologies, Inc. SM8150 V2 PM8150 Google Inc. MSM task: 000000002a3dfffe task.stack: 00000000a3933121 pc : __cfi_check_fail+0x1c/0x24 lr : __cfi_check_fail+0x1c/0x24 ... Call trace: __cfi_check_fail+0x1c/0x24 name_to_dev_t+0x0/0x468 nfnetlink_parse_nat_setup+0x234/0x258 ctnetlink_parse_nat_setup+0x4c/0x228 ctnetlink_new_conntrack+0x590/0xc40 nfnetlink_rcv_msg+0x31c/0x4d4 netlink_rcv_skb+0x100/0x184 nfnetlink_rcv+0xf4/0x180 netlink_unicast+0x360/0x770 netlink_sendmsg+0x5a0/0x6a4 ___sys_sendmsg+0x314/0x46c SyS_sendmsg+0xb4/0x108 el0_svc_naked+0x34/0x38
Fixes: c1d10adb4a521 ("[NETFILTER]: Add ctnetlink port for nf_conntrack") Signed-off-by: Will McVicker willmcvicker@google.com
net/netfilter/nf_conntrack_netlink.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c index 31fa94064a62..0b89609a6e9d 100644 --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -1129,6 +1129,8 @@ ctnetlink_parse_tuple(const struct nlattr * const cda[], if (!tb[CTA_TUPLE_IP]) return -EINVAL;
- if (l3num != NFPROTO_IPV4 && l3num != NFPROTO_IPV6)
return -EOPNOTSUPP;err = ctnetlink_parse_tuple_ip(tb[CTA_TUPLE_IP], tuple); -- 2.28.0.297.g1956fa8f8d-goog
Pablo Neira Ayuso pablo@netfilter.org wrote:
Hi Will,
Given this is for -stable maintainers only, I'd suggest:
Specify what -stable kernel versions this patch applies to. Explain that this problem is gone since what kernel version.
Maybe clarify that this is only for stable in the patch subject, e.g. [PATCH -stable v3] netfilter: nat: add a range check for l3/l4
Hmm, we silently accept a tuple that we can't really deal with, no?
- if (l3num != NFPROTO_IPV4 && l3num != NFPROTO_IPV6)
return -EOPNOTSUPP;
I vote to apply this to nf.git
On Fri, Aug 28, 2020 at 06:45:51PM +0200, Florian Westphal wrote:
Pablo Neira Ayuso pablo@netfilter.org wrote:
Hi Will,
Given this is for -stable maintainers only, I'd suggest:
Specify what -stable kernel versions this patch applies to. Explain that this problem is gone since what kernel version.
Maybe clarify that this is only for stable in the patch subject, e.g. [PATCH -stable v3] netfilter: nat: add a range check for l3/l4
Hmm, we silently accept a tuple that we can't really deal with, no?
Oh, I overlook, existing kernels are affected. You're right.
- if (l3num != NFPROTO_IPV4 && l3num != NFPROTO_IPV6)
return -EOPNOTSUPP;I vote to apply this to nf.git
I have rebased this patch on top of nf.git, attached what I'll apply to nf.git.
Hi Will, Pablo,
On Tue, Aug 04, 2020 at 01:37:11PM +0200, Pablo Neira Ayuso wrote:
This patch is much smaller and if you confirm this is address the issue, then this is awesome.
Did that ever get confirmed? AFAICT, nothing ended up landing in the stable trees for this.
Cheers,
Will
On Mon, Aug 03, 2020 at 06:31:56PM +0000, William Mcvicker wrote: [...]
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c index 31fa94064a62..56d310f8b29a 100644 --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -1129,6 +1129,8 @@ ctnetlink_parse_tuple(const struct nlattr * const cda[], if (!tb[CTA_TUPLE_IP]) return -EINVAL;
- if (l3num >= NFPROTO_NUMPROTO)
return -EINVAL;l3num can only be either NFPROTO_IPV4 or NFPROTO_IPV6.
Other than that, bail out with EOPNOTSUPP.
Thank you.
Hi Will,
Pablo is going to add the latest patch to the nf.git tree. Once that happens, I'm going to propose the patch in nf.git get cherry-picked to the -stable branches.
Thanks, Will
On Tue, Sep 1, 2020 at 8:36 AM Will Deacon will@kernel.org wrote:
Hi Will, Pablo,
On Tue, Aug 04, 2020 at 01:37:11PM +0200, Pablo Neira Ayuso wrote:
This patch is much smaller and if you confirm this is address the issue, then this is awesome.
Did that ever get confirmed? AFAICT, nothing ended up landing in the stable trees for this.
Cheers,
Will
On Mon, Aug 03, 2020 at 06:31:56PM +0000, William Mcvicker wrote: [...]
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c index 31fa94064a62..56d310f8b29a 100644 --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -1129,6 +1129,8 @@ ctnetlink_parse_tuple(const struct nlattr * const cda[], if (!tb[CTA_TUPLE_IP]) return -EINVAL;
- if (l3num >= NFPROTO_NUMPROTO)
return -EINVAL;l3num can only be either NFPROTO_IPV4 or NFPROTO_IPV6.
Other than that, bail out with EOPNOTSUPP.
Thank you.
linux-stable-mirror@lists.linaro.org
-
Florian Westphal -
Pablo Neira Ayuso -
Will Deacon -
Will McVicker -
William Mcvicker