Emit a warning when running on Tomcat without ALLOW_ENCODED_SLASH=true

[gitblit]

Originally reported on Google Code with ID 126

load this bundle into gitblit v1.1.0-SNAPSHOT , then browse to http://server/gitblit/blob/sandbox.git/6e509ff0e6fcc94de5d1fa1a4aed6fc39ecd75b7/bar%2Ftest.txt

Reported by jasonpyeron on 2012-08-30 17:58:52

---

- _Attachment: [sandbox.bundle](https://storage.googleapis.com/google-code-attachments/gitblit/issue-126/comment-0/sandbox.bundle)_

[gitblit]

You are likely running Gitblit on Tomcat OR are running Gitblit behind a proxy.

Set web.mountParameters = false
or web.forwardSlashCharacter = !

The demo, which sits behind a reverse proxy, must use ! as the folder delimiter because
the proxy re-encodes %2f back to / before passing the request to Gitblit.  This change
breaks Gitlbit url scheme.  Using ! or ~ will bypass the proxy's meddling.

example. https://demo-gitblit.rhcloud.com/tree/gitblit.git/1ab5b3081374c79867ceff2917bccae6dd1a2878/src!com!gitblit

Reported by James.Moger on 2012-08-30 19:46:39

[gitblit]

why does tomcat break it?

Reported by jasonpyeron on 2012-08-30 23:59:11

[gitblit]

the blob wicket page constructor is not even called, and if the %2f is placed as a /
then an exception for too many paramters is thrown as expected.

Reported by jasonpyeron on 2012-08-31 00:01:10

[gitblit]

Tomcat is openly hostile to %2F by default.  You have to explicitly allow Tomcat to
process %2F by passing a command line option to the JVM.  Running on Tomcat behind
a proxy will introduce two layers of trouble with %2F so even if you allow Tomcat to
process %2F, a proxy is equally likely to mangle urls too.

# Some servlet containers (e.g. Tomcat >= 6.0.10) disallow '/' (%2F) encoding
# in URLs as a security precaution for proxies. This setting tells Gitblit
# to preemptively replace '/' with '*' or '!' for url string parameters.
# 
# https://issues.apache.org/jira/browse/WICKET-1303
# http://tomcat.apache.org/security-6.html#Fixed_in_Apache_Tomcat_6.0.10
# Add -Dorg.apache.tomcat.util.buf.UDecoder.ALLOW_ENCODED_SLASH=true to your
# CATALINA_OPTS or to your JVM launch parameters
# 
# SINCE 0.5.2
web.forwardSlashCharacter = /

Reported by James.Moger on 2012-08-31 01:06:46

[gitblit]

going to make patch to emit warning on gitblit startup if tomcat based and invalid condition
exists.

I cannot change the summary of this issue to:

GitBlit does not emit a warning when the environment is not properly setup to process
paths.

Reported by jasonpyeron on 2012-08-31 01:23:40

[gitblit]

also change to enhancement

Reported by jasonpyeron on 2012-08-31 01:33:52

[gitblit]

Reported by James.Moger on 2012-08-31 01:36:48

• Labels added: Type-Enhancement
• Labels removed: Type-Defect

[gitblit]

2 questions:

1: what url should the warning message point to?
2: com.gitblit.GitBlit#configureContext(IStoredSettings settings, boolean startFederation)
now calls logCVE_2007_0450(). Do you want that code in GitBlit.java or should it go
in com.bitblit.utils?

Reported by jasonpyeron on 2012-08-31 14:08:24

[gitblit]

It's fine in GitBlit.java.

I would point to the Apache url and I would output the instruction about "add -D...".

Reported by James.Moger on 2012-08-31 14:22:04

[gitblit]

you would not point to the FAQ (http://gitblit.com/faq.html) for gitblit?

Reported by jasonpyeron on 2012-08-31 15:17:29

[gitblit]

hmm it has an impact on JBOSS, and any other system using an embedded tomcat too.

Reported by jasonpyeron on 2012-08-31 15:22:28

[gitblit]

please see https://github.com/pdinc-oss/gitblit/commit/ae3573bb503d0e52087abb7064ae6f6662aaf968

Reported by jasonpyeron on 2012-08-31 15:28:45

[gitblit]

Hmm.  That is kinda complex.  Gitblit tries to follow the KISS principle when it can.
I think something like this would be equally effective.

private void logCVE_2007_0450() {
    if (serverStatus.isGO) {
        return;
    }

    if (!settings.getBoolean(Keys.web.mountParameters, true)) {
        // parameterized urls
        return;
    }

    if ('/' != settings.getChar(Keys.web.forwardSlashCharacter. '/')) {
        // using alternate char such as ! or ~
        return;
    }

    String [] troublemakers = { "tomcat", "jboss", "tomee" };
    for (String troublemaker : troublemakers) {
        if (serverStatus.servletContainer.toLowerCase().contains(troublemaker)) {
            String val = System.getProperty("org.apache.tomcat.util.buf.UDecoder.ALLOW_ENCODED_SLASH");
            if (StringUtils.isEmpty(val) || !Boolean.parseBoolean(val)) {
                logger.warn("You are using a Tomcat-based server and your current settings
will prevent grouped 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");          
            return;
        }
    }
}

Reported by James.Moger on 2012-08-31 15:59:39

[gitblit]

that was failing on several of our test systems. The fix for tomcat was a breaking fix.


That code is the exact problem and test case. I used reflection to avoid compile dependencies.

I can collapse the catch blocks.

Reported by jasonpyeron on 2012-08-31 16:06:01

[gitblit]

"that was failing on several of our test systems. The fix for tomcat was a breaking
fix."

Can you elaborate on this statement?  What was "that"?  The container name approach?

I recognize that you are exercising the offending Tomcat code.  I'm ok with reflective
approaches for deps I closely track (I use reflection with JGit, for example) but I'm
not crazy about reflection on the servlet container just because its pretty fragile
and subject to the Tomcat team changing the API.  I suppose this class may never change
/ has never changed.  And there is pretty low risk if this check fails to work so I
guess reflection is fine.  The exception handling, I agree, is too much.  One handler
(not including your actual test handler) will suffice.

Reported by James.Moger on 2012-08-31 16:26:20

[gitblit]

Yes container naming was a problem, becuase then a list of names/versions would have
to be kept. The code is not yet polished, but the concept is sound. If the tomcat people
change their ways the code will not go belly up.

I am polishing (reducing) it now.

Reported by jasonpyeron on 2012-08-31 19:57:04

[gitblit]

https://github.com/pdinc-oss/gitblit/blame/912d3432d961a6eab9d4a28cabb3992db1c16955/src/com/gitblit/GitBlit.java

much more clean. take a look when you get a chance.

Reported by jasonpyeron on 2012-08-31 20:17:34

[gitblit]

The fragility of the Tomcat API to this was kind of a point. I wanted it to emit a warning
if we know there is a problem, a debug if there was a failure of testing, an info if
we do not know anything.

Reported by jasonpyeron on 2012-08-31 20:19:40

[gitblit]

That looks better.  But I will probably still revise it some and move some/all to a
utility class, as you had thought of before. That check adds +75 lines to a class that
is already bloated.

Reported by James.Moger on 2012-08-31 21:07:23

[gitblit]

tell me the package and I can clean it up.

I would suggest com.gitblit.utils or a sub package there of.

Reported by jasonpyeron on 2012-08-31 21:18:40

[gitblit]

Yeah, that is a good place.

Reported by James.Moger on 2012-08-31 21:24:21

[gitblit]

ok it is nice and neat now.

let me know if there should be any more changes.

Reported by jasonpyeron on 2012-08-31 23:46:04

[gitblit]

Reported by James.Moger on 2012-09-06 21:42:48

• Status changed: Queued
• Labels added: Milestone-1.2.0

[gitblit]

v1.2.0 has been deployed.

Reported by James.Moger on 2013-01-01 01:06:25

• Status changed: Fixed