ANN: NEVER use clojure.core/read or read-string for reading untrusted data

[Andy Fingerhut]

Following up on the thread "*read-eval* vulnerability", I started writing some documentation for how to read Clojure data safely. That isn't ready yet, but before I get the time to finish that I wanted to quickly get out a warning that is obvious to some, but probably not all:

NEVER use clojure.core/read or read-string for reading data from untrusted sources, only trusted ones. Even from trusted sources, binding *read-eval* to false is probably a good idea, but that depends on your particular use case.


An example I wrote on ClojureDocs.org for function clojure.core/read several months ago was very badly wrong. It said that binding *read-eval* to false would cause clojure.core/read to read data safely, even if that data came from an untrusted source.

I have modified that example to be a lot longer, and hopefully as correct and scary as it should be. Please take a look at it if you use read or read-string anywhere in your Clojure code:

http://clojuredocs.org/clojure_core/clojure.core/read

Andy

[Andy Fingerhut]

And just in case it gets edited by someone else before you have a chance to read it, I've copied and pasted the current version below for reference. Correction/comments/questions all welcome.

;; WARNING: You SHOULD NOT use clojure.core/read or clojure.core/read-string to
;; read data from untrusted sources. They were designed only for reading Clojure
;; code and data from trusted sources (e.g. files that you know you wrote
;; yourself, and no one else has permission to modify them).

;; Instead, either:

;; (a) use another data serialization format such as JSON, XML, etc. and a
;; library for reading them that you trust not to have vulnerabilities, or

