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 8400ebeee6..96eab99483 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; @@ -162,10 +163,17 @@ 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. + * 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 + * + * @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. 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 d2aa20a152..d2483c219f 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,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 ---------------------------------------------------------- /** 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 ca53cac312..1243a57630 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) @@ -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;