Re: ntsec patch #4: passwd and group

["Pierre A. Humblet" <Pierre dot Humblet at ieee dot org>]

Corinna Vinschen wrote:
> 
> On Fri, Nov 08, 2002 at 10:15:56AM -0500, Pierre A. Humblet wrote:
> > The main changes are in the passwd/group emulation code,
> > which has been totally redone to cover the four cases.
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> *Shudder*, is that actually needed?  Doesn't fixing single problems help?

Have a look. It's not bad at all (IMHO) and it works.

> > Additionally two new functions have been created:
> > getpwsid and getgrsid.
> 
> Isn't that functionality already given by cygsid::getfrompw() and
> cygsid::getfromgr()?

Those functions don't scan the passwd file, they take a specific
pw entry as argument and return the sid. The new functions take a 
sid as argument and return a pw entry (behave as e.g. getpwuid). 

> > There is one design decision:
> >  when the group entry is missing, the code calls
> >  LookupAccountSidA to get the current group name.
> >  That will cause delays for domain users starting
> >  Cygwin while disconnected from the PDC
> >  (but only if /etc/group is incomplete).
> >  The alternative is to always use "unknown".
> 
> Yeah, we should think a bit about this.  Is that actually a problem?
> AFAICS, the LookupAccountSidA() calls are only asking the local box.

But the localbox will contact the PDC if the sid isn't local. 
We have tested that already. Some people, with badly configured 
group files, will think it's a problem! In my opinion it should 
be a motivation to fix the group file. At any rate it's an unusual
case to be disconnected from the PDC and to have a bad group file.

> > 2) I thought that the passwd/group files where only
> >    read "for the first cygwin process that start up
> >    on a given console", to use Chris' words in
> >    http://cygwin.com/ml/cygwin-patches/2002-q4/msg00024.html
> 
> I discussed this with Chris in innumerable one-on-ones but we
> never found a satisfactory solution for keeping the data just
> once in memory.  I can't reiterate right away but every new
> idea had a flaw.  I'm still at times thinking about something
> with shared memory but there are as usual security concerns.

Can you elaborate? Why is it a problem to store them in the 
cygheap? 
I saw Chris' comments about slowing things down. That makes 
sense for programs that never stat or create a file with uid 
and gid different from the current user.
However when Chris was doing the test this caching mechanism
wasn't present yet, stating or creating any file would force
a passwd/group reread. So it's more surprising.
 
> > 4) Altough I see some mutual exclusion stuff, I don't see
> >    what prevents two threads from simultaneously updating
> >    the internal copies, or one thread to free them while
> >    they are used by another.
> 
> The passwd_lock and group_locks.  Look into the other functions, e.g.:
> 
>   getgrnam32 (const char *name)
>   {
>     if (group_state  <= initializing)
>         read_etc_group ();

Right, that's what I meant by "I see some mutual exclusion stuff".
Assuming it works, after the thread has gone through the test and
is happily scanning the internal passwd, a user could touch passwd 
and another thread could come along and free that internal structure.

 >>    In fact applications such as sshd would benefit from
> >>    rereading the files (if needed) *before* forks or execs,
> >>    so that a single reread can serve all future children,
> >>    but that approach does not help with thread issues.

That was just an observation, not a proposal. 

Pierre