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

--- 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;