[PATCH] fix getgrouplist without nscd

[PATCH] fix getgrouplist without nscd

[Joakim Sindholt]

[-- Attachment #1: Type: text/plain, Size: 345 bytes --]

There were two issues here. First, if socket() fails then it's treated 
as an unrecoverable error and second: the response buffer was not 
initialized and so a recoverable error (ie. no nscd) would result in UB.

dgp tested this patch and confirmed on IRC that it worked in the 
no-nscd no-socket case and I have tested it in the has-nscd case.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-fix-getgrouplist-without-nscd.patch --]
[-- Type: text/x-patch, Size: 1279 bytes --]

From 9251aff794832e0e37f5b747b69e756ac836f181 Mon Sep 17 00:00:00 2001
From: Joakim Sindholt <opensource@zhasha.com>
Date: Tue, 12 Jul 2016 10:13:23 +0200
Subject: [PATCH] fix getgrouplist without nscd

---
 src/passwd/getgrouplist.c | 2 +-
 src/passwd/nscd_query.c   | 8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/passwd/getgrouplist.c b/src/passwd/getgrouplist.c
index 43e5182..fd0bf5f 100644
--- a/src/passwd/getgrouplist.c
+++ b/src/passwd/getgrouplist.c
@@ -17,7 +17,7 @@ int getgrouplist(const char *user, gid_t gid, gid_t *groups, int *ngroups)
 	struct group *res;
 	FILE *f;
 	int swap = 0;
-	int32_t resp[INITGR_LEN];
+	int32_t resp[INITGR_LEN] = {0};
 	uint32_t *nscdbuf = 0;
 	char *buf = 0;
 	char **mem = 0;
diff --git a/src/passwd/nscd_query.c b/src/passwd/nscd_query.c
index d38e371..8641e4f 100644
--- a/src/passwd/nscd_query.c
+++ b/src/passwd/nscd_query.c
@@ -40,7 +40,13 @@ retry:
 	buf[0] = NSCDVERSION;
 
 	fd = socket(PF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
-	if (fd < 0) return NULL;
+	if (fd < 0) {
+		if (errno == EACCES || errno == EAFNOSUPPORT || errno == EPROTONOSUPPORT) {
+			errno = errno_save;
+			return fopen("/dev/null", "re");
+		}
+		return 0;
+	}
 
 	if(!(f = fdopen(fd, "r"))) {
 		close(fd);
-- 
2.7.3

Re: [PATCH] fix getgrouplist without nscd

[Rich Felker]

On Tue, Jul 12, 2016 at 11:30:57AM +0200, Joakim Sindholt wrote:
> There were two issues here. First, if socket() fails then it's
> treated as an unrecoverable error and second: the response buffer
> was not initialized and so a recoverable error (ie. no nscd) would
> result in UB.
> 
> dgp tested this patch and confirmed on IRC that it worked in the
> no-nscd no-socket case and I have tested it in the has-nscd case.

> >From 9251aff794832e0e37f5b747b69e756ac836f181 Mon Sep 17 00:00:00 2001
> From: Joakim Sindholt <opensource@zhasha.com>
> Date: Tue, 12 Jul 2016 10:13:23 +0200
> Subject: [PATCH] fix getgrouplist without nscd
> 
> ---
>  src/passwd/getgrouplist.c | 2 +-
>  src/passwd/nscd_query.c   | 8 +++++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/passwd/getgrouplist.c b/src/passwd/getgrouplist.c
> index 43e5182..fd0bf5f 100644
> --- a/src/passwd/getgrouplist.c
> +++ b/src/passwd/getgrouplist.c
> @@ -17,7 +17,7 @@ int getgrouplist(const char *user, gid_t gid, gid_t *groups, int *ngroups)
>  	struct group *res;
>  	FILE *f;
>  	int swap = 0;
> -	int32_t resp[INITGR_LEN];
> +	int32_t resp[INITGR_LEN] = {0};
>  	uint32_t *nscdbuf = 0;
>  	char *buf = 0;
>  	char **mem = 0;
> diff --git a/src/passwd/nscd_query.c b/src/passwd/nscd_query.c
> index d38e371..8641e4f 100644
> --- a/src/passwd/nscd_query.c
> +++ b/src/passwd/nscd_query.c
> @@ -40,7 +40,13 @@ retry:
>  	buf[0] = NSCDVERSION;
>  
>  	fd = socket(PF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
> -	if (fd < 0) return NULL;
> +	if (fd < 0) {
> +		if (errno == EACCES || errno == EAFNOSUPPORT || errno == EPROTONOSUPPORT) {
> +			errno = errno_save;
> +			return fopen("/dev/null", "re");
> +		}
> +		return 0;
> +	}
>  
>  	if(!(f = fdopen(fd, "r"))) {
>  		close(fd);
> -- 
> 2.7.3
> 

I just ran across this patch that got lost last year while looking at
the git history for src/passwd and seeing commit
39494a273eaa6b714e0fa0c59ce7a1f5fbc80a1e. Was there any further
progress or discussion on it I might be overlooking? I think the
concept looks right but I wonder if there's a good way to avoid the
spurious /dev/null access.

Rich

Re: [PATCH] fix getgrouplist without nscd

[Rich Felker]

On Wed, Jun 21, 2017 at 07:23:10PM -0400, Rich Felker wrote:
> On Tue, Jul 12, 2016 at 11:30:57AM +0200, Joakim Sindholt wrote:
> > There were two issues here. First, if socket() fails then it's
> > treated as an unrecoverable error and second: the response buffer
> > was not initialized and so a recoverable error (ie. no nscd) would
> > result in UB.
> > 
> > dgp tested this patch and confirmed on IRC that it worked in the
> > no-nscd no-socket case and I have tested it in the has-nscd case.
> 
> > >From 9251aff794832e0e37f5b747b69e756ac836f181 Mon Sep 17 00:00:00 2001
> > From: Joakim Sindholt <opensource@zhasha.com>
> > Date: Tue, 12 Jul 2016 10:13:23 +0200
> > Subject: [PATCH] fix getgrouplist without nscd
> > 
> > ---
> >  src/passwd/getgrouplist.c | 2 +-
> >  src/passwd/nscd_query.c   | 8 +++++++-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/passwd/getgrouplist.c b/src/passwd/getgrouplist.c
> > index 43e5182..fd0bf5f 100644
> > --- a/src/passwd/getgrouplist.c
> > +++ b/src/passwd/getgrouplist.c
> > @@ -17,7 +17,7 @@ int getgrouplist(const char *user, gid_t gid, gid_t *groups, int *ngroups)
> >  	struct group *res;
> >  	FILE *f;
> >  	int swap = 0;
> > -	int32_t resp[INITGR_LEN];
> > +	int32_t resp[INITGR_LEN] = {0};
> >  	uint32_t *nscdbuf = 0;
> >  	char *buf = 0;
> >  	char **mem = 0;

As noted on irc, this part is redundant; __nscd_query already zeros
the buffer.

> > diff --git a/src/passwd/nscd_query.c b/src/passwd/nscd_query.c
> > index d38e371..8641e4f 100644
> > --- a/src/passwd/nscd_query.c
> > +++ b/src/passwd/nscd_query.c
> > @@ -40,7 +40,13 @@ retry:
> >  	buf[0] = NSCDVERSION;
> >  
> >  	fd = socket(PF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
> > -	if (fd < 0) return NULL;
> > +	if (fd < 0) {
> > +		if (errno == EACCES || errno == EAFNOSUPPORT || errno == EPROTONOSUPPORT) {
> > +			errno = errno_save;
> > +			return fopen("/dev/null", "re");
> > +		}
> > +		return 0;
> > +	}
> >  
> >  	if(!(f = fdopen(fd, "r"))) {
> >  		close(fd);
> > -- 
> > 2.7.3
> > 
> 
> I just ran across this patch that got lost last year while looking at
> the git history for src/passwd and seeing commit
> 39494a273eaa6b714e0fa0c59ce7a1f5fbc80a1e. Was there any further
> progress or discussion on it I might be overlooking? I think the
> concept looks right but I wonder if there's a good way to avoid the
> spurious /dev/null access.

And the (unwritten, AFAIK) contract of __nscd_query is that the caller
is not expected to attempt to read from the FILE returned unless the
header obtained in the initial query indicates there's data to be
read. This is why it's able to return a FILE for an unconnected
socket, and it also means we should be fine returning a FILE for an
invalid (negative) fd.

This should probably be documented with a quick comment somewhere too,
but I can do that separately.

Anyway I think the main question left is just whether the above errno
cases are correct and cover the ways socket could fail for policy or
configuration reasons (as opposed to transient/resource failures).

Rich