All of lore.kernel.org
 help / color / mirror / Atom feed
* [iptables PATCH 0/2] Critical: Unbreak nfnl_osf tool
@ 2020-05-09 11:51 Phil Sutter
  2020-05-09 11:51 ` [iptables PATCH 1/2] nfnl_osf: Fix broken conversion to nfnl_query() Phil Sutter
  2020-05-09 11:52 ` [iptables PATCH 2/2] nfnl_osf: Improve error handling Phil Sutter
  0 siblings, 2 replies; 7+ messages in thread
From: Phil Sutter @ 2020-05-09 11:51 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

I managed to render nfnl_osf tool useless with my (obviously untested)
conversion to nfnl_query(). Unbreak it and also fix delete functionality
which was already broken before I started messing with it.

Phil Sutter (2):
  nfnl_osf: Fix broken conversion to nfnl_query()
  nfnl_osf: Improve error handling

 utils/nfnl_osf.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [iptables PATCH 1/2] nfnl_osf: Fix broken conversion to nfnl_query()
  2020-05-09 11:51 [iptables PATCH 0/2] Critical: Unbreak nfnl_osf tool Phil Sutter
@ 2020-05-09 11:51 ` Phil Sutter
  2020-05-09 11:52 ` [iptables PATCH 2/2] nfnl_osf: Improve error handling Phil Sutter
  1 sibling, 0 replies; 7+ messages in thread
From: Phil Sutter @ 2020-05-09 11:51 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Due to missing NLM_F_ACK flag in request, nfnetlink code in kernel
didn't create an own ACK message but left it upon subsystem to ACK or
not. Since nfnetlink_osf doesn't ACK by itself, nfnl_query() got stuck
waiting for a reply.

Whoever did the conversion from deprecated nfnl_talk() obviously didn't
even test basic functionality of the tool.

Fixes: 52aa15098ebd6 ("nfnl_osf: Replace deprecated nfnl_talk() by nfnl_query()")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 utils/nfnl_osf.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/utils/nfnl_osf.c b/utils/nfnl_osf.c
index 15d531975e11d..922d90ac135b7 100644
--- a/utils/nfnl_osf.c
+++ b/utils/nfnl_osf.c
@@ -378,9 +378,11 @@ static int osf_load_line(char *buffer, int len, int del)
 	memset(buf, 0, sizeof(buf));
 
 	if (del)
-		nfnl_fill_hdr(nfnlssh, nmh, 0, AF_UNSPEC, 0, OSF_MSG_REMOVE, NLM_F_REQUEST);
+		nfnl_fill_hdr(nfnlssh, nmh, 0, AF_UNSPEC, 0, OSF_MSG_REMOVE,
+			      NLM_F_ACK | NLM_F_REQUEST);
 	else
-		nfnl_fill_hdr(nfnlssh, nmh, 0, AF_UNSPEC, 0, OSF_MSG_ADD, NLM_F_REQUEST | NLM_F_CREATE);
+		nfnl_fill_hdr(nfnlssh, nmh, 0, AF_UNSPEC, 0, OSF_MSG_ADD,
+			      NLM_F_ACK | NLM_F_REQUEST | NLM_F_CREATE);
 
 	nfnl_addattr_l(nmh, sizeof(buf), OSF_ATTR_FINGER, &f, sizeof(struct xt_osf_user_finger));
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [iptables PATCH 2/2] nfnl_osf: Improve error handling
  2020-05-09 11:51 [iptables PATCH 0/2] Critical: Unbreak nfnl_osf tool Phil Sutter
  2020-05-09 11:51 ` [iptables PATCH 1/2] nfnl_osf: Fix broken conversion to nfnl_query() Phil Sutter
@ 2020-05-09 11:52 ` Phil Sutter
  2020-05-09 17:28   ` Pablo Neira Ayuso
  1 sibling, 1 reply; 7+ messages in thread
From: Phil Sutter @ 2020-05-09 11:52 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

For some error cases, no log message was created - hence apart from the
return code there was no indication of failing execution.

When loading a line fails, don't abort but continue with the remaining
file contents. The current pf.os file in this repository serves as
proof-of-concept: Loading all entries succeeds, but when deleting, lines
700, 701 and 704 return ENOENT. Not continuing means the remaining
entries are not cleared.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 utils/nfnl_osf.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/utils/nfnl_osf.c b/utils/nfnl_osf.c
index 922d90ac135b7..8a74423fc8428 100644
--- a/utils/nfnl_osf.c
+++ b/utils/nfnl_osf.c
@@ -392,7 +392,7 @@ static int osf_load_line(char *buffer, int len, int del)
 static int osf_load_entries(char *path, int del)
 {
 	FILE *inf;
-	int err = 0;
+	int err = 0, lineno = 0;
 	char buf[1024];
 
 	inf = fopen(path, "r");
@@ -402,7 +402,9 @@ static int osf_load_entries(char *path, int del)
 	}
 
 	while(fgets(buf, sizeof(buf), inf)) {
-		int len;
+		int len, rc;
+
+		lineno++;
 
 		if (buf[0] == '#' || buf[0] == '\n' || buf[0] == '\r')
 			continue;
@@ -414,9 +416,11 @@ static int osf_load_entries(char *path, int del)
 
 		buf[len] = '\0';
 
-		err = osf_load_line(buf, len, del);
-		if (err)
-			break;
+		rc = osf_load_line(buf, len, del);
+		if (rc) {
+			ulog_err("Failed to load line %d", lineno);
+			err = rc;
+		}
 
 		memset(buf, 0, sizeof(buf));
 	}
@@ -448,6 +452,7 @@ int main(int argc, char *argv[])
 
 	if (!fingerprints) {
 		err = -ENOENT;
+		ulog_err("Missing fingerprints file argument");
 		goto err_out_exit;
 	}
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [iptables PATCH 2/2] nfnl_osf: Improve error handling
  2020-05-09 11:52 ` [iptables PATCH 2/2] nfnl_osf: Improve error handling Phil Sutter
