Bug Hunting Session
Bug 98210 - Pipe socket directory permissions checks incorrect (R_OK not needed) and inconsistent
Summary: Pipe socket directory permissions checks incorrect (R_OK not needed) and inco...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: All Linux (All)
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:5.2.0 target:5.1.2
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-26 16:33 UTC by Hank Leininger
Modified: 2016-10-25 19:08 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
Only require W_OK for socket dir, not R_OK, and bail with a useful error (1.54 KB, patch)
2016-02-26 16:33 UTC, Hank Leininger
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Hank Leininger 2016-02-26 16:33:18 UTC
Created attachment 123015 [details]
Only require W_OK for socket dir, not R_OK, and bail with a useful error

Libreoffice silently fails at startup on UNIX if /tmp and /var/tmp are mode 1733 (i.e. not world-readable).  The splash will paint, but then it exits, with no error messages.  Proposed patch attached.

It is a common system-hardening recommendation to make per-user TMPDIRs, and to remove read permission from world-writeable directories.  Generally programs that create tempfiles know their names, and so do not need to readdir() the temp directories.

In LibreOffice there are two implementations of pipe-path-selection; both insist on R_OK in addition to W_OK, but they are otherwise divergent in implementation and behavior.  Neither catches errors and prints a diagnostic error message.

If I understand the flow correctly, during startup one process calls osl_psz_createPipe() from sal/osl/unx/pipe.cxx to create the OSL_PIPE_... socket.  Another process calls get_pipe_path() from desktop/unx/source/start.c to attach to it.

These two implement things slightly differently.  They both default to /tmp followed by /var/tmp (PIPEDEFAULTPATH and PIPEALTERNATEPATH respectively).  They both ignore things like the TMPDIR environment variable.  (Possibly this is to avoid odd behavior if TMPDIR is on a shared filesystem, or one that does not support socket files, etc.  See for example https://bz.apache.org/ooo/show_bug.cgi?id=55153 , this was deemed "not a bug" in 2005... that is not the not-a-bug that I am talking about :).

However, from there on they diverge.  desktop/unx/source/start.c contains:

      if ( access( PIPEDEFAULTPATH, R_OK|W_OK ) == 0 )
          rtl_uString_newFromAscii( &pResult, PIPEDEFAULTPATH );
      else
          rtl_uString_newFromAscii( &pResult, PIPEALTERNATEPATH );

...So, it checks if /tmp is both readable & writable, and if not, it selects /var/tmp - but performs no checks at all on /var/tmp.

Meanwhile, sal/osl/unx/pipe.cxx contains:

      if (access(PIPEDEFAULTPATH, R_OK|W_OK) == 0)
      {
          strncpy(name, PIPEDEFAULTPATH, sizeof(name));
      }
      else if (access(PIPEALTERNATEPATH, R_OK|W_OK) == 0)
      {
          strncpy(name, PIPEALTERNATEPATH, sizeof(name));
      }
      else if (!cpyBootstrapSocketPath (name, sizeof (name)))
      {
          return nullptr;
      }

...It performs access() checks on both, falling back to cpyBootstrapSocketPath, which uses OSL_SOCKET_PATH.  It looks to me like this was added for iOS and Android, and that such platforms use a different launcher than desktop/unx/source/start.c.  Since the latter was never updated to use OSL_SOCKET_PATH as a fallback, it isn't usable there (if that was in fact intended).  The caller of the caller of the caller of get_pipe_path() eventually gets the memo to give up, but no error is printed to the user, libreoffice just exits.

The code has changed over the years, but as far back as the initial import in 2000-09-18, we see:

^9399c66 (Jens-Heiner Rechtien 2000-09-18 14:18:43 +0000 209)     if (access(PIPEDEFAULTPATH, O_RDWR) == 0)

So +r has been required for a very long time.

Anyway, I rebuilt LibreOffice with a patch I'll attach that:

a) removes the R_OK part of the access() checks in both start.c and pipe.cxx

