[iptables PATCH 0/2] Critical: Unbreak nfnl_osf tool

[iptables PATCH 0/2] Critical: Unbreak nfnl_osf tool

[Phil Sutter]

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

[iptables PATCH 1/2] nfnl_osf: Fix broken conversion to nfnl_query()

[Phil Sutter]

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

[iptables PATCH 2/2] nfnl_osf: Improve error handling

[Phil Sutter]

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

Re: [iptables PATCH 2/2] nfnl_osf: Improve error handling

[Pablo Neira Ayuso]

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?

Re: [iptables PATCH 2/2] nfnl_osf: Improve error handling

[Phil Sutter]

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

Re: [iptables PATCH 2/2] nfnl_osf: Improve error handling

[Pablo Neira Ayuso]

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 :-)

Re: [iptables PATCH 2/2] nfnl_osf: Improve error handling

[Phil Sutter]

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