From 6f442a4a7a66a0ba6d5189a0430cd5e95fb39ea8 Mon Sep 17 00:00:00 2001 From: James Moger Date: Sat, 16 Nov 2013 09:12:10 -0500 Subject: [PATCH] Automatically adjust web.forwardSlash on Tomcat containers One issue that frequently arises in the discussion group and the bug tracker is how Tomcat automatically re-encodes %2f as '/' which breaks url parameters with %2f. After documenting this in half a dozen places it still comes up. Clearly I haven't done enough. Gitblit will now act on, instead of just report, an improperly configured web.forwardSlash character on Tomcat containers. This will make Gitblit "just work" for more users and will make the world a better place. Change-Id: I344428804070a2d6082022cf6b80e2a3d83cea84 --- src/main/java/com/gitblit/GitBlit.java | 6 +- .../com/gitblit/utils/ContainerUtils.java | 66 ++++++------------- 2 files changed, 25 insertions(+), 47 deletions(-) diff --git a/src/main/java/com/gitblit/GitBlit.java b/src/main/java/com/gitblit/GitBlit.java index f191d6a6..97372e11 100644 --- a/src/main/java/com/gitblit/GitBlit.java +++ b/src/main/java/com/gitblit/GitBlit.java @@ -3554,8 +3554,6 @@ public class GitBlit implements ServletContextListener { configureFanout(); configureGitDaemon(); configureCommitCache(); - - ContainerUtils.CVE_2007_0450.test(); } protected void configureMailExecutor() { @@ -3817,6 +3815,10 @@ public class GitBlit implements ServletContextListener { FileSettings settings = new FileSettings(localSettings.getAbsolutePath()); configureContext(settings, base, true); } + + // WAR or Express is likely to be running on a Tomcat. + // Test for the forward-slash/%2F issue and auto-adjust settings. + ContainerUtils.CVE_2007_0450.test(settings); } settingsModel = loadSettingModels(); diff --git a/src/main/java/com/gitblit/utils/ContainerUtils.java b/src/main/java/com/gitblit/utils/ContainerUtils.java index 613bf97a..d0dfcd1f 100644 --- a/src/main/java/com/gitblit/utils/ContainerUtils.java +++ b/src/main/java/com/gitblit/utils/ContainerUtils.java @@ -22,7 +22,7 @@ import java.lang.reflect.Method; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import com.gitblit.GitBlit; +import com.gitblit.IStoredSettings; import com.gitblit.Keys; /** @@ -41,37 +41,18 @@ public class ContainerUtils * @see http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2007-0450 * @author jpyeron */ - public static class CVE_2007_0450 - { + public static class CVE_2007_0450 { /** * This method will test for know issues in certain containers where %2F * is blocked from use in URLs. It will emit a warning to the logger if * the configuration of Tomcat causes the URL processing to fail on %2F. */ - public static void test() - { - if (GitBlit.getBoolean(Keys.web.mountParameters, true) - && ((GitBlit.getChar(Keys.web.forwardSlashCharacter, '/')) == '/' || (GitBlit.getChar( - Keys.web.forwardSlashCharacter, '/')) == '\\')) - { - try - { - if (GitBlit.isGO()) - ; - else if (logCVE_2007_0450Tomcat()) - ; - // else if (logCVE_2007_0450xxx()); - else - { - LOGGER.info("Unknown container, cannot check for CVE-2007-0450 aplicability"); - } - } - catch (Throwable t) - { - LOGGER.warn("Failure in checking for CVE-2007-0450 aplicability", t); - } + public static void test(IStoredSettings settings) { + boolean mounted = settings.getBoolean(Keys.web.mountParameters, true); + char fsc = settings.getChar(Keys.web.forwardSlashCharacter, '/'); + if (mounted && ((fsc == '/') || (fsc == '\\'))) { + logCVE_2007_0450Tomcat(settings); } - } /** @@ -84,10 +65,8 @@ public class ContainerUtils * @return true if it recognizes Tomcat, false if it does not recognize * Tomcat */ - private static boolean logCVE_2007_0450Tomcat() - { - try - { + private static boolean logCVE_2007_0450Tomcat(IStoredSettings settings) { + try { byte[] test = "http://server.domain:8080/context/servlet/param%2fparam".getBytes(); // ByteChunk mb=new ByteChunk(); @@ -95,37 +74,34 @@ public class ContainerUtils Object mb = cByteChunk.newInstance(); // mb.setBytes(test, 0, test.length); - Method mByteChunck_setBytes = cByteChunk.getMethod("setBytes", byte[].class, int.class, int.class); - mByteChunck_setBytes.invoke(mb, test, 0, test.length); + Method setBytes = cByteChunk.getMethod("setBytes", byte[].class, int.class, int.class); + setBytes.invoke(mb, test, 0, test.length); // UDecoder ud=new UDecoder(); Class cUDecoder = Class.forName("org.apache.tomcat.util.buf.UDecoder"); Object ud = cUDecoder.newInstance(); // ud.convert(mb,false); - Method mUDecoder_convert = cUDecoder.getMethod("convert", cByteChunk, boolean.class); + Method convert = cUDecoder.getMethod("convert", cByteChunk, boolean.class); - try - { - mUDecoder_convert.invoke(ud, mb, false); - } - catch (InvocationTargetException e) - { - if (e.getTargetException() != null && e.getTargetException() instanceof CharConversionException) - { + try { + convert.invoke(ud, mb, false); + } catch (InvocationTargetException e) { + if (e.getTargetException() != null && e.getTargetException() instanceof CharConversionException) { LOGGER.warn("You are using a Tomcat-based server and your current settings will prevent grouped repositories, forks, personal repositories, and tree navigation from working properly. Please review the FAQ for details about running Gitblit on Tomcat. http://gitblit.com/faq.html and http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2007-0450"); + LOGGER.warn("Overriding {} and setting to {}", Keys.web.forwardSlashCharacter, "!"); + settings.overrideSetting(Keys.web.forwardSlashCharacter, "!"); return true; } throw e; } - } - catch (Throwable t) - { + } catch (Throwable t) { // The apache url decoder internals are different, this is not a // Tomcat matching the failure pattern for CVE-2007-0450 if (t instanceof ClassNotFoundException || t instanceof NoSuchMethodException - || t instanceof IllegalArgumentException) + || t instanceof IllegalArgumentException) { return false; + } LOGGER.debug("This is a tomcat, but the test operation failed somehow", t); } return true; -- 2.39.5