b) adds to start.c the missing access() check for /var/tmp

c) adds an error-and-quit to start.c if the (now relaxed) access() checks still fail.

This compiles and runs fine with /tmp and /var/tmp mode 1733, under some simple light usage, but I have not tested extensively.

Notably, I did _not_ attempt to implement the OSL_SOCKET_PATH fallback in start.c, because I'm not sure that's desired/intended, and it's more room for me to get something wrong.
Comment 1 Julien Nabet 2016-02-26 18:51:33 UTC
Perhaps you might be interested in submitting your patch to LO gerrit.
If yes, this link will guide you:
https://wiki.documentfoundation.org/Development/GetInvolved
(knowing that you've already done it a good part of it: find a bug, build LO, create a patch, there are just a few steps left)
Comment 2 Hank Leininger 2016-02-27 01:33:31 UTC
Thanks for the suggestion; done:

https://gerrit.libreoffice.org/#/c/22727/
Comment 3 Hank Leininger 2016-02-27 02:21:40 UTC
Hm, OK, the Jenkins build after I pushed, failed for MacOSX: http://ci.libreoffice.org/job/lo_gerrit_master/12420/Gerrit=Gerrit,Platform=MacOSX/console

I don't have an OSX environment to test.  However, a) it died in an area of the code I did not touch, and b) several recent Jenkins builds of master for MacOSX failed in what looks like the same way/place, such as:

http://ci.libreoffice.org/job/lo_gerrit_master/12417/Gerrit=Gerrit,Platform=MacOSX/console

http://ci.libreoffice.org/job/lo_gerrit_master/12416/Gerrit=Gerrit,Platform=MacOSX/console


So, I think that means that master is/was already broken.  I'm 100% new to the LibreOffice development ecosystem, so I don't know how common an occurrence this is, or if I should do anything other than wait.  What would you recommend?

Thanks!
Comment 4 Julien Nabet 2016-02-27 08:17:28 UTC
(In reply to Hank Leininger from comment #3)
> Hm, OK, the Jenkins build after I pushed, failed for MacOSX:
> http://ci.libreoffice.org/job/lo_gerrit_master/12420/Gerrit=Gerrit,
> Platform=MacOSX/console
> 
> I don't have an OSX environment to test.  However, a) it died in an area of
> the code I did not touch, and b) several recent Jenkins builds of master for
> MacOSX failed in what looks like the same way/place, such as:
> 
> http://ci.libreoffice.org/job/lo_gerrit_master/12417/Gerrit=Gerrit,
> Platform=MacOSX/console
> 
> http://ci.libreoffice.org/job/lo_gerrit_master/12416/Gerrit=Gerrit,
> Platform=MacOSX/console
> 
> 
> So, I think that means that master is/was already broken.  I'm 100% new to
> the LibreOffice development ecosystem, so I don't know how common an
> occurrence this is, or if I should do anything other than wait.  What would
> you recommend?
> 
> Thanks!

Just be patient :-) There are a lot of patches which have been submitted and there aren't dozens of devs.
About MacOs part, don't worry for the moment. Indeed Tinderboxes show pb with MacOs (see http://tinderbox.libreoffice.org/MASTER/status.html)
Anyway, thank you for your contribution!
Comment 5 Commit Notification 2016-03-02 08:18:58 UTC
Hank Leininger committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=87c011134031374cf9104ec2fc39ef121d8a6802

tdf#98210 do not require R_OK for pipe dir

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 6 Commit Notification 2016-03-09 10:50:48 UTC
Hank Leininger committed a patch related to this issue.
It has been pushed to "libreoffice-5-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=39c3b7e39f76a2aacf4c1b659f98c0fe897dc396&h=libreoffice-5-1

tdf#98210 do not require R_OK for pipe dir

It will be available in 5.1.2.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 7 Julien Nabet 2016-03-09 10:53:45 UTC
Let's put this one to FIXED.