From: Mark Johnston <markj@freebsd.org>
Subject: Re: fix a gap in relayd config validation
To: tech@openbsd.org
Date: Tue, 6 Aug 2024 15:24:15 -0400

On Fri, Jul 05, 2024 at 11:02:03AM -0400, Mark Johnston wrote:
> relayd has a -n option which lets one validate a relayd.conf(5)
> configuration prior to starting the daemon.  There is a case where
> validation succeeds but relayd fails to start anyway: when a redirect
> rule name is longer than pf's limit.  This is fixed by the patch below.
> 
> More specifically, relayd uses a buffer of size SRV_NAME_SIZE (64) to
> store the rule name, whereas pf's limit is PF_TABLE_NAME_SIZE (32).
> relayd's parser checks for truncation of the name, but because of the
> size mismatch, it doesn't catch the case where the name fits in relayd's
> buffer but is too long for pf.  The patch simply uses pf's constants to
> size the name and tag buffers, so that a rule like
> 
> 	redirect "thisnameisabitlongerthanthepflimit" {
> 	  listen on 127.0.0.1 tcp port 80
> 	  forward to <localhost> port 80 check http "/" code 200
> 	}
> 
> triggers an error from the parser before init_tables() tries to copy it
> and fails.  To make life a bit easier, the offending name is now printed
> by the parser as well.
> 
> I tested the patch on an OpenBSD 7.5 system and against the regression
> suite.

Is anyone else interested in reviewing and perhaps committing the patch
below?  I see one OK from Florian here:
https://marc.info/?l=openbsd-tech&m=172044845706968&w=2

Thanks in advance.

> diff --git a/usr.sbin/relayd/parse.y b/usr.sbin/relayd/parse.y
> index 739ff164e2e..a559fec40e7 100644
> --- a/usr.sbin/relayd/parse.y
> +++ b/usr.sbin/relayd/parse.y
> @@ -490,7 +490,7 @@ rdr		: REDIRECT STRING	{
>  			if (strlcpy(srv->conf.name, $2,
>  			    sizeof(srv->conf.name)) >=
>  			    sizeof(srv->conf.name)) {
> -				yyerror("redirection name truncated");
> +				yyerror("redirection name truncated: %s", $2);
>  				free($2);
>  				free(srv);
>  				YYERROR;
> @@ -628,7 +628,8 @@ rdroptsl	: forwardmode TO tablespec interface	{
>  			if (strlcpy(rdr->conf.tag, $3,
>  			    sizeof(rdr->conf.tag)) >=
>  			    sizeof(rdr->conf.tag)) {
> -				yyerror("redirection tag name truncated");
> +				yyerror("redirection tag name truncated: %s",
> +				    $3);
>  				free($3);
>  				YYERROR;
>  			}
> diff --git a/usr.sbin/relayd/relayd.h b/usr.sbin/relayd/relayd.h
> index 2f55c2bb23b..9195be57ab0 100644
> --- a/usr.sbin/relayd/relayd.h
> +++ b/usr.sbin/relayd/relayd.h
> @@ -57,9 +57,7 @@
>  #define LABEL_NAME_SIZE		1024
>  #define TAG_NAME_SIZE		64
>  #define TABLE_NAME_SIZE		64
> -#define	RD_TAG_NAME_SIZE	64
>  #define	RT_LABEL_SIZE		32
> -#define SRV_NAME_SIZE		64
>  #define MAX_NAME_SIZE		64
>  #define SRV_MAX_VIRTS		16
>  #define TLS_NAME_SIZE		512
> @@ -545,8 +543,8 @@ struct rdr_config {
>  	objid_t			 backup_id;
>  	int			 mode;
>  	union hashkey		 key;
> -	char			 name[SRV_NAME_SIZE];
> -	char			 tag[RD_TAG_NAME_SIZE];
> +	char			 name[PF_TABLE_NAME_SIZE];
> +	char			 tag[PF_TAG_NAME_SIZE];
>  	struct timeval		 timeout;
>  };
>