Re: [PATCH 1/1] netfilter: nat: add range checks for access to nf_nat_l[34]protos[]
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 +, 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.
Re: [PATCH 1/1] netfilter: nat: add range checks for access to nf_nat_l[34]protos[]
Hi William, On Fri, Jul 31, 2020 at 12:26:11AM +, 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.
Re: [PATCH 1/1] netfilter: nat: add range checks for access to nf_nat_l[34]protos[]
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(). > 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. Thanks, Will On 07/29/2020, Pablo Neira Ayuso wrote: > Hi Will, > > On Mon, Jul 27, 2020 at 05:57:20PM +, Will McVicker wrote: > > The indexes to the nf_nat_l[34]protos arrays come from userspace. So we > > need to make sure that before indexing the arrays, we verify the index > > is within the array bounds in order to prevent an OOB memory access. > > 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 = 0xa3933121) > > CPU: 4 PID: 5614 Comm: poc Tainted: G S W O4.14.180-g051355490483 > > Hardware name: Qualcomm Technologies, Inc. SM8150 V2 PM8150 Google Inc. MSM > > task: 2a3dfffe task.stack: a3933121 > > 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 > > If this oops is only triggerable from userspace, I think a sanity > check in nfnetlink_parse_nat_setup should suffice to reject > unsupported layer 3 and layer 4 protocols. > > I mean, in this patch I see more chunks in the packet path, such as > nf_nat_l3proto_ipv4 that should never happen. I would just fix the > userspace ctnetlink path. > > BTW, do you have a Fixes: tag for this? This will be useful for > -stable maintainer to pick up this fix. > > Thanks.
Re: [PATCH 1/1] netfilter: nat: add range checks for access to nf_nat_l[34]protos[]
Hi Will, On Mon, Jul 27, 2020 at 05:57:20PM +, Will McVicker wrote: > The indexes to the nf_nat_l[34]protos arrays come from userspace. So we > need to make sure that before indexing the arrays, we verify the index > is within the array bounds in order to prevent an OOB memory access. > 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 = 0xa3933121) > CPU: 4 PID: 5614 Comm: poc Tainted: G S W O4.14.180-g051355490483 > Hardware name: Qualcomm Technologies, Inc. SM8150 V2 PM8150 Google Inc. MSM > task: 2a3dfffe task.stack: a3933121 > 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 If this oops is only triggerable from userspace, I think a sanity check in nfnetlink_parse_nat_setup should suffice to reject unsupported layer 3 and layer 4 protocols. I mean, in this patch I see more chunks in the packet path, such as nf_nat_l3proto_ipv4 that should never happen. I would just fix the userspace ctnetlink path. BTW, do you have a Fixes: tag for this? This will be useful for -stable maintainer to pick up this fix. Thanks.
[PATCH 1/1] netfilter: nat: add range checks for access to nf_nat_l[34]protos[]
The indexes to the nf_nat_l[34]protos arrays come from userspace. So we
need to make sure that before indexing the arrays, we verify the index
is within the array bounds in order to prevent an OOB memory access.
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 = 0xa3933121)
CPU: 4 PID: 5614 Comm: poc Tainted: G S W O4.14.180-g051355490483
Hardware name: Qualcomm Technologies, Inc. SM8150 V2 PM8150 Google Inc. MSM
task: 2a3dfffe task.stack: a3933121
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
Signed-off-by: Will McVicker
---
net/ipv4/netfilter/nf_nat_l3proto_ipv4.c | 6 --
net/ipv6/netfilter/nf_nat_l3proto_ipv6.c | 5 +++--
net/netfilter/nf_nat_core.c | 27 ++--
net/netfilter/nf_nat_helper.c| 4
4 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c
b/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c
index 6115bf1ff6f0..1912fc66147c 100644
--- a/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c
@@ -218,7 +218,8 @@ int nf_nat_icmp_reply_translation(struct sk_buff *skb,
return 1;
l4proto = __nf_nat_l4proto_find(NFPROTO_IPV4, inside->ip.protocol);
- if (!nf_nat_ipv4_manip_pkt(skb, hdrlen + sizeof(inside->icmp),
+ if (!l4proto ||
+ !nf_nat_ipv4_manip_pkt(skb, hdrlen + sizeof(inside->icmp),
l4proto, &ct->tuplehash[!dir].tuple, !manip))
return 0;
@@ -234,7 +235,8 @@ int nf_nat_icmp_reply_translation(struct sk_buff *skb,
/* Change outer to look like the reply to an incoming packet */
nf_ct_invert_tuplepr(&target, &ct->tuplehash[!dir].tuple);
l4proto = __nf_nat_l4proto_find(NFPROTO_IPV4, 0);
- if (!nf_nat_ipv4_manip_pkt(skb, 0, l4proto, &target, manip))
+ if (!l4proto ||
+ !nf_nat_ipv4_manip_pkt(skb, 0, l4proto, &target, manip))
return 0;
return 1;
diff --git a/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
b/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
index ca6d38698b1a..a72840baf27b 100644
--- a/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
+++ b/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
@@ -228,7 +228,8 @@ int nf_nat_icmpv6_reply_translation(struct sk_buff *skb,
return 1;
l4proto = __nf_nat_l4proto_find(NFPROTO_IPV6, inside->ip6.nexthdr);
- if (!nf_nat_ipv6_manip_pkt(skb, hdrlen + sizeof(inside->icmp6),
+ if (!l4proto ||
+ !nf_nat_ipv6_manip_pkt(skb, hdrlen + sizeof(inside->icmp6),
l4proto, &ct->tuplehash[!dir].tuple, !manip))
return 0;
@@ -245,7 +246,7 @@ int nf_nat_icmpv6_reply_translation(struct sk_buff *skb,
nf_ct_invert_tuplepr(&target, &ct->tuplehash[!dir].tuple);
l4proto = __nf_nat_l4proto_find(NFPROTO_IPV6, IPPROTO_ICMPV6);
- if (!nf_nat_ipv6_manip_pkt(skb, 0, l4proto, &target, manip))
+ if (!l4proto || !nf_nat_ipv6_manip_pkt(skb, 0, l4proto, &target, manip))
return 0;
return 1;
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 2268b10a9dcf..d28185f38955 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -64,12 +64,16 @@ struct nat_net {
inline const struct nf_nat_l3proto *
__nf_nat_l3proto_find(u8 family)
{
+ if (family >= NFPROTO_NUMPROTO)
+ return NULL;
return rcu_dereference(nf_nat_l3protos[family]);
}
inline const struct nf_nat_l4proto *
__nf_nat_l4proto_find(u8 family, u8 protonum)
{
+ if (family >= NFPROTO_NUMPROTO)
+ return NULL;
return rcu_dereference(nf_nat_l4protos[family][protonum]);
}
EXPORT_SYMBOL_GPL(__nf_nat_l4proto_find);
@@ -317,7 +321,7 @@ find_best_ips_proto(const struct nf_conntrack_zone *zone,
* range. It might not be possible to get a unique tuple, but we try.
* At worst (or if we race), we will end up with a final duplicate in
* __ip_conntrack_confirm and drop the packet. */
-static void
+static int
get_unique_tuple(struct nf_conntrack_tuple *tuple,
const struct nf_conntrack_tuple *orig_tuple,
const struct nf_nat_range2 *range,
@@ -328,13 +332,22 @@ get_unique_tuple(struct nf_conntrack_tuple *tuple,
const struct nf_nat_l3proto *l3proto;
const struct nf_nat_l4p
