#970 inspect mercurial commands in order to detect write requests

The HgPermissionFilter will now inspect the used mercurial command, of all requests which are using a read method like GET, HEAD, OPTIONS or TRACE and tread every one as write request, expect:
- no command was specified with the request (this is required for the hgweb ui)
- the command in the query string was found in the list of read commands
- if query string contains the batch command, then all commands specified in X-HgArg headers must be in the list of read commands
This change is required, in order to fix CVE-2018-1000132 for SCM-Manager.

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/web/HgPermissionFilter.java

@@ -78,6 +78,9 @@ public class HgPermissionFilter extends ProviderPermissionFilter
   @Override
   protected boolean isWriteRequest(HttpServletRequest request)
   {
-    return !READ_METHODS.contains(request.getMethod());
+    if (READ_METHODS.contains(request.getMethod())) {
+      return WireProtocol.isWriteRequest(request);
+    }
+    return true;
   }
 }

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/web/WireProtocol.java

@@ -0,0 +1,192 @@
+/**
+ * Copyright (c) 2018, Sebastian Sdorra
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright notice,
+ *    this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright notice,
+ *    this list of conditions and the following disclaimer in the documentation
+ *    and/or other materials provided with the distribution.
+ * 3. Neither the name of SCM-Manager; nor the names of its
+ *    contributors may be used to endorse or promote products derived from this
+ *    software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE FOR ANY
+ * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * http://bitbucket.org/sdorra/scm-manager
+ *
+ */
+
+
+package sonia.scm.web;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import com.google.common.collect.*;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import sonia.scm.util.HttpUtil;
+
+import javax.servlet.http.HttpServletRequest;
+import java.util.*;
+
+/**
+ * WireProtocol provides methods for handling the mercurial wire protocol.
+ *
+ * @see <a href="https://goo.gl/WaVJzw">Mercurial Wire Protocol</a>
+ */
+public final class WireProtocol {
+
+  private static final Logger LOG = LoggerFactory.getLogger(WireProtocol.class);
+
+  private static final Set<String> READ_COMMANDS = ImmutableSet.of(
+    "batch", "between", "branchmap", "branches", "capabilities", "changegroup", "changegroupsubset", "clonebundles",
+    "getbundle", "heads", "hello", "listkeys", "lookup", "known", "stream_out",
+    // could not find lheads in the wireprotocol description but mercurial 4.5.2 uses it for clone
+    "lheads"
+  );
+
+  private static final Set<String> WRITE_COMMANDS = ImmutableSet.of(
+    "pushkey", "unbundle"
+  );
+
+  private WireProtocol() {
+  }
+
+  /**
+   * Returns {@code true} if the request is a write request. The method will always return {@code true}, expect for the
+   * following cases:
+   *
+   * - no command was specified with the request (is required for the hgweb ui)
+   * - the command in the query string was found in the list of read request
+   * - if query string contains the batch command, then all commands specified in X-HgArg headers must be
+   *   in the list of read request
+   *
+   * @param request http request
+   *
+   * @return {@code true} for write requests.
+   */
+  public static boolean isWriteRequest(HttpServletRequest request) {
+    List<String> commands = commandsOf(request);
+    boolean write = isWriteRequest(commands);
+    LOG.trace("mercurial request {} is write: {}", commands, write);
+    return write;
+  }
+
+  @VisibleForTesting
+  static boolean isWriteRequest(List<String> commands) {
+    return !READ_COMMANDS.containsAll(commands);
+  }
+
+  @VisibleForTesting
+  static List<String> commandsOf(HttpServletRequest request) {
+    List<String> listOfCmds = Lists.newArrayList();
+    String cmd = getCommandFromQueryString(request);
+    if (cmd != null) {
+      listOfCmds.add(cmd);
+      if (isBatchCommand(cmd)) {
+        parseHgArgHeaders(request, listOfCmds);
+      }
+    }
+    return Collections.unmodifiableList(listOfCmds);
+  }
+
+  private static void parseHgArgHeaders(HttpServletRequest request, List<String> listOfCmds) {
+    Enumeration headerNames = request.getHeaderNames();
+    while (headerNames.hasMoreElements()) {
+      String header = (String) headerNames.nextElement();
+      parseHgArgHeader(request, listOfCmds, header);
+    }
+  }
+
+  private static void parseHgArgHeader(HttpServletRequest request, List<String> listOfCmds, String header) {
+    if (isHgArgHeader(header)) {
+      String value = getHeaderDecoded(request, header);
+      if (isHgArgCommandHeader(value)) {
+        parseHgCommandHeader(listOfCmds, value);
+      }
+    }
+  }
+
+  private static void parseHgCommandHeader(List<String> listOfCmds, String value) {
+    String[] cmds = value.substring(5).split(";");
+    for (String cmd : cmds ) {
+      String normalizedCmd = normalize(cmd);
+      int index = normalizedCmd.indexOf(' ');
+      if (index > 0) {
+        listOfCmds.add(normalizedCmd.substring(0, index));
+      } else {
+        listOfCmds.add(normalizedCmd);
+      }
+    }
+  }
+
+  private static String normalize(String cmd) {
+    return cmd.trim().toLowerCase(Locale.ENGLISH);
+  }
+
+  private static boolean isHgArgCommandHeader(String value) {
+    return value.startsWith("cmds=");
+  }
+
+  private static String getHeaderDecoded(HttpServletRequest request, String header) {
+    return HttpUtil.decode(Strings.nullToEmpty(request.getHeader(header)));
+  }
+
+  private static boolean isHgArgHeader(String header) {
+    return header.toLowerCase(Locale.ENGLISH).startsWith("x-hgarg-");
+  }
+
+  private static boolean isBatchCommand(String cmd) {
+    return "batch".equalsIgnoreCase(cmd);
+  }
+
+  private static String getCommandFromQueryString(HttpServletRequest request) {
+    // we can't use getParameter, because this would inspect the body for form parameters as well
+    Multimap<String, String> queryParameterMap = createQueryParameterMap(request);
+
+    Collection<String> cmd = queryParameterMap.get("cmd");
+    Preconditions.checkArgument(cmd.size() <= 1, "found more than one cmd query parameter");
+    Iterator<String> iterator = cmd.iterator();
+
+    String command = null;
+    if (iterator.hasNext()) {
+      command = iterator.next();
+    }
+    return command;
+  }
+
+  private static Multimap<String,String> createQueryParameterMap(HttpServletRequest request) {
+    Multimap<String,String> parameterMap = HashMultimap.create();
+
+    String queryString = request.getQueryString();
+    if (!Strings.isNullOrEmpty(queryString)) {
+
+      String[] parameters = queryString.split("&");
+      for (String parameter : parameters) {
+        int index = parameter.indexOf('=');
+        if (index > 0) {
+          parameterMap.put(parameter.substring(0, index), parameter.substring(index + 1));
+        } else {
+          parameterMap.put(parameter, "true");
+        }
+      }
+
+    }
+
+    return parameterMap;
+  }
+}

scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/web/HgPermissionFilterTest.java

@@ -85,6 +85,7 @@ public class HgPermissionFilterTest {
 
   private boolean isWriteRequest(String method) {
     HttpServletRequest request = mock(HttpServletRequest.class);
+    when(request.getQueryString()).thenReturn("cmd=capabilities");
     when(request.getMethod()).thenReturn(method);
     return filter.isWriteRequest(request);
   }
@@ -163,6 +164,17 @@ public class HgPermissionFilterTest {
     assertIsWriteRequest(wireProtocol.pushkey("markone&namespace=bookmarks&new=ef5993bb4abb32a0565c347844c6d939fc4f4b98&old="));
   }
 
+  /**
+   * Tests {@link HgPermissionFilter#isWriteRequest(HttpServletRequest)} with a write request hidden in a batch GET
+   * request.
+   *
+   * @see <a href="https://goo.gl/poascp">Issue #970</a>
+   */
+  @Test
+  public void testIsWriteRequestWithBookmarkPushInABatch() {
+    assertIsWriteRequest(wireProtocol.batch("pushkey key=markthree,namespace=bookmarks,new=187ddf37e237c370514487a0bb1a226f11a780b3,old="));
+  }
+
   private void assertIsReadRequest(HttpServletRequest request) {
     assertFalse(filter.isWriteRequest(request));
   }

scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/web/WireProtocolRequestMockFactory.java

@@ -1,7 +1,11 @@
 package sonia.scm.web;
 
+import com.google.common.collect.Lists;
+
 import javax.servlet.http.HttpServletRequest;
 
+import java.util.Collections;
+import java.util.List;
 import java.util.Locale;
 
 import static org.mockito.Mockito.*;
@@ -21,55 +25,64 @@ public class WireProtocolRequestMockFactory {
   }
 
   public HttpServletRequest capabilities() {
-    return base("GET", "?cmd=capabilities");
+    return base("GET", "cmd=capabilities");
   }
 
   public HttpServletRequest listkeys(Namespace namespace) {
-    HttpServletRequest request = base("GET", "?cmd=capabilities");
+    HttpServletRequest request = base("GET", "cmd=capabilities");
     header(request, "vary", "X-HgArg-1");
     header(request, "x-hgarg-1", namespaceValue(namespace));
     return request;
   }
 
   public HttpServletRequest branchmap() {
-    return base("GET", "?cmd=branchmap");
+    return base("GET", "cmd=branchmap");
   }
 
   public HttpServletRequest batch(String... args) {
-    HttpServletRequest request = base("GET", "?cmd=batch");
+    HttpServletRequest request = base("GET", "cmd=batch");
     args(request, "cmds", args);
     return request;
   }
 
   public HttpServletRequest unbundle(long contentLength, String... heads) {
-    HttpServletRequest request = base("POST", "?cmd=unbundle");
+    HttpServletRequest request = base("POST", "cmd=unbundle");
     header(request, "Content-Length", String.valueOf(contentLength));
     args(request, "heads", heads);
     return request;
   }
 
   public HttpServletRequest pushkey(String... keys) {
-    HttpServletRequest request = base("POST", "?cmd=pushkey");
+    HttpServletRequest request = base("POST", "cmd=pushkey");
     args(request, "key", keys);
     return request;
   }
 
   public HttpServletRequest known(String... nodes) {
-    HttpServletRequest request = base("POST", "?cmd=pushkey");
+    HttpServletRequest request = base("GET", "cmd=known");
     args(request, "nodes", nodes);
     return request;
   }
 
   private void args(HttpServletRequest request, String prefix, String[] values) {
+    List<String> headers = Lists.newArrayList();
+
     StringBuilder vary = new StringBuilder();
     for ( int i=0; i<values.length; i++ ) {
+      String header = "X-HgArg-" + (i+1);
+
       if (i>0) {
         vary.append(",");
       }
-      vary.append("X-HgArg-" + (i+1));
+
-      header(request, "X-HgArg-" + (i+1), prefix + "=" + values[i]);
+      vary.append(header);
+      headers.add(header);
+
+      header(request, header, prefix + "=" + values[i]);
     }
     header(request, "Vary", vary.toString());
+
+    when(request.getHeaderNames()).thenReturn(Collections.enumeration(headers));
   }
 
   private HttpServletRequest base(String method, String queryStringValue) {

scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/web/WireProtocolTest.java

@@ -0,0 +1,162 @@
+/**
+ * Copyright (c) 2018, Sebastian Sdorra
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright notice,
+ *    this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright notice,
+ *    this list of conditions and the following disclaimer in the documentation
+ *    and/or other materials provided with the distribution.
+ * 3. Neither the name of SCM-Manager; nor the names of its
+ *    contributors may be used to endorse or promote products derived from this
+ *    software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE FOR ANY
+ * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * http://bitbucket.org/sdorra/scm-manager
+ *
+ */
+
+
+package sonia.scm.web;
+
+import com.google.common.collect.Lists;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+
+import javax.servlet.http.HttpServletRequest;
+
+import java.util.Collections;
+import java.util.List;
+
+import static org.hamcrest.Matchers.contains;
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.when;
+
+/**
+ * Unit tests for {@link WireProtocol}.
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class WireProtocolTest {
+
+  @Mock
+  private HttpServletRequest request;
+
+  @Test
+  public void testIsWriteRequestOnPost() {
+    assertIsWriteRequest("capabilities", "unbundle");
+  }
+
+  @Test
+  public void testIsWriteRequest() {
+    assertIsWriteRequest("unbundle");
+    assertIsWriteRequest("capabilities", "unbundle");
+    assertIsWriteRequest("capabilities", "postkeys");
+    assertIsReadRequest();
+    assertIsReadRequest("capabilities");
+    assertIsReadRequest("capabilities", "branches", "branchmap");
+  }
+
+  private void assertIsWriteRequest(String... commands) {
+    List<String> cmdList = Lists.newArrayList(commands);
+    assertTrue(WireProtocol.isWriteRequest(cmdList));
+  }
+
+  private void assertIsReadRequest(String... commands) {
+    List<String> cmdList = Lists.newArrayList(commands);
+    assertFalse(WireProtocol.isWriteRequest(cmdList));
+  }
+
+  @Test
+  public void testGetCommandsOf() {
+    expectQueryCommand("capabilities", "cmd=capabilities");
+    expectQueryCommand("unbundle", "cmd=unbundle");
+    expectQueryCommand("unbundle", "prefix=stuff&cmd=unbundle");
+    expectQueryCommand("unbundle", "cmd=unbundle&suffix=stuff");
+    expectQueryCommand("unbundle", "prefix=stuff&cmd=unbundle&suffix=stuff");
+    expectQueryCommand("unbundle", "bool=&cmd=unbundle");
+    expectQueryCommand("unbundle", "bool&cmd=unbundle");
+    expectQueryCommand("unbundle", "prefix=stu==ff&cmd=unbundle");
+  }
+
+  @Test
+  public void testGetCommandsOfWithBatch() {
+    prepareBatch("cmds=heads ;known nodes,ef5993bb4abb32a0565c347844c6d939fc4f4b98");
+    List<String> commands = WireProtocol.commandsOf(request);
+    assertThat(commands, contains("batch", "heads", "known"));
+  }
+
+  @Test
+  public void testGetCommandsOfWithBatchEncoded() {
+    prepareBatch("cmds=heads+%3Bknown+nodes%3Def5993bb4abb32a0565c347844c6d939fc4f4b98");
+    List<String> commands = WireProtocol.commandsOf(request);
+    assertThat(commands, contains("batch", "heads", "known"));
+  }
+
+  @Test
+  public void testGetCommandsOfWithBatchAndMutlipleLines() {
+    prepareBatch(
+      "cmds=heads+%3Bknown+nodes%3Def5993bb4abb32a0565c347844c6d939fc4f4b98",
+      "cmds=unbundle; postkeys",
+      "cmds= branchmap p1=r2,p2=r4; listkeys"
+    );
+    List<String> commands = WireProtocol.commandsOf(request);
+    assertThat(commands, contains("batch", "heads", "known", "unbundle", "postkeys", "branchmap", "listkeys"));
+  }
+
+  private void prepareBatch(String... args) {
+    when(request.getQueryString()).thenReturn("cmd=batch");
+    List<String> headers = Lists.newArrayList();
+    for (int i=0; i<args.length; i++) {
+      String header = "X-HgArg-" + (i+1);
+      headers.add(header);
+      when(request.getHeader(header)).thenReturn(args[i]);
+    }
+    when(request.getHeaderNames()).thenReturn(Collections.enumeration(headers));
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void testGetCommandsOfWithMultipleCommandsInQueryString() {
+    when(request.getQueryString()).thenReturn("cmd=abc&cmd=def");
+    WireProtocol.commandsOf(request);
+  }
+
+  @Test
+  public void testGetCommandsOfWithoutCmdInQueryString() {
+    when(request.getQueryString()).thenReturn("abc=def&123=456");
+    assertTrue(WireProtocol.commandsOf(request).isEmpty());
+  }
+
+  @Test
+  public void testGetCommandsOfWithEmptyQueryString() {
+    when(request.getQueryString()).thenReturn("");
+    assertTrue(WireProtocol.commandsOf(request).isEmpty());
+  }
+
+  @Test
+  public void testGetCommandsOfWithNullQueryString() {
+    assertTrue(WireProtocol.commandsOf(request).isEmpty());
+  }
+
+  private void expectQueryCommand(String expected, String queryString) {
+    when(request.getQueryString()).thenReturn(queryString);
+    List<String> commands = WireProtocol.commandsOf(request);
+    assertEquals(1, commands.size());
+    assertTrue(commands.contains(expected));
+  }
+
+}