bad request

Hi,

Vulnerability tests that ran against our site showed that URL's with
percentage encoded UTF8 characters creates a 500 (internal server
error) response, where I think they should actually respond with 404
or 405.

Some examples:

GET /% breaks when WAUrl class #decodePercent: tries to read off the
end of the stream.
GET /%C0 breaks for the same reason, but in GRPharoUtf8CodecStream |
next: (more cases in there!)
GET /%C0%10 raises GRInvalidUtf8Error.
GET /%C0%AE returns 404 (not found).

I'd like to improve on this, if you agree.

I need some guidance on how to improve regarding the construction of
the request. This is the code in WAServerAdaptor | contextFor:

It creates a request (self requestFor: aNativeRequest) without
handling exceptions. This means that the outer exception handler
catches it and returns a 500.

I am not sure where to handle errors such as these and to elegantly
ensure that the response is "bad request" if the request could not be
parsed. I also see that there's a nice badRequest method on
WAResponse, without any senders in my image. Perhaps this is some
perform: magic that I can't trace.

Any help will be appreciated.

Thanks
Otto
_______________________________________________
seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

On Mon, Dec 23, 2013 at 4:25 PM, Otto Behrens <[hidden email]> wrote:
> Hi,
>
> Vulnerability tests that ran against our site showed that URL's with
> percentage encoded UTF8 characters creates a 500 (internal server
> error) response, where I think they should actually respond with 404
> or 405.

I think in this very specific case 400 (bad request would be the way to go).

So right now FastCGI already turns it into a 500?

> I am not sure where to handle errors such as these and to elegantly
> ensure that the response is "bad request" if the request could not be
> parsed. I also see that there's a nice badRequest method on
> WAResponse, without any senders in my image. Perhaps this is some
> perform: magic that I can't trace.

That's simply a convenience method for setting the status code.

> Any help will be appreciated.

There could be all sorts of things that could go wrong causing an
exception. The http version for example could be "A.B", cookie parsing
could fail and URL decoding could fail [1].
Should the response generated in the exception case be a WAResponse or
a "native" response? WAResponse response may be a bit tricky because
you don't have WARequestContext because you don't have WARequest. So
it's probably going to be "native" response, this also avoids the
other issue of what you're going to do if response conversion
(#responseFrom:) throws some kind of exception.
So in the end all what you do is use a 400 instead of a 500 for some
exception classes.

 [1] http://code.google.com/p/seaside/issues/detail?id=764

Cheers
Philippe
_______________________________________________
seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

Thanks for the response.

>> Vulnerability tests that ran against our site showed that URL's with
>> percentage encoded UTF8 characters creates a 500 (internal server
>> error) response, where I think they should actually respond with 404
>> or 405.
>
> I think in this very specific case 400 (bad request would be the way to go).

Yes, agree. Not 405.

>> It creates a request (self requestFor: aNativeRequest) without
>> handling exceptions. This means that the outer exception handler
>> catches it and returns a 500.
>
> So right now FastCGI already turns it into a 500?

Yes, the GS one does

> There could be all sorts of things that could go wrong causing an
> exception. The http version for example could be "A.B", cookie parsing
> could fail and URL decoding could fail [1].

Yes, agree. Parsing the UTF-8 encoded characters in the URL is one.

Looking at rfc6265, I could not specifically pick up appropriate
responses if a cookie could not be parsed (or any other header field /
part of the header). One conclusion that I could find was that the
server should "degrade gracefully" when receiving invalid cookies. But
this is not necessarily the inability to parse the cookies in the
first place. And then a similar question arises for the parsing of
other header fields. Please let me know if you have better references
to read up on.

This is my best conclusion - please comment:
For someone providing a web application, the server implementer can either
1) be strict respond with 400 (bad request) if it finds something it
dislikes in the request (either the http method + URL, headers or
body),
or 2) ignore inappropriate parts of the request. Of course, some parts
of the request can't be ignored, so one will have to distinguish
carefully.

> Should the response generated in the exception case be a WAResponse or
> a "native" response? WAResponse response may be a bit tricky because
> you don't have WARequestContext because you don't have WARequest. So
> it's probably going to be "native" response, this also avoids the
> other issue of what you're going to do if response conversion
> (#responseFrom:) throws some kind of exception.
> So in the end all what you do is use a 400 instead of a 500 for some
> exception classes.

Ok, I understand some of the predicament. But I don't understand how I
would use 400 and not 500 in these cases.

A "native" request would be something that Comanche / Zinc / FastCGI
would create and pass to Seaside (i.e. WAServerAdaptor >> process:)

In the error case that the Seaside framework does not handle, the web
server / fast cgi server implements a fallback handler for server
errors (500 Internal Server Error). (e.g. HttpService >>
handleDispatchErrorsIn:). This is the error handler invoked in this
discussion where the URL contains percent-encoded parts that break the
UTF-8 decoding.

It seems if I want to handle any of these errors (consistently), I
have to modify Comanche, Zinc and FastCGI adaptors.

I think we should allow the Seaside framework to handle these errors
as well. I understand giving the WAResponse the responsibility to
handle it can be tricky, but it may just be the right place.
Currently, it does handle some of the 400 range of errors (eg. not
found).

This is my suggestion: Create the WARequestContext on the native
request without parsing it.

Parsing the URL effectively comes down to handleFiltered: on
WADispatcher that will try to get the url via the consumer on the
request. If parsing it breaks, the request is marked as a bad request.
The dispatcher could pass to the default handler. The parsing errors
of the other parts of the request would then go to the handler
dispatched to. And that could in turn be dispatched to an error
handler.

This gives the user of the Seaside framework the flexibility of
handling these errors elegantly as well. Only errors that break the
seaside framework itself, the ones that somehow fall through
WAExceptionHandler >> #handleExceptionsDuring: would then trigger the
Comanche / Zinc / FastCGI handler.
_______________________________________________
seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

I got this to work in our application. I added adaptor and
nativeRequest to WARequest, and constructed it with these:

WAServerAdaptor >> requestFor: aNativeRequest
"Answer a request object for aNativeRequest."

^ WARequest basicNew
adaptor: self;
nativeRequest: aNativeRequest;
yourself

I then lazy initialized the variables on WARequest, using the messages
in the current version of requestFor:. For example:

WARequest >> uri
^ uri ifNil: [ uri := adaptor ifNotNil: [ (adaptor requestUrlFor:
nativeRequest) seasideUrl ] ]

This could be better, I suppose (I personally don't really like lazy
initializations). But a lot of tests construct the request with
specific URLs, methods and what not. So I would need to refactor these
to use a simple native request. I then changed WARequestContext to be
aware of a parsing error (must be better, should be on request
itself...)

The reason why this reports in our application is because the parsing
of the url is invoked again later on in the process, which throws an
exception.

The source modifications are here:
https://github.com/finworks/Seaside30/tree/handleRequestParsingError.
(A fork from https://github.com/glassdb/Seaside30)

1. Please let me know if you think this can generally work
2. Do you think I should continue and make the changes on Seaside "trunk"?
3. I suppose http://www.squeaksource.com/Seaside30LGPL is the place to do it?
4. Seaside 3.1 only?

Cheers
Otto
_______________________________________________
seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

> to use a simple native request. I then changed WARequestContext to be
> aware of a parsing error (must be better, should be on request
> itself...)

Made another change - WADispatcher now reponds with bad request.

> The source modifications are here:
> https://github.com/finworks/Seaside30/tree/handleRequestParsingError.
> (A fork from https://github.com/glassdb/Seaside30)

If you attempted to see changes up to now, there were none. Sorry, I
pushed 2 minutes ago.
_______________________________________________
seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

On Thu, Dec 26, 2013 at 12:05 PM, Otto Behrens <[hidden email]> wrote:
I generally prefer option 1) because this way when something goes
wrong on the server the client gets notified. Otherwise client errors
can be really annoying to debug.

something like

process: aNativeRequest
    ^ [ self basicProcess ]
        on: self badRequestExceptionSet
        do: [ :e | self badRequestFor: e ]

badRequestExceptionSet
  ^ GRInvalidUtf8Error, WAInvalidUrlSyntaxError

badRequestFor: anError
  "default implementation, trigger the servers internal error handler
  sublcasses can answer a 'native' bad request (HTTP 400) respone"
  anError signal

> A "native" request would be something that Comanche / Zinc / FastCGI
> would create and pass to Seaside (i.e. WAServerAdaptor >> process:)

Yes.

> In the error case that the Seaside framework does not handle, the web
> server / fast cgi server implements a fallback handler for server
> errors (500 Internal Server Error). (e.g. HttpService >>
> handleDispatchErrorsIn:). This is the error handler invoked in this
> discussion where the URL contains percent-encoded parts that break the
> UTF-8 decoding.
>
> It seems if I want to handle any of these errors (consistently), I
> have to modify Comanche, Zinc and FastCGI adaptors.

Yes

> I think we should allow the Seaside framework to handle these errors
> as well. I understand giving the WAResponse the responsibility to
> handle it can be tricky, but it may just be the right place.
> Currently, it does handle some of the 400 range of errors (eg. not
> found).

WAResponse does not handle 400. Somebody somewhere creates a 400
response just like somebody somewhere creates a 200 response.

> This is my suggestion: Create the WARequestContext on the native
> request without parsing it.

I'm not liking it. The propose of WARequestContext is that it holds on
to (among other things) a WARequest and a WAResponse. We just got an
exeption when trying to create a WARequest. This smells like having a
partially initialized object that will just cause other exceptions
further down the road.

> Parsing the URL effectively comes down to handleFiltered: on
> WADispatcher that will try to get the url via the consumer on the
> request. If parsing it breaks, the request is marked as a bad request.
> The dispatcher could pass to the default handler. The parsing errors
> of the other parts of the request would then go to the handler
> dispatched to. And that could in turn be dispatched to an error
> handler.

No, the URL is parsed in WAServerAdaptor >> #requestUrlFor:

> This gives the user of the Seaside framework the flexibility of
> handling these errors elegantly as well. Only errors that break the
> seaside framework itself, the ones that somehow fall through
> WAExceptionHandler >> #handleExceptionsDuring: would then trigger the
> Comanche / Zinc / FastCGI handler.

Cheers
Philippe
_______________________________________________
seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

On Thu, Dec 26, 2013 at 2:02 PM, Otto Behrens <[hidden email]> wrote:
>> to use a simple native request. I then changed WARequestContext to be
>> aware of a parsing error (must be better, should be on request
>> itself...)
>
> Made another change - WADispatcher now reponds with bad request.

IMHO dipsatcher is the wrong place to do this. WADispatcher is just a
request handler that delegates to other request handlers. There is no
requirement that you have a dispatcher in the handler chain (it is the
default though).

Sorry for being so negative. I understand you're trying to fix an
important issue for you and I'm just responding with "I don't like it"
and don't really have a better solution myself.

Cheers
Philippe
_______________________________________________
seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

>> Made another change - WADispatcher now reponds with bad request.
>
> IMHO dipsatcher is the wrong place to do this. WADispatcher is just a
> request handler that delegates to other request handlers. There is no
> requirement that you have a dispatcher in the handler chain (it is the
> default though).

Ok, yes, you're right.

> Sorry for being so negative. I understand you're trying to fix an
> important issue for you and I'm just responding with "I don't like it"
> and don't really have a better solution myself.

No problem. I'm arguing for alternatives because I want to test if
they can work. So thanks for the responses and the patience.

I think that this should not only be an important issue for me, but
for other users if Seaside too. And that is because I believe that we
should work out ways for the framework to handle errors in a
consistent way. Well, it is not the end of the world when the user of
the application get a crude response for a bad request, or for a 'not
found', but could be better.

Thanks
Otto
_______________________________________________
seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

Yes, agree. I could not find "invalid cookie" kind of responses when
searching for how others solve this kind of problem though. Having
said this, I'd also rather respond with 'bad request' if I find
anything I don't like in the request.

I suppose we also have to distinguish between unable to parse the
cookie header and an unexpected key or value in the cookie header. If
the server receives a key that it does not recognise, should it
respond with bad request?

This means that every subclass of WAServerAdaptor have to implement a
native bad request response. Is there an elegant (i.e. non monkey
patched) way for me to customize this response? That's why I thought
the seaside framework must handle it, because I can customise there.

>> It seems if I want to handle any of these errors (consistently), I
>> have to modify Comanche, Zinc and FastCGI adaptors.
>
> Yes

Ok, would be soo nice if I can do it once in my application though
(because I do it there already).

>> I think we should allow the Seaside framework to handle these errors
>> as well. I understand giving the WAResponse the responsibility to
>> handle it can be tricky, but it may just be the right place.
>> Currently, it does handle some of the 400 range of errors (eg. not
>> found).
>
> WAResponse does not handle 400. Somebody somewhere creates a 400
> response just like somebody somewhere creates a 200 response.

Sorry, the request handler may create a WAResponse with code 400.

>> This is my suggestion: Create the WARequestContext on the native
>> request without parsing it.
>
> I'm not liking it. The propose of WARequestContext is that it holds on
> to (among other things) a WARequest and a WAResponse. We just got an
> exeption when trying to create a WARequest. This smells like having a
> partially initialized object that will just cause other exceptions
> further down the road.

Yes, I share your discomfort. But, the same problem applies to the
response. When creating the context, the response is empty. Exceptions
raised during the construction of the response may leave it in a bad
state if it is not properly handled, not true?

It would be better then to parse it in one shot then. And set the
WARequest to be bad could be another option. The request handler can
then take care of a bad request.

>> Parsing the URL effectively comes down to handleFiltered: on
>> WADispatcher that will try to get the url via the consumer on the
>> request. If parsing it breaks, the request is marked as a bad request.
>> The dispatcher could pass to the default handler. The parsing errors
>> of the other parts of the request would then go to the handler
>> dispatched to. And that could in turn be dispatched to an error
>> handler.
>
> No, the URL is parsed in WAServerAdaptor >> #requestUrlFor:

I meant that if we delay the parsing, #requestUrlFor: would be sent at
a later stage.
_______________________________________________
seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

On Sun, Dec 29, 2013 at 8:26 AM, Otto Behrens <[hidden email]> wrote:
No. Among other things it's hard to know in the adapter which cookies
the code later will accept.

Yes

"it" meaning context or response

> It would be better then to parse it in one shot then. And set the
> WARequest to be bad could be another option. The request handler can
> then take care of a bad request.

That sounds better. It also addresses the issue of what happens when
the exception handler access the same header that signals an exception
or a different header that signals an exception as well. I guess you
could do the check in WARequestHandler >> #handle:. Would you extend
WAErrorHandler to have a #handleBadRequest: method or where would you
eventually handle it? Would be argument be the original exception, the
native request or the bad WARequest?

Cheers
Philippe
_______________________________________________
seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside