[PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam

[PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam

[Rudolph Pereira]

[-- Attachment #1.1: Type: text/plain, Size: 465 bytes --]

Hi,

currently, the getgr, getpw, and getspnam functions in musl return ERANGE
when the allocated buffer is not large enough, but do not set errno to the
same value. This causes issues with utilities, for example the "shadow"
utilities (useradd/mod, groupmod, etc.) which assume this behaviour (which
at least gnu libc exhibits) and leads to groups having a small limit on the
number of members.

The attached patch, against 1.1.16, corrects this.

Cheers,
Rudolph

[-- Attachment #1.2: Type: text/html, Size: 579 bytes --]

[-- Attachment #2: 1100-getX-errno.patch --]
[-- Type: text/x-patch, Size: 1120 bytes --]

--- musl-1.1.16.orig/src/passwd/getgr_r.c
+++ musl-1.1.16/src/passwd/getgr_r.c
@@ -1,5 +1,6 @@
 #include "pwf.h"
 #include <pthread.h>
+#include <errno.h>
 
 #define FIX(x) (gr->gr_##x = gr->gr_##x-line+buf)
 
@@ -19,6 +20,7 @@
 	if (*res && size < len + (nmem+1)*sizeof(char *) + 32) {
 		*res = 0;
 		rv = ERANGE;
+		errno = ERANGE;
 	}
 	if (*res) {
 		buf += (16-(uintptr_t)buf)%16;
--- musl-1.1.16.orig/src/passwd/getpw_r.c
+++ musl-1.1.16/src/passwd/getpw_r.c
@@ -1,5 +1,6 @@
 #include "pwf.h"
 #include <pthread.h>
+#include <errno.h>
 
 #define FIX(x) (pw->pw_##x = pw->pw_##x-line+buf)
 
@@ -16,6 +17,7 @@
 	if (*res && size < len) {
 		*res = 0;
 		rv = ERANGE;
+		errno = ERANGE;
 	}
 	if (*res) {
 		memcpy(buf, line, len);
--- musl-1.1.16.orig/src/passwd/getspnam_r.c
+++ musl-1.1.16/src/passwd/getspnam_r.c
@@ -3,6 +3,7 @@
 #include <sys/stat.h>
 #include <ctype.h>
 #include <pthread.h>
+#include <errno.h>
 #include "pwf.h"
 
 /* This implementation support Openwall-style TCB passwords in place of
@@ -104,6 +105,7 @@
 		}
 		if (buf[k-1] != '\n') {
 			rv = ERANGE;
+			errno = ERANGE;
 			break;
 		}

Re: [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam

[Rich Felker]

On Tue, Jun 06, 2017 at 12:21:39PM -0400, Rudolph Pereira wrote:
> Hi,
> 
> currently, the getgr, getpw, and getspnam functions in musl return ERANGE
> when the allocated buffer is not large enough, but do not set errno to the
> same value. This causes issues with utilities, for example the "shadow"
> utilities (useradd/mod, groupmod, etc.) which assume this behaviour (which
> at least gnu libc exhibits) and leads to groups having a small limit on the
> number of members.
> 
> The attached patch, against 1.1.16, corrects this.
> 
> Cheers,
> Rudolph

> --- musl-1.1.16.orig/src/passwd/getgr_r.c
> +++ musl-1.1.16/src/passwd/getgr_r.c
> @@ -1,5 +1,6 @@
>  #include "pwf.h"
>  #include <pthread.h>
> +#include <errno.h>
>  
>  #define FIX(x) (gr->gr_##x = gr->gr_##x-line+buf)
>  
> @@ -19,6 +20,7 @@
>  	if (*res && size < len + (nmem+1)*sizeof(char *) + 32) {
>  		*res = 0;
>  		rv = ERANGE;
> +		errno = ERANGE;
>  	}
>  	if (*res) {
>  		buf += (16-(uintptr_t)buf)%16;
> --- musl-1.1.16.orig/src/passwd/getpw_r.c
> +++ musl-1.1.16/src/passwd/getpw_r.c
> @@ -1,5 +1,6 @@
>  #include "pwf.h"
>  #include <pthread.h>
> +#include <errno.h>
>  
>  #define FIX(x) (pw->pw_##x = pw->pw_##x-line+buf)
>  
> @@ -16,6 +17,7 @@
>  	if (*res && size < len) {
>  		*res = 0;
>  		rv = ERANGE;
> +		errno = ERANGE;
>  	}
>  	if (*res) {
>  		memcpy(buf, line, len);
> --- musl-1.1.16.orig/src/passwd/getspnam_r.c
> +++ musl-1.1.16/src/passwd/getspnam_r.c
> @@ -3,6 +3,7 @@
>  #include <sys/stat.h>
>  #include <ctype.h>
>  #include <pthread.h>
> +#include <errno.h>
>  #include "pwf.h"
>  
>  /* This implementation support Openwall-style TCB passwords in place of
> @@ -104,6 +105,7 @@
>  		}
>  		if (buf[k-1] != '\n') {
>  			rv = ERANGE;
> +			errno = ERANGE;
>  			break;
>  		}

I don't think this patch is complete. A nonzero value of rv can also
come from __getpw_a/__getgr_a. Insted, just before return there should
probably be:

	if (rv) errno = rv;

Does that look correct? I'm not sure about getspnam_r; it might
actually be missing some error cases right now.

Rich

Re: [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam

[Rudolph Pereira]

[-- Attachment #1.1: Type: text/plain, Size: 2812 bytes --]

Hi Rich,

thanks for the feedback. I've attached a patch that implements errno
setting as you suggested, other than a couple of cases where the code
immediately returns. This also brings it in line with existing code
(in __getpw_a/__getgr_a) so makes things more consistent. Please see
attached - note this is against HEAD.

On 8 June 2017 at 20:00, Rich Felker <dalias@libc.org> wrote:

> On Tue, Jun 06, 2017 at 12:21:39PM -0400, Rudolph Pereira wrote:
> > Hi,
> >
> > currently, the getgr, getpw, and getspnam functions in musl return ERANGE
> > when the allocated buffer is not large enough, but do not set errno to
> the
> > same value. This causes issues with utilities, for example the "shadow"
> > utilities (useradd/mod, groupmod, etc.) which assume this behaviour
> (which
> > at least gnu libc exhibits) and leads to groups having a small limit on
> the
> > number of members.
> >
> > The attached patch, against 1.1.16, corrects this.
> >
> > Cheers,
> > Rudolph
>
> > --- musl-1.1.16.orig/src/passwd/getgr_r.c
> > +++ musl-1.1.16/src/passwd/getgr_r.c
> > @@ -1,5 +1,6 @@
> >  #include "pwf.h"
> >  #include <pthread.h>
> > +#include <errno.h>
> >
> >  #define FIX(x) (gr->gr_##x = gr->gr_##x-line+buf)
> >
> > @@ -19,6 +20,7 @@
> >       if (*res && size < len + (nmem+1)*sizeof(char *) + 32) {
> >               *res = 0;
> >               rv = ERANGE;
> > +             errno = ERANGE;
> >       }
> >       if (*res) {
> >               buf += (16-(uintptr_t)buf)%16;
> > --- musl-1.1.16.orig/src/passwd/getpw_r.c
> > +++ musl-1.1.16/src/passwd/getpw_r.c
> > @@ -1,5 +1,6 @@
> >  #include "pwf.h"
> >  #include <pthread.h>
> > +#include <errno.h>
> >
> >  #define FIX(x) (pw->pw_##x = pw->pw_##x-line+buf)
> >
> > @@ -16,6 +17,7 @@
> >       if (*res && size < len) {
> >               *res = 0;
> >               rv = ERANGE;
> > +             errno = ERANGE;
> >       }
> >       if (*res) {
> >               memcpy(buf, line, len);
> > --- musl-1.1.16.orig/src/passwd/getspnam_r.c
> > +++ musl-1.1.16/src/passwd/getspnam_r.c
> > @@ -3,6 +3,7 @@
> >  #include <sys/stat.h>
> >  #include <ctype.h>
> >  #include <pthread.h>
> > +#include <errno.h>
> >  #include "pwf.h"
> >
> >  /* This implementation support Openwall-style TCB passwords in place of
> > @@ -104,6 +105,7 @@
> >               }
> >               if (buf[k-1] != '\n') {
> >                       rv = ERANGE;
> > +                     errno = ERANGE;
> >                       break;
> >               }
>
> I don't think this patch is complete. A nonzero value of rv can also
> come from __getpw_a/__getgr_a. Insted, just before return there should
> probably be:
>
>         if (rv) errno = rv;
>
> Does that look correct? I'm not sure about getspnam_r; it might
> actually be missing some error cases right now.
>
> Rich
>

[-- Attachment #1.2: Type: text/html, Size: 3984 bytes --]

[-- Attachment #2: getX-errno-v2.patch --]
[-- Type: text/x-patch, Size: 1317 bytes --]

diff --git a/src/passwd/getpw_r.c b/src/passwd/getpw_r.c
index e8cc811..0c87ab0 100644
--- a/src/passwd/getpw_r.c
+++ b/src/passwd/getpw_r.c
@@ -27,6 +27,7 @@ static int getpw_r(const char *name, uid_t uid, struct passwd *pw, char *buf, si
 	}
  	free(line);
 	pthread_setcancelstate(cs, 0);
+	if (rv) errno = rv;
 	return rv;
 }
 
diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c
index 9233952..47ce3d3 100644
--- a/src/passwd/getspnam_r.c
+++ b/src/passwd/getspnam_r.c
@@ -72,14 +72,24 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct
 
 	/* Disallow potentially-malicious user names */
 	if (*name=='.' || strchr(name, '/') || !l)
+	{
+		errno = EINVAL;
 		return EINVAL;
+	}
 
 	/* Buffer size must at least be able to hold name, plus some.. */
-	if (size < l+100) return ERANGE;
+	if (size < l+100)
+	{
+		errno = EINVAL;
+		return ERANGE;
+	}
 
 	/* Protect against truncation */
 	if (snprintf(path, sizeof path, "/etc/tcb/%s/shadow", name) >= sizeof path)
+	{
+		errno = EINVAL;
 		return EINVAL;
+	}
 
 	fd = open(path, O_RDONLY|O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC);
 	if (fd >= 0) {
@@ -112,5 +122,6 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct
 		break;
 	}
 	pthread_cleanup_pop(1);
+	if (rv) errno = rv;
 	return rv;
 }

Re: [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam

[Rich Felker]

On Sun, Jun 11, 2017 at 12:59:51PM -0400, Rudolph Pereira wrote:
> Hi Rich,
> 
> thanks for the feedback. I've attached a patch that implements errno
> setting as you suggested, other than a couple of cases where the code
> immediately returns. This also brings it in line with existing code
> (in __getpw_a/__getgr_a) so makes things more consistent. Please see
> attached - note this is against HEAD.

It looks like you omitted the change to getgr_r.c corresponding to
this one for getpw_r.c:

> diff --git a/src/passwd/getpw_r.c b/src/passwd/getpw_r.c
> index e8cc811..0c87ab0 100644
> --- a/src/passwd/getpw_r.c
> +++ b/src/passwd/getpw_r.c
> @@ -27,6 +27,7 @@ static int getpw_r(const char *name, uid_t uid, struct passwd *pw, char *buf, si
>  	}
>   	free(line);
>  	pthread_setcancelstate(cs, 0);
> +	if (rv) errno = rv;
>  	return rv;
>  }

Also:

> diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c
> index 9233952..47ce3d3 100644
> --- a/src/passwd/getspnam_r.c
> +++ b/src/passwd/getspnam_r.c
> @@ -72,14 +72,24 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct
>  
>  	/* Disallow potentially-malicious user names */
>  	if (*name=='.' || strchr(name, '/') || !l)
> +	{
> +		errno = EINVAL;
>  		return EINVAL;
> +	}

Please use consistent style for braces (open brace on if line).
Alternatively (if you don't balk at the style; not sure if others will
like it), this works:

-  		return EINVAL;
+  		return errno = EINVAL;

Rich

Re: [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam

[Rudolph Pereira]

[-- Attachment #1.1: Type: text/plain, Size: 1888 bytes --]

Hi,

please see the updated attached patch. In terms of style, I've implemented
your latter suggestion as it is more compact, but happy either way.

Cheers,
Rudolph

On 14 June 2017 at 20:05, Rich Felker <dalias@libc.org> wrote:

> On Sun, Jun 11, 2017 at 12:59:51PM -0400, Rudolph Pereira wrote:
> > Hi Rich,
> >
> > thanks for the feedback. I've attached a patch that implements errno
> > setting as you suggested, other than a couple of cases where the code
> > immediately returns. This also brings it in line with existing code
> > (in __getpw_a/__getgr_a) so makes things more consistent. Please see
> > attached - note this is against HEAD.
>
> It looks like you omitted the change to getgr_r.c corresponding to
> this one for getpw_r.c:
>
> > diff --git a/src/passwd/getpw_r.c b/src/passwd/getpw_r.c
> > index e8cc811..0c87ab0 100644
> > --- a/src/passwd/getpw_r.c
> > +++ b/src/passwd/getpw_r.c
> > @@ -27,6 +27,7 @@ static int getpw_r(const char *name, uid_t uid, struct
> passwd *pw, char *buf, si
> >       }
> >       free(line);
> >       pthread_setcancelstate(cs, 0);
> > +     if (rv) errno = rv;
> >       return rv;
> >  }
>
> Also:
>
> > diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c
> > index 9233952..47ce3d3 100644
> > --- a/src/passwd/getspnam_r.c
> > +++ b/src/passwd/getspnam_r.c
> > @@ -72,14 +72,24 @@ int getspnam_r(const char *name, struct spwd *sp,
> char *buf, size_t size, struct
> >
> >       /* Disallow potentially-malicious user names */
> >       if (*name=='.' || strchr(name, '/') || !l)
> > +     {
> > +             errno = EINVAL;
> >               return EINVAL;
> > +     }
>
> Please use consistent style for braces (open brace on if line).
> Alternatively (if you don't balk at the style; not sure if others will
> like it), this works:
>
> -               return EINVAL;
> +               return errno = EINVAL;
>
> Rich
>

[-- Attachment #1.2: Type: text/html, Size: 2583 bytes --]

[-- Attachment #2: getX-errno-v3.patch --]
[-- Type: text/x-patch, Size: 1638 bytes --]

diff --git a/src/passwd/getgr_r.c b/src/passwd/getgr_r.c
index 7246e8a..f3e8f60 100644
--- a/src/passwd/getgr_r.c
+++ b/src/passwd/getgr_r.c
@@ -34,6 +34,7 @@ static int getgr_r(const char *name, gid_t gid, struct group *gr, char *buf, siz
  	free(mem);
  	free(line);
 	pthread_setcancelstate(cs, 0);
+	if (rv) errno = rv;
 	return rv;
 }
 
diff --git a/src/passwd/getpw_r.c b/src/passwd/getpw_r.c
index e8cc811..0c87ab0 100644
--- a/src/passwd/getpw_r.c
+++ b/src/passwd/getpw_r.c
@@ -27,6 +27,7 @@ static int getpw_r(const char *name, uid_t uid, struct passwd *pw, char *buf, si
 	}
  	free(line);
 	pthread_setcancelstate(cs, 0);
+	if (rv) errno = rv;
 	return rv;
 }
 
diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c
index 9233952..e488b67 100644
--- a/src/passwd/getspnam_r.c
+++ b/src/passwd/getspnam_r.c
@@ -72,14 +72,15 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct
 
 	/* Disallow potentially-malicious user names */
 	if (*name=='.' || strchr(name, '/') || !l)
-		return EINVAL;
+		return errno = EINVAL;
 
 	/* Buffer size must at least be able to hold name, plus some.. */
-	if (size < l+100) return ERANGE;
+	if (size < l+100)
+		return errno = EINVAL;
 
 	/* Protect against truncation */
 	if (snprintf(path, sizeof path, "/etc/tcb/%s/shadow", name) >= sizeof path)
-		return EINVAL;
+		return errno = EINVAL;
 
 	fd = open(path, O_RDONLY|O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC);
 	if (fd >= 0) {
@@ -112,5 +113,6 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct
 		break;
 	}
 	pthread_cleanup_pop(1);
+	if (rv) errno = rv;
 	return rv;
 }

Re: [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam

[Rich Felker]

On Thu, Jun 15, 2017 at 08:18:18AM -0400, Rudolph Pereira wrote:
> Hi,
> 
> please see the updated attached patch. In terms of style, I've implemented
> your latter suggestion as it is more compact, but happy either way.

Thanks. Committing it.

Rich

> On 14 June 2017 at 20:05, Rich Felker <dalias@libc.org> wrote:
> 
> > On Sun, Jun 11, 2017 at 12:59:51PM -0400, Rudolph Pereira wrote:
> > > Hi Rich,
> > >
> > > thanks for the feedback. I've attached a patch that implements errno
> > > setting as you suggested, other than a couple of cases where the code
> > > immediately returns. This also brings it in line with existing code
> > > (in __getpw_a/__getgr_a) so makes things more consistent. Please see
> > > attached - note this is against HEAD.
> >
> > It looks like you omitted the change to getgr_r.c corresponding to
> > this one for getpw_r.c:
> >
> > > diff --git a/src/passwd/getpw_r.c b/src/passwd/getpw_r.c
> > > index e8cc811..0c87ab0 100644
> > > --- a/src/passwd/getpw_r.c
> > > +++ b/src/passwd/getpw_r.c
> > > @@ -27,6 +27,7 @@ static int getpw_r(const char *name, uid_t uid, struct
> > passwd *pw, char *buf, si
> > >       }
> > >       free(line);
> > >       pthread_setcancelstate(cs, 0);
> > > +     if (rv) errno = rv;
> > >       return rv;
> > >  }
> >
> > Also:
> >
> > > diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c
> > > index 9233952..47ce3d3 100644
> > > --- a/src/passwd/getspnam_r.c
> > > +++ b/src/passwd/getspnam_r.c
> > > @@ -72,14 +72,24 @@ int getspnam_r(const char *name, struct spwd *sp,
> > char *buf, size_t size, struct
> > >
> > >       /* Disallow potentially-malicious user names */
> > >       if (*name=='.' || strchr(name, '/') || !l)
> > > +     {
> > > +             errno = EINVAL;
> > >               return EINVAL;
> > > +     }
> >
> > Please use consistent style for braces (open brace on if line).
> > Alternatively (if you don't balk at the style; not sure if others will
> > like it), this works:
> >
> > -               return EINVAL;
> > +               return errno = EINVAL;
> >
> > Rich
> >

> diff --git a/src/passwd/getgr_r.c b/src/passwd/getgr_r.c
> index 7246e8a..f3e8f60 100644
> --- a/src/passwd/getgr_r.c
> +++ b/src/passwd/getgr_r.c
> @@ -34,6 +34,7 @@ static int getgr_r(const char *name, gid_t gid, struct group *gr, char *buf, siz
>   	free(mem);
>   	free(line);
>  	pthread_setcancelstate(cs, 0);
> +	if (rv) errno = rv;
>  	return rv;
>  }
>  
> diff --git a/src/passwd/getpw_r.c b/src/passwd/getpw_r.c
> index e8cc811..0c87ab0 100644
> --- a/src/passwd/getpw_r.c
> +++ b/src/passwd/getpw_r.c
> @@ -27,6 +27,7 @@ static int getpw_r(const char *name, uid_t uid, struct passwd *pw, char *buf, si
>  	}
>   	free(line);
>  	pthread_setcancelstate(cs, 0);
> +	if (rv) errno = rv;
>  	return rv;
>  }
>  
> diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c
> index 9233952..e488b67 100644
> --- a/src/passwd/getspnam_r.c
> +++ b/src/passwd/getspnam_r.c
> @@ -72,14 +72,15 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct
>  
>  	/* Disallow potentially-malicious user names */
>  	if (*name=='.' || strchr(name, '/') || !l)
> -		return EINVAL;
> +		return errno = EINVAL;
>  
>  	/* Buffer size must at least be able to hold name, plus some.. */
> -	if (size < l+100) return ERANGE;
> +	if (size < l+100)
> +		return errno = EINVAL;
>  
>  	/* Protect against truncation */
>  	if (snprintf(path, sizeof path, "/etc/tcb/%s/shadow", name) >= sizeof path)
> -		return EINVAL;
> +		return errno = EINVAL;
>  
>  	fd = open(path, O_RDONLY|O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC);
>  	if (fd >= 0) {
> @@ -112,5 +113,6 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct
>  		break;
>  	}
>  	pthread_cleanup_pop(1);
> +	if (rv) errno = rv;
>  	return rv;
>  }