[PATCH] pcap: prevent crashes when output `FILE *` is null

[PATCH] 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, check that the pointer is not null before using it.  If it is
null, then periodically attempt to open it again.  We only return an
error from interp_pcap on those occasions when we try and fail to open
the output file, in order to avoid spamming the ulogd log-file every
time a packet isn't written.

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

diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c
index e7798f20c8fc..5b2ca64d3393 100644
--- a/output/pcap/ulogd_output_PCAP.c
+++ b/output/pcap/ulogd_output_PCAP.c
@@ -82,6 +82,8 @@ struct pcap_sf_pkthdr {
 #define ULOGD_PCAP_SYNC_DEFAULT	0
 #endif
 
+#define MAX_OUTFILE_CHECK_DELTA 64
+
 #define NIPQUAD(addr) \
 	((unsigned char *)&addr)[0], \
 	((unsigned char *)&addr)[1], \
@@ -107,6 +109,7 @@ static struct config_keyset pcap_kset = {
 };
 
 struct pcap_instance {
+	time_t last_outfile_check, next_outfile_check_delta;
 	FILE *of;
 };
 
@@ -142,12 +145,53 @@ static struct ulogd_key pcap_keys[INTR_IDS] = {
 
 #define GET_FLAGS(res, x)	(res[x].u.source->flags)
 
+static int append_create_outfile(struct ulogd_pluginstance *);
+
+static int
+check_outfile(struct ulogd_pluginstance *upi)
+{
+	struct pcap_instance *pi = (struct pcap_instance *) &upi->private;
+	time_t now;
+	int ret;
+
+	if (pi->of)
+		return 0;
+
+	now = time(NULL);
+
+	if (now < pi->last_outfile_check + pi->next_outfile_check_delta)
+		return -EAGAIN;
+
+	ret = append_create_outfile(upi);
+
+	if (!ret) {
+		pi->last_outfile_check = 0;
+		pi->next_outfile_check_delta = 1;
+		return 0;
+	}
+
+	pi->last_outfile_check = now;
+	if (pi->next_outfile_check_delta <= MAX_OUTFILE_CHECK_DELTA / 2)
+		pi->next_outfile_check_delta *= 2;
+
+	return ret;
+}
+
 static int interp_pcap(struct ulogd_pluginstance *upi)
 {
 	struct pcap_instance *pi = (struct pcap_instance *) &upi->private;
 	struct ulogd_key *res = upi->input.keys;
 	struct pcap_sf_pkthdr pchdr;
 
+	switch (check_outfile(upi)) {
+	case 0:
+		break;
+	case -EAGAIN:
+		return ULOGD_IRET_OK;
+	default:
+		return ULOGD_IRET_ERR;
+	}
+
 	pchdr.caplen = ikey_get_u32(&res[1]);
 
 	/* Try to set the len field correctly, if we know the protocol. */
@@ -275,6 +319,11 @@ static int configure_pcap(struct ulogd_pluginstance *upi,
 
 static int start_pcap(struct ulogd_pluginstance *upi)
 {
+	struct pcap_instance *pi = (struct pcap_instance *) &upi->private;
+
+	pi->last_outfile_check = 0;
+	pi->next_outfile_check_delta = 1;
+
 	return append_create_outfile(upi);
 }
 
-- 
2.39.0

[PATCH ulogd2 v2] 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, check that the pointer is not null before using it.  If it is
null, then periodically attempt to open it again.  We only return an
error from interp_pcap on those occasions when we try and fail to open
the output file, in order to avoid spamming the ulogd log-file every
time a packet isn't written.

Link: https://bugs.launchpad.net/ubuntu/+source/ulogd2/+bug/1429778
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 Change since v1: correct subject-prefix.
 
 output/pcap/ulogd_output_PCAP.c | 49 +++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c
index e7798f20c8fc..5b2ca64d3393 100644
--- a/output/pcap/ulogd_output_PCAP.c
+++ b/output/pcap/ulogd_output_PCAP.c
@@ -82,6 +82,8 @@ struct pcap_sf_pkthdr {
 #define ULOGD_PCAP_SYNC_DEFAULT	0
 #endif
 
+#define MAX_OUTFILE_CHECK_DELTA 64
+
 #define NIPQUAD(addr) \
 	((unsigned char *)&addr)[0], \
 	((unsigned char *)&addr)[1], \
@@ -107,6 +109,7 @@ static struct config_keyset pcap_kset = {
 };
 
 struct pcap_instance {
+	time_t last_outfile_check, next_outfile_check_delta;
 	FILE *of;
 };
 
@@ -142,12 +145,53 @@ static struct ulogd_key pcap_keys[INTR_IDS] = {
 
 #define GET_FLAGS(res, x)	(res[x].u.source->flags)
 
+static int append_create_outfile(struct ulogd_pluginstance *);
+
+static int
+check_outfile(struct ulogd_pluginstance *upi)
+{
+	struct pcap_instance *pi = (struct pcap_instance *) &upi->private;
+	time_t now;
+	int ret;
+
+	if (pi->of)
+		return 0;
+
+	now = time(NULL);
+
+	if (now < pi->last_outfile_check + pi->next_outfile_check_delta)
+		return -EAGAIN;
+
+	ret = append_create_outfile(upi);
+
+	if (!ret) {
+		pi->last_outfile_check = 0;
+		pi->next_outfile_check_delta = 1;
+		return 0;
+	}
+
+	pi->last_outfile_check = now;
+	if (pi->next_outfile_check_delta <= MAX_OUTFILE_CHECK_DELTA / 2)
+		pi->next_outfile_check_delta *= 2;
+
+	return ret;
+}
+
 static int interp_pcap(struct ulogd_pluginstance *upi)
 {
 	struct pcap_instance *pi = (struct pcap_instance *) &upi->private;
 	struct ulogd_key *res = upi->input.keys;
 	struct pcap_sf_pkthdr pchdr;
 
+	switch (check_outfile(upi)) {
+	case 0:
+		break;
+	case -EAGAIN:
+		return ULOGD_IRET_OK;
+	default:
+		return ULOGD_IRET_ERR;
+	}
+
 	pchdr.caplen = ikey_get_u32(&res[1]);
 
 	/* Try to set the len field correctly, if we know the protocol. */
@@ -275,6 +319,11 @@ static int configure_pcap(struct ulogd_pluginstance *upi,
 
 static int start_pcap(struct ulogd_pluginstance *upi)
 {
+	struct pcap_instance *pi = (struct pcap_instance *) &upi->private;
+
+	pi->last_outfile_check = 0;
+	pi->next_outfile_check_delta = 1;
+
 	return append_create_outfile(upi);
 }
 
-- 
2.39.0

Re: [PATCH ulogd2 v2] 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.
> 
> Instead, check that the pointer is not null before using it.  If it is
> null, then periodically attempt to open it again.  We only return an
> error from interp_pcap on those occasions when we try and fail to open
> the output file, in order to avoid spamming the ulogd log-file every
> time a packet isn't written.
> 
> Link: https://bugs.launchpad.net/ubuntu/+source/ulogd2/+bug/1429778
>
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  Change since v1: correct subject-prefix.
>  
>  output/pcap/ulogd_output_PCAP.c | 49 +++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c
> index e7798f20c8fc..5b2ca64d3393 100644
> --- a/output/pcap/ulogd_output_PCAP.c
> +++ b/output/pcap/ulogd_output_PCAP.c
> @@ -82,6 +82,8 @@ struct pcap_sf_pkthdr {
>  #define ULOGD_PCAP_SYNC_DEFAULT	0
>  #endif
>  
> +#define MAX_OUTFILE_CHECK_DELTA 64
> +
>  #define NIPQUAD(addr) \
>  	((unsigned char *)&addr)[0], \
>  	((unsigned char *)&addr)[1], \
> @@ -107,6 +109,7 @@ static struct config_keyset pcap_kset = {
>  };
>  
>  struct pcap_instance {
> +	time_t last_outfile_check, next_outfile_check_delta;
>  	FILE *of;
>  };
>  
> @@ -142,12 +145,53 @@ static struct ulogd_key pcap_keys[INTR_IDS] = {
>  
>  #define GET_FLAGS(res, x)	(res[x].u.source->flags)
>  
> +static int append_create_outfile(struct ulogd_pluginstance *);
> +
> +static int
> +check_outfile(struct ulogd_pluginstance *upi)
> +{
> +	struct pcap_instance *pi = (struct pcap_instance *) &upi->private;
> +	time_t now;
> +	int ret;
> +
> +	if (pi->of)
> +		return 0;

I think its better to fix this at the source, i.e. in
signal_handler_task().  It should probably *first* try to open the file,
and only close the old one if that worked.

Does that make sense to you?

Re: [PATCH ulogd2 v2] pcap: prevent crashes when output `FILE *` is null

[Jeremy Sowden]

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

On 2023-03-15, at 22:44:47 +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.
> > 
> > Instead, check that the pointer is not null before using it.  If it is
> > null, then periodically attempt to open it again.  We only return an
> > error from interp_pcap on those occasions when we try and fail to open
> > the output file, in order to avoid spamming the ulogd log-file every
> > time a packet isn't written.
> 
> I think its better to fix this at the source, i.e. in
> signal_handler_task().  It should probably *first* try to open the file,
> and only close the old one if that worked.
> 
> Does that make sense to you?

Yeah, that would be simpler.  v3 to follow.

J.

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