Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 1 Mar 2020 02:17:37 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] musl: lutimes: Add checks for input parameters

On Sun, Mar 01, 2020 at 02:57:30PM +0800, Liu Jie wrote:
> For the input parameter struct timeval tv, need to
> determine whether it is invalid inputs.
> 
> Signed-off-by: Liu Jie <liujie1@...wei.com>
> ---
>  src/legacy/lutimes.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/src/legacy/lutimes.c b/src/legacy/lutimes.c
> index 2e5502d1..6e7e1be3 100644
> --- a/src/legacy/lutimes.c
> +++ b/src/legacy/lutimes.c
> @@ -2,13 +2,22 @@
>  #include <sys/stat.h>
>  #include <sys/time.h>
>  #include <fcntl.h>
> +#include <stdio.h>
> +#include <errno.h>
>  
>  int lutimes(const char *filename, const struct timeval tv[2])
>  {
>  	struct timespec times[2];
> -	times[0].tv_sec  = tv[0].tv_sec;
> -	times[0].tv_nsec = tv[0].tv_usec * 1000;
> -	times[1].tv_sec  = tv[1].tv_sec;
> -	times[1].tv_nsec = tv[1].tv_usec * 1000;
> -	return utimensat(AT_FDCWD, filename, times, AT_SYMLINK_NOFOLLOW);
> +	if (tv != NULL) {
> +		if (tv[0].tv_sec < 0 || tv[0].tv_usec < 0 ||
> +		    tv[1].tv_sec < 0 || tv[1].tv_usec < 0) {
> +			errno = EINVAL;
> +			return -1;
> +		}
> +		times[0].tv_sec  = tv[0].tv_sec;
> +		times[0].tv_nsec = tv[0].tv_usec * 1000;
> +		times[1].tv_sec  = tv[1].tv_sec;
> +		times[1].tv_nsec = tv[1].tv_usec * 1000;
> +	}
> +	return utimensat(AT_FDCWD, filename, tv ? times : NULL, AT_SYMLINK_NOFOLLOW);
>  }
> -- 
> 2.17.1

This patch causes uninitialized timespecs to be used if a null pointer
is passed, silently corrupting data. If there is any historical
documented precedent for this function accepting a null pointer and
doing something meaningful, then the patch needs to do whatever that
meaningful thing is rather than usign uninitialized data. If not, the
preferred behavior is the current behavior: to crash so that the usage
error is caught.

Rich

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.