[PATCH] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv

[PATCH] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv

[Mateusz Jurczyk]

Verify that the length of the socket buffer is sufficient to cover the
entire nlh->nlmsg_len field before accessing that field for further
input sanitization. If the client only supplies 1-3 bytes of data in
sk_buff, then nlh->nlmsg_len remains partially uninitialized and
contains leftover memory from the corresponding kernel allocation.
Operating on such data may result in indeterminate evaluation of the
nlmsg_len < NLMSG_HDRLEN expression.

The bug was discovered by a runtime instrumentation designed to detect
use of uninitialized memory in the kernel. The patch prevents this and
other similar tools (e.g. KMSAN) from flagging this behavior in the future.

Signed-off-by: Mateusz Jurczyk <mjurczyk@google.com>
---
 net/netfilter/nfnetlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 80f5ecf2c3d7..c634cfca40ec 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -491,7 +491,8 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 {
 	struct nlmsghdr *nlh = nlmsg_hdr(skb);
 
-	if (nlh->nlmsg_len < NLMSG_HDRLEN ||
+	if (skb->len < sizeof(nlh->nlmsg_len) ||
+	    nlh->nlmsg_len < NLMSG_HDRLEN ||
 	    skb->len < nlh->nlmsg_len)
 		return;
 
-- 
2.13.1.508.gb3defc5cc-goog

Re: [PATCH] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv

[Eric Dumazet]

On Wed, 2017-06-07 at 14:35 +0200, Mateusz Jurczyk wrote:
> Verify that the length of the socket buffer is sufficient to cover the
> entire nlh->nlmsg_len field before accessing that field for further
> input sanitization. If the client only supplies 1-3 bytes of data in
> sk_buff, then nlh->nlmsg_len remains partially uninitialized and
> contains leftover memory from the corresponding kernel allocation.
> Operating on such data may result in indeterminate evaluation of the
> nlmsg_len < NLMSG_HDRLEN expression.
> 
> The bug was discovered by a runtime instrumentation designed to detect
> use of uninitialized memory in the kernel. The patch prevents this and
> other similar tools (e.g. KMSAN) from flagging this behavior in the future.
> 
> Signed-off-by: Mateusz Jurczyk <mjurczyk@google.com>
> ---
>  net/netfilter/nfnetlink.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
> index 80f5ecf2c3d7..c634cfca40ec 100644
> --- a/net/netfilter/nfnetlink.c
> +++ b/net/netfilter/nfnetlink.c
> @@ -491,7 +491,8 @@ static void nfnetlink_rcv(struct sk_buff *skb)
>  {
>  	struct nlmsghdr *nlh = nlmsg_hdr(skb);
>  
> -	if (nlh->nlmsg_len < NLMSG_HDRLEN ||
> +	if (skb->len < sizeof(nlh->nlmsg_len) ||

This assumes nlmsg_len is first field of the structure.

offsetofend() might be more descriptive, one does not have to check the
structure to make sure the code is correct.

Or simply use the more common form :

	if (skb->len < NLMSG_HDRLEN ||

> +	    nlh->nlmsg_len < NLMSG_HDRLEN ||
>  	    skb->len < nlh->nlmsg_len)
>  		return;
>  

[PATCH v2] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv

[Mateusz Jurczyk]

Verify that the length of the socket buffer is sufficient to cover the
nlmsghdr structure before accessing the nlh->nlmsg_len field for further
input sanitization. If the client only supplies 1-3 bytes of data in
sk_buff, then nlh->nlmsg_len remains partially uninitialized and
contains leftover memory from the corresponding kernel allocation.
Operating on such data may result in indeterminate evaluation of the
nlmsg_len < NLMSG_HDRLEN expression.

The bug was discovered by a runtime instrumentation designed to detect
use of uninitialized memory in the kernel. The patch prevents this and
other similar tools (e.g. KMSAN) from flagging this behavior in the future.

Signed-off-by: Mateusz Jurczyk <mjurczyk@google.com>
---
Changes in v2:
  - Compare skb->len against NLMSG_HDRLEN to avoid assuming the layout of
    the nlmsghdr structure.

 net/netfilter/nfnetlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 80f5ecf2c3d7..1f9667f52be5 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -491,7 +491,8 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 {
 	struct nlmsghdr *nlh = nlmsg_hdr(skb);
 
-	if (nlh->nlmsg_len < NLMSG_HDRLEN ||
+	if (skb->len < NLMSG_HDRLEN ||
+	    nlh->nlmsg_len < NLMSG_HDRLEN ||
 	    skb->len < nlh->nlmsg_len)
 		return;
 
-- 
2.13.1.508.gb3defc5cc-goog

Re: [PATCH v2] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv

[Pablo Neira Ayuso]

On Wed, Jun 07, 2017 at 03:50:38PM +0200, Mateusz Jurczyk wrote:
> Verify that the length of the socket buffer is sufficient to cover the
> nlmsghdr structure before accessing the nlh->nlmsg_len field for further
> input sanitization. If the client only supplies 1-3 bytes of data in
> sk_buff, then nlh->nlmsg_len remains partially uninitialized and
> contains leftover memory from the corresponding kernel allocation.
> Operating on such data may result in indeterminate evaluation of the
> nlmsg_len < NLMSG_HDRLEN expression.
> 
> The bug was discovered by a runtime instrumentation designed to detect
> use of uninitialized memory in the kernel. The patch prevents this and
> other similar tools (e.g. KMSAN) from flagging this behavior in the future.

Applied, thanks.

Re: [PATCH v2] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv

[Pablo Neira Ayuso]

On Tue, Jun 27, 2017 at 05:58:25PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Jun 07, 2017 at 03:50:38PM +0200, Mateusz Jurczyk wrote:
> > Verify that the length of the socket buffer is sufficient to cover the
> > nlmsghdr structure before accessing the nlh->nlmsg_len field for further
> > input sanitization. If the client only supplies 1-3 bytes of data in
> > sk_buff, then nlh->nlmsg_len remains partially uninitialized and
> > contains leftover memory from the corresponding kernel allocation.
> > Operating on such data may result in indeterminate evaluation of the
> > nlmsg_len < NLMSG_HDRLEN expression.
> > 
> > The bug was discovered by a runtime instrumentation designed to detect
> > use of uninitialized memory in the kernel. The patch prevents this and
> > other similar tools (e.g. KMSAN) from flagging this behavior in the future.
> 
> Applied, thanks.

Wait, I keeping this back after closer look.

I think we have to remove this:

        if (nlh->nlmsg_len < NLMSG_HDRLEN || <---
            skb->len < NLMSG_HDRLEN + sizeof(struct nfgenmsg))
                return;

in nfnetlink_rcv_skb_batch()

now that we make this unfront check from nfnetlink_rcv().

P.S: Sorry I couldn't look at this any sooner, I've been busy in the
last weeks preparing things for the upcoming Netfilter Workshop.

Re: [PATCH v2] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

On Tue, Jun 27, 2017 at 07:05:27PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jun 27, 2017 at 05:58:25PM +0200, Pablo Neira Ayuso wrote:
> > On Wed, Jun 07, 2017 at 03:50:38PM +0200, Mateusz Jurczyk wrote:
> > > Verify that the length of the socket buffer is sufficient to cover the
> > > nlmsghdr structure before accessing the nlh->nlmsg_len field for further
> > > input sanitization. If the client only supplies 1-3 bytes of data in
> > > sk_buff, then nlh->nlmsg_len remains partially uninitialized and
> > > contains leftover memory from the corresponding kernel allocation.
> > > Operating on such data may result in indeterminate evaluation of the
> > > nlmsg_len < NLMSG_HDRLEN expression.
> > > 
> > > The bug was discovered by a runtime instrumentation designed to detect
> > > use of uninitialized memory in the kernel. The patch prevents this and
> > > other similar tools (e.g. KMSAN) from flagging this behavior in the future.
> > 
> > Applied, thanks.
> 
> Wait, I keeping this back after closer look.
> 
> I think we have to remove this:
> 
>         if (nlh->nlmsg_len < NLMSG_HDRLEN || <---
>             skb->len < NLMSG_HDRLEN + sizeof(struct nfgenmsg))
>                 return;
> 
> in nfnetlink_rcv_skb_batch()
> 
> now that we make this unfront check from nfnetlink_rcv().

BTW, I can just mangle your patch here to delete such line to speed up
things. See the mangled patch that is attached to this email.

Thanks.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 828 bytes --]

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 80f5ecf2c3d7..ff1f4ce6fba4 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -463,8 +463,7 @@ static void nfnetlink_rcv_skb_batch(struct sk_buff *skb, struct nlmsghdr *nlh)
 	if (msglen > skb->len)
 		msglen = skb->len;
 
-	if (nlh->nlmsg_len < NLMSG_HDRLEN ||
-	    skb->len < NLMSG_HDRLEN + sizeof(struct nfgenmsg))
+	if (skb->len < NLMSG_HDRLEN + sizeof(struct nfgenmsg))
 		return;
 
 	err = nla_parse(cda, NFNL_BATCH_MAX, attr, attrlen, nfnl_batch_policy,
@@ -491,7 +490,8 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 {
 	struct nlmsghdr *nlh = nlmsg_hdr(skb);
 
-	if (nlh->nlmsg_len < NLMSG_HDRLEN ||
+	if (skb->len < NLMSG_HDRLEN ||
+	    nlh->nlmsg_len < NLMSG_HDRLEN ||
 	    skb->len < nlh->nlmsg_len)
 		return;
 

Re: [PATCH v2] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv

[Mateusz Jurczyk]

On Thu, Jun 29, 2017 at 6:22 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Jun 27, 2017 at 07:05:27PM +0200, Pablo Neira Ayuso wrote:
>> On Tue, Jun 27, 2017 at 05:58:25PM +0200, Pablo Neira Ayuso wrote:
>> > On Wed, Jun 07, 2017 at 03:50:38PM +0200, Mateusz Jurczyk wrote:
>> > > Verify that the length of the socket buffer is sufficient to cover the
>> > > nlmsghdr structure before accessing the nlh->nlmsg_len field for further
>> > > input sanitization. If the client only supplies 1-3 bytes of data in
>> > > sk_buff, then nlh->nlmsg_len remains partially uninitialized and
>> > > contains leftover memory from the corresponding kernel allocation.
>> > > Operating on such data may result in indeterminate evaluation of the
>> > > nlmsg_len < NLMSG_HDRLEN expression.
>> > >
>> > > The bug was discovered by a runtime instrumentation designed to detect
>> > > use of uninitialized memory in the kernel. The patch prevents this and
>> > > other similar tools (e.g. KMSAN) from flagging this behavior in the future.
>> >
>> > Applied, thanks.
>>
>> Wait, I keeping this back after closer look.
>>
>> I think we have to remove this:
>>
>>         if (nlh->nlmsg_len < NLMSG_HDRLEN || <---
>>             skb->len < NLMSG_HDRLEN + sizeof(struct nfgenmsg))
>>                 return;
>>
>> in nfnetlink_rcv_skb_batch()
>>
>> now that we make this unfront check from nfnetlink_rcv().
>
> BTW, I can just mangle your patch here to delete such line to speed up
> things. See the mangled patch that is attached to this email.

Sure, I think the condition in nfnetlink_rcv_skb_batch() can be now
safely removed. Feel free to proceed with the mangled patch. Thanks.

Re: [netfilter-core] [PATCH v2] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv

[Pablo Neira Ayuso]

On Thu, Jun 29, 2017 at 06:22:40PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jun 27, 2017 at 07:05:27PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Jun 27, 2017 at 05:58:25PM +0200, Pablo Neira Ayuso wrote:
> > > On Wed, Jun 07, 2017 at 03:50:38PM +0200, Mateusz Jurczyk wrote:
> > > > Verify that the length of the socket buffer is sufficient to cover the
> > > > nlmsghdr structure before accessing the nlh->nlmsg_len field for further
> > > > input sanitization. If the client only supplies 1-3 bytes of data in
> > > > sk_buff, then nlh->nlmsg_len remains partially uninitialized and
> > > > contains leftover memory from the corresponding kernel allocation.
> > > > Operating on such data may result in indeterminate evaluation of the
> > > > nlmsg_len < NLMSG_HDRLEN expression.
> > > > 
> > > > The bug was discovered by a runtime instrumentation designed to detect
> > > > use of uninitialized memory in the kernel. The patch prevents this and
> > > > other similar tools (e.g. KMSAN) from flagging this behavior in the future.
> > > 
> > > Applied, thanks.
> > 
> > Wait, I keeping this back after closer look.
> > 
> > I think we have to remove this:
> > 
> >         if (nlh->nlmsg_len < NLMSG_HDRLEN || <---
> >             skb->len < NLMSG_HDRLEN + sizeof(struct nfgenmsg))
> >                 return;
> > 
> > in nfnetlink_rcv_skb_batch()
> > 
> > now that we make this unfront check from nfnetlink_rcv().
> 
> BTW, I can just mangle your patch here to delete such line to speed up
> things. See the mangled patch that is attached to this email.

OK, I have applied this to the nf tree.

Thanks!