merge with branch issue-320

scm-core/src/main/java/sonia/scm/util/HttpUtil.java

@@ -35,6 +35,7 @@ package sonia.scm.util;
 
 //~--- non-JDK imports --------------------------------------------------------
 
+import com.google.common.base.CharMatcher;
 import com.google.common.base.Strings;
 
 import org.slf4j.Logger;
@@ -162,11 +163,18 @@ public final class HttpUtil
 
   /**
    * Pattern for url normalization
-   * @sincee 1.26
+   * @since 1.26
    */
   private static final Pattern PATTERN_URLNORMALIZE =
     Pattern.compile("(?:(http://[^:]+):80(/.+)?|(https://[^:]+):443(/.+)?)");
   
+  /**
+   * CharMatcher to select cr/lf and '%' characters
+   * @since 1.28
+   */
+  private static final CharMatcher CRLF_CHARMATCHER =
+    CharMatcher.anyOf("\n\r%");
+
   //~--- constructors ---------------------------------------------------------
 
   /**
@@ -240,6 +248,30 @@ public final class HttpUtil
       SEPARATOR_PARAMETER_VALUE).append(value).toString();
   }
 
+  /**
+   * Throws an {@link IllegalArgumentException} if the parameter contains
+   * illegal characters which could imply a CRLF injection attack.
+   * <stronng>Note:</strong> the current implementation throws the
+   * {@link IllegalArgumentException} also if the parameter contains a "%". So
+   * you have to decode your parameters before the check,
+   *
+   * @param parameter value
+   *
+   * @since 1.28
+   */
+  public static void checkForCRLFInjection(String parameter)
+  {
+    if (CRLF_CHARMATCHER.matchesAnyOf(parameter))
+    {
+      logger.error(
+        "parameter \"{}\" contains a character which could be an indicator for a crlf injection",
+        parameter);
+
+      throw new IllegalArgumentException(
+        "parameter contains an illegal character");
+    }
+  }
+
   /**
    * Method description
    *
@@ -331,6 +363,22 @@ public final class HttpUtil
     return url;
   }
 
+  /**
+   * Remove all chars from the given parameter, which could be used for
+   * CRLF injection attack. <stronng>Note:</strong> the current implementation
+   * the "%" char is also removed from the source parameter.
+   *
+   * @param parameter value
+   *
+   * @return the parameter value without crlf chars
+   *
+   * @since 1.28
+   */
+  public static String removeCRLFInjectionChars(String parameter)
+  {
+    return CRLF_CHARMATCHER.removeFrom(parameter);
+  }
+
   /**
    * Method description
    *

scm-core/src/test/java/sonia/scm/util/HttpUtilTest.java

@@ -79,6 +79,84 @@ public class HttpUtilTest
       HttpUtil.normalizeUrl("http://www.scm-manager:8080"));
   }
 
+  /**
+   * Method description
+   *
+   */
+  @Test(expected = IllegalArgumentException.class)
+  public void testCheckForCRLFInjectionFailure1()
+  {
+    HttpUtil.checkForCRLFInjection("any%0D%0A");
+  }
+
+  /**
+   * Method description
+   *
+   */
+  @Test(expected = IllegalArgumentException.class)
+  public void testCheckForCRLFInjectionFailure2()
+  {
+    HttpUtil.checkForCRLFInjection("123\nabc");
+  }
+
+  /**
+   * Method description
+   *
+   */
+  @Test(expected = IllegalArgumentException.class)
+  public void testCheckForCRLFInjectionFailure3()
+  {
+    HttpUtil.checkForCRLFInjection("123\rabc");
+  }
+
+  /**
+   * Method description
+   *
+   */
+  @Test(expected = IllegalArgumentException.class)
+  public void testCheckForCRLFInjectionFailure4()
+  {
+    HttpUtil.checkForCRLFInjection("123\r\nabc");
+  }
+
+  /**
+   * Method description
+   *
+   */
+  @Test(expected = IllegalArgumentException.class)
+  public void testCheckForCRLFInjectionFailure5()
+  {
+    HttpUtil.checkForCRLFInjection("123%abc");
+  }
+
+  /**
+   * Method description
+   *
+   */
+  @Test
+  public void testCheckForCRLFInjectionSuccess()
+  {
+    HttpUtil.checkForCRLFInjection("123");
+    HttpUtil.checkForCRLFInjection("abc");
+    HttpUtil.checkForCRLFInjection("abcka");
+  }
+
+  /**
+   * Method description
+   *
+   */
+  @Test
+  public void testRemoveCRLFInjectionChars()
+  {
+    assertEquals("any0D0A", HttpUtil.removeCRLFInjectionChars("any%0D%0A"));
+    assertEquals("123abc", HttpUtil.removeCRLFInjectionChars("123\nabc"));
+    assertEquals("123abc", HttpUtil.removeCRLFInjectionChars("123\r\nabc"));
+    assertEquals("123abc", HttpUtil.removeCRLFInjectionChars("123%abc"));
+    assertEquals("123abc", HttpUtil.removeCRLFInjectionChars("123abc"));
+    assertEquals("123", HttpUtil.removeCRLFInjectionChars("123"));
+
+  }
+
   //~--- get methods ----------------------------------------------------------
 
   /**

scm-webapp/src/main/java/sonia/scm/api/rest/resources/RepositoryResource.java

@@ -74,6 +74,7 @@ import sonia.scm.repository.api.RepositoryServiceFactory;
 import sonia.scm.security.RepositoryPermission;
 import sonia.scm.security.ScmSecurityException;
 import sonia.scm.util.AssertUtil;
+import sonia.scm.util.HttpUtil;
 import sonia.scm.util.Util;
 
 //~--- JDK imports ------------------------------------------------------------
@@ -509,6 +510,7 @@ public class RepositoryResource
       {
         builder.setPath(path);
       }
+
       //J-
       builder.setDisableLastCommit(disableLastCommit)
              .setDisableSubRepositoryDetection(disableSubRepositoryDetection)
@@ -791,6 +793,12 @@ public class RepositoryResource
 
       output = new BrowserStreamingOutput(service, builder, path);
 
+     /**
+      * protection for crlf injection
+      * see https://bitbucket.org/sdorra/scm-manager/issue/320/crlf-injection-vulnerability-in-diff-api
+      */
+      path = HttpUtil.removeCRLFInjectionChars(path);
+      
       String contentDispositionName = getContentDispositionNameFromPath(path);
 
       response = Response.ok(output).header("Content-Disposition",
@@ -846,6 +854,12 @@ public class RepositoryResource
     AssertUtil.assertIsNotEmpty(id);
     AssertUtil.assertIsNotEmpty(revision);
 
+    /**
+     * check for a crlf injection attack
+     * see https://bitbucket.org/sdorra/scm-manager/issue/320/crlf-injection-vulnerability-in-diff-api
+     */
+    HttpUtil.checkForCRLFInjection(revision);
+
     RepositoryService service = null;
     Response response = null;