[PATCH weston] libweston: Make module loading safe against long paths

[Peter Hutterer]

On Tue, Nov 29, 2016 at 10:18:30AM +0000, Daniel Stone wrote:
> Avoid any buffer overflows here by checking we don't go over PATH_MAX
> with stupid module names.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>

Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>

Cheers,
   Peter

> ---
>  compositor/main.c      | 15 ++++++++++++---
>  libweston/compositor.c | 15 ++++++++++++---
>  2 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/compositor/main.c b/compositor/main.c
> index 080aa61..2aa4936 100644
> --- a/compositor/main.c
> +++ b/compositor/main.c
> @@ -766,19 +766,28 @@ wet_load_module(const char *name, const char *entrypoint)
>  	const char *builddir = getenv("WESTON_BUILD_DIR");
>  	char path[PATH_MAX];
>  	void *module, *init;
> +	size_t len;
>  
>  	if (name == NULL)
>  		return NULL;
>  
>  	if (name[0] != '/') {
>  		if (builddir)
> -			snprintf(path, sizeof path, "%s/.libs/%s", builddir, name);
> +			len = snprintf(path, sizeof path, "%s/.libs/%s", builddir,
> +				       name);
>  		else
> -			snprintf(path, sizeof path, "%s/%s", MODULEDIR, name);
> +			len = snprintf(path, sizeof path, "%s/%s", MODULEDIR,
> +				       name);
>  	} else {
> -		snprintf(path, sizeof path, "%s", name);
> +		len = snprintf(path, sizeof path, "%s", name);
>  	}
>  
> +	/* snprintf returns the length of the string it would've written,
> +	 * _excluding_ the NUL byte. So even being equal to the size of
> +	 * our buffer is an error here. */
> +	if (len >= sizeof path)
> +		return NULL;
> +
>  	module = dlopen(path, RTLD_NOW | RTLD_NOLOAD);
>  	if (module) {
>  		weston_log("Module '%s' already loaded\n", path);
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index ee0a007..6457858 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -5225,19 +5225,28 @@ weston_load_module(const char *name, const char *entrypoint)
>  	const char *builddir = getenv("WESTON_BUILD_DIR");
>  	char path[PATH_MAX];
>  	void *module, *init;
> +	size_t len;
>  
>  	if (name == NULL)
>  		return NULL;
>  
>  	if (name[0] != '/') {
>  		if (builddir)
> -			snprintf(path, sizeof path, "%s/.libs/%s", builddir, name);
> +			len = snprintf(path, sizeof path, "%s/.libs/%s",
> +				       builddir, name);
>  		else
> -			snprintf(path, sizeof path, "%s/%s", LIBWESTON_MODULEDIR, name);
> +			len = snprintf(path, sizeof path, "%s/%s",
> +				       LIBWESTON_MODULEDIR, name);
>  	} else {
> -		snprintf(path, sizeof path, "%s", name);
> +		len = snprintf(path, sizeof path, "%s", name);
>  	}
>  
> +	/* snprintf returns the length of the string it would've written,
> +	 * _excluding_ the NUL byte. So even being equal to the size of
> +	 * our buffer is an error here. */
> +	if (len >= sizeof path)
> +		return NULL;
> +
>  	module = dlopen(path, RTLD_NOW | RTLD_NOLOAD);
>  	if (module) {
>  		weston_log("Module '%s' already loaded\n", path);
> -- 
> 2.9.3
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>