[PATCH] src: netlink_linearize: Fix bug for redirect target

[Shivani Bhardwaj]

Before this patch,
$ sudo nft --debug=netlink add rule ip nat post ip protocol tcp redirect to 
100-200
ip nat post
  [ payload load 1b @ network header + 9 => reg 1 ]
  [ cmp eq reg 1 0x0006 ]
  [ immediate reg 1 0x6400 ]
  [ immediate reg 2 0xc800 ]
  [ redir proto_min reg 1 proto_max reg 5 ]

:1:1-56: Error: Could not process rule: Invalid argument
add rule ip nat post ip protocol tcp redirect to 100-200


After this patch,
$ sudo nft --debug=netlink add rule ip nat post ip protocol tcp redirect to 
100-200
ip nat post
  [ payload load 1b @ network header + 9 => reg 1 ]
  [ cmp eq reg 1 0x0006 ]
  [ immediate reg 1 0x6400 ]
  [ immediate reg 2 0xc800 ]
  [ redir proto_min reg 1 proto_max reg 2 ]

Signed-off-by: Shivani Bhardwaj 
---
 src/netlink_linearize.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 7c6ef16..dfe8dca 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -861,17 +861,17 @@ static void netlink_gen_redir_stmt(struct 
netlink_linearize_ctx *ctx,
 pmin_reg);
netlink_gen_expr(ctx, stmt->redir.proto->right,
 pmax_reg);
-   nftnl_expr_set_u32(nle,
- NFTNL_EXPR_REDIR_REG_PROTO_MIN,
- pmin_reg);
-   nftnl_expr_set_u32(nle,
- NFTNL_EXPR_REDIR_REG_PROTO_MAX,
- pmax_reg);
+   netlink_put_register(nle,
+NFTNL_EXPR_REDIR_REG_PROTO_MIN,
+pmin_reg);
+   netlink_put_register(nle,
+NFTNL_EXPR_REDIR_REG_PROTO_MAX,
+pmax_reg);
} else {
netlink_gen_expr(ctx, stmt->redir.proto, pmin_reg);
-   nftnl_expr_set_u32(nle,
- NFTNL_EXPR_REDIR_REG_PROTO_MIN,
- pmin_reg);
+   netlink_put_register(nle,
+NFTNL_EXPR_REDIR_REG_PROTO_MIN,
+pmin_reg);
}
}
 
-- 
1.9.1

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

[PATCH nft 04/11] evaluate: don't adjust offset from resolve_protocol_conflict()

[Pablo Neira Ayuso]

This is not itself a conflict, move this check out of this function.