@ 2020-05-09 17:28   ` Pablo Neira Ayuso
  2020-05-11 11:31     ` Phil Sutter
  0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2020-05-09 17:28 UTC (permalink / raw)
  To: Phil Sutter; +Cc: netfilter-devel

On Sat, May 09, 2020 at 01:52:00PM +0200, Phil Sutter wrote:
> For some error cases, no log message was created - hence apart from the
> return code there was no indication of failing execution.
> 
> When loading a line fails, don't abort but continue with the remaining
> file contents. The current pf.os file in this repository serves as
> proof-of-concept: Loading all entries succeeds, but when deleting, lines
> 700, 701 and 704 return ENOENT. Not continuing means the remaining
> entries are not cleared.

Did you look at why are these lines returning ENOENT?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [iptables PATCH 2/2] nfnl_osf: Improve error handling
  2020-05-09 17:28   ` Pablo Neira Ayuso
@ 2020-05-11 11:31     ` Phil Sutter
  2020-05-12 12:49       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 7+ messages in thread
From: Phil Sutter @ 2020-05-11 11:31 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Hi Pablo,

On Sat, May 09, 2020 at 07:28:07PM +0200, Pablo Neira Ayuso wrote:
> On Sat, May 09, 2020 at 01:52:00PM +0200, Phil Sutter wrote:
> > For some error cases, no log message was created - hence apart from the
> > return code there was no indication of failing execution.
> > 
> > When loading a line fails, don't abort but continue with the remaining
> > file contents. The current pf.os file in this repository serves as
> > proof-of-concept: Loading all entries succeeds, but when deleting, lines
> > 700, 701 and 704 return ENOENT. Not continuing means the remaining
> > entries are not cleared.
> 
> Did you look at why are these lines returning ENOENT?

If I understand the code right, line 700 is a duplicate of line 698, 701
of 699 and 704 of 702. This is because 'W*' parses identical to 'W0' and
in right-hand side only the first three text fields (genre, version and
subtype) are relevant - the rest is ignored.

When adding, this doesn't become visible because flag NLM_F_EXCL is not
specified. If it is, kernel returns EEXISTS for those lines.

