Request.Form: can we have an alternative without query values?

[Rodrigo Moraes]

Dear gophers,

Wouldn't it be nice to have an alternative http.Request field that
doesn't include form values from the URL query? It could be used by
Request.ParseForm() to merge query+form values into Request.Form.

The current behavior can be nastily exploited in apps that don't take
care enough. As pointed on IRC:

http://play.golang.org/p/ncnuZKToE_

The hack to not have mixed query values (also from IRC):

tmp := request.URL
request.URL = nil
request.ParseForm()
request.URL = tmp

...or parse the body yourself. Security was not my primary concern,
though, but the above example does look weird. HTTP-spec wise, is
there a reason why query+form are merged?

-- rodrigo

[Kyle Lemons]

On Wed, May 16, 2012 at 6:41 AM, Rodrigo Moraes <rodrigo...@gmail.com> wrote:

Dear gophers,

Wouldn't it be nice to have an alternative http.Request field that
doesn't include form values from the URL query? It could be used by
Request.ParseForm() to merge query+form values into Request.Form.

The current behavior can be nastily exploited in apps that don't take
care enough. As pointed on IRC:

   http://play.golang.org/p/ncnuZKToE_

If I'm not mistaken, this "exploit" requires controlling the form's action.  If an attacker can control that, they could also probably redirect the user to their own server and steal all of the information and then redirect them back to the original action with properly-formed (but compromised) POST data.  If you are concerned about this in your webapps, it is probably trivial to add a quick `if r.Method = "POST" { r.URL.RawQuery = "" }`, though I would personally recommend auditing where the form tags get their action (in my own apps, it's always hard-coded in the template).

[Brad Fitzpatrick]

Russ wanted r.FormValue("foo") to just work.

Keeping that behavior, it's possible we might one day add accessors for just-body parameters, but probably not new exported fields.

On Wed, May 16, 2012 at 6:41 AM, Rodrigo Moraes <rodrigo...@gmail.com> wrote:

[Rodrigo Moraes]

On May 16, 1:38 pm, Brad Fitzpatrick wrote:
> Keeping that behavior, it's possible we might one day add accessors for
> just-body parameters, but probably not new exported fields.

That would be enough! Thanks.

-- rodrigo

[Rodrigo Moraes]

On May 16, 11:38 am, Peter Thrun wrote:
> What is the exploit?  I don't see a problem in the posted code.

I'd guess that accepting query parameters as form data just makes form
tampering a lot easier. But security wasn't my primary concern, just
came out while discussing this on IRC.

I just wanted to access form data separately. For now, cleaning
Request.URL.RawQuery will do the trick.

-- rodrigo

[Kyle Lemons]

On Wed, May 16, 2012 at 12:20 PM, Rodrigo Moraes <rodrigo...@gmail.com> wrote:

On May 16, 11:38 am, Peter Thrun wrote:
> What is the exploit?  I don't see a problem in the posted code.

I'd guess that accepting query parameters as form data just makes form
tampering a lot easier.

As I mentioned, I don't really think this is the case.

 

But security wasn't my primary concern, just
came out while discussing this on IRC.

I just wanted to access form data separately.

Here is where I start to take issue: I think it's poor design to care where you get your form values.  I wouldn't mind if FormValues only got the data from the canonical source for the current method (GET -> query params, POST -> form body), but putting that in your code doesn't seem like the correct approach.  The PHP language (from my view) encourages people to care about the difference, but as soon as you do you make it harder to do simple things like unit test your code.  Often it is super easy to control form responses in query parameters for testing and they also are very good for creating links to pre-populate a form (akin to "mailto" links that provide the subject for you).  When you start caring where the data came from, the logic here becomes much more difficult.

[Rodrigo Moraes]

On May 16, 6:46 pm, Kyle Lemons wrote:
> I think it's poor design to care where
> you get your form values.

This sounds a bit extreme. It is a common and reasonable practice to
care about, and sometimes you are told to care about. For this section
of the OAuth spec for example:

http://tools.ietf.org/html/draft-ietf-oauth-v2-26#section-4.1.3

...I should not ignore if a request parameter came from the query or
the body. Agree?

> The PHP language (from my view) encourages people to care
> about the difference,

Actually, they had $_REQUEST the last time I used it and that included
query, form *and* cookie values, and it was quite misused and
considered a bad thing. Not the same thing, but I think the
distinction did not came from there. It came from CGI times. And
today, for example, in Python's most popular Request wrappers (WebOb,
Werkzeug, Django's Request etc), there's a distinction between query
and form values, although they also offer query-or-form accessors.

-- rodrigo

[si guy]

I agree with Kyle, have written lots of php and I've always felt that $_POST and friends solve one problem (namespacing) but create two others: obstructing testing, and providing the illusion of security(http requests aren't exactly difficult to forge, especially if the client is compromised with browser malware).

