* [PATCH conntrackd 0/8] unsorted fixes
@ 2015-08-18 17:28 Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 1/8] conntrackd: fix sanitization of expection attribute in the wire format Pablo Neira Ayuso
` (7 more replies)
0 siblings, 8 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2015-08-18 17:28 UTC (permalink / raw)
To: netfilter-devel; +Cc: pwouters, fweimer
The following patchset contains a set of fixes for conntrackd as reported by
Paul Wouters.
Let me know if you have any concern with it, thank you.
Pablo Neira Ayuso (8):
conntrackd: fix sanitization of expection attribute in the wire format
conntrackd: NTA_MAX is also an invalid attribute
conntrackd: fix leak in fork_process_new()
conntrackd: fix descriptor leak in do_local_request()
conntrackd: fix error handling in nfq_queue_cb()
conntrackd: simplify branch in tcp_accept()
conntrackd: use strncpy to set up the cache name
conntrackd: missing break in expectation message parser function
src/cache.c | 5 +++--
src/cthelper.c | 29 +++++++++++++++--------------
src/local.c | 9 ++++++---
src/parse.c | 8 +++++---
src/process.c | 2 ++
src/tcp.c | 12 +++++-------
6 files changed, 36 insertions(+), 29 deletions(-)
--
1.7.10.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH conntrackd 1/8] conntrackd: fix sanitization of expection attribute in the wire format
2015-08-18 17:28 [PATCH conntrackd 0/8] unsorted fixes Pablo Neira Ayuso
@ 2015-08-18 17:28 ` Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 2/8] conntrackd: NTA_MAX is also an invalid attribute Pablo Neira Ayuso
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2015-08-18 17:28 UTC (permalink / raw)
To: netfilter-devel; +Cc: pwouters, fweimer
The maximum number of attribute is NTA_EXP_MAX for expectation sync messages.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/parse.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/parse.c b/src/parse.c
index f3ec6ac..878e354 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -510,7 +510,7 @@ int msg2exp(struct nf_expect *exp, struct nethdr *net, size_t remain)
ATTR_NETWORK2HOST(attr);
if (attr->nta_len > len)
goto err;
- if (attr->nta_attr > NTA_MAX)
+ if (attr->nta_attr >= NTA_EXP_MAX)
goto err;
if (attr->nta_len < NTA_LENGTH(0))
goto err;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH conntrackd 2/8] conntrackd: NTA_MAX is also an invalid attribute
2015-08-18 17:28 [PATCH conntrackd 0/8] unsorted fixes Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 1/8] conntrackd: fix sanitization of expection attribute in the wire format Pablo Neira Ayuso
@ 2015-08-18 17:28 ` Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 3/8] conntrackd: fix leak in fork_process_new() Pablo Neira Ayuso
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2015-08-18 17:28 UTC (permalink / raw)
To: netfilter-devel; +Cc: pwouters, fweimer
Otherwise this can result in an off-by-one array access.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/parse.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/parse.c b/src/parse.c
index 878e354..3ac4092 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -297,7 +297,7 @@ int msg2ct(struct nf_conntrack *ct, struct nethdr *net, size_t remain)
return -1;
if (attr->nta_len < NTA_LENGTH(0))
return -1;
- if (attr->nta_attr > NTA_MAX)
+ if (attr->nta_attr >= NTA_MAX)
return -1;
if (h[attr->nta_attr].size &&
attr->nta_len != h[attr->nta_attr].size)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH conntrackd 3/8] conntrackd: fix leak in fork_process_new()
2015-08-18 17:28 [PATCH conntrackd 0/8] unsorted fixes Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 1/8] conntrackd: fix sanitization of expection attribute in the wire format Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 2/8] conntrackd: NTA_MAX is also an invalid attribute Pablo Neira Ayuso
@ 2015-08-18 17:28 ` Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 4/8] conntrackd: fix descriptor leak in do_local_request() Pablo Neira Ayuso
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2015-08-18 17:28 UTC (permalink / raw)
To: netfilter-devel; +Cc: pwouters, fweimer
Release the child_process structure in case that fork() fails.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/process.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/process.c b/src/process.c
index 7f0a395..3ddad5f 100644
--- a/src/process.c
+++ b/src/process.c
@@ -48,6 +48,8 @@ int fork_process_new(int type, int flags, void (*cb)(void *data), void *data)
if (c->pid > 0)
list_add(&c->head, &process_list);
+ else
+ free(c);
return pid;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH conntrackd 4/8] conntrackd: fix descriptor leak in do_local_request()
2015-08-18 17:28 [PATCH conntrackd 0/8] unsorted fixes Pablo Neira Ayuso
` (2 preceding siblings ...)
2015-08-18 17:28 ` [PATCH conntrackd 3/8] conntrackd: fix leak in fork_process_new() Pablo Neira Ayuso
@ 2015-08-18 17:28 ` Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 5/8] conntrackd: fix error handling in nfq_queue_cb() Pablo Neira Ayuso
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2015-08-18 17:28 UTC (permalink / raw)
To: netfilter-devel; +Cc: pwouters, fweimer
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/local.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/local.c b/src/local.c
index 453799a..85e5180 100644
--- a/src/local.c
+++ b/src/local.c
@@ -147,11 +147,14 @@ int do_local_request(int request,
ret = send(fd, &request, sizeof(int), 0);
if (ret == -1)
- return -1;
+ goto err1;
do_local_client_step(fd, step);
local_client_destroy(fd);
-
+
return 0;
+err1:
+ local_client_destroy(fd);
+ return -1;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH conntrackd 5/8] conntrackd: fix error handling in nfq_queue_cb()
2015-08-18 17:28 [PATCH conntrackd 0/8] unsorted fixes Pablo Neira Ayuso
` (3 preceding siblings ...)
2015-08-18 17:28 ` [PATCH conntrackd 4/8] conntrackd: fix descriptor leak in do_local_request() Pablo Neira Ayuso
@ 2015-08-18 17:28 ` Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 6/8] conntrackd: simplify branch in tcp_accept() Pablo Neira Ayuso
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2015-08-18 17:28 UTC (permalink / raw)
To: netfilter-devel; +Cc: pwouters, fweimer
Make sure we have a clean exit on error, everything needs to be properly
released.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/cthelper.c | 29 +++++++++++++++--------------
src/local.c | 2 +-
2 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/src/cthelper.c b/src/cthelper.c
index 6537515..54eb830 100644
--- a/src/cthelper.c
+++ b/src/cthelper.c
@@ -277,11 +277,11 @@ static int nfq_queue_cb(const struct nlmsghdr *nlh, void *data)
if (!attr[NFQA_PAYLOAD]) {
dlog(LOG_ERR, "packet with no payload");
- goto err;
+ goto err1;
}
if (!attr[NFQA_CT] || !attr[NFQA_CT_INFO]) {
dlog(LOG_ERR, "no CT attached to this packet");
- goto err;
+ goto err1;
}
pkt = mnl_attr_get_payload(attr[NFQA_PAYLOAD]);
@@ -292,22 +292,22 @@ static int nfq_queue_cb(const struct nlmsghdr *nlh, void *data)
queue_num = ntohs(nfg->res_id);
if (pkt_get(pkt, pktlen, ntohs(ph->hw_protocol), &protoff))
- goto err;
+ goto err1;
ct = nfct_new();
if (ct == NULL)
- goto err;
+ goto err1;
if (nfct_payload_parse(mnl_attr_get_payload(attr[NFQA_CT]),
mnl_attr_get_payload_len(attr[NFQA_CT]),
l3num, ct) < 0) {
dlog(LOG_ERR, "cannot convert message to CT");
- goto err;
+ goto err2;
}
myct = calloc(1, sizeof(struct myct));
if (myct == NULL)
- goto err;
+ goto err2;
myct->ct = ct;
ctinfo = ntohl(mnl_attr_get_u32(attr[NFQA_CT_INFO]));
@@ -315,15 +315,15 @@ static int nfq_queue_cb(const struct nlmsghdr *nlh, void *data)
/* XXX: 256 bytes enough for possible NAT mangling in helpers? */
pktb = pktb_alloc(AF_INET, pkt, pktlen, 256);
if (pktb == NULL)
- goto err;
+ goto err3;
/* Misconfiguration: if no helper found, accept the packet. */
helper = helper_run(pktb, protoff, myct, ctinfo, queue_num, &verdict);
if (!helper)
- goto err_pktb;
+ goto err4;
if (pkt_verdict_issue(helper, myct, queue_num, id, verdict, pktb) < 0)
- goto err_pktb;
+ goto err4;
nfct_destroy(ct);
if (myct->exp != NULL)
@@ -333,18 +333,19 @@ static int nfq_queue_cb(const struct nlmsghdr *nlh, void *data)
free(myct);
return MNL_CB_OK;
-err_pktb:
+err4:
pktb_free(pktb);
-err:
+err3:
+ free(myct);
+err2:
+ nfct_destroy(ct);
+err1:
/* In case of error, we don't want to disrupt traffic. We accept all.
* This is connection tracking after all. The policy is not to drop
* packet unless we enter some inconsistent state.
*/
pkt_verdict_error(queue_num, id);
- if (ct != NULL)
- nfct_destroy(ct);
-
return MNL_CB_OK;
}
diff --git a/src/local.c b/src/local.c
index 85e5180..3395b4c 100644
--- a/src/local.c
+++ b/src/local.c
@@ -77,7 +77,7 @@ int do_local_server_step(struct local_server *server, void *data,
int rfd;
struct sockaddr_un local;
socklen_t sin_size = sizeof(struct sockaddr_un);
-
+
rfd = accept(server->fd, (struct sockaddr *) &local, &sin_size);
if (rfd == -1)
return -1;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH conntrackd 6/8] conntrackd: simplify branch in tcp_accept()
2015-08-18 17:28 [PATCH conntrackd 0/8] unsorted fixes Pablo Neira Ayuso
` (4 preceding siblings ...)
2015-08-18 17:28 ` [PATCH conntrackd 5/8] conntrackd: fix error handling in nfq_queue_cb() Pablo Neira Ayuso
@ 2015-08-18 17:28 ` Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 7/8] conntrackd: use strncpy to set up the cache name Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 8/8] conntrackd: missing break in expectation message parser function Pablo Neira Ayuso
7 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2015-08-18 17:28 UTC (permalink / raw)
To: netfilter-devel; +Cc: pwouters, fweimer
The same code is executed regardless the reason why accept() has failed.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/tcp.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/tcp.c b/src/tcp.c
index af27c46..c8f2544 100644
--- a/src/tcp.c
+++ b/src/tcp.c
@@ -247,13 +247,11 @@ int tcp_accept(struct tcp_sock *m)
/* the other peer wants to connect ... */
ret = accept(m->fd, NULL, NULL);
if (ret == -1) {
- if (errno != EAGAIN) {
- /* unexpected error. Give us another try. */
- m->state = TCP_SERVER_ACCEPTING;
- } else {
- /* waiting for new connections. */
- m->state = TCP_SERVER_ACCEPTING;
- }
+ /* unexpected error: Give us another try. Or we have hit
+ * -EAGAIN, in that case we remain in the accepting connections
+ * state.
+ */
+ m->state = TCP_SERVER_ACCEPTING;
} else {
/* the peer finally got connected. */
if (fcntl(ret, F_SETFL, O_NONBLOCK) == -1) {
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH conntrackd 7/8] conntrackd: use strncpy to set up the cache name
2015-08-18 17:28 [PATCH conntrackd 0/8] unsorted fixes Pablo Neira Ayuso
` (5 preceding siblings ...)
2015-08-18 17:28 ` [PATCH conntrackd 6/8] conntrackd: simplify branch in tcp_accept() Pablo Neira Ayuso
@ 2015-08-18 17:28 ` Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 8/8] conntrackd: missing break in expectation message parser function Pablo Neira Ayuso
7 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2015-08-18 17:28 UTC (permalink / raw)
To: netfilter-devel; +Cc: pwouters, fweimer
This is not exposed, but use the strncpy() variant to calm down static code
validators.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/cache.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/cache.c b/src/cache.c
index 7c41e54..79a024f 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -34,7 +34,7 @@ struct cache_feature *cache_feature[CACHE_MAX_FEATURE] = {
};
struct cache *cache_create(const char *name, enum cache_type type,
- unsigned int features,
+ unsigned int features,
struct cache_extra *extra,
struct cache_ops *ops)
{
@@ -53,7 +53,8 @@ struct cache *cache_create(const char *name, enum cache_type type,
return NULL;
memset(c, 0, sizeof(struct cache));
- strcpy(c->name, name);
+ strncpy(c->name, name, CACHE_MAX_NAMELEN);
+ c->name[CACHE_MAX_NAMELEN - 1] = '\0';
c->type = type;
for (i = 0; i < CACHE_MAX_FEATURE; i++) {
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH conntrackd 8/8] conntrackd: missing break in expectation message parser function
2015-08-18 17:28 [PATCH conntrackd 0/8] unsorted fixes Pablo Neira Ayuso
` (6 preceding siblings ...)
2015-08-18 17:28 ` [PATCH conntrackd 7/8] conntrackd: use strncpy to set up the cache name Pablo Neira Ayuso
@ 2015-08-18 17:28 ` Pablo Neira Ayuso
7 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2015-08-18 17:28 UTC (permalink / raw)
To: netfilter-devel; +Cc: pwouters, fweimer
Fortunately, the TLVs come in order in the message, however, if the order is
changed we'll incorrectly set up the expectation.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/parse.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/parse.c b/src/parse.c
index 3ac4092..919d36c 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -524,13 +524,15 @@ int msg2exp(struct nf_expect *exp, struct nethdr *net, size_t remain)
attr = NTA_NEXT(attr, len);
continue;
}
- switch(exp_h[attr->nta_attr].exp_attr) {
+ switch (exp_h[attr->nta_attr].exp_attr) {
case ATTR_EXP_MASTER:
exp_h[attr->nta_attr].parse(master, attr->nta_attr,
NTA_DATA(attr));
+ break;
case ATTR_EXP_EXPECTED:
exp_h[attr->nta_attr].parse(expected, attr->nta_attr,
NTA_DATA(attr));
+ break;
case ATTR_EXP_MASK:
exp_h[attr->nta_attr].parse(mask, attr->nta_attr,
NTA_DATA(attr));
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-08-18 17:22 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-18 17:28 [PATCH conntrackd 0/8] unsorted fixes Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 1/8] conntrackd: fix sanitization of expection attribute in the wire format Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 2/8] conntrackd: NTA_MAX is also an invalid attribute Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 3/8] conntrackd: fix leak in fork_process_new() Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 4/8] conntrackd: fix descriptor leak in do_local_request() Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 5/8] conntrackd: fix error handling in nfq_queue_cb() Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 6/8] conntrackd: simplify branch in tcp_accept() Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 7/8] conntrackd: use strncpy to set up the cache name Pablo Neira Ayuso
2015-08-18 17:28 ` [PATCH conntrackd 8/8] conntrackd: missing break in expectation message parser function Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).