Re: <strong>CVS Security Vulnerability</strong>

[Derek Robert Price]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Derek Robert Price wrote:

> Hi all,
>
> For those who don't know, cvshome.org is currently down because it was
> hacked, via its CVS server we believe.  cvshome.org was used to send
> an email that contains an exploit for the security vulnerabiliy
> CAN-2004-0396
> <http://www.cve.mitre.org/cgi-bin/cvename.cgi?name=CAN-2004-0396>
> patched in releases 1.11.16 & 1.12.8.


For those who would rather patch older releases of CVS, I've attached
an almost-clean patch for this vulnerability.

Derek
- --
                *8^)

Email: derek@ximbiot.com

Get CVS support at <http://ximbiot.com>!
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFAs1G9LD1OTBfyMaQRAuB2AKCVVaL1aqCM8RMB46IEuqF/VPoQgwCg4yym
1oh6KAAts+bZQZI+eLJrxP4=
=BYUt
-----END PGP SIGNATURE-----

--- server.c    Tue Apr  6 16:20:55 2004
+++ /home/devel/mjc/server.c    Fri May 21 03:53:03 2004
@@ -1622,8 +1622,7 @@
     char *cp;
     char *timefield;
 
-    if (error_pending ())
-       return;
+    if (error_pending ()) return;
 
     if (outside_dir (arg))
        return;
@@ -1637,9 +1636,28 @@
            && strlen (arg) == cp - name
            && strncmp (arg, name, cp - name) == 0)
        {
-           timefield = strchr (cp + 1, '/') + 1;
-           if (*timefield != '=')
+           if (!(timefield = strchr (cp + 1, '/')) || *++timefield == '\0')
+           {
+               /* We didn't find the record separator or it is followed by
+                * the end of the string, so just exit.
+                */
+               if (alloc_pending (80))
+                   sprintf (pending_error_text,
+                            "E Malformed Entry encountered.");
+               return;
+           }
+           /* If the time field is not currently empty, then one of
+            * serve_modified, serve_is_modified, & serve_unchanged were
+            * already called for this file.  We would like to ignore the
+            * reinvocation silently or, better yet, exit with an error
+            * message, but we just avoid the copy-forward and overwrite the
+            * value from the last invocation instead.  See the comment below
+            * for more.
+            */
+           if (*timefield == '/')
            {
+               /* Copy forward one character.  Space was allocated for this
+                * already in serve_entry().  */
                cp = timefield + strlen (timefield);
                cp[1] = '\0';
                while (cp > timefield)
@@ -1647,8 +1665,17 @@
                    *cp = cp[-1];
                    --cp;
                }
-               *timefield = '=';
            }
+           /* If *TIMEFIELD wasn't "/", we assume that it was because of
+            * multiple calls to Is-Modified & Unchanged by the client and
+            * just overwrite the value from the last call.  Technically, we
+            * should probably either ignore calls after the first or send the
+            * client an error, since the client/server protocol specification
+            * specifies that only one call to either Is-Modified or Unchanged
+            * is allowed, but broken versions of WinCVS & TortoiseCVS rely on
+            * this behavior.
+            */
+           *timefield = '=';
            break;
        }
     }
@@ -1665,8 +1692,7 @@
     /* Have we found this file in "entries" yet.  */
     int found;
 
-    if (error_pending ())
-       return;
+    if (error_pending ()) return;
 
     if (outside_dir (arg))
        return;
@@ -1681,9 +1707,28 @@
            && strlen (arg) == cp - name
            && strncmp (arg, name, cp - name) == 0)
        {
-           timefield = strchr (cp + 1, '/') + 1;
-           if (!(timefield[0] == 'M' && timefield[1] == '/'))
+           if (!(timefield = strchr (cp + 1, '/')) || *++timefield == '\0')
            {
+               /* We didn't find the record separator or it is followed by
+                * the end of the string, so just exit.
+                */
+               if (alloc_pending (80))
+                   sprintf (pending_error_text,
+                            "E Malformed Entry encountered.");
+               return;
+           }
+           /* If the time field is not currently empty, then one of
+            * serve_modified, serve_is_modified, & serve_unchanged were
+            * already called for this file.  We would like to ignore the
+            * reinvocation silently or, better yet, exit with an error
+            * message, but we just avoid the copy-forward and overwrite the
+            * value from the last invocation instead.  See the comment below
+            * for more.
+            */
+           if (*timefield == '/')
+           {
+               /* Copy forward one character.  Space was allocated for this
+                * already in serve_entry().  */
                cp = timefield + strlen (timefield);
                cp[1] = '\0';
                while (cp > timefield)
@@ -1691,8 +1736,17 @@
                    *cp = cp[-1];
                    --cp;
                }
-               *timefield = 'M';
            }
+           /* If *TIMEFIELD wasn't "/", we assume that it was because of
+            * multiple calls to Is-Modified & Unchanged by the client and
+            * just overwrite the value from the last call.  Technically, we
+            * should probably either ignore calls after the first or send the
+            * client an error, since the client/server protocol specification
+            * specifies that only one call to either Is-Modified or Unchanged
+            * is allowed, but broken versions of WinCVS & TortoiseCVS rely on
+            * this behavior.
+            */
+           *timefield = 'M';
            if (kopt != NULL)
            {
                if (alloc_pending (strlen (name) + 80))
@@ -1747,8 +1801,29 @@
 {
     struct an_entry *p;
     char *cp;
+    int i = 0;
     if (error_pending()) return;
-    p = (struct an_entry *) xmalloc (sizeof (struct an_entry));
+
+    /* Verify that the entry is well-formed.  This can avoid problems later.
+     * At the moment we only check that the Entry contains five slashes in
+     * approximately the correct locations since some of the code makes
+     * assumptions about this.
+     */
+    cp = arg;
+    if (*cp == 'D') cp++;
+    while (i++ < 5)
+    {
+       if (!cp || *cp != '/')
+       {
+           if (alloc_pending (80))
+               sprintf (pending_error_text,
+                        "E protocol error: Malformed Entry");
+           return;
+       }
+       cp = strchr (cp + 1, '/');
+    }
+
+    p = xmalloc (sizeof (struct an_entry));
     if (p == NULL)
     {
        pending_error = ENOMEM;