From 500a082a3fa49f7ffa9811f8a46449275c09f9d4 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 28 Jan 2013 13:04:12 +0100 Subject: [PATCH 1/3] 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. + * Note: 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; From e8e288ccf027aca876e26be1596bbe08abf93d14 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 28 Jan 2013 13:20:22 +0100 Subject: [PATCH 2/3] fix another possible crlf injection, see issue #320 --- .../src/main/java/sonia/scm/util/HttpUtil.java | 17 ++++++++++++++++- .../test/java/sonia/scm/util/HttpUtilTest.java | 16 ++++++++++++++++ .../api/rest/resources/RepositoryResource.java | 8 +++++++- 3 files changed, 39 insertions(+), 2 deletions(-) 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 adc268acd5..ad1ff64760 100644 --- a/scm-core/src/main/java/sonia/scm/util/HttpUtil.java +++ b/scm-core/src/main/java/sonia/scm/util/HttpUtil.java @@ -243,7 +243,6 @@ public class HttpUtil * * @param parameter value * - * @return true if the request comes from the web interface. * @since 1.28 */ public static void checkForCRLFInjection(String parameter) @@ -350,6 +349,22 @@ public class HttpUtil return url; } + /** + * Remove all chars from the given parameter, which could be used for + * CRLF injection attack. Note: 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 * 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 750f5a7d70..d2483c219f 100644 --- a/scm-core/src/test/java/sonia/scm/util/HttpUtilTest.java +++ b/scm-core/src/test/java/sonia/scm/util/HttpUtilTest.java @@ -141,6 +141,22 @@ public class HttpUtilTest 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 ---------------------------------------------------------- /** 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 75c0a7b7d0..738fd45822 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 @@ -793,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", @@ -849,7 +855,7 @@ public class RepositoryResource AssertUtil.assertIsNotEmpty(revision); /** - * HttpUtil.checkForCRLFInjection(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); From 5ca6931b727ba093cbcccac9ad9ee54b17a2880b Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 4 Feb 2013 15:48:25 +0100 Subject: [PATCH 3/3] close branch issue-320