Assuming that an established, authenticated and secure connection's $_POST could be trusted bit me once.... Never again.

[Rodrigo Moraes]

I think you entered territory that is out of scope of this request. I
didn't argue that form data is inherently secure, but I'll maintain
that accepting form data from query parameters makes form tampering
easier. The most amateur form of tampering, that doesn't even require
HTML injection, just some social skills. But this is not the point.
The distinction between query and form values is a simple feature
established in frameworks from major languages. Even the HTTP
specification makes the distinction between GET and POST forms. Unit-
testing these forms is just a function away, don't be lazy. I feel sad
to have to argue in favor of this.

-- rodrigo

[Rodrigo Moraes]

It was rejected, anyway: http://code.google.com/p/go/issues/detail?id=3630

-- rodrigo

[Kyle Lemons]

On Wed, May 16, 2012 at 3:24 PM, Rodrigo Moraes <rodrigo...@gmail.com> wrote:

On May 16, 6:46 pm, Kyle Lemons wrote:
> I think it's poor design to care where
> you get your form values.

This sounds a bit extreme. It is a common and reasonable practice to
care about, and sometimes you are told to care about. For this section
of the OAuth spec for example:

   http://tools.ietf.org/html/draft-ietf-oauth-v2-26#section-4.1.3

...I should not ignore if a request parameter came from the query or
the body. Agree?

I don't see where it says you should ignore the request if it was sent via query parameters in there, but I only skimmed it.  OAuth also, unless I am gravely mistaken, already contains provisions to make XSRF difficult.  If this is the case, then I'd side with Postel's Law on this one and say to just use FormValue.

[Patrick Mylund Nielsen]

These are two different things:

