From: Stefan Sperling <stsp@stsp.name>
Subject: Re: Assert against argv[] overflow in got_dial_ssh()
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: gameoftrees@openbsd.org
Date: Sun, 5 Sep 2021 23:43:14 +0200

On Sun, Sep 05, 2021 at 11:12:22PM +0200, Christian Weisgerber wrote:
> Everytime I look at dial_ssh(), I see a caltrop on the street waiting
> for somebody to step on.
> 
> Can we assert() against somebody adding an extra argument and
> accidentally overflowing argv[]?

Sure. This sems like a good idea.
I think we even stepped into this trap before, if I recall correctly?

> diff 5e5da8c4bcc83f7737a115b8da52fc3935fe3a6b /home/naddy/got
> blob - 1220c48e7f2ace97395c12e5a3af43b9c0bdc410
> file + lib/dial.c
> --- lib/dial.c
> +++ lib/dial.c
> @@ -20,6 +20,7 @@
>  #include <sys/socket.h>
>  #include <netdb.h>
>  
> +#include <assert.h>
>  #include <err.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -31,6 +32,10 @@
>  
>  #include "got_lib_dial.h"
>  
> +#ifndef nitems
> +#define nitems(_a) (sizeof((_a)) / sizeof((_a)[0]))
> +#endif
> +
>  #ifndef ssizeof
>  #define ssizeof(_x) ((ssize_t)(sizeof(_x)))
>  #endif
> @@ -216,6 +221,7 @@ got_dial_ssh(pid_t *newpid, int *newfd, const char *ho
>  	argv[i++] = (char *)cmd;
>  	argv[i++] = (char *)path;
>  	argv[i++] = NULL;
> +	assert(i <= nitems(argv));
>  
>  	if (socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, pfd) == -1)
>  		return got_error_from_errno("socketpair");
> -- 
> Christian "naddy" Weisgerber                          naddy@mips.inka.de
> 
>