Protect fork() against dll- and exe-updates.

Corinna Vinschen corinna-cygwin@cygwin.com
Thu Jan 28 18:43:00 GMT 2016


Hi Michael,

On Jan 26 17:23, Michael Haubenwallner wrote:
> On 12/08/2015 10:48 PM, Corinna Vinschen wrote:
> > On Nov 16 19:07, Michael Haubenwallner wrote:
> [...]
> > Would you mind terribly to split this patch into a patchset so we
> > get a set of smaller patches as far as it makes sense?  I'm a bit
> > reluctant to apply such a big patch in one go.  I did this myself
> > a lot back in the pre-git CVS times, but the longer I work with git
> > the more I appreciate patches split into sets of smaller patches.
> > They are easier to review and *much* easier to handle when bisecting
> > the code in case of searching for a bug.
> 
> Accepted, fine with me!
> 've thought of splitting the patch along something like:
> * declare additional struct members
> * define additional methods as noop
> * call new noop methods in existing code
> * allocate additional memory
> * open additional handles
> * implement preparation methods
> * implement nomination methods
> * synchronize (use semaphore)
> * implement cleanup
> * implement hardlink-creation
> * activate hardlink-creation
> * probably more...
> 
> Will post them to -patches then.

Thanks, but splitting into a handful separately functional patches
should be sufficient.

> > Another thing occured to me:  Doesn't using this stuff break the output
> > of /proc/<PID>/maps?
> 
> Whenever the original binary has been removed - it is moved to trashbin
> actually, it turns out that /proc/<PID>/maps shows the trashbin-filename.
> Not sure if you call this "broken" - or what would be "unbroken" here.

No, what I mean is, what does it show if the file has *not* been deleted
yet?  The path to /usr/bin/foo or the path to /var/run/cygfork/<SID>/foo?
The latter case would be rather irritating.

This behaviour may not be that bad in case of running with a deleted
object, though.  On Linux /proc/$PID/maps prints the name of a deleted
object with a "(deleted)" tag.  Maybe we can get there, too.  Do we have
the information from where the file has been originally loaded still
available at that point?  I guess the answer is no...

> >> +/* Return *nameptr probably prefixed with "\\??\\", but
> >> +   update *nameptr to stay without the "\\??\\" prefix. */
> >> +PWCHAR
> >> +dll::to_ntname (PWCHAR *nameptr, WCHAR ntnamebuf[NT_MAX_PATH])
> > 
> > Why does dll::to_ntname need a second parameter if it's always called
> > on the buffer returned by dll::nt_max_path_buf?
> 
> This merely is to indicate that it does need some buffer. When removing
> the second parameter, I'd rename to something like dll::buffered_ntname.

ack

> >> +{
> >> +  /* avoid using path_conv here: cygheap might not be
> >> +     initialized when started from non-cygwin process,
> >> +     or still might be frozen in_forkee */
> >> +  PWCHAR ntname = *nameptr;
> >> +  if ((*nameptr)[1] == L':')
> > 
> > What if Cygwin has been installed on a network drive with no drive
> > letter attached?  In that case you'd have to deal with UNC paths,
> > but they are ignored here.
> 
> Isn't the UNC prefix cut off right after GetModuleFileNameW in
> dll_list::alloc?

Not necessarily.  GetModuleFileNameW returns \\?\ paths as well.  It
depends on how the executable has been loaded.

> Actually, I didn't fully grok the subtleties along "\\?\", "\??\",
> "UNC\", "\\" and their usage differences with functions from
> kernel32.dll and ntdll.dll yet.  Is it possible in every case that
> ntdll just may need the additional prefix "\??\", while kernel32 takes
> the same path without that prefix?

It's not very complicated, just puzzeling at first:

Short Win32 paths:

Drive letter path:   C:\foo\bar
UNC path:            \\server\share\baz

Equivalent long Win32 paths:

Drive letter path:   \\?\C:\foo\bar
UNC path:            \\?\UNC\server\share\baz

Equivalent native NT paths:

Drive letter path:   \??\C:\foo\bar
UNC path:            \??\UNC\server\share\baz

Note 1: Long Win32 paths are identical to internal NT paths, except
	for the first question mark replaced by a backslash.  Why?
	I have no idea.  History, probably.

Note 2: Short Drive letter paths simply get a "\??\" prepended to be
	converted to NT paths.

Note 3: UNC paths get their first backslash replace with "\??\UNC",
        so one backslash is lost on the way to the native NT form.

Note 4: "\??\" is just the name of a(*) virtual directory in the native
	NT namespace which contains symlinks to the actual devices.  The
	"winobj" tool from sysinternals is quite instructive.  Exposure
	to the Win32 world is performed by the QueryDosDevices and
	DefineDosDevice calls.

	(*) Actually more than one, what with system and user symlinks
	    merged into a single view...

> I do think of storing only the ntdll-name in the structures, and have a non-
> prefix name pointer into that ntdll-name buffer for use by CreateProcess&co then.

Good idea in general.  Just the UNC path problem above is slightly in
the way.

> >> +      LARGE_INTEGER zero = { 0 };
> >> +      d->fii.FileId = zero;
> > 
> > Oops, sorry, FileId -> NameIndex :}
> 
> IndexNumber, actually.

Duh, right.

> >> @@ -396,7 +497,7 @@ dll_list::detach (void *retaddr)
> >>    if (!myself || in_forkee)
> >>      return;
> >>    guard (true);
> >> -  if ((d = find (retaddr)))
> >> +  if ((d = find (retaddr)) && d->type != DLL_SELF)
> > 
> > dll_list::find is only ever used to find dlls with d->type != DLL_SELF.
> > Wouldn't it make sense to move this check into the method?
> 
> You mean into the dll_list::find method?

