Re: ntsec patch #4: passwd and group

[Corinna Vinschen <cygwin-patches at cygwin dot com>]

On Sun, Nov 17, 2002 at 10:44:18PM -0500, Pierre A. Humblet wrote:
> Corinna,
> 
> Almost same as last week, but with the diff and ChangeLog against current CVS.
> I also made a small change.
> [...]

Some questions and comments:

> 
> 	* security.h: [...]. Undeclare internal_getpwent.
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
			     You didn't.

> 	* passwd.cc (getpwsid): Create.
> 	(internal_getpwent): Suppress.
> 	(read_etc_passwd): Make static. Rewrite the code for the completion
> 	line. Set curr_lines to 0.
> 	(parse_pwd): Change type to static int. Return 0 for short lines.
> 	(add_pwd_line): Pay attention to the value of parse_pwd.     
> 	(search_for): Do not look for nor return the DEFAULT_UID.
> 	* grp.cc (read_etc_group): Make static. Free gr_mem and set 
> 	curr_lines to 0. Always call add_pwd_line. Rewrite the code for the 
> 	completion line.
> 	(parse_grp): If grp.gr_mem is empty, set it to &null_ptr.
> 	Never NULL gr_passwd. 
> 	(getgrgid32): Only return the default if ntsec is off and the gid is 
> 	ILLEGAL_GID.
> 	* sec_helper.cc (cygsid::get_id): Use getpwsid and getgrsid;
> 	(cygsid_getfrompw): Clean up last line.
> 	(cygsid_getfromgr): Ditto.
> 	(is_grp_member): Use getpwuid32 and getgrgid32.
> 	* uinfo.cc (internal_getlogin): Set DEFAULT_GID at start.
> 	Use getpwsid. Move the read of /etc/group after the second access 
> 	of /etc/passwd. Change some debug_printf. 

> -/* FIXME: should be static but this is called in uinfo_init outside this
> -   file */
> -void
> +static void
>  read_etc_group ()

Do I miss something?  I don't see that in this patch.

> @@ -150,76 +145,74 @@ read_etc_group ()
>    if (group_state != initializing)
>      {
>        group_state = initializing;
> +      for (int i = 0; i < curr_lines; i++)
> +	if ((group_buf + i)->gr_mem != &null_ptr)
> +	  free ((group_buf + i)->gr_mem);
> +
> +      curr_lines = 0;
>        if (gr.open ("/etc/group"))
>  	{
>  	  char *line;
>  	  while ((line = gr.gets ()) != NULL)
> -	    if (strlen (line))
> -	      add_grp_line (line);
> +            add_grp_line (line);
> 
>  	  group_state.set_last_modified (gr.get_fhandle (), gr.get_fname ());
> -	  group_state = loaded;
>  	  gr.close ();
>  	  debug_printf ("Read /etc/group, %d lines", curr_lines);
>  	}
> -      else /* /etc/group doesn't exist -- create default one in memory */
> -	{
> -	  char group_name [UNLEN + 1];
> -	  DWORD group_name_len = UNLEN + 1;
> -	  char domain_name [INTERNET_MAX_HOST_NAME_LENGTH + 1];
> -	  DWORD domain_name_len = INTERNET_MAX_HOST_NAME_LENGTH + 1;
> -	  SID_NAME_USE acType;
> +
> +      /* Complete /etc/group in memory if needed */
> +      if (!getgrgid32 (myself->gid))
         
?!? How is that supposed to work?  We're in group_state==initializing,
therefore in getgrgid32(), read_etc_group() is called.  Isn't that
somewhat dangerous?

> +	      cygheap->user.groups.pgsid.string (strbuf);
> +	      if (!(gr = getgrsid (cygheap->user.groups.pgsid)))
> +	        {
> +		  if (!LookupAccountSidA (NULL, cygheap->user.groups.pgsid,
> +					  group_name, &group_name_len,
> +					  domain_name, &domain_name_len,
> +					  &acType))
> +		    debug_printf ("Failed to get primary group name. %E");

Didn't you propose to get rid of the LookupAccountSidA() calls?

> -  return allow_ntsec ? NULL : default_grp;
> +  return (!allow_ntsec && gid == ILLEGAL_GID)?default_grp:NULL;

I'd better like

     return allow_ntsec || gid != ILLEGAL_GID ? NULL : default_grp;


>  extern "C" struct __group16 *
> @@ -482,13 +474,9 @@ setgroups32 (int ngroups, const __gid32_
>        for (int gidy = 0; gidy < gidx; gidy++)
>  	if (grouplist[gidy] == grouplist[gidx])
>  	  goto found; /* Duplicate */
> -      for (int gidy = 0; (gr = internal_getgrent (gidy)); ++gidy)
> -	if (gr->gr_gid == (__gid32_t) grouplist[gidx])
> -	  {
> -	    if (gsids.addfromgr (gr))
> -	      goto found;
> -	    break;
> -	  }
> +      if ((gr = getgrgid32 (grouplist[gidx])) &&

Ahem, I thought we agreed that we don't call external functions from
inside Cygwin?  Never mind, there are still some of them which we have
to eliminate, anyway.

> @@ -208,24 +190,17 @@ is_grp_member (__uid32_t uid, __gid32_t
>      }
> 
>    /* Otherwise try getting info from examining passwd and group files. */
> -  for (int idx = 0; (pw = internal_getpwent (idx)); ++idx)
> -    if ((__uid32_t) pw->pw_uid == uid)
> -      {
> -	/* If gid == primary group of uid, return immediately. */
> -	if ((__gid32_t) pw->pw_gid == gid)
> -	  return TRUE;
> -	/* Otherwise search for supplementary user list of this group. */
> -	for (idx = 0; (gr = internal_getgrent (idx)); ++idx)
> -	  if ((__gid32_t) gr->gr_gid == gid)
> -	    {
> -	      if (gr->gr_mem)
> -		for (idx = 0; gr->gr_mem[idx]; ++idx)
> -		  if (strcasematch (cygheap->user.name (), gr->gr_mem[idx]))
> -		    return TRUE;
> -	      return FALSE;
> -	    }
> -        return FALSE;
> -      }
> +  if ((pw = getpwuid32 (uid)))

Same here.  Somehow it's a step in the wrong direction...

Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Developer                                mailto:cygwin@cygwin.com
Red Hat, Inc.