musl: lutimes: Add checks for input parameters

Submitted by Liu Jie on March 1, 2020, 6:57 a.m.

Details

Message ID 20200301065730.146013-1-liujie1@huawei.com
State New
Series "musl: lutimes: Add checks for input parameters"
Headers show

Commit Message

Liu Jie March 1, 2020, 6:57 a.m.
For the input parameter struct timeval tv, need to
determine whether it is invalid inputs.

Signed-off-by: Liu Jie <liujie1@huawei.com>
---
 src/legacy/lutimes.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

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);
 }

Comments

Rich Felker March 1, 2020, 7:17 a.m.
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@huawei.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
Markus Wichmann March 1, 2020, 8:37 a.m.
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.
>

Why? lutimes() is a Linux-specific function, so the manpage is as close
to a specification as you're ever going to get, and it does not specify
an EINVAL return.

Adding the NULL pointer check, though, is probably justified, given that
the manpage states that lutimes() acts "in the same way as utimes(2)"
(with an irrelevant exception afterwards), and utimes() allows for a
NULL tv input.

The kernel itself also checks the input values again. While I usually am
in favor of failing faster, in this case I fail to see the benefit.
Especially since you're not testing for the one case that could make the
kernel accept a timestamp that was invalid on input: An overflowing one.
But you don't test for the upper limit.

Oh, and the seconds are allowed to be negative. If someone wants to set
a timestamp from before 1970, the libc is the wrong place to stop them.
If such dates are invalid from your application's perspective, filter
that there.

Have a nice Sunday,
Markus
Samuel Holland March 1, 2020, 8:37 p.m.
On 3/1/20 1:17 AM, Rich Felker wrote:
> 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@huawei.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.

How do you see that uninitialized timespecs are used? times is only passed to
utimensat if tv is nonnull, and in that case times is initialized.

Regards,
Samuel
Rich Felker March 1, 2020, 8:39 p.m.
On Sun, Mar 01, 2020 at 02:37:38PM -0600, Samuel Holland wrote:
> On 3/1/20 1:17 AM, Rich Felker wrote:
> > 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@huawei.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.
> 
> How do you see that uninitialized timespecs are used? times is only passed to
> utimensat if tv is nonnull, and in that case times is initialized.

Oh, I misread it. In that case it seems like it might be correct
as-is.

Rich
Rich Felker March 20, 2020, 7:54 p.m.
On Sun, Mar 01, 2020 at 03:39:21PM -0500, Rich Felker wrote:
> On Sun, Mar 01, 2020 at 02:37:38PM -0600, Samuel Holland wrote:
> > On 3/1/20 1:17 AM, Rich Felker wrote:
> > > 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@huawei.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.
> > 
> > How do you see that uninitialized timespecs are used? times is only passed to
> > utimensat if tv is nonnull, and in that case times is initialized.
> 
> Oh, I misread it. In that case it seems like it might be correct
> as-is.

I've looked back into this, and indeed I think the concept of the
patch makes sense. The equivalent logic for utimes() is buried in
__futimesat, but can't be reused directly because __futimesat doesn't
take a flags argument for AT_NOFOLLOW. There are some subtle
differences to how they handle error cases, so it'd be nice to figure
out exactly what the right behavior should be and get it right both
places.

Sorry for taking a while to get back to this.

Rich