[PATCH ulogd2 v3 0/2] pcap: prevent crashes when output `FILE *` is null

[PATCH ulogd2 v3 0/2] pcap: prevent crashes when output `FILE *` is null

[Jeremy Sowden]

If ulogd2 receives a signal it will attempt to re-open the pcap output
file.  If this fails (because the permissions or ownership have changed
for example), the FILE pointer will be null and when the next packet
comes in, the null pointer will be passed to fwrite and ulogd will
crash.

The first patch simplifies the logic of the code that opens the output
file, and the second avoids closing the existing stream if `fopen`
fails.

Link: https://bugs.launchpad.net/ubuntu/+source/ulogd2/+bug/1429778

Change since v2

 * The first patch is new.
 * In the second patch, just keep the old stream open, rather than
   disabling output and trying to reopen at intervals.

Change since v1

 * Correct subject-prefix.

Jeremy Sowden (2):
  pcap: simplify opening of output file
  pcap: prevent crashes when output `FILE *` is null

 output/pcap/ulogd_output_PCAP.c | 50 +++++++++++++--------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

-- 
2.39.2

[PATCH ulogd2 v3 1/2] pcap: simplify opening of output file

[Jeremy Sowden]

Instead of statting the file, and choosing the mode with which to open
it and whether to write the PCAP header based on the result, always open
it with mode "a" and _then_ stat it.  This simplifies the flow-control
and avoids a race between statting and opening.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/pcap/ulogd_output_PCAP.c | 42 ++++++++++++---------------------
 1 file changed, 15 insertions(+), 27 deletions(-)

diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c
index e7798f20c8fc..220fc6dec5fe 100644
--- a/output/pcap/ulogd_output_PCAP.c
+++ b/output/pcap/ulogd_output_PCAP.c
@@ -220,33 +220,21 @@ static int append_create_outfile(struct ulogd_pluginstance *upi)
 {
 	struct pcap_instance *pi = (struct pcap_instance *) &upi->private;
 	char *filename = upi->config_kset->ces[0].u.string;
-	struct stat st_dummy;
-	int exist = 0;
-
-	if (stat(filename, &st_dummy) == 0 && st_dummy.st_size > 0)
-		exist = 1;
-
-	if (!exist) {
-		pi->of = fopen(filename, "w");
-		if (!pi->of) {
-			ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n",
-				  filename,
-				  strerror(errno));
-			return -EPERM;
-		}
-		if (!write_pcap_header(pi)) {
-			ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n",
-				  strerror(errno));
-			return -ENOSPC;
-		}
-	} else {
-		pi->of = fopen(filename, "a");
-		if (!pi->of) {
-			ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", 
-				filename,
-				strerror(errno));
-			return -EPERM;
-		}		
+	struct stat st_of;
+
+	pi->of = fopen(filename, "a");
+	if (!pi->of) {
+		ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n",
+			  filename,
+			  strerror(errno));
+		return -EPERM;
+	}
+	if (fstat(fileno(pi->of), &st_of) == 0 && st_of.st_size == 0) {
+	    if (!write_pcap_header(pi)) {
+		    ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n",
+			      strerror(errno));
+		    return -ENOSPC;
+	    }
 	}
 
 	return 0;
-- 
2.39.2

