[Monotone-devel] 0.25.1 release candidate -- testing appreciated

[Nathaniel Smith]

I've just committed a minimal change to 0.25 to close the recently
discovered security hole.  This bug allows one person with commit
access to run arbitrary code on the machines of people who check out
their tree.  The proposed 0.25.1 works on my machine, but I'd
appreciate hearing from other people, especially on non-linux
platforms.  (Since this is the first time we've ever used a "stable
release branch", we don't have the same automatic testing
infrastructure set up for them...)

To test:
  -- pull from venge.net as usual
  -- monotone checkout -b net.venge.monotone.0.25-fixes <whatever>
  -- cd <whatever>; autoreconf -i; ./configure
  -- make check
     Make sure that all tests pass.
  -- ./testsuite -v 292
     Make sure that this test actually does interesting things; it is
     the test to make sure that the security hole is fixed, but since
     it requires gunzip and unbase64 capabilities, it will silently
     succeed if those cannot be found.

Thanks.

The only change versus 0.25 is the following patch:

--- paths.cc    cb6280a09253523cfa6c8b206214528b7cd9b5d2
+++ paths.cc    d453375361f010f02ba40ea7151d49214496c6c7
@@ -156,10 +156,25 @@
   return true;
 }
 
+// This function considers MT, mt, Mt, mT to all be bookkeeping paths, because
+// on case insensitive filesystems, files put in any of them may end up in MT
+// instead.  This allows arbitrary code execution.  A better solution would be
+// to fix this in the working directory writing code -- this prevents all-unix
+// projects from naming things "mt", which is a bit rude -- but this is a bug
+// fix release.
 static inline bool
 in_bookkeeping_dir(std::string const & path)
 {
- return path == "MT" || (path.size() >= 3 && (path.substr(0, 3) == "MT/"));
+  if (path.size() == 0 || (path[0] != 'M' && path[0] != 'm'))
+    return false;
+  if (path.size() == 1 || (path[1] != 'T' && path[1] != 't'))
+    return false;
+ // if we've gotten here, the first two letters are M and T, in either upper
+ // or lower case.  So if that is the whole path, or else if it continues but
+  // the next character is /, then this is a bookkeeping path.
+  if (path.size() == 2 || (path[2] == '/'))
+    return true;
+  return false;
 }

I'd also appreciate comments from other developers on whether this
patch looks correct, and its approach reasonable :-).

Cheers,
-- Nathaniel

-- 
So let us espouse a less contested notion of truth and falsehood, even
if it is philosophically debatable (if we listen to philosophers, we
must debate everything, and there would be no end to the discussion).
  -- Serendipities, Umberto Eco