;; (b) if you want a serialization format that can be read safely and looks like
;; Clojure data structures, use edn (https://github.com/edn-format/edn), for
;; which Clojure 1.5 has functions clojure.edn/read and clojure.edn/read-string
;; to read them safely.

;; You definitely should not use clojure.core/read or read-string if *read-eval*
;; has its default value of true, because an attacker could cause your
;; application to execute arbitrary code while it is reading. Example:

user=> (read-string "#=(clojure.java.shell/sh \"echo\" \"hi\")")
{:exit 0, :out "hi\n", :err ""}

;; It is straightforward to modify the example above into more destructive
;; ones that remove all of your files, copy them to someone else's computer
;; over the Internet, install Trojans, etc.

;; Even if you do bind *read-eval* to false first, like so:

(defn read-string-unsafely [s]
(binding [*read-eval* false]
(read-string s)))

;; you may hope you are safe reading untrusted data that way, but in Clojure 1.4
;; and earlier, an attacker can send data that causes your system to execute
;; arbitrary Java constructors. Most of these are benign, but it only takes one
;; to ruin your application's day. Examples that should scare you:

;; This causes a socket to be opened, as long as the JVM
;; sandboxing allows it.
(read-string-unsafely "#java.net.Socket[\"www.google.com\" 80]")

;; This causes precious-file.txt to be created if it doesn't
;; exist, or if it does exist, its contents will be erased (given
;; appropriate JVM sandboxing permissions, and underlying OS file
;; permissions).
(read-string-unsafely "#java.io.FileWriter[\"precious-file.txt\"]")

;; The particular issue of executing arbitrary Java constructors used in the
;; examples above no longer works in Clojure 1.5 when *read-eval* is false.
;; Even so, you SHOULD NEVER USE clojure.core/read or clojure.core/read-string
;; for reading untrusted data.

;; If you understand all of the above, and want to use read or read-string to
;; read data from a _trusted_ source, continue on below.

;; read wants *in* set to a java.io.PushbackReader.
;; with-open sets *in* and closes it after it's done.
;; *read-eval* specifies whether to evaluate #=() forms
;; when reading.
(defn read-from-file-with-trusted-contents [filename]
(with-open
[r (java.io.PushbackReader.
(clojure.java.io/reader filename))]
(binding [*read-eval* false]
(read r))))

user=> (spit "testfile.txt" "{:a 1 :b 2 :c 3}")
nil
user=> (read-from-file-with-trusted-contents "testfile.txt")
{:a 1, :b 2, :c 3}

[AtKaaZ]

thank you, this was an easy read even for me

--
--
You received this message because you are subscribed to the Google
Groups "Clojure" group.
To post to this group, send email to clo...@googlegroups.com
Note that posts from new members are moderated - please be patient with your first post.
To unsubscribe from this group, send email to
clojure+u...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/clojure?hl=en
---
You received this message because you are subscribed to the Google Groups "Clojure" group.
To unsubscribe from this group and stop receiving emails from it, send an email to clojure+u...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

--

Please correct me if I'm wrong or incomplete,
even if you think I'll subconsciously hate it.

[Mimmo Cosenza]

thanks by me too.

mimmo

[Brent Millare]

Is it possible to elaborate on the reasons for not using read/read-string with *read-eval* bound to false for clojure 1.5 and greater? Is it just because they can execute dumb code that takes CPU cycles? It's not obvious to me.

Best,

Brent

[Andy Fingerhut]

I will try to get confirmation of my semi-educated guesses below and add it to my longer writeup, but I believe the main reasons are:

(1) clojure.edn/read and read-string have been added to Clojure 1.5, designed for use in reading data from untrusted sources without any of the kinds of side effects that clojure.core/read and read-string have.

(2) Attempting to find and plug all possible potential security vulnerabilities in clojure.core/read and read-string when *read-eval* is bound to false would make it too easy to miss something, especially when combined with avoiding breakage of existing functionality needed by the Clojure compiler.  Some of that functionality relies upon the side effects that can occur during read.

It isn't just clojure.core/read executing code that can consume CPU cycles that is the issue, it is clojure.core/read executing code that can wreak havoc with your system and allow attackers to gain remote control of it.

Andy

[Phil Hagelberg]

Andy Fingerhut writes:

> It isn't just clojure.core/read executing code that can consume CPU
> cycles that is the issue, it is clojure.core/read executing code that
> can wreak havoc with your system and allow attackers to gain remote
> control of it.

Are there specific known problems with binding *read-eval* to false?
Relying on read-edn makes it difficult to write libraries that are
backwards-compatible.

-Phil

[Andy Fingerhut]

I don't know of any problems with Clojure 1.5's clojure.core/read or read-string while binding *read-eval* to false. I do know of problems with them when using Clojure 1.4 and earlier (see below).

I know it is strongly recommended to use an edn reader instead of the full clojure.core/read and read-string, for data from untrusted sources.

One possibility I left out of my previous message, for lack of remembering it, is that the new contrib lib tools.reader provides an edn reader that works with Clojure 1.4 and later (it could be made to work with Clojure 1.3 if there is enough interest -- it currently uses 1.4-specific ex-info).

http://github.com/clojure/tools.reader
http://build.clojure.org/job/tools.reader-test-matrix

Examples of dangerous side effects that can occur with clojure.core/read and read-string in Clojure 1.4 and earlier:

(defn read-string-unsafely [s]
(binding [*read-eval* false]
(read-string s)))

;; This causes a socket to be opened, as long as the JVM
;; sandboxing allows it.
(read-string-unsafely "#java.net.Socket[\"www.google.com\" 80]")

;; This causes precious-file.txt to be created if it doesn't
;; exist, or if it does exist, its contents will be erased (given
;; appropriate JVM sandboxing permissions, and underlying OS file
;; permissions).
(read-string-unsafely "#java.io.FileWriter[\"precious-file.txt\"]")

Those examples throw exceptions without calling the constructors in Clojure 1.5-RC15.

Andy

[Phil Hagelberg]

Andy Fingerhut writes:

> Examples of dangerous side effects that can occur with
> clojure.core/read and read-string in Clojure 1.4 and earlier:
>

> ;; This causes precious-file.txt to be created if it doesn't
> ;; exist, or if it does exist, its contents will be erased (given
> ;; appropriate JVM sandboxing permissions, and underlying OS file
> ;; permissions).
> (read-string-unsafely "#java.io.FileWriter[\"precious-file.txt\"]")

Thanks for clarifying. That is quite unfortunate. A separate library
will help for backwards-compatibility though.

-Phil

[Andy Fingerhut]

Yes, agreed it is unfortunate.

I have updated my version of the cheatsheet at http://jafingerhut.github.com

Now it mentions the new clojure.tools.reader.edn versions of read and read-string (http://github.com/clojure/tools.reader), and no longer links to the clojure.core versions at all. Anyone who needs those can find them elsewhere.

The clojure.tools.reader.edn versions should never trigger code execution as a side effect, with the exception of calling data reader functions, which are under the control of the caller. Nicola Mometto recently updated that library to support not only Clojure 1.4 but also Clojure 1.3, but that enhancement might not be in the public repositories until the next release.

I will see if Alex Miller or someone else with permissions can help me update the main cheat sheet at http://clojure.org/cheatsheet, too.

Andy