Yep.

> >> diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
> >> index 084f8f0..dd0f9a6 100644
> >> --- a/winsup/cygwin/fork.cc
> >> +++ b/winsup/cygwin/fork.cc
> >> @@ -356,8 +351,19 @@ frok::parent (volatile char * volatile stack_here)
> >>  
> >>    while (1)
> >>      {
> >> +      dlls.request_forkables ();
> > 
> > I'm aware that hold_everything has been called but, is that safe?
> > request_forkables calls update_forkables_needs which in turn uses the
> > static dll::nt_max_path_buf buffer...
> 
> While I have a vague idea only on what hold_everything actually does,
> isn't one of the ideas for the static buffer to be useable even while
> everything else (heap in particular) is on hold?
> Especially as that static buffer does have the NO_COPY attribute...

Yeah, right.

> > So what happens with a path where the parent dir path is > NAME_MAX?
> > If I didn't miss something, this won't work for very long paths.
> 
> Reading through the docs I've got the impression that while NT_MAX_PATH is
> to hold a very long path, a single path part is still limited to NAME_MAX,
> but I may easily be wrong here.

Well, yes, NAME_MAX is the maximum length of a single path component.
But the comment preceeding mangle_as_filename says:

  "Mangle full srcpath as one single filename ..."

The function replaces backslashes with commas.  So, IIUC, an incoming
path consisting of, e.g., a dir with NAME_MAX length, a backslash, and
a filename with NAME_MAX length will be converted to an invalid path
with a single path component of 2 * NAME_MAX + 1 (comma) length.  What
am I missing?

> >> +  wcscpy (forkable_name, dirx_name);
> > 
> > Suggestion: Use
> > 
> >      PWCHAR p = wcpcpy (forkable_name, dirx_name);
> >      
> > You can use p later on...
> 
> > ...and here as in
> > 
> >          mangle_as_filename (p, name, &lastpathsep);
> > 
> > This avoids calling wcslen twice.
> 
> ok, 've not seen wcpcpy before.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/wcpcpy.html

> > Is it safe to use sec_none_nih for all of them?
> 
> As long as /var/run/cygfork exists with tmpdir-like permission,
> sec_none_nih seems fine: For the directories created, ls -l shows
> 'drwxr-xr-x+' permissions.

That's inherited from the parent.  Hmm, ok, yeah, that sounds like
the right thing.  We can revisit it later if a need arises for some
reason.

> > Alternatively we could add a cygheap->installation_root_len member.
> 
> Feels like subsequent optimization - beyond this feature patch for now.

Ok.

> >> +      if (d->fbi.LastWriteTime.QuadPart > newest.QuadPart)
> >> +	newest = d->fbi.LastWriteTime;
> > 
> > LastWriteTime?  This is supposed to check if a newer DLL replaced
> > an older in-use one, so shouldn't that be CreationTime?
> 
> CreationTime bumps even when creating a hardlink,

Not on my machine.  Observe the "Birth" date in stat after creating a
hardlink to a file:

  $ stat tsock.c | grep Birth
   Birth: 2016-01-11 18:57:13.584871900 +0100
  $ ln tsock.c tsock.link
  $ stat tsock.c | grep Birth
  $ stat tsock.c tsock.link | grep Birth
   Birth: 2016-01-11 18:57:13.584871900 +0100
   Birth: 2016-01-11 18:57:13.584871900 +0100

Are you confusing CreationTime with ChangeTime by any chance?

> while LastWriteTime
> seems to more properly tell about the last file-content modification.

LastWriteTime might be sufficient, but the file is actually recreated
when replacing it, it's not overwritten, that's why CreationTime sounds
more correct to me.  Unfortunately CreationTime is as much unsafe
against tampering as is LastWriteTime, so never mind.

A bit awkward is the name of the method, though.  Ctime is the POSIX
equivalent for ChangeTime, not for LastWriteTime.  Either mtime when
using LastWriteTime or birthtime when using CreationTime would be
better, methinks.

> >> +/* Create the nominated forkable hardlinks and directories as necessary,
> >> +   as well as the .local file for dll-redirection. */
> >> +bool
> >> +dll_list::create_forkables ()
> >> +{
> >> 
> >> +  if (!mkdirs (buf, &sec_none_nih, lastsepcount))
> > 
> > Again, sec_none_nih for the entire directory tree?
> 
> As I didn't fully grok the windows security too: What is your concern here?

Not sure.  I was just mulling over the default perms resulting from
it being inadaquate.  Let's just keep it in mind.

> > What's missing is user documentation for this feature.  A bit of
> > description what happens, what has to be done by the user, and what
> > limitations it has would be helpful.
> 
> Agreed: Where to add such docs?

Good question.  The docs have grown a bit wild...  maybe create a chapter
in pathnames.xml for now.  We can move it around later.

There's one point I forgot when I reviewed the patch last year.  You're
doing the check if /var/run/cygfork exists in dll_list::create_forkables
once per process, per loaded DLL.  Why?  In theory, if you found out
that you can't create a hardlink once, you're done.

What I'd like to see is that failing to hardlink a file the first time
*because /var/run/cygfork does not exist* results in setting a flag in
cygheap so that no other process in the process tree ever tries to use
this feature again.  An explicit check for existence of the dir seems
necessary at some early stage in the code.  Creating the directory and
exiting Cygwin processes is required to activate the feature then, but
that's ok.  The impact on users not using this feature should be as low
as possible.

I'm still not overly excited about using the existence of the dir alone
as marker to activate this feature, but that can be added later...


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-developers/attachments/20160128/41c8d822/attachment.sig>


More information about the Cygwin-developers mailing list