Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit a warning when running on Tomcat without ALLOW_ENCODED_SLASH=true #422

Closed
gitblit opened this issue Aug 12, 2015 · 24 comments
Closed

Emit a warning when running on Tomcat without ALLOW_ENCODED_SLASH=true #422

gitblit opened this issue Aug 12, 2015 · 24 comments

Comments

@gitblit
Copy link
Owner

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

why does tomcat break it?

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

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

also change to enhancement

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

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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

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

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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

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

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

"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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

Yeah, that is a good place.

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

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

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

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

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

v1.2.0 has been deployed.

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

  • Status changed: Fixed

@gitblit gitblit closed this as completed Aug 12, 2015
@flaix flaix added this to the 1.2.0 milestone Dec 13, 2016
@flaix flaix added this to the 1.2.0 milestone Dec 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants