Re: [netfilter-core] [PATCH v2] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv
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!
Re: [PATCH v2] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv
On Thu, Jun 29, 2017 at 6:22 PM, Pablo Neira Ayusowrote: > 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: [PATCH v2] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv
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. 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
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
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.
[PATCH v2] netfilter: nfnetlink: Improve input length sanitization in nfnetlink_rcv
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--- 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