NPE in param parsing middleware on certain query strings

[cespare]

We occasionally get NPEs in ring's middleware parsing code when we get certain malformed query strings (or post bodies).

Here's a small example:

(require '[ring.middleware.params :as params])
(require '[ring.middleware.nested-params :as nested-params])
(let [app (-> (constantly {:status 200 :body "hello!"})
              nested-params/wrap-nested-params
              params/wrap-params)]
  (app {:uri "/", :query-string "x=%\"%6346#%#/&#736%"})) ;=> NPE

The problem arises because wrap-params creates a :params map with a nil key and then wrap-nested-params crashes when it sees that.

(require '[ring.util.codec :as codec])
(codec/form-decode "x=%\"%6346#%#/&#736%") ; => {"x" nil, nil nil}

I'm not sure whether wrap-params should never produce a nil key, or whether wrap-nested-params should ignore nil keys (or both).

[weavejester]

Thanks for the report. I believe the wrap-params function should ignore keys that have invalid (nil) values. There's nothing we can really do with a key that points to an invalid value, and it saves middleware further down the like, like wrap-nested-params from worrying about it.

[cespare]

I believe the wrap-params function should ignore keys that have invalid (nil) values.

That sounds fine to me, but the cause of the crash is nil map keys, not nil map values. You can construct a query string that causes the crash by producing a :params map with a nil key pointing to a non-nil value: just use the string "%\"%6346#%#=x" in the above examples.

(codec/form-decode "%\"%6346#%#=x") ;=> {nil "x"}

[cespare]

Or maybe I'm misunderstanding what you meant by "keys that have invalid (nil) values" and "a key that points to an invalid value".

[weavejester]

Oh, well, everything I said goes double for nil keys. They definitely shouldn't be produced by wrap-params.

[weavejester]

Might be fixed by ring-clojure/ring-codec#8. That PR seems to have missed out on a release, so I'll go and release Ring-Codec 1.0.1 and see if that helps.

[cespare]

I'm not seeing how url-decode is involved here. wrap-params eventually calls codec/form-decode which calls codec/form-decode-str which does

(try
  (URLDecoder/decode encoded (or encoding "UTF-8"))
  (catch Exception _ nil)))

(I don't understand why ring-codec implements its own url encoding/decoding but also uses URLEncoder and URLDecoder for some things, but that's a separate topic.)

[weavejester]

Yes, you're right about that PR. The codec/form-decode function is unrelated. My mistake.

In case you're interested, the reason Ring-Codec has both url-decode and form-decode is because they differ slightly in how they handle certain characters.

(codec/url-decode "foo+bar")
=> "foo+bar"
(codec/form-decode "foo+bar")
=> "foo bar"

The URLEncoder and URLDecoder classes are misnamed, because they don't perform encoding and decoding for URLs, but for the application/x-www-form-urlencoded content type.

[codahale]

I just ran into this with a URI of the form /ts.swf?jsinitfunctio%gn=alert%60PoC%20PoC%20PoC%60:

java.lang.NullPointerException: null
    at clojure.core$name.invokeStatic(core.clj:1546)
    at ring.middleware.nested_params$parse_nested_keys.invokeStatic(nested_params.clj:5)
    at ring.middleware.nested_params$parse_nested_keys.invoke(nested_params.clj:5)
    at ring.middleware.nested_params$nest_params$fn__22536.invoke(nested_params.clj:54)
    at clojure.lang.ArrayChunk.reduce(ArrayChunk.java:58)
    at clojure.core.protocols$fn__6750.invokeStatic(protocols.clj:136)
    at clojure.core.protocols$fn__6750.invoke(protocols.clj:124)
    at clojure.core.protocols$fn__6710$G__6705__6719.invoke(protocols.clj:19)
    at clojure.core.protocols$seq_reduce.invokeStatic(protocols.clj:31)
    at clojure.core.protocols$fn__6738.invokeStatic(protocols.clj:75)
    at clojure.core.protocols$fn__6738.invoke(protocols.clj:75)
    at clojure.core.protocols$fn__6684$G__6679__6697.invoke(protocols.clj:13)
    at clojure.core$reduce.invokeStatic(core.clj:6545)
    at ring.middleware.nested_params$nest_params.invokeStatic(nested_params.clj:47)
    at ring.middleware.nested_params$nest_params.invoke(nested_params.clj:47)

[camdez]

Proposed solution for this here: ring-clojure/ring-codec#25

[camdez]

From my perspective, this issue was resolved with ring-clojure/ring-codec#25. Close? /cc @cespare