Re: [PATCH ulogd2 v3 1/2] pcap: simplify opening of output file

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> Instead of statting the file, and choosing the mode with which to open
> it and whether to write the PCAP header based on the result, always open
> it with mode "a" and _then_ stat it.  This simplifies the flow-control
> and avoids a race between statting and opening.
> 
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  output/pcap/ulogd_output_PCAP.c | 42 ++++++++++++---------------------
>  1 file changed, 15 insertions(+), 27 deletions(-)
> 
> diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c
> index e7798f20c8fc..220fc6dec5fe 100644
> --- a/output/pcap/ulogd_output_PCAP.c
> +++ b/output/pcap/ulogd_output_PCAP.c
> @@ -220,33 +220,21 @@ static int append_create_outfile(struct ulogd_pluginstance *upi)
>  {

> +	struct stat st_of;
> +
> +	pi->of = fopen(filename, "a");
> +	if (!pi->of) {
> +		ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n",
> +			  filename,
> +			  strerror(errno));
> +		return -EPERM;
> +	}
> +	if (fstat(fileno(pi->of), &st_of) == 0 && st_of.st_size == 0) {
> +	    if (!write_pcap_header(pi)) {
> +		    ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n",
> +			      strerror(errno));
> +		    return -ENOSPC;

LGTM, but should that fclose() before -ENOSPC?

Re: [PATCH ulogd2 v3 1/2] pcap: simplify opening of output file

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Jeremy Sowden <jeremy@azazel.net> wrote:
> > Instead of statting the file, and choosing the mode with which to open
> > it and whether to write the PCAP header based on the result, always open
> > it with mode "a" and _then_ stat it.  This simplifies the flow-control
> > and avoids a race between statting and opening.
> > 
> > Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> > ---
> >  output/pcap/ulogd_output_PCAP.c | 42 ++++++++++++---------------------
> >  1 file changed, 15 insertions(+), 27 deletions(-)
> > 
> > diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c
> > index e7798f20c8fc..220fc6dec5fe 100644
> > --- a/output/pcap/ulogd_output_PCAP.c
> > +++ b/output/pcap/ulogd_output_PCAP.c
> > @@ -220,33 +220,21 @@ static int append_create_outfile(struct ulogd_pluginstance *upi)
> >  {
> 
> > +	struct stat st_of;
> > +
> > +	pi->of = fopen(filename, "a");
> > +	if (!pi->of) {
> > +		ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n",
> > +			  filename,
> > +			  strerror(errno));
> > +		return -EPERM;
> > +	}
> > +	if (fstat(fileno(pi->of), &st_of) == 0 && st_of.st_size == 0) {
> > +	    if (!write_pcap_header(pi)) {
> > +		    ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n",
> > +			      strerror(errno));
> > +		    return -ENOSPC;
> 
> LGTM, but should that fclose() before -ENOSPC?

AFAICS this doesn't really matter, ulogd will exit().

SIGHUP case doesn't handle errors.

Re: [PATCH ulogd2 v3 1/2] pcap: simplify opening of output file

[Pablo Neira Ayuso]

On Thu, Mar 16, 2023 at 11:07:53AM +0000, Jeremy Sowden wrote:
> Instead of statting the file, and choosing the mode with which to open
> it and whether to write the PCAP header based on the result, always open
> it with mode "a" and _then_ stat it.  This simplifies the flow-control
> and avoids a race between statting and opening.
> 
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  output/pcap/ulogd_output_PCAP.c | 42 ++++++++++++---------------------
>  1 file changed, 15 insertions(+), 27 deletions(-)
> 
> diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c
> index e7798f20c8fc..220fc6dec5fe 100644
> --- a/output/pcap/ulogd_output_PCAP.c
> +++ b/output/pcap/ulogd_output_PCAP.c
> @@ -220,33 +220,21 @@ static int append_create_outfile(struct ulogd_pluginstance *upi)
>  {
>  	struct pcap_instance *pi = (struct pcap_instance *) &upi->private;
>  	char *filename = upi->config_kset->ces[0].u.string;
> -	struct stat st_dummy;
> -	int exist = 0;
> -
> -	if (stat(filename, &st_dummy) == 0 && st_dummy.st_size > 0)
> -		exist = 1;
> -
> -	if (!exist) {
> -		pi->of = fopen(filename, "w");
> -		if (!pi->of) {
> -			ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n",
> -				  filename,
> -				  strerror(errno));
> -			return -EPERM;
> -		}
> -		if (!write_pcap_header(pi)) {
> -			ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n",
> -				  strerror(errno));
> -			return -ENOSPC;
> -		}
> -	} else {
> -		pi->of = fopen(filename, "a");
> -		if (!pi->of) {
> -			ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", 
> -				filename,
> -				strerror(errno));
> -			return -EPERM;
> -		}		
> +	struct stat st_of;
> +
> +	pi->of = fopen(filename, "a");
> +	if (!pi->of) {
> +		ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n",
> +			  filename,
> +			  strerror(errno));
> +		return -EPERM;
> +	}
> +	if (fstat(fileno(pi->of), &st_of) == 0 && st_of.st_size == 0) {
> +	    if (!write_pcap_header(pi)) {
        ^^^^
coding style nitpick, it can be fixed before applying it.

> +		    ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n",
> +			      strerror(errno));
> +		    return -ENOSPC;
> +	    }
>  	}
>  
>  	return 0;
> -- 
> 2.39.2
> 

Re: [PATCH ulogd2 v3 1/2] pcap: simplify opening of output file

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 2546 bytes --]

On 2023-03-16, at 20:02:15 +0100, Pablo Neira Ayuso wrote:
> On Thu, Mar 16, 2023 at 11:07:53AM +0000, Jeremy Sowden wrote:
> > Instead of statting the file, and choosing the mode with which to open
> > it and whether to write the PCAP header based on the result, always open
> > it with mode "a" and _then_ stat it.  This simplifies the flow-control
> > and avoids a race between statting and opening.
> > 
> > Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> > ---
> >  output/pcap/ulogd_output_PCAP.c | 42 ++++++++++++---------------------
> >  1 file changed, 15 insertions(+), 27 deletions(-)
> > 
> > diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c
> > index e7798f20c8fc..220fc6dec5fe 100644
> > --- a/output/pcap/ulogd_output_PCAP.c
> > +++ b/output/pcap/ulogd_output_PCAP.c
> > @@ -220,33 +220,21 @@ static int append_create_outfile(struct ulogd_pluginstance *upi)
> >  {
> >  	struct pcap_instance *pi = (struct pcap_instance *) &upi->private;
> >  	char *filename = upi->config_kset->ces[0].u.string;
> > -	struct stat st_dummy;
> > -	int exist = 0;
> > -
> > -	if (stat(filename, &st_dummy) == 0 && st_dummy.st_size > 0)
> > -		exist = 1;
> > -
> > -	if (!exist) {
> > -		pi->of = fopen(filename, "w");
> > -		if (!pi->of) {
> > -			ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n",
> > -				  filename,
> > -				  strerror(errno));
> > -			return -EPERM;
> > -		}
> > -		if (!write_pcap_header(pi)) {
> > -			ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n",
> > -				  strerror(errno));
> > -			return -ENOSPC;
> > -		}
> > -	} else {
> > -		pi->of = fopen(filename, "a");
> > -		if (!pi->of) {
> > -			ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", 
> > -				filename,
> > -				strerror(errno));
> > -			return -EPERM;
> > -		}		
> > +	struct stat st_of;
> > +
> > +	pi->of = fopen(filename, "a");
> > +	if (!pi->of) {
> > +		ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n",
> > +			  filename,
> > +			  strerror(errno));
> > +		return -EPERM;
> > +	}
> > +	if (fstat(fileno(pi->of), &st_of) == 0 && st_of.st_size == 0) {
> > +	    if (!write_pcap_header(pi)) {
>         ^^^^
> coding style nitpick,

Whoops.  Not sure what happened there.

> it can be fixed before applying it.

Please.

J.

> > +		    ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n",
> > +			      strerror(errno));
> > +		    return -ENOSPC;
> > +	    }
> >  	}
> >  
> >  	return 0;
> > -- 
> > 2.39.2
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH ulogd2 v3 2/2] pcap: prevent crashes when output `FILE *` is null

[Jeremy Sowden]

If ulogd2 receives a signal it will attempt to re-open the pcap output
file.  If this fails (because the permissions or ownership have changed
for example), the FILE pointer will be null and when the next packet
comes in, the null pointer will be passed to fwrite and ulogd will
crash.

Instead, assign the return value of `fopen` to a local variable, and
only close the existing stream if `fopen` succeeded.

Link: https://bugs.launchpad.net/ubuntu/+source/ulogd2/+bug/1429778
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/pcap/ulogd_output_PCAP.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c
index 220fc6dec5fe..0bb1f32b53ed 100644
--- a/output/pcap/ulogd_output_PCAP.c
+++ b/output/pcap/ulogd_output_PCAP.c
@@ -221,14 +221,18 @@ static int append_create_outfile(struct ulogd_pluginstance *upi)
 	struct pcap_instance *pi = (struct pcap_instance *) &upi->private;
 	char *filename = upi->config_kset->ces[0].u.string;
 	struct stat st_of;
+	FILE *of;
 
-	pi->of = fopen(filename, "a");
-	if (!pi->of) {
+	of = fopen(filename, "a");
+	if (!of) {
 		ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n",
 			  filename,
 			  strerror(errno));
 		return -EPERM;
 	}
+	if (pi->of)
+		fclose(pi->of);
+	pi->of = of;
 	if (fstat(fileno(pi->of), &st_of) == 0 && st_of.st_size == 0) {
 	    if (!write_pcap_header(pi)) {
 		    ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n",
@@ -236,18 +240,14 @@ static int append_create_outfile(struct ulogd_pluginstance *upi)
 		    return -ENOSPC;
 	    }
 	}
-
 	return 0;
 }
 
 static void signal_pcap(struct ulogd_pluginstance *upi, int signal)
 {
-	struct pcap_instance *pi = (struct pcap_instance *) &upi->private;
-
 	switch (signal) {
 	case SIGHUP:
 		ulogd_log(ULOGD_NOTICE, "reopening capture file\n");
-		fclose(pi->of);
 		append_create_outfile(upi);
 		break;
 	default:
-- 
2.39.2

Re: [PATCH ulogd2 v3 0/2] pcap: prevent crashes when output `FILE *` is null

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> If ulogd2 receives a signal it will attempt to re-open the pcap output
> file.  If this fails (because the permissions or ownership have changed
> for example), the FILE pointer will be null and when the next packet
> comes in, the null pointer will be passed to fwrite and ulogd will
> crash.
> 
> The first patch simplifies the logic of the code that opens the output
> file, and the second avoids closing the existing stream if `fopen`
> fails.
> 
> Link: https://bugs.launchpad.net/ubuntu/+source/ulogd2/+bug/1429778
> 
> Change since v2
> 
>  * The first patch is new.
>  * In the second patch, just keep the old stream open, rather than
>    disabling output and trying to reopen at intervals.

LGTM, thanks Jeremy.

Re: [PATCH ulogd2 v3 0/2] pcap: prevent crashes when output `FILE *` is null

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> If ulogd2 receives a signal it will attempt to re-open the pcap output
> file.  If this fails (because the permissions or ownership have changed
> for example), the FILE pointer will be null and when the next packet
> comes in, the null pointer will be passed to fwrite and ulogd will
> crash.
> 
> The first patch simplifies the logic of the code that opens the output
> file, and the second avoids closing the existing stream if `fopen`
> fails.
> 
> Link: https://bugs.launchpad.net/ubuntu/+source/ulogd2/+bug/1429778
> 
> Change since v2
> 
>  * The first patch is new.
>  * In the second patch, just keep the old stream open, rather than
>    disabling output and trying to reopen at intervals.

Applied, please double-check the mangling done in patch #1 and send
a followup fix if needed.

Re: [PATCH ulogd2 v3 0/2] pcap: prevent crashes when output `FILE *` is null

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

On 2023-03-17, at 00:36:19 +0100, Florian Westphal wrote:
> Jeremy Sowden <jeremy@azazel.net> wrote:
> > If ulogd2 receives a signal it will attempt to re-open the pcap output
> > file.  If this fails (because the permissions or ownership have changed
> > for example), the FILE pointer will be null and when the next packet
> > comes in, the null pointer will be passed to fwrite and ulogd will
> > crash.
> > 
> > The first patch simplifies the logic of the code that opens the output
> > file, and the second avoids closing the existing stream if `fopen`
> > fails.
> > 
> > Link: https://bugs.launchpad.net/ubuntu/+source/ulogd2/+bug/1429778
> > 
> > Change since v2
> > 
> >  * The first patch is new.
> >  * In the second patch, just keep the old stream open, rather than
> >    disabling output and trying to reopen at intervals.
> 
> Applied, please double-check the mangling done in patch #1 and send
> a followup fix if needed.

Thanks, Florian.  LGTM.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]