Re: [patch] mmap() MAP_TEXT implementation (to use for shared libraries)

On Fri, Sep 07, 2012 at 02:05:28PM -0400, John Baldwin wrote:
> On Friday, September 07, 2012 12:42:18 pm Konstantin Belousov wrote:
> > On Fri, Sep 07, 2012 at 12:21:37PM -0400, John Baldwin wrote:
> > > On Friday, September 07, 2012 12:02:08 pm Konstantin Belousov wrote:
> > > > On Fri, Sep 07, 2012 at 05:12:37PM +0200, Svatopluk Kraus wrote:
> > > > > On Tue, Sep 4, 2012 at 6:00 PM, John Baldwin <jhb_at_freebsd.org> wrote:
> > > > > > On Tuesday, September 04, 2012 9:00:39 am Konstantin Belousov wrote:
> > > > > >> 2. I do not see what would prevent malicious local user from mmaping
> > > > > >>    arbitrary file readonly with MAP_TEXT, thus blocking any modifications
> > > > > >>    to the file. Note that this is not a problem for executables, because
> > > > > >>    kernel only sets VV_TEXT on executables if +x permission is set and
> > > > > >>    file is valid binary which kernel is able to execute.
> > > > > >>
> > > > > >>    E.g. you might block log writes with VV_TEXT, or other user editing
> > > > > >>    session or whatever, having just read access to corresponding files.
> > > > > >>
> > > > > >> Am I wrong ?
> > > > > >
> > > > > > Hmm, I do think 2) is a bit of a show-stopper.  I do wonder why one needs
> > > > > > MAP_TEXT at all or if you could key this off of mmap() with PROT_EXEC?
> > > > > > Do we require +x permissions for PROT_EXEC?  No, it seems we only require
> > > > > > a file opened with FREAD.  Hmm, perhaps rtld could open a separate fd for
> > > > > > PROT_EXEC mappings that used O_EXEC and mmap()'ing an O_EXEC fd could enable
> > > > > > VV_TEXT?  That would require a file to have +x permisson for an mmap() to
> > > > > > enable VV_TEXT.  It would also make MAP_TEXT unneeded.
> > > > > 
> > > > > It sounds good for me. I will try to patch it this way. However, do
> > > > > you think that will be acceptable to set +x permission to shared
> > > > > libraries in general?
> > > 
> > > Shared libraries have +x by default already.  You could make rtld fall back
> > > to using the O_RDONLY fd if it can't open an O_EXEC fd in which case you don't
> > > get the VV_TEXT protection, but rtld would be backwards compatible.
> > Where is it ? On fresh stable/9 machine, I have in /lib:
> > -r--r--r--  1 root  wheel     7216 Dec  3  2011 libipx.so.5
> > -r--r--r--  1 root  wheel    19800 Jun 28 18:33 libjail.so.1
> > -r--r--r--  1 root  wheel    13752 Jun 28 18:33 libkiconv.so.4
> > ...
> 
> Hmm, I was looking in /usr/local/lib.  It seems at least some ports do
> install libraries with +x:
> 
> -rwxr-xr-x  1 root  wheel    210494 Apr  8  2011 /usr/local/lib/libxml++-2.6.so.2
> -rwxr-xr-x  1 root  wheel   1515416 Apr  8  2011 /usr/local/lib/libxml2.so.5
> -rwxr-xr-x  1 root  wheel     44389 Apr  8  2011 /usr/local/lib/libxmlparse.so.1
> -rwxr-xr-x  1 root  wheel    100672 Apr  8  2011 /usr/local/lib/libxmltok.so.1
> -rwxr-xr-x  1 root  wheel    266775 Apr  8  2011 /usr/local/lib/libxslt.so.2
> -rw-r--r--  1 root  wheel    764602 Apr  8  2011 /usr/local/lib/libxvidcore.so.4
> -rwxr-xr-x  1 root  wheel     53379 Apr  9  2011 /usr/local/lib/libzip.so.1
> 
> > > > Setting +x on shared libraries can be done. But setting VV_TEXT for
> > > > such mappings is definitely non-standard behaviour, that could cause
> > > > locking surprises for unaware system administrator. The issuw would be
> > > > very hard to diagnose.
> > > 
> > > I'm not sure it would be that obscure.  It's the same as getting an error that
> > > a binary is busy.  In that case you would resort to 'ps' to find the offending
> > > process.  For this case (a shared library being busy), you could look at the
> > > output of 'procstat -af' to find which processes have executable mappings of
> > > the library.
> > 
> > I much more worry about rtld reporting 'shared library is busy'. It is fine
> > to not be able to write to mapped library.
> > 
> > Rtld errors are usually quite worrying for users, and they just do not
> > understand them.
> 
> I think these would be rare?  There's no good reason for anything to write to
> a shared library that I can think of.  install(1) does an atomic rename to swap
> in the new libraries already.

After a second thought, I do not like your proposal as well. +x is set for
shebang scripts, and allowing PROT_EXEC to set VV_TEXT for them means
that such scripts are subject for write denial.