search

racket / web-server

Other
90 stars 47 forks source link

Consider disabling ;-separated query parameters by default #100

Open samth opened 3 years ago

samth commented 3 years ago

Recently, people have pointed out that the combination of common caching proxies and web frameworks that treat ; as a query separator can lead to security problems; see here: https://snyk.io/blog/cache-poisoning-in-popular-open-source-packages/

This led Python to change the default behavior: https://bugs.python.org/issue42967

See also this article: https://lwn.net/Articles/846847/

We have a few choices here:

  1. Do nothing (probably a bad idea)
  2. Enable users of the web server to disable ; as a separator.
  3. Like 2, but make that the default. (This is what Python did)
jeapostrophe commented 3 years ago

If I understand correctly, Racket already does this in via current-alist-separator-mode

https://github.com/racket/racket/blob/master/racket/collects/net/uri-codec.rkt#L249-L255

As far as the Web server goes, I believe you can just change that parameter and all is well. So are you asking to change the default?

Now, alternatively, you might be talking about the fact that ; is a reserved character in URI path segments called a "parameter" (https://tools.ietf.org/html/rfc3986#section-3.3), which is why the net/url has the path/param structure. The Web server uses that to embed the continuation ids in parallel to any query parameters. Changing this would be very invasive, because there's no other channel we could use that wouldn't interfere with the user's program...

samdphillips commented 2 years ago

It would seem to me that setting the default for (current-alist-separator-mode) to 'amp would be the most sensible approach and similar to the choice the python devs made.

samth commented 2 years ago

Yes, that's what I was thinking.

LiberalArtist commented 2 years ago

Substantively, this seems like a good change to me.

Changing the default for current-alist-separator-mode would, strictly speaking, be a backwards incompatible change. That may be the right choice for all clients of net/uri-codec. (I don't think I've ever seen a url in the wild that uses ; to separate query parameters.) But we do have the option of handling this within the web server via the "safety limits" construct: we could add an #:alist-separator-mode argument defaulting to 'amp for make-safety-limits, but to 'amp-or-semi for make-unlimited-safety-limits, and then parameterize accordingly when reading requests.

My recollection from when we made a breaking change for the sake of security and added the "safety limits" construct in the process is that it would have been good to have had broader discussion first.