From 500a082a3fa49f7ffa9811f8a46449275c09f9d4 Mon Sep 17 00:00:00 2001
From: Sebastian Sdorra <sebastian.sdorra@cloudogu.com>
Date: Mon, 28 Jan 2013 13:04:12 +0100
Subject: [PATCH] fix possible crlf injection, see issue #320

---
 .../main/java/sonia/scm/util/HttpUtil.java    | 30 +++++++++
 .../java/sonia/scm/util/HttpUtilTest.java     | 62 +++++++++++++++++++
 .../rest/resources/RepositoryResource.java    |  8 +++
 3 files changed, 100 insertions(+)

diff --git a/scm-core/src/main/java/sonia/scm/util/HttpUtil.java b/scm-core/src/main/java/sonia/scm/util/HttpUtil.java
index 2d7d5ac9ba..adc268acd5 100644
--- a/scm-core/src/main/java/sonia/scm/util/HttpUtil.java
+++ b/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;
@@ -164,6 +165,10 @@ public class HttpUtil
   private static final Pattern PATTERN_URLNORMALIZE =
     Pattern.compile("(?:(http://[^:]+):80(/.+)?|(https://[^:]+):443(/.+)?)");
 
+  /** Field description */
+  private static final CharMatcher CRLF_CHARMATCHER =
+    CharMatcher.anyOf("\n\r%");
+
   //~--- methods --------------------------------------------------------------
 
   /**
@@ -229,6 +234,31 @@ public 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
+   *
+   * @return true if the request comes from the web interface.
+   * @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
    *
diff --git a/scm-core/src/test/java/sonia/scm/util/HttpUtilTest.java b/scm-core/src/test/java/sonia/scm/util/HttpUtilTest.java
index d2aa20a152..750f5a7d70 100644
--- a/scm-core/src/test/java/sonia/scm/util/HttpUtilTest.java
+++ b/scm-core/src/test/java/sonia/scm/util/HttpUtilTest.java
@@ -79,6 +79,68 @@ 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");
+  }
+
   //~--- get methods ----------------------------------------------------------
 
   /**
diff --git a/scm-webapp/src/main/java/sonia/scm/api/rest/resources/RepositoryResource.java b/scm-webapp/src/main/java/sonia/scm/api/rest/resources/RepositoryResource.java
index 494eab42fa..75c0a7b7d0 100644
--- a/scm-webapp/src/main/java/sonia/scm/api/rest/resources/RepositoryResource.java
+++ b/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)
@@ -846,6 +848,12 @@ public class RepositoryResource
     AssertUtil.assertIsNotEmpty(id);
     AssertUtil.assertIsNotEmpty(revision);
 
+    /**
+     * HttpUtil.checkForCRLFInjection(revision);
+     * see https://bitbucket.org/sdorra/scm-manager/issue/320/crlf-injection-vulnerability-in-diff-api
+     */
+    HttpUtil.checkForCRLFInjection(revision);
+
     RepositoryService service = null;
     Response response = null;