Details

Applies to

1.8.2 and later (unix installations)

Purpose

bug fix - Unable to install under setuid/setgid wrapper.

Description

This patch replaces disrecommended os.access() usage with recommended feckless open.

Testing

Limited testing has been performed.

• edit an existing page, save, and note changes
• attempt to edit a protected (Help) page (note "Immutable Page")
• create a new page, save, and note changes

User Documentation

HelpOnInstalling/SetuidWrapperOnPosix

Rationale

os.access(...W_OK...) does not report whether or not the current process is able to write. Rather it reports whether the login process (i.e. web server agent) is able to write.

Improper use of os.access() in Page.py prevents adding pages or saving changes if moin.cgi is installed under setuid wrapper.

The danger of granting write privilege to the web server agent is that it gives wiki file ownership to the web server agent and so overrides wiki security settings. Denying write privilege to the web server agent, without this patch, prevents the wiki from adding or changing pages.

Patch

Rather than mark a page immutable if the web server cannot write it, mark it immutable if an attempt to open it (for appending nothing) fails.

   1 --- moin-1.8.2/MoinMoin/Page.py	2009-02-07 20:31:01.000000000 -0500
   2 +++ moin-1.8.2/MoinMoin/Page.py	2009-04-26 16:43:48.000000000 -0400
   3 @@ -598,7 +598,16 @@
   4          @rtype: bool
   5          @return: true, if this page is writable or does not exist
   6          """
   7 -        return os.access(self._text_filename(), os.W_OK) or not self.exists()
   8 +        result = True
   9 +        if self.exists():
  10 +            try:
  11 +                # Attempt to append nothing fecklessly
  12 +                open(self._text_filename(), 'a').close()
  13 +            except IOError:
  14 +                # Failed open should return errno.EACCES
  15 +                # Any repeatable error will prevent edit/creation
  16 +                result = False
  17 +        return result
  18  
  19      def isUnderlayPage(self, includeDeleted=True):
  20          """ Does this page live in the underlay dir?

Include read-only dictionaries in those searched.

   1 --- moin-1.8.2/MoinMoin/action/SpellCheck.py	2009-01-06 19:26:50.000000000 -0500
   2 +++ moin-1.8.2/MoinMoin/action/SpellCheck.py	2009-04-22 00:49:12.000000000 -0400
   3 @@ -39,7 +39,7 @@
   4      # validate candidate list (leave out directories!)
   5      wordsfiles = []
   6      for f in candidates:
   7 -        if os.path.isfile(f) and os.access(f, os.F_OK | os.R_OK):
   8 +        if os.path.isfile(f):
   9              wordsfiles.append(f)
  10  
  11      # return validated file list

Don't fail to start just because pages directory is read-only.

   1 --- moin-1.8.2/MoinMoin/config/multiconfig.py	2009-01-23 23:39:19.000000000 -0500
   2 +++ moin-1.8.2/MoinMoin/config/multiconfig.py	2009-04-27 09:21:47.000000000 -0400
   3 @@ -535,7 +535,6 @@
   4          Both data and underlay should exists and allow read, write and
   5          execute.
   6          """
   7 -        mode = os.F_OK | os.R_OK | os.W_OK | os.X_OK
   8          for attr in ('data_dir', 'data_underlay_dir'):
   9              path = getattr(self, attr)
  10  
  11 @@ -544,7 +543,7 @@
  12                  continue
  13  
  14              path_pages = os.path.join(path, "pages")
  15 -            if not (os.path.isdir(path_pages) and os.access(path_pages, mode)):
  16 +            if not os.path.isdir(path_pages):
  17                  msg = """
  18  %(attr)s "%(path)s" does not exist, or has incorrect ownership or
  19  permissions.

Discussion

• Historical (resolved) issues on separate page

• Three files continue to use os.access.
  • At this time I don't know how (or if) they should be altered.

    • moin-1.8.2/MoinMoin/logfile/__init__.py

    • moin-1.8.2/MoinMoin/script/export/dump.py

    • moin-1.8.2/MoinMoin/server/__init__.py

    • -- Unw 2009-04-25 22:40:23

  • Note about incomplete patches: if there is a fundamental problem with os.access, then all os.access usage would have to get removed from moin, not just some.

    • Further, I got some feedback on IRC from BastianBlank that calling the Python interpreter that way (with UID != EUID) shouldn't be a supported configuration (maybe he could add a comment here, I guess he can explain that better than I could).

  • -- 2009-05-11 00:00:01

  • In the last two weeks, I've looked at the remaining os.access usage. I haven't assembled a test environment, but it looks like all three should in fact be removed and replaced. The problem is depending on os.access() for a side-effect (logical and of file permissions mask) rather than its design purpose (report of user authority). See http://linux.die.net/man/2/access for details.

    • -- Unw 2009-05-11 02:40:23

  • I would appreciate BastianBlank's explanation.

    • -- Unw 2009-05-12 00:40:23

Plan

• Priority:
• Assigned to:
• Status: patches exist, but are incomplete

CategoryMoinMoinPatch