1. Distinguishing between GET and POST parameters helps protect users
against themselves when they get social-engineered into clicking a
link to an application that doesn't make any distinction. One simple
cannot argue that it is as easy to get a user to make a POST request
as it is to get them to click a link -- it isn't. You _can_ argue if
there are really applications where this could have any negative
effect (without any input from the user who's being tricked), and
whether simply checking the request method or applying the workaround
would be sufficient. I think both of them are (although what Brad
suggested wouldn't hurt.) Developers who care about security will
likely use CSRF tokens anyway.

2. Never trust user input.

[Kyle Lemons]

On Thu, May 17, 2012 at 10:21 AM, Patrick Mylund Nielsen <pat...@patrickmylund.com> wrote:

These are two different things:

1. Distinguishing between GET and POST parameters helps protect users
against themselves when they get social-engineered into clicking a
link to an application that doesn't make any distinction. One simple
cannot argue that it is as easy to get a user to make a POST request
as it is to get them to click a link -- it isn't. You _can_ argue if
there are really applications where this could have any negative
effect (without any input from the user who's being tricked), and
whether simply checking the request method or applying the workaround
would be sufficient. I think both of them are (although what Brad
suggested wouldn't hurt.) Developers who care about security will
likely use CSRF tokens anyway.

To be clear, I'm not talking about limiting whether your form input requires GET or POST, only how your code gets its form data.  I have no problem with throwing a 405 if they GET when submitting a form.  Social engineering a user into clicking a link is clearly easy, but this is a GET request.  In order for a form submission to have query parameters from a browser requires those query parameters to be a part of the action in the form element, which is not something that can be simply "socially engineered."  In a unit test, however, it is trivial to construct at POST /path/to/endpoint?a=b&c=d, and if you are populating a form in your template from a request, it is easy to prepopulate them via a link.

[Rodrigo Moraes]

On May 17, 1:56 pm, Kyle Lemons wrote:
> I don't see where it says you should ignore the request if it was sent via
> query parameters in there, but I only skimmed it.  OAuth also, unless I am
> gravely mistaken, already contains provisions to make XSRF difficult. If
> this is the case, then I'd side with Postel's

> Law<http://en.wikipedia.org/wiki/Robustness_principle> on

> this one and say to just use FormValue.

It doesn't say this specifically. It only defines that for some
requests parameters are passed in the body. Others use the URL query.
I feel that it is a better choice to follow it strictly, just cleaning
r.URL.RawQuery when body parameters are required. Not a big deal
anyway, but I'd like to hear other interpretations.

FormValue() is not very handy because multiple values for the same key
result in invalid requests, so you have to deal with url.Values
directly anyway. values.Get() is the option after checking length,
then.

Here's another example. protorpc [1] accepts URL encoded requests,
among other formats. To implement something like that in Go you'd also
want to ignore the URL query values. Would you, if go-rpcgen had a url-
encoded Protocol? That's a very specific case, anyway.

-- rodrigo

[1] http://code.google.com/p/google-protorpc/

[Patrick Mylund Nielsen]

I agree with you about not being able to influence the action value of
a form on a page, but if you don't make a distinction between GET and
POST in Go, or distinguish between body and query parameters, then a
malicious person can very easily fool the average user into clicking

"Your Facebook account has expired
Please reactivate it at <a
href="http://yoursite.com/yourdeleteaccounthandler?confirmed=1>facebook.com/reactivate</a>"

even if the confirmation form itself is on yoursite.com/deleteaccount.
This sort of stuff fools people all the time.

But, if you care about protecting against stuff like this, I think
your time is better spent setting and checking for a session cookie,
CSRF token and Referrer header, than worrying about whether the
information came from the querystring or the body. The only tidbit is
that using CSRF tokens in querystrings could leak them to someone.

[Patrick Mylund Nielsen]

And just to clear, as well: I'm not trying to encourage anyone to
trust POST requests over GET. It's entirely feasible to fool people
into clicking a link to a page with some javascript that POSTs to your
handler. You absolutely should use something like session- or even
one-time-CSRF tokens for forms.

On Thu, May 17, 2012 at 7:45 PM, Patrick Mylund Nielsen

[Jesse McNelis]

On Fri, May 18, 2012 at 3:45 AM, Patrick Mylund Nielsen
<pat...@patrickmylund.com> wrote:
> I agree with you about not being able to influence the action value of
> a form on a page, but if you don't make a distinction between GET and
> POST in Go, or distinguish between body and query parameters, then a
> malicious person can very easily fool the average user into clicking

The Go http pkg makes a distinction between GET and POST. It just
doesn't make a distinction between POST'ed form values and url query
values.

The issue becomes a CSRF problem when you have a <form action=""
method="post"> form so that it just posts to the current url which
might have had additional url query values added by a malicious user
that gave the user the link.

Setting action="" in a form is a bad idea anyway because you're giving
a third party the ability to modify where your form will post to (as
long as the GET for it still resolves to the form).








--
=====================
http://jessta.id.au

[Patrick Mylund Nielsen]

If the Go developer just sets up a handler and does
req.FormValue("foo"), there is no distinction. FormValue does
ParseForm.

It's a CSRF problem (albeit a different one) regardless of whether the
page posts to itself or another page. An attacker can just point to
the actual handler, since it's just a regular URL. Your scenario is
actually the best reason I can think of to have a concrete way (rather
than a workaround) to distinguish between body and query values, since
this is significant even if you're sure they're POSTing and coming
from the page/with a CSRF token. You don't even have to have
explicitly written action="."--that's the default.

Picture something like a user given a URL that redirects to something
like http://yoursite.com/?email=bwahahaha%40eliteteam.cn (maybe this
scenario isn't so realistic, but you get the drift)

<form method="POST">
<input name="name">...</input>
<input name="cc">...</input>
<input name="ccexp">...</input>
<input name="cvc">...</input>
<input name="email">...</input>
<input type="submit"...></input>
</form>

The "confirm account" link and session nonce will be sent to the
attacker, who goes on to pay his bills.

Best,
Patrick

[Patrick Mylund Nielsen]

So, what Rodrigo said :)

On Thu, May 17, 2012 at 10:08 PM, Patrick Mylund Nielsen

[si guy]

Yes but... An attacker could just as easily give a user a link to a page that auto-posts all of this via javascript or flash or something.

Trusting that a post is actually a post seems isomorphic to trusting user input..... Not a pretty picture I agree.

-Simon Watt

[Rodrigo Moraes]

OpenID 1.1:

"When a message is sent as a POST, OpenID parameters MUST only be sent
in, and extracted from, the POST body."

http://openid.net/specs/openid-authentication-1_1.html

OAuth 2:

"The parameters can only be transmitted in the request body and MUST
NOT be included in the request URI."

http://tools.ietf.org/html/draft-ietf-oauth-v2-26

-- rodrigo

[Brad Fitzpatrick]

On Thu, May 17, 2012 at 9:14 PM, Rodrigo Moraes <rodrigo...@gmail.com> wrote:

OpenID 1.1:

"When a message is sent as a POST, OpenID parameters MUST only be sent
in, and extracted from, the POST body."

http://openid.net/specs/openid-authentication-1_1.html

proof that I do care about this issue. :)

[Patrick Mylund Nielsen]

The issue has been re-opened for Go1.1 --
http://code.google.com/p/go/issues/detail?id=3630.

I've started working on a patch, but by all means feel free to beat me to it.

[Rodrigo Moraes]

On May 18, 1:33 am, Patrick Mylund Nielsen wrote:
> The issue has been re-opened for Go1.1 --http://code.google.com/p/go/issues/detail?id=3630.

>
> I've started working on a patch, but by all means feel free to beat me to it.

Wooo! Thank you, Patrick.

-- rodrigo

[Rodrigo Moraes]

On May 18, 1:26 am, Brad Fitzpatrick wrote:
> >http://openid.net/specs/openid-authentication-1_1.html
>
> proof that I do care about this issue. :)

Heh. I thought you were being sarcastic, then I looked at the bottom
of that page.

-- rodrigo