Signed-off-by: Pablo Neira Ayuso 
---
 src/evaluate.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index a294070..5ef035b 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -395,11 +395,6 @@ static int resolve_protocol_conflict(struct eval_ctx *ctx,
struct stmt *nstmt = NULL;
int link, err;
 
-   if (desc == payload->payload.desc) {
-   payload->payload.offset += ctx->pctx.protocol[base].offset;
-   return 0;
-   }
-
err = supersede_dep(ctx, desc, payload);
if (err <= 0)
return err;
@@ -448,6 +443,14 @@ static int __expr_evaluate_payload(struct eval_ctx *ctx, 
struct expr *expr)
return -1;
list_add_tail(>list, >stmt->list);
} else {
+   /* No conflict: Same payload protocol as context, adjust offset
+* if needed.
+*/
+   if (desc == payload->payload.desc) {
+   payload->payload.offset +=
+   ctx->pctx.protocol[base].offset;
+   return 0;
+   }
/* If we already have context and this payload is on the same
 * base, try to resolve the protocol conflict.
 */
-- 
2.1.4

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

[PATCH nft 05/11] evaluate: only try to replace dummy protocol from link-layer context

[Pablo Neira Ayuso]

Add proto_is_dummy() that returns true for netdev and inet family, the
only two using a dummy link-layer protocol base definition.

Rename supersede_dep() to meta_iiftype_gen_dependency() since this is
generating the implicit meta iiftype check for netdev and inet.

This patch also gets rid of the have->length check. The tests pass fine
without this so I suspect this is superfluos.

Signed-off-by: Pablo Neira Ayuso 
---
 src/evaluate.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 5ef035b..eb442d5 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -359,19 +359,14 @@ conflict_resolution_gen_dependency(struct eval_ctx *ctx, 
int protocol,
  * ip saddr adds meta dependency on ipv4 packets
  * ether saddr adds another dependeny on ethernet frames.
  */
-static int supersede_dep(struct eval_ctx *ctx, const struct proto_desc *have,
-struct expr *payload)
+static int meta_iiftype_gen_dependency(struct eval_ctx *ctx,
+  const struct proto_desc *have,
+  struct expr *payload)
 {
enum proto_bases base = payload->payload.base;
struct stmt *nstmt;
uint16_t type;
 
-   if (payload->payload.base != PROTO_BASE_LL_HDR || have->length)
-   return 1;
-
-   if (have != _inet && have != _netdev)
-   return 1;
-
if (proto_dev_type(payload->payload.desc, ) < 0)
return expr_error(ctx->msgs, payload,
  "protocol specification is invalid "
@@ -387,6 +382,11 @@ static int supersede_dep(struct eval_ctx *ctx, const 
struct proto_desc *have,
return 0;
 }
 
+static bool proto_is_dummy(const struct proto_desc *desc)
+{
+   return desc == _inet || desc == _netdev;
+}
+
 static int resolve_protocol_conflict(struct eval_ctx *ctx,
 const struct proto_desc *desc,
 struct expr *payload)
@@ -395,9 +395,12 @@ static int resolve_protocol_conflict(struct eval_ctx *ctx,
struct stmt *nstmt = NULL;
int link, err;
 
-   err = supersede_dep(ctx, desc, payload);
-   if (err <= 0)
-   return err;
+   if (payload->payload.base == PROTO_BASE_LL_HDR &&
+   proto_is_dummy(desc)) {
+   err = meta_iiftype_gen_dependency(ctx, desc, payload);
+   if (err <= 0)
+   return err;
+   }
 
if (base < PROTO_BASE_MAX) {
const struct proto_desc *next = ctx->pctx.protocol[base + 
1].desc;
-- 
2.1.4

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

[PATCH nft 11/11] tests/py: test vlan on ingress

[Pablo Neira Ayuso]

This generates the same code as bridge does, but it includes this check
in first place.

  [ meta load iiftype => reg 1 ]
  [ cmp eq reg 1 0x0001 ]

Signed-off-by: Pablo Neira Ayuso 
---
 tests/py/bridge/vlan.t|   2 +
 tests/py/bridge/vlan.t.payload.netdev | 235 ++
 2 files changed, 237 insertions(+)
 create mode 100644 tests/py/bridge/vlan.t.payload.netdev

diff --git a/tests/py/bridge/vlan.t b/tests/py/bridge/vlan.t
index e7c75bb..526d7cc 100644
--- a/tests/py/bridge/vlan.t
+++ b/tests/py/bridge/vlan.t
@@ -1,6 +1,8 @@
 :input;type filter hook input priority 0
+:ingress;type filter hook ingress device lo priority 0
 
 *bridge;test-bridge;input
+*netdev;test-netdev;ingress
 
 vlan id 4094;ok
 vlan id 0;ok
diff --git a/tests/py/bridge/vlan.t.payload.netdev 
b/tests/py/bridge/vlan.t.payload.netdev
new file mode 100644
index 000..62c7adf
--- /dev/null
+++ b/tests/py/bridge/vlan.t.payload.netdev
@@ -0,0 +1,235 @@
+# vlan id 4094
+netdev test-netdev ingress 
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x0001 ]
+  [ payload load 2b @ link header + 12 => reg 1 ]
+  [ cmp eq reg 1 0x0081 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0xff0f ) ^ 0x ]
+  [ cmp eq reg 1 0xfe0f ]
+
+# vlan id 0
+netdev test-netdev ingress 
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x0001 ]
+  [ payload load 2b @ link header + 12 => reg 1 ]
+  [ cmp eq reg 1 0x0081 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0xff0f ) ^ 0x ]
+  [ cmp eq reg 1 0x ]
+
+# vlan id 4094 vlan cfi 0
+netdev test-netdev ingress 
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x0001 ]
+  [ payload load 2b @ link header + 12 => reg 1 ]
+  [ cmp eq reg 1 0x0081 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0xff0f ) ^ 0x ]
+  [ cmp eq reg 1 0xfe0f ]
+  [ payload load 1b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0x0010 ) ^ 0x ]
+  [ cmp eq reg 1 0x ]
+
+# vlan id 4094 vlan cfi != 1
+netdev test-netdev ingress 
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x0001 ]
+  [ payload load 2b @ link header + 12 => reg 1 ]
+  [ cmp eq reg 1 0x0081 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0xff0f ) ^ 0x ]
+  [ cmp eq reg 1 0xfe0f ]
+  [ payload load 1b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0x0010 ) ^ 0x ]
+  [ cmp neq reg 1 0x0010 ]
+
+# vlan id 4094 vlan cfi 1
+netdev test-netdev ingress 
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x0001 ]
+  [ payload load 2b @ link header + 12 => reg 1 ]
+  [ cmp eq reg 1 0x0081 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0xff0f ) ^ 0x ]
+  [ cmp eq reg 1 0xfe0f ]
+  [ payload load 1b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0x0010 ) ^ 0x ]
+  [ cmp eq reg 1 0x0010 ]
+
+# ether type vlan vlan id 4094
+netdev test-netdev ingress 
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x0001 ]
+  [ payload load 2b @ link header + 12 => reg 1 ]
+  [ cmp eq reg 1 0x0081 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0xff0f ) ^ 0x ]
+  [ cmp eq reg 1 0xfe0f ]
+
+# ether type vlan vlan id 0
+netdev test-netdev ingress 
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x0001 ]
+  [ payload load 2b @ link header + 12 => reg 1 ]
+  [ cmp eq reg 1 0x0081 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0xff0f ) ^ 0x ]
+  [ cmp eq reg 1 0x ]
+
+# ether type vlan vlan id 4094 vlan cfi 0
+netdev test-netdev ingress 
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x0001 ]
+  [ payload load 2b @ link header + 12 => reg 1 ]
+  [ cmp eq reg 1 0x0081 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0xff0f ) ^ 0x ]
+  [ cmp eq reg 1 0xfe0f ]
+  [ payload load 1b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0x0010 ) ^ 0x ]
+  [ cmp eq reg 1 0x ]
+
+# ether type vlan vlan id 4094 vlan cfi 1
+netdev test-netdev ingress 
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x0001 ]
+  [ payload load 2b @ link header + 12 => reg 1 ]
+  [ cmp eq reg 1 0x0081 ]
+  [ payload load 2b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0xff0f ) ^ 0x ]
+  [ cmp eq reg 1 0xfe0f ]
+  [ payload load 1b @ link header + 14 => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0x0010 ) ^ 0x ]
+  [ cmp eq reg 1 0x0010 ]
+
+# vlan id 4094 tcp dport 22
+netdev test-netdev ingress 
+  [ meta load iiftype => reg 1 ]
+  [ cmp eq reg 1 0x0001 ]
+  [ payload load 2b @ link header + 12 => reg 1 ]
+ 

[PATCH nft 09/11] proto: proto_dev_type() returns interface type for base protocols too

[Pablo Neira Ayuso]

The device protocol definition provides a mapping between the interface
type, ie. ARPHDR_*, and the overlying protocol base definition, eg.
proto_eth.

This patch updates proto_dev_type() so it also returns a mapping for
these overlying ethernet protocol definitions, ie. ip, ip6, vlan, ip,
arp.

This patch required to resolve problems with automatic dependency
generation for vlan in the netdev and inet families.

Signed-off-by: Pablo Neira Ayuso 
---
 src/proto.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/proto.c b/src/proto.c
index 65ee158..0cd9fdb 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -94,13 +94,21 @@ static const struct dev_proto_desc dev_proto_desc[] = {
  */
 int proto_dev_type(const struct proto_desc *desc, uint16_t *res)
 {
-   unsigned int i;
+   const struct proto_desc *base;
+   unsigned int i, j;
 
for (i = 0; i < array_size(dev_proto_desc); i++) {
-   if (dev_proto_desc[i].desc == desc) {
+   base = dev_proto_desc[i].desc;
+   if (base == desc) {
*res = dev_proto_desc[i].type;
return 0;
}
+   for (j = 0; j < array_size(base->protocols); j++) {
+   if (base->protocols[j].desc == desc) {
+   *res = dev_proto_desc[i].type;
+   return 0;
+   }
+   }
}
return -1;
 }
-- 
2.1.4

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

[PATCH nft 10/11] src: annotate follow up dependency just after killing another

[Pablo Neira Ayuso]

The inet and netdev families generate two implicit dependencies to check
for the interface type, so we have to check just after killing an implicit
dependency if there is another that we should annotate to kill it as well.

Signed-off-by: Pablo Neira Ayuso 
---
 src/netlink_delinearize.c | 45 +++--
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index cb9c3ab..bbe1876 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1031,6 +1031,30 @@ static void integer_type_postprocess(struct expr *expr)
}
 }
 
+static void payload_dependency_save(struct rule_pp_ctx *ctx, unsigned int base,
+   struct stmt *nstmt, struct expr *tmp)
+{
+   unsigned int proto = mpz_get_be16(tmp->value);
+   const struct proto_desc *desc, *next;
+   bool stacked_header = false;
+
+   desc = ctx->pctx.protocol[base].desc;
+
+   assert(desc);
+   if (desc) {
+   next = proto_find_upper(desc, proto);
+   stacked_header = next && next->base == base;
+   }
+
+   if (stacked_header) {
+   ctx->pctx.protocol[base].desc = next;
+   ctx->pctx.protocol[base].offset += desc->length;
+   payload_dependency_store(ctx, nstmt, base - 1);
+   } else {
+   payload_dependency_store(ctx, nstmt, base);
+   }
+}
+
 static void payload_match_expand(struct rule_pp_ctx *ctx,
 struct expr *expr,
 struct expr *payload)
@@ -1068,26 +1092,11 @@ static void payload_match_expand(struct rule_pp_ctx 
*ctx,
 */
if (ctx->pbase == PROTO_BASE_INVALID &&
left->flags & EXPR_F_PROTOCOL) {
-   unsigned int proto = mpz_get_be16(tmp->value);
-   const struct proto_desc *desc, *next;
-   bool stacked_header = false;
-
-   desc = ctx->pctx.protocol[base].desc;
-   assert(desc);
-   if (desc) {
-   next = proto_find_upper(desc, proto);
-   stacked_header = next && next->base == base;
-   }
-
-   if (stacked_header) {
-   ctx->pctx.protocol[base].desc = next;
-   ctx->pctx.protocol[base].offset += desc->length;
-   payload_dependency_store(ctx, nstmt, base - 1);
-   } else {
-   payload_dependency_store(ctx, nstmt, base);
-   }
+   payload_dependency_save(ctx, base, nstmt, tmp);
} else {
payload_dependency_kill(ctx, nexpr->left);
+   if (left->flags & EXPR_F_PROTOCOL)
+   payload_dependency_save(ctx, base, nstmt, tmp);
}
}
list_del(>stmt->list);
-- 
2.1.4

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

[PATCH V2] netfilter: h323: avoid potential attack

[Zhouyi Zhou]

Thanks Eric for your review and advice.

I think hackers chould build a malicious h323 packet to overflow
the pointer p which will panic during the memcpy(addr, p, len)

For example, he may fabricate a very large taddr->ipAddress.ip;

Signed-off-by: Zhouyi Zhou 
---
 net/netfilter/nf_conntrack_h323_main.c |7 +++
 1 file changed, 7 insertions(+)

diff --git a/net/netfilter/nf_conntrack_h323_main.c 
b/net/netfilter/nf_conntrack_h323_main.c
index 9511af0..ccd08c5 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -110,6 +110,7 @@ int (*nat_q931_hook) (struct sk_buff *skb,
 
 static DEFINE_SPINLOCK(nf_h323_lock);
 static char *h323_buffer;
+#define CHECK_BOUND(p, n) ((void *)p + n - (void *)h323_buffer > 65536)
 
 static struct nf_conntrack_helper nf_conntrack_helper_h245;
 static struct nf_conntrack_helper nf_conntrack_helper_q931[];
@@ -247,6 +248,9 @@ static int get_h245_addr(struct nf_conn *ct, const unsigned 
char *data,
return 0;
}
 
+   if (CHECK_BOUND(p, len + sizeof(__be16)))
+   return 0;
+
memcpy(addr, p, len);
memset((void *)addr + len, 0, sizeof(*addr) - len);
memcpy(port, p + len, sizeof(__be16));
@@ -669,6 +673,9 @@ int get_h225_addr(struct nf_conn *ct, unsigned char *data,
return 0;
}
 
+   if (CHECK_BOUND(p, len + sizeof(__be16)))
+   return 0;
+
memcpy(addr, p, len);
memset((void *)addr + len, 0, sizeof(*addr) - len);
memcpy(port, p + len, sizeof(__be16));
-- 
1.7.10.4


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

Re: [PATCH V2] netfilter: h323: avoid potential attack

[Eric Dumazet]

On Thu, 2016-01-28 at 16:59 +0800, Zhouyi Zhou wrote:
> Thanks Eric for your review and advice.
> 
> I think hackers chould build a malicious h323 packet to overflow
> the pointer p which will panic during the memcpy(addr, p, len)
> 
> For example, he may fabricate a very large taddr->ipAddress.ip;
> 
> Signed-off-by: Zhouyi Zhou 
> ---

Except you did not address my feedback about potentially reading not
initialized memory.

if the frame length was 1000 bytes, then surely accessing memory at
offset 8000 will either read garbage, or read data from a prior frame
and leak secrets.



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

Re: [PATCH 1/2] netfilter: ipvs: avoid unused variable warnings

[Arnd Bergmann]

On Thursday 28 January 2016 08:39:53 Simon Horman wrote:
> On Wed, Jan 27, 2016 at 10:01:42PM +0200, Julian Anastasov wrote:
> > 
> >   Hello,
> > 
> > On Wed, 27 Jan 2016, Arnd Bergmann wrote:
> > 
> > > The proc_create() and remove_proc_entry() functions do not reference
> > > their arguments when CONFIG_PROC_FS is disabled, so we get a couple
> > > of warnings about unused variables in IPVS:
> > > 
> > > ipvs/ip_vs_app.c:608:14: warning: unused variable 'net' 
> > > [-Wunused-variable]
> > > ipvs/ip_vs_ctl.c:3950:14: warning: unused variable 'net' 
> > > [-Wunused-variable]
> > > ipvs/ip_vs_ctl.c:3994:14: warning: unused variable 'net' 
> > > [-Wunused-variable]
> > > 
> > > This removes the local variables and instead looks them up separately
> > > for each use, which obviously avoids the warning.
> > > 
> > > Signed-off-by: Arnd Bergmann 
> > > Fixes: 4c50a8ce2b63 ("netfilter: ipvs: avoid unused variable warning")
> > 
> >   Looks like your previous patch for ip_vs_app_net_cleanup
> > was delayed in ipvs-next tree. I guess, Simon should drop it and
> > use this one instead when net-next opens:
> > 
> > Acked-by: Julian Anastasov 
> 
> Thanks, and sorry about not pushing the other patch to net-next.
> I have dropped it and queued up this one in its place.

Ah, I had not realized that the other patch was still in ipvs-next
and not merged in mainline. I did most of my testing on linux-next
(with the previous patch) and then validated the new one on
4.5-rc1, which led me to update it to contain the same hunk again.

Replacing the original patch works fine though, thanks for picking
it up!

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

Re: Re: [PATCH V2] netfilter: h323: avoid potential attack

[Eric Dumazet]

On Thu, 2016-01-28 at 06:00 -0800, Eric Dumazet wrote:
> On Thu, 2016-01-28 at 21:14 +0800, Zhouyi Zhou wrote:
> 
> > My patch is intend to prevent kernel panic, to prevent reading garbage
> > or read data from a prior frame and leak secrets, the prototypes of the 
> > get_h2x5_addr functions and the functions that call get_h2x5_addr should
> > be changed, should we do this? 
> 
> In term of security, panics are better than allowing attacker to read
> data from other people, like a password.
> 
> BTW, are you able to trigger any panic ?
> 
> I am not familiar with this code, it is not obvious.
> 
> If a fix is needed, better doing it right.
> 


BTW, this module is protected by a lock (nf_h323_lock)

So adding a variable like 'h323_buffer_valid_bytes' that would contain
the number of valid bytes would not require to change prototypes.

This variable would be written when skb_header_pointer() is used in
get_tpkt_data() / get_udp_data()



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

Re: [PATCH V2] netfilter: h323: avoid potential attack

[Florian Westphal]

Zhouyi Zhou  wrote:
> Thanks Eric for your review and advice.
> 
> I think hackers chould build a malicious h323 packet to overflow
> the pointer p which will panic during the memcpy(addr, p, len)
> 
> For example, he may fabricate a very large taddr->ipAddress.ip;

Can you be more specific?

h323_buffer is backend storage for skb_header_pointer, i.e.
this will error out early when we ask for more data than is available in
packet.

I don't understand how this could overflow anything.
Even assuming 64k packet we'd still have enough room in h323_buffer
for an ipv6 address, no? (we skip the l3/l4 header when extracting
packet payload).

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

[PATCH V3] netfilter: h323: avoid potential attack

[Zhouyi Zhou]

I think hackers chould build a malicious h323 packet to overflow
the pointer p which will panic during the memcpy(addr, p, len)
For example, he may fabricate a very large taddr->ipAddress.ip;
As suggested by Eric, this module is protected by a lock (nf_h323_lock)
so adding a variable h323_buffer_valid_bytes that would contain
the number of valid bytes would not require to change prototypes of
get_h2x5_addr.

Signed-off-by: Zhouyi Zhou 
Signed-off-by: Eric Dumazet 
Reviewed-by: Sergei Shtylyov  

---
 net/netfilter/nf_conntrack_h323_main.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/net/netfilter/nf_conntrack_h323_main.c 
b/net/netfilter/nf_conntrack_h323_main.c
index 9511af0..65d84bc 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -110,6 +110,11 @@ int (*nat_q931_hook) (struct sk_buff *skb,
 
 static DEFINE_SPINLOCK(nf_h323_lock);
 static char *h323_buffer;
+static unsigned int h323_buffer_valid_bytes;
+/* check offset overflow and out of range data reference */
+#define CHECK_BOUND(p, n) ((n) > h323_buffer_valid_bytes ||\
+  ((void *)(p) + (n) - (void *)h323_buffer \
+   > h323_buffer_valid_bytes))
 
 static struct nf_conntrack_helper nf_conntrack_helper_h245;
 static struct nf_conntrack_helper nf_conntrack_helper_q931[];
@@ -145,6 +150,7 @@ static int get_tpkt_data(struct sk_buff *skb, unsigned int 
protoff,
 
if (*data == NULL) {/* first TPKT */
/* Get first TPKT pointer */
+   h323_buffer_valid_bytes = tcpdatalen;
tpkt = skb_header_pointer(skb, tcpdataoff, tcpdatalen,
  h323_buffer);
BUG_ON(tpkt == NULL);
@@ -247,6 +253,9 @@ static int get_h245_addr(struct nf_conn *ct, const unsigned 
char *data,
return 0;
}
 
+   if (CHECK_BOUND(p, len + sizeof(__be16)))
+   return 0;
+
memcpy(addr, p, len);
memset((void *)addr + len, 0, sizeof(*addr) - len);
memcpy(port, p + len, sizeof(__be16));
@@ -669,6 +678,9 @@ int get_h225_addr(struct nf_conn *ct, unsigned char *data,
return 0;
}
 
+   if (CHECK_BOUND(p, len + sizeof(__be16)))
+   return 0;
+
memcpy(addr, p, len);
memset((void *)addr + len, 0, sizeof(*addr) - len);
memcpy(port, p + len, sizeof(__be16));
@@ -1248,6 +1260,7 @@ static unsigned char *get_udp_data(struct sk_buff *skb, 
unsigned int protoff,
if (dataoff >= skb->len)
return NULL;
*datalen = skb->len - dataoff;
+   h323_buffer_valid_bytes = *datalen;
return skb_header_pointer(skb, dataoff, *datalen, h323_buffer);
 }
 
-- 
1.9.1


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

Re: [PATCH V2] netfilter: h323: avoid potential attack

[Sergei Shtylyov]

Hello.

On 1/28/2016 11:59 AM, Zhouyi Zhou wrote:


Thanks Eric for your review and advice.

I think hackers chould build a malicious h323 packet to overflow
the pointer p which will panic during the memcpy(addr, p, len)

For example, he may fabricate a very large taddr->ipAddress.ip;

Signed-off-by: Zhouyi Zhou 
---
  net/netfilter/nf_conntrack_h323_main.c |7 +++
  1 file changed, 7 insertions(+)

diff --git a/net/netfilter/nf_conntrack_h323_main.c 
b/net/netfilter/nf_conntrack_h323_main.c
index 9511af0..ccd08c5 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -110,6 +110,7 @@ int (*nat_q931_hook) (struct sk_buff *skb,

  static DEFINE_SPINLOCK(nf_h323_lock);
  static char *h323_buffer;
+#define CHECK_BOUND(p, n) ((void *)p + n - (void *)h323_buffer > 65536)


   You have to enclose the macro parameters in parens when used in expression.

MBR, Sergei

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

Re: [PATCH V2] netfilter: h323: avoid potential attack

[One Thousand Gnomes]

On Thu, 28 Jan 2016 06:00:50 -0800
Eric Dumazet  wrote:

> On Thu, 2016-01-28 at 21:14 +0800, Zhouyi Zhou wrote:
> 
> > My patch is intend to prevent kernel panic, to prevent reading garbage
> > or read data from a prior frame and leak secrets, the prototypes of the 
> > get_h2x5_addr functions and the functions that call get_h2x5_addr should
> > be changed, should we do this?   
> 
> In term of security, panics are better than allowing attacker to read
> data from other people, like a password.
> 
> BTW, are you able to trigger any panic ?

We have a smattering of panics in this code in bugzilla going back years.

https://bugzilla.kernel.org/show_bug.cgi?id=12473

etc

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

Re: Re: [PATCH V2] netfilter: h323: avoid potential attack

[Zhouyi Zhou]

Thanks for your advices.
I will take your advice if I could have the opportunity
to write the final code.

As matter of factor, I trigger this bug when I tried to 
migrate H323 code to other operating systems.

This could trigger a panic because get_h2x5_addr functions
is the first time we ever try to ask for potentially
out of range data because of H323 style decodings 
(the value taddr->ipAddress.ip is decoded in the midway between
calling skb_header_pointer and calling get_h2x5_addr)



> -Original Messages-
> From: "Florian Westphal" 
> Sent Time: Thursday, January 28, 2016
> To: "Zhouyi Zhou" 
> Cc: eric.duma...@gmail.com, pa...@netfilter.org, ka...@trash.net, 
> kad...@blackhole.kfki.hu, da...@davemloft.net, 
> netfilter-devel@vger.kernel.org, coret...@netfilter.org, 
> net...@vger.kernel.org, linux-ker...@vger.kernel.or, "Zhouyi Zhou" 
> 
> Subject: Re: [PATCH V2] netfilter: h323: avoid potential attack
> 
> Zhouyi Zhou  wrote:
> > Thanks Eric for your review and advice.
> > 
> > I think hackers chould build a malicious h323 packet to overflow
> > the pointer p which will panic during the memcpy(addr, p, len)
> > 
> > For example, he may fabricate a very large taddr->ipAddress.ip;
> 
> Can you be more specific?
> 
> h323_buffer is backend storage for skb_header_pointer, i.e.
> this will error out early when we ask for more data than is available in
> packet.
> 
> I don't understand how this could overflow anything.
> Even assuming 64k packet we'd still have enough room in h323_buffer
> for an ipv6 address, no? (we skip the l3/l4 header when extracting
> packet payload).
> 




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