Cheers, Phil

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [iptables PATCH 2/2] nfnl_osf: Improve error handling
  2020-05-11 11:31     ` Phil Sutter
@ 2020-05-12 12:49       ` Pablo Neira Ayuso
  2020-05-12 13:34         ` Phil Sutter
  0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2020-05-12 12:49 UTC (permalink / raw)
  To: Phil Sutter, netfilter-devel

On Mon, May 11, 2020 at 01:31:12PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Sat, May 09, 2020 at 07:28:07PM +0200, Pablo Neira Ayuso wrote:
> > On Sat, May 09, 2020 at 01:52:00PM +0200, Phil Sutter wrote:
> > > For some error cases, no log message was created - hence apart from the
> > > return code there was no indication of failing execution.
> > > 
> > > When loading a line fails, don't abort but continue with the remaining
> > > file contents. The current pf.os file in this repository serves as
> > > proof-of-concept: Loading all entries succeeds, but when deleting, lines
> > > 700, 701 and 704 return ENOENT. Not continuing means the remaining
> > > entries are not cleared.
> > 
> > Did you look at why are these lines returning ENOENT?
> 
> If I understand the code right, line 700 is a duplicate of line 698, 701
> of 699 and 704 of 702. This is because 'W*' parses identical to 'W0' and
> in right-hand side only the first three text fields (genre, version and
> subtype) are relevant - the rest is ignored.

I see, in the userspace parser, W0 and W* are being handled as
OSF_WSS_PLAIN.

> When adding, this doesn't become visible because flag NLM_F_EXCL is not
> specified. If it is, kernel returns EEXISTS for those lines.

In the kernel, the struct nf_osf_user_finger is used as key to
identify each line, given they are identical.

So it looks like this EEXIST has been there since the beginning.

This patchset LGTM, it's just that the user might get confused if it
see errors when using this tool, probably turning this into a warning
is fine.

Or at least, include this information in the commit message so this
does not get lost :-)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [iptables PATCH 2/2] nfnl_osf: Improve error handling
  2020-05-12 12:49       ` Pablo Neira Ayuso
@ 2020-05-12 13:34         ` Phil Sutter
  0 siblings, 0 replies; 7+ messages in thread
From: Phil Sutter @ 2020-05-12 13:34 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Hi Pablo,

On Tue, May 12, 2020 at 02:49:49PM +0200, Pablo Neira Ayuso wrote:
> On Mon, May 11, 2020 at 01:31:12PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Sat, May 09, 2020 at 07:28:07PM +0200, Pablo Neira Ayuso wrote:
> > > On Sat, May 09, 2020 at 01:52:00PM +0200, Phil Sutter wrote:
> > > > For some error cases, no log message was created - hence apart from the
> > > > return code there was no indication of failing execution.
> > > > 
> > > > When loading a line fails, don't abort but continue with the remaining
> > > > file contents. The current pf.os file in this repository serves as
> > > > proof-of-concept: Loading all entries succeeds, but when deleting, lines
> > > > 700, 701 and 704 return ENOENT. Not continuing means the remaining
> > > > entries are not cleared.
> > > 
> > > Did you look at why are these lines returning ENOENT?
> > 
> > If I understand the code right, line 700 is a duplicate of line 698, 701
> > of 699 and 704 of 702. This is because 'W*' parses identical to 'W0' and
> > in right-hand side only the first three text fields (genre, version and
> > subtype) are relevant - the rest is ignored.
> 
> I see, in the userspace parser, W0 and W* are being handled as
> OSF_WSS_PLAIN.
> 
> > When adding, this doesn't become visible because flag NLM_F_EXCL is not
> > specified. If it is, kernel returns EEXISTS for those lines.
> 
> In the kernel, the struct nf_osf_user_finger is used as key to
> identify each line, given they are identical.
> 
> So it looks like this EEXIST has been there since the beginning.
> 
> This patchset LGTM, it's just that the user might get confused if it
> see errors when using this tool, probably turning this into a warning
> is fine.

Yes, at least it's unfortunate that the default fingerprint file
triggers them. We could drop the offending lines, but then again re-sync
with OpenBSD won't be trivial anymore.

From my PoV we may also just ignore the error conditions. Most important
bit here is to not stop on error, at least not when deleting.

> Or at least, include this information in the commit message so this
> does not get lost :-)

Yes, I'll extend the commit message. Thanks for the reminder.

Cheers, Phil

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-05-12 13:34 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-09 11:51 [iptables PATCH 0/2] Critical: Unbreak nfnl_osf tool Phil Sutter
2020-05-09 11:51 ` [iptables PATCH 1/2] nfnl_osf: Fix broken conversion to nfnl_query() Phil Sutter
2020-05-09 11:52 ` [iptables PATCH 2/2] nfnl_osf: Improve error handling Phil Sutter
2020-05-09 17:28   ` Pablo Neira Ayuso
2020-05-11 11:31     ` Phil Sutter
2020-05-12 12:49       ` Pablo Neira Ayuso
2020-05-12 13:34         ` Phil Sutter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.