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

grizzly-http silently fails when encoded backslashes are included in the URI #2028

Closed
xonev opened this issue Nov 29, 2018 · 8 comments
Closed

Comments

@xonev
Copy link

xonev commented Nov 29, 2018

I don't have full steps to reproduce because the way our application is configured is somewhat complicated, but I do know that the problem lies in HttpRequestURIDecoder::normalizeBytes (and normalizeChars) - see here.

Essentially, since the URI is normalized after the URI decoding happens, you have a case where an encoded backslash (%5C) is converted to a forward slash or the normalizeBytes method returns false and then grizzly silently swallows an error (unless you have logging set to FINE) before returning a 500 response.

In fact, the whole idea of decoding before the path has been broken up into components seems completely backwards, since any encoded forward slashes (%2F) will be decoded and then subsequently treated as normal slashes, which they should not be. Unless, of course, you have encoded forward slashes disabled, which I understand was implemented to plug a security hole that I am guessing was caused by decoding the URI before breaking up the path into its components.

@xonev
Copy link
Author

xonev commented Nov 29, 2018

In summary - actual behavior is that encoded backslashes throw an error in the best case and expected behavior is that encoded backslashes would be included in the path components.

@xonev
Copy link
Author

xonev commented Nov 29, 2018

Looking at it more closely, it appears that decoding the URI in HttpHandlerChain.doHandle is in itself a mistake. Maybe I'm not understanding why it's necessary, but it seems to me that passing decoded URIs to the Mapper is actually a mistake.

@DJDaveMark
Copy link

DJDaveMark commented Jun 20, 2019

GlassFish 4.1.1
Grizzly 2.3.23

I also came across this issue too. I have URIs with encoded JSON which have Strings with \" which is correctly encoded to %5C%22 but the request fails silently.

My Solution
I switched to Tomcat with the following in catalina.properties

org.apache.catalina.connector.CoyoteAdapter.ALLOW_BACKSLASH=true
org.apache.tomcat.util.buf.UDecoder.ALLOW_ENCODED_SLASH=true

Everything I Tried With GlassFish
I tried adding all of the following JVM options to domain.xml (and via NetBeans > web app > Properties > Run > VM Options) but none of it worked:

<jvm-options>-Dcom.sun.grizzly.util.http.HttpRequestURIDecoder.ALLOW_ENCODED_BACKSLASH=true</jvm-options>
<jvm-options>-Dcom.sun.grizzly.util.buf.UDecoder.ALLOW_ENCODED_SLASH=true</jvm-options>
<jvm-options>-Dorg.glassfish.grizzly.util.buf.UDecoder.ALLOW_ENCODED_SLASH=true</jvm-options>

and also tried adding this to domain.xml everywhere:

<http encoded-slash-enabled="true" ...>

and also tried via the asadmin tool (the second options doesn't exist):

./asadmin set configs.config.server-config.network-config.protocols.protocol.http-listener-1.http.encoded-slash-enabled=true
./asadmin set configs.config.server-config.network-config.protocols.protocol.http-listener-1.http.encoded-backslash-enabled=true

Still nothing worked. So I decided to debug and realized why it was never going to work. Here's the relevant code:

package org.glassfish.grizzly.http.util;
public class HttpRequestURIDecoder {
    protected static final boolean ALLOW_BACKSLASH = false;
    ...
        if (... == '\\') {
            if (ALLOW_BACKSLASH) {
                ... = '/'; // replace with forward slash
            } else {
                return false;
            }
        }

So in my case it fails since backslashes aren't allowed, but best case scenario (still bad), it gets converted to a forward slash and won't be picked up by the correct handler (path no longer matches).

Interestingly, I come across issue 1233, which proposed this patch from 1.9.x but I don't see that anywhere in the source code for any current version of Grizzly (and it wouldn't have solved the problem anyway).

@thehpi
Copy link

thehpi commented Sep 3, 2020

I would also love too see this fix implemented. It's an extremely simple fix so I should not be a lot of work.

@smillidge
Copy link
Contributor

If it's simple how about raising a PR?

@thehpi
Copy link

thehpi commented Sep 3, 2020

smilidge, as I'm not a contributor, would you be so kind to create it?

@smillidge
Copy link
Contributor

Anybody with an ECA on file can raise a PR. I'm not a committer on this project.

@mkarg
Copy link
Member

mkarg commented Nov 18, 2020

The bug is, as originally described by @xonev, that Grizzly harms the following rule of RFC 3986:
"When a URI is dereferenced, the components and subcomponents significant to the scheme-specific dereferencing process (if any) must be parsed and separated before the percent-encoded octets within those components can be safely decoded, as otherwise the data may be mistaken for component delimiters.". So we have to separate two things here. (a) Fixing the actual bug of decoding percent too early, and (b) replacing of backslashes into slashes must only happen for component separators, not for component-content.

It would be great if some Grizzly expert would chime in and fix (a), as I suppose (b) is a different topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants