[v2,5/8] genhomedircon: Add uid and gid to struct user_entry
diff mbox

Message ID 1461391499-20593-6-git-send-email-jason@perfinion.com
State Superseded
Headers show

Commit Message

Jason Zaman April 23, 2016, 6:04 a.m. UTC
Signed-off-by: Jason Zaman <jason@perfinion.com>
---
 libsemanage/src/genhomedircon.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

Comments

Stephen Smalley April 27, 2016, 5:04 p.m. UTC | #1
On 04/23/2016 02:04 AM, Jason Zaman wrote:
> Signed-off-by: Jason Zaman <jason@perfinion.com>
> ---
>  libsemanage/src/genhomedircon.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
> index 1a7882c..56c58e0 100644
> --- a/libsemanage/src/genhomedircon.c
> +++ b/libsemanage/src/genhomedircon.c
> @@ -82,10 +82,13 @@
>  #define FALLBACK_PREFIX "user"
>  #define FALLBACK_LEVEL "s0"
>  #define FALLBACK_NAME ".*"
> +#define FALLBACK_UIDGID "[0-9]+"
>  #define DEFAULT_LOGIN "__default__"
>  
>  typedef struct user_entry {
>  	char *name;
> +	char *uid;
> +	char *gid;
>  	char *sename;
>  	char *prefix;
>  	char *home;
> @@ -628,11 +631,13 @@ static int name_user_cmp(char *key, semanage_user_t ** val)
>  }
>  
>  static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
> -			   const char *sen, const char *pre, const char *h,
> -			   const char *l)
> +			   const char *u, const char *g, const char *sen,
> +			   const char *pre, const char *h, const char *l)
>  {
>  	genhomedircon_user_entry_t *temp = NULL;
>  	char *name = NULL;
> +	char *uid = NULL;
> +	char *gid = NULL;
>  	char *sename = NULL;
>  	char *prefix = NULL;
>  	char *home = NULL;
> @@ -644,6 +649,12 @@ static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
>  	name = strdup(n);
>  	if (!name)
>  		goto cleanup;
> +	uid = strdup(u);
> +	if (!uid)
> +		goto cleanup;
> +	gid = strdup(g);
> +	if (!gid)
> +		goto cleanup;
>  	sename = strdup(sen);
>  	if (!sename)
>  		goto cleanup;
> @@ -658,6 +669,8 @@ static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
>  		goto cleanup;
>  
>  	temp->name = name;
> +	temp->uid = uid;
> +	temp->gid = gid;
>  	temp->sename = sename;
>  	temp->prefix = prefix;
>  	temp->home = home;
> @@ -669,6 +682,8 @@ static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
>  
>        cleanup:
>  	free(name);
> +	free(uid);
> +	free(gid);
>  	free(sename);
>  	free(prefix);
>  	free(home);
> @@ -687,6 +702,8 @@ static void pop_user_entry(genhomedircon_user_entry_t ** list)
>  	temp = *list;
>  	*list = temp->next;
>  	free(temp->name);
> +	free(temp->uid);
> +	free(temp->gid);
>  	free(temp->sename);
>  	free(temp->prefix);
>  	free(temp->home);
> @@ -738,7 +755,8 @@ static int setup_fallback_user(genhomedircon_settings_t * s)
>  					level = FALLBACK_LEVEL;
>  			}
>  
> -			if (push_user_entry(&(s->fallback), FALLBACK_NAME, 0, 0,
> +			if (push_user_entry(&(s->fallback), FALLBACK_NAME,
> +					    FALLBACK_UIDGID, FALLBACK_UIDGID,
>  					    seuname, prefix, "", level) != 0)
>  				errors = STATUS_ERR;
>  			semanage_user_key_free(key);
> @@ -768,6 +786,8 @@ static genhomedircon_user_entry_t *get_users(genhomedircon_settings_t * s,
>  	const char *seuname = NULL;
>  	const char *prefix = NULL;
>  	const char *level = NULL;
> +	char uid[10];
> +	char gid[10];

You need to allow space for the NUL terminator.

>  	struct passwd pwstorage, *pwent = NULL;
>  	unsigned int i;
>  	long rbuflen;
> @@ -852,7 +872,13 @@ static genhomedircon_user_entry_t *get_users(genhomedircon_settings_t * s,
>  		}
>  		if (ignore(pwent->pw_dir))
>  			continue;
> -		if (push_user_entry(&head, name, seuname,
> +
> +		if (snprintf(uid, sizeof(uid), "%d", pwent->pw_uid) < 0
> +		 || snprintf(gid, sizeof(gid), "%d", pwent->pw_gid) < 0) {

Should you be using %u instead of %d?
Also, snprintf returns >= size if the output was truncated, not < 0.

> +			*errors = STATUS_ERR;
> +			goto cleanup;
> +		}
> +		if (push_user_entry(&head, name, uid, gid, seuname,
>  				    prefix, pwent->pw_dir, level) != STATUS_SUCCESS) {
>  			*errors = STATUS_ERR;
>  			break;
>
Jason Zaman April 28, 2016, 5:53 p.m. UTC | #2
On Wed, Apr 27, 2016 at 01:04:25PM -0400, Stephen Smalley wrote:
> On 04/23/2016 02:04 AM, Jason Zaman wrote:
> > Signed-off-by: Jason Zaman <jason@perfinion.com>
> > ---
> >  libsemanage/src/genhomedircon.c | 34 ++++++++++++++++++++++++++++++----
> >  1 file changed, 30 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
> > index 1a7882c..56c58e0 100644
> > --- a/libsemanage/src/genhomedircon.c
> > +++ b/libsemanage/src/genhomedircon.c
> > @@ -82,10 +82,13 @@
> >  #define FALLBACK_PREFIX "user"
> >  #define FALLBACK_LEVEL "s0"
> >  #define FALLBACK_NAME ".*"
> > +#define FALLBACK_UIDGID "[0-9]+"
> >  #define DEFAULT_LOGIN "__default__"
> >  
> >  typedef struct user_entry {
> >  	char *name;
> > +	char *uid;
> > +	char *gid;
> >  	char *sename;
> >  	char *prefix;
> >  	char *home;
> > @@ -628,11 +631,13 @@ static int name_user_cmp(char *key, semanage_user_t ** val)
> >  }
> >  
> >  static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
> > -			   const char *sen, const char *pre, const char *h,
> > -			   const char *l)
> > +			   const char *u, const char *g, const char *sen,
> > +			   const char *pre, const char *h, const char *l)
> >  {
> >  	genhomedircon_user_entry_t *temp = NULL;
> >  	char *name = NULL;
> > +	char *uid = NULL;
> > +	char *gid = NULL;
> >  	char *sename = NULL;
> >  	char *prefix = NULL;
> >  	char *home = NULL;
> > @@ -644,6 +649,12 @@ static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
> >  	name = strdup(n);
> >  	if (!name)
> >  		goto cleanup;
> > +	uid = strdup(u);
> > +	if (!uid)
> > +		goto cleanup;
> > +	gid = strdup(g);
> > +	if (!gid)
> > +		goto cleanup;
> >  	sename = strdup(sen);
> >  	if (!sename)
> >  		goto cleanup;
> > @@ -658,6 +669,8 @@ static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
> >  		goto cleanup;
> >  
> >  	temp->name = name;
> > +	temp->uid = uid;
> > +	temp->gid = gid;
> >  	temp->sename = sename;
> >  	temp->prefix = prefix;
> >  	temp->home = home;
> > @@ -669,6 +682,8 @@ static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
> >  
> >        cleanup:
> >  	free(name);
> > +	free(uid);
> > +	free(gid);
> >  	free(sename);
> >  	free(prefix);
> >  	free(home);
> > @@ -687,6 +702,8 @@ static void pop_user_entry(genhomedircon_user_entry_t ** list)
> >  	temp = *list;
> >  	*list = temp->next;
> >  	free(temp->name);
> > +	free(temp->uid);
> > +	free(temp->gid);
> >  	free(temp->sename);
> >  	free(temp->prefix);
> >  	free(temp->home);
> > @@ -738,7 +755,8 @@ static int setup_fallback_user(genhomedircon_settings_t * s)
> >  					level = FALLBACK_LEVEL;
> >  			}
> >  
> > -			if (push_user_entry(&(s->fallback), FALLBACK_NAME, 0, 0,
> > +			if (push_user_entry(&(s->fallback), FALLBACK_NAME,
> > +					    FALLBACK_UIDGID, FALLBACK_UIDGID,
> >  					    seuname, prefix, "", level) != 0)
> >  				errors = STATUS_ERR;
> >  			semanage_user_key_free(key);
> > @@ -768,6 +786,8 @@ static genhomedircon_user_entry_t *get_users(genhomedircon_settings_t * s,
> >  	const char *seuname = NULL;
> >  	const char *prefix = NULL;
> >  	const char *level = NULL;
> > +	char uid[10];
> > +	char gid[10];
> 
> You need to allow space for the NUL terminator.

2^32 = 4294967296 so 10 digits + null. i'll send an updated patch.
> 
> >  	struct passwd pwstorage, *pwent = NULL;
> >  	unsigned int i;
> >  	long rbuflen;
> > @@ -852,7 +872,13 @@ static genhomedircon_user_entry_t *get_users(genhomedircon_settings_t * s,
> >  		}
> >  		if (ignore(pwent->pw_dir))
> >  			continue;
> > -		if (push_user_entry(&head, name, seuname,
> > +
> > +		if (snprintf(uid, sizeof(uid), "%d", pwent->pw_uid) < 0
> > +		 || snprintf(gid, sizeof(gid), "%d", pwent->pw_gid) < 0) {
> 
> Should you be using %u instead of %d?
yes, its unsigned, will fix.

> Also, snprintf returns >= size if the output was truncated, not < 0.

From the man page:
RETURN VALUE
[...] Thus, a return value of size or more means that the output was truncated.
If an output error is encountered, a negative value is returned.

I definitely need to check <0. but do I *also* need to check >= size? I
dont think that can ever happen since 10chars+NULL fits fine.

-- Jason

> > +			*errors = STATUS_ERR;
> > +			goto cleanup;
> > +		}
> > +		if (push_user_entry(&head, name, uid, gid, seuname,
> >  				    prefix, pwent->pw_dir, level) != STATUS_SUCCESS) {
> >  			*errors = STATUS_ERR;
> >  			break;
Stephen Smalley April 28, 2016, 6:13 p.m. UTC | #3
On 04/28/2016 01:53 PM, Jason Zaman wrote:
> On Wed, Apr 27, 2016 at 01:04:25PM -0400, Stephen Smalley wrote:
>> On 04/23/2016 02:04 AM, Jason Zaman wrote:
>>> Signed-off-by: Jason Zaman <jason@perfinion.com>
>>> ---
>>>  libsemanage/src/genhomedircon.c | 34 ++++++++++++++++++++++++++++++----
>>>  1 file changed, 30 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
>>> index 1a7882c..56c58e0 100644
>>> --- a/libsemanage/src/genhomedircon.c
>>> +++ b/libsemanage/src/genhomedircon.c
>>> @@ -82,10 +82,13 @@
>>>  #define FALLBACK_PREFIX "user"
>>>  #define FALLBACK_LEVEL "s0"
>>>  #define FALLBACK_NAME ".*"
>>> +#define FALLBACK_UIDGID "[0-9]+"
>>>  #define DEFAULT_LOGIN "__default__"
>>>  
>>>  typedef struct user_entry {
>>>  	char *name;
>>> +	char *uid;
>>> +	char *gid;
>>>  	char *sename;
>>>  	char *prefix;
>>>  	char *home;
>>> @@ -628,11 +631,13 @@ static int name_user_cmp(char *key, semanage_user_t ** val)
>>>  }
>>>  
>>>  static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
>>> -			   const char *sen, const char *pre, const char *h,
>>> -			   const char *l)
>>> +			   const char *u, const char *g, const char *sen,
>>> +			   const char *pre, const char *h, const char *l)
>>>  {
>>>  	genhomedircon_user_entry_t *temp = NULL;
>>>  	char *name = NULL;
>>> +	char *uid = NULL;
>>> +	char *gid = NULL;
>>>  	char *sename = NULL;
>>>  	char *prefix = NULL;
>>>  	char *home = NULL;
>>> @@ -644,6 +649,12 @@ static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
>>>  	name = strdup(n);
>>>  	if (!name)
>>>  		goto cleanup;
>>> +	uid = strdup(u);
>>> +	if (!uid)
>>> +		goto cleanup;
>>> +	gid = strdup(g);
>>> +	if (!gid)
>>> +		goto cleanup;
>>>  	sename = strdup(sen);
>>>  	if (!sename)
>>>  		goto cleanup;
>>> @@ -658,6 +669,8 @@ static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
>>>  		goto cleanup;
>>>  
>>>  	temp->name = name;
>>> +	temp->uid = uid;
>>> +	temp->gid = gid;
>>>  	temp->sename = sename;
>>>  	temp->prefix = prefix;
>>>  	temp->home = home;
>>> @@ -669,6 +682,8 @@ static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
>>>  
>>>        cleanup:
>>>  	free(name);
>>> +	free(uid);
>>> +	free(gid);
>>>  	free(sename);
>>>  	free(prefix);
>>>  	free(home);
>>> @@ -687,6 +702,8 @@ static void pop_user_entry(genhomedircon_user_entry_t ** list)
>>>  	temp = *list;
>>>  	*list = temp->next;
>>>  	free(temp->name);
>>> +	free(temp->uid);
>>> +	free(temp->gid);
>>>  	free(temp->sename);
>>>  	free(temp->prefix);
>>>  	free(temp->home);
>>> @@ -738,7 +755,8 @@ static int setup_fallback_user(genhomedircon_settings_t * s)
>>>  					level = FALLBACK_LEVEL;
>>>  			}
>>>  
>>> -			if (push_user_entry(&(s->fallback), FALLBACK_NAME, 0, 0,
>>> +			if (push_user_entry(&(s->fallback), FALLBACK_NAME,
>>> +					    FALLBACK_UIDGID, FALLBACK_UIDGID,
>>>  					    seuname, prefix, "", level) != 0)
>>>  				errors = STATUS_ERR;
>>>  			semanage_user_key_free(key);
>>> @@ -768,6 +786,8 @@ static genhomedircon_user_entry_t *get_users(genhomedircon_settings_t * s,
>>>  	const char *seuname = NULL;
>>>  	const char *prefix = NULL;
>>>  	const char *level = NULL;
>>> +	char uid[10];
>>> +	char gid[10];
>>
>> You need to allow space for the NUL terminator.
> 
> 2^32 = 4294967296 so 10 digits + null. i'll send an updated patch.
>>
>>>  	struct passwd pwstorage, *pwent = NULL;
>>>  	unsigned int i;
>>>  	long rbuflen;
>>> @@ -852,7 +872,13 @@ static genhomedircon_user_entry_t *get_users(genhomedircon_settings_t * s,
>>>  		}
>>>  		if (ignore(pwent->pw_dir))
>>>  			continue;
>>> -		if (push_user_entry(&head, name, seuname,
>>> +
>>> +		if (snprintf(uid, sizeof(uid), "%d", pwent->pw_uid) < 0
>>> +		 || snprintf(gid, sizeof(gid), "%d", pwent->pw_gid) < 0) {
>>
>> Should you be using %u instead of %d?
> yes, its unsigned, will fix.
> 
>> Also, snprintf returns >= size if the output was truncated, not < 0.
> 
>>From the man page:
> RETURN VALUE
> [...] Thus, a return value of size or more means that the output was truncated.
> If an output error is encountered, a negative value is returned.
> 
> I definitely need to check <0. but do I *also* need to check >= size? I
> dont think that can ever happen since 10chars+NULL fits fine.

I don't think either case is actually possible here (< 0 should only
occur with printf or fprintf variants, not s*printf, and as you note,
the truncation case should be covered).
Jason Zaman April 29, 2016, 12:01 p.m. UTC | #4
On Thu, Apr 28, 2016 at 02:13:30PM -0400, Stephen Smalley wrote:
> On 04/28/2016 01:53 PM, Jason Zaman wrote:
> > On Wed, Apr 27, 2016 at 01:04:25PM -0400, Stephen Smalley wrote:
> >> On 04/23/2016 02:04 AM, Jason Zaman wrote:
> >>> +		if (snprintf(uid, sizeof(uid), "%d", pwent->pw_uid) < 0
> >>> +		 || snprintf(gid, sizeof(gid), "%d", pwent->pw_gid) < 0) {
> >>
> >> Should you be using %u instead of %d?
> > yes, its unsigned, will fix.
> > 
> >> Also, snprintf returns >= size if the output was truncated, not < 0.
> > 
> >>From the man page:
> > RETURN VALUE
> > [...] Thus, a return value of size or more means that the output was truncated.
> > If an output error is encountered, a negative value is returned.
> > 
> > I definitely need to check <0. but do I *also* need to check >= size? I
> > dont think that can ever happen since 10chars+NULL fits fine.
> 
> I don't think either case is actually possible here (< 0 should only
> occur with printf or fprintf variants, not s*printf, and as you note,
> the truncation case should be covered).

So I think this is correct but i noticed a few more things in the man
page so I am just going to be cautious and check them all anyway.

1) glibc changed bahaviour:
"The glibc implementation of the functions snprintf() and vsnprintf()
conforms to the C99 standard, that is, behaves as described above, since
glibc  version  2.1.   Until  glibc 2.0.6, they would return -1 when the
output was truncated."

2) it looks like there might possibly be locale issues for some of the
stranger ones? i dont think it'd be an issue but having the check doesnt
exactly harm anything since genhomedircon is only run once when building
a policy. This also raises the issue of if there are locale issues
should semodule and friends be checking/resetting LANG/LC_NUMERIC for
sanity early on?

I'm going to send v3 of this patch with these fixes. Do you want me to
re-send the whole set or is just this one enough?

-- Jason

Patch
diff mbox

diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
index 1a7882c..56c58e0 100644
--- a/libsemanage/src/genhomedircon.c
+++ b/libsemanage/src/genhomedircon.c
@@ -82,10 +82,13 @@ 
 #define FALLBACK_PREFIX "user"
 #define FALLBACK_LEVEL "s0"
 #define FALLBACK_NAME ".*"
+#define FALLBACK_UIDGID "[0-9]+"
 #define DEFAULT_LOGIN "__default__"
 
 typedef struct user_entry {
 	char *name;
+	char *uid;
+	char *gid;
 	char *sename;
 	char *prefix;
 	char *home;
@@ -628,11 +631,13 @@  static int name_user_cmp(char *key, semanage_user_t ** val)
 }
 
 static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
-			   const char *sen, const char *pre, const char *h,
-			   const char *l)
+			   const char *u, const char *g, const char *sen,
+			   const char *pre, const char *h, const char *l)
 {
 	genhomedircon_user_entry_t *temp = NULL;
 	char *name = NULL;
+	char *uid = NULL;
+	char *gid = NULL;
 	char *sename = NULL;
 	char *prefix = NULL;
 	char *home = NULL;
@@ -644,6 +649,12 @@  static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
 	name = strdup(n);
 	if (!name)
 		goto cleanup;
+	uid = strdup(u);
+	if (!uid)
+		goto cleanup;
+	gid = strdup(g);
+	if (!gid)
+		goto cleanup;
 	sename = strdup(sen);
 	if (!sename)
 		goto cleanup;
@@ -658,6 +669,8 @@  static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
 		goto cleanup;
 
 	temp->name = name;
+	temp->uid = uid;
+	temp->gid = gid;
 	temp->sename = sename;
 	temp->prefix = prefix;
 	temp->home = home;
@@ -669,6 +682,8 @@  static int push_user_entry(genhomedircon_user_entry_t ** list, const char *n,
 
       cleanup:
 	free(name);
+	free(uid);
+	free(gid);
 	free(sename);
 	free(prefix);
 	free(home);
@@ -687,6 +702,8 @@  static void pop_user_entry(genhomedircon_user_entry_t ** list)
 	temp = *list;
 	*list = temp->next;
 	free(temp->name);
+	free(temp->uid);
+	free(temp->gid);
 	free(temp->sename);
 	free(temp->prefix);
 	free(temp->home);
@@ -738,7 +755,8 @@  static int setup_fallback_user(genhomedircon_settings_t * s)
 					level = FALLBACK_LEVEL;
 			}
 
-			if (push_user_entry(&(s->fallback), FALLBACK_NAME, 0, 0,
+			if (push_user_entry(&(s->fallback), FALLBACK_NAME,
+					    FALLBACK_UIDGID, FALLBACK_UIDGID,
 					    seuname, prefix, "", level) != 0)
 				errors = STATUS_ERR;
 			semanage_user_key_free(key);
@@ -768,6 +786,8 @@  static genhomedircon_user_entry_t *get_users(genhomedircon_settings_t * s,
 	const char *seuname = NULL;
 	const char *prefix = NULL;
 	const char *level = NULL;
+	char uid[10];
+	char gid[10];
 	struct passwd pwstorage, *pwent = NULL;
 	unsigned int i;
 	long rbuflen;
@@ -852,7 +872,13 @@  static genhomedircon_user_entry_t *get_users(genhomedircon_settings_t * s,
 		}
 		if (ignore(pwent->pw_dir))
 			continue;
-		if (push_user_entry(&head, name, seuname,
+
+		if (snprintf(uid, sizeof(uid), "%d", pwent->pw_uid) < 0
+		 || snprintf(gid, sizeof(gid), "%d", pwent->pw_gid) < 0) {
+			*errors = STATUS_ERR;
+			goto cleanup;
+		}
+		if (push_user_entry(&head, name, uid, gid, seuname,
 				    prefix, pwent->pw_dir, level) != STATUS_SUCCESS) {
 			*errors = STATUS_ERR;
 			break;