8 ms·

Clojure's Default Reader is Unsafe

80 points by llambda 14y ago

matthavener 14y ago
;; especially since read-string will be fixed in clojure 1.5 so that ;; binding *read-eval* to false around it will make it safe, as far as In clojure 1.5 with read-eval bound to false, the reader is still unsafe for malicious input. Since the Clojure reader must be able to construct arbitrary Java objects via their constructors, any constructor can be called from the reader even with read-eval bound to false.
- fogus 14y ago
  So you wouldn't want to use the reader if safety is needed. Instead you would use the EDN data functions. http://clojure.github.com/clojure/#clojure.edn http://clojure.github.com/clojure/#clojure.edn
drcode 14y ago
Glad I ran across this this week- I would have most likely ended up with a security hole in an app otherwise. (I had thought read-eval=false was an adequate safeguard...)
baq 14y ago
executing code you didn't write is unsecure. news at 11. ...am i missing something? isn't this the same problem as with evaling json in javascript?
- praptak 14y ago
  No, it would be somewhat analogous a json parser that doesn't only return a passive data struct but also executes arbitrary code.
- djpowell 14y ago
  Lisps separate the notion of reading strings into data structures, from evaluating those data structures. So it would be easy to assume that read-string was safe to use, but there were actually some lesser-known features in the Clojure reader that allowed code execution.
- martinced 14y ago
  You're missing that the whole point here is that when read-eval is set to false then, precisely, you're making your intent very clear: you do NOT want to execute code you didn't write. The problem is that there's a hole due to Java constructors being called (even when read-eval is set to false).
puredanger 14y ago
I think this is an excellent write-up of the state of the world with Clojure 1.5: http://clojuredocs.org/clojure_core/clojure.core/read http://clojuredocs.org/clojure_core/clojure.core/read In short, don't use read-eval at all if you want safe reading. Use the clojure.edn alternatives to read data safely in those circumstances.
- jfb 14y ago
  In addition, I was very happy to see it presented as valid Clojure with explicating commentary, rather than as an information-light "screencast".
- willismichael 14y ago
  ;; 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. Use an edn reader or a different data serialization format. From that comment it seems like the Java constructor loophole has been closed. With that gone, I don't understand why the reader is still dangerous. What am I missing?
  - mjw 14y ago
    I was wondering this too, and deliberately tried to break something using the clojure 1.5 reader with read-eval false. (Makes a change from building things all day!). Result: user=> (binding [*read-eval* false] (read-string "``````````x")) OutOfMemoryError Java heap space clojure.lang.RT.cons (RT.java:570)
  - bjeanes 14y ago
    There's a huge IRC log that explains the reasoning here. Mostly, it comes down to: you can't trust that libraries haven't changed read-eval back, which means that now instead of being assured that a certain fn can never be harmful, you would need to audit all libraries you use. It's the assumption itself (that read and read-string are safe) what makes it unsafe...
pfraze 14y ago
This is basically what got Ruby with its YAML reader, isn't it? EDIT: I'm not familiar enough with clojure; is read-line supposed to be safe?
- martinced 14y ago
  It's the same issue as long as you consider that any evaluation of unwanted code is the same issue and if you consider that what happens here with the Java constructors is really "eval'ing" code. Here I'd say it's a bit different: it's kinda the whole of having read-eval set to true to be able to evaluate code. So when read-eval is set to false and yet rogue code happens to be called there's a serious issue. Basically the issue is that under the hood some Java constructors seems to be called and, if I understand the issue correctly, by knowing how Clojure works you can exploit Clojure / the JVM by crafting precisely what you want to pass to Java constructors (which shouldn't be called in the first place). Here as I understand it it's not an "eval" exploit per se: it's Java constructors which are passed carefully crafted input.
- benatkin 14y ago
  This is read-string, not read-line. It doesn't mean read a string but to read from a string. It reads the first clojure expression from a string. It isn't supposed to be safe, though it can be tempting to treat it as though it's safe. - It's similar to the YAML reader, in that Java objects can be created. See "deftype, defrecord, and constructor calls" here: http://clojure.org/reader http://clojure.org/reader - On the positive side for the clojure reader, it isn't widely described as a serialization format and people are less inclined to use it for user input. This is a pretty important difference. - On the negative side, you can not only instantiate objects, but you can execute code. This is what the example in the article does. You can disable that by setting read-eval. - Also on the negative side, it lacks something quite as secure as YAML's safe_load. You can tell it not to evaluate code, but there isn't yet a way to disable instantiating objects. As puredanger pointed out, EDN is a better alternative that also comes with clojure. It's designed for data. Clojure's code format is a superset of EDN. http://clojure.github.com/clojure/clojure.edn-api.html http://clojure.github.com/clojure/clojure.edn-api.html
- stonemetal 14y ago
lucian1900 14y ago
In short, use an explicit serialisation format (edn) instead of read. Which mirrors the advice for JavaScript of using a JSON parser instead of eval.
pjungwir 14y ago
I'm feeling vindicated for this comment now: https://news.ycombinator.com/item?id=5258993 As more people start adopting Lispy languages, it will be important to build up a cultural awareness of what's dangerous. Things like not evaling code is obvious enough (I'd hope), but avoiding read less so. That seems less like "avoid eval" and more like "avoid gets".
- djacobs 14y ago
  It's true that in general, people need to understand what is dangerous in each language they use. However, as far as I can tell, the problem here has nothing to do with homoiconicity. The "get string" vulnerability, as you call it, comes out of Clojure's (excellent) JVM support, and not from the Lisp side of its family tree. A "read" is not just a read only because reading Java means creating and compiling arbitrary classes and objects. That, in turn, is largely because Java is not homoiconic.
- betterunix 14y ago
  Keep in mind that this is a Clojure-specific issue. In Common Lisp, eval-suppressing macros should prevent this sort of thing (but I would not rely on that -- it is easy to accidentally create a reader macro that fails to check those variables). Personally, I would only use something like read if I were reading a configuration file i.e. something that only someone with privileged access can modify. I would not even consider using read for arbitrary, untrustworthy data; it is just silly in this day and age. It may make the code look prettier, but the fact that arbitrary data structures can be created and the potential to execute arbitrary code is enough to keep me away.
- spacemanaki 14y ago
  > That seems less like "avoid eval" and more like "avoid gets". Assuming that by `gets` you mean Ruby's `gets`, this is not quite right. `read` (and `read-string`) are not just generic IO functions. They are parsers for Clojure code. There are plenty of other varied forms of reading input to a string, but `read` does more.
  - pjungwir 14y ago
fogus 14y ago
Clojure's reader is designed to provide the types of tasks useful for Lisps; specifically to take a string and convert it to data structures. However, like most Lisps Clojure has some special syntax that does various tasks at read-time like the #= tag that might not be safe to use as a webform processor, but it's critical to the way that Clojure works. In Clojure 1.5 there are EDN functions that handle a subset of the Clojure syntax and parse strings into data types like numbers, maps and vectors (and others). It's meant to deal with data only and not "reader forms". I wrote a Ring middleware to handle EDN data and recently ported it to use the new 1.5 EDN reader. https://github.com/fogus/ring-edn/blob/master/src/ring/middleware/edn.clj#L15 https://github.com/fogus/ring-edn/blob/master/src/ring/middl... It looks almost exactly like the old clojure.core/read-string except it will not execute any dangerous code.
- solussd 14y ago
  neat!
- stouset 14y ago
  Anything that can turn strings into symbols is dangerous code. Symbols can't be garbage collected, and an attacker can conduct a denial of service by forcing you to load unlimited numbers of uniquely-named symbols.
  - technomancy 14y ago
    Symbols can be GCed as of 1.2.
yawnt 14y ago
this looks like an interesting alternative https://github.com/clojure/tools.reader https://github.com/clojure/tools.reader

80 points by llambda 14y ago

;; especially since read-string will be fixed in clojure 1.5 so that ;; binding *read-eval* to false around it will make it safe, as far as In clojure 1.5 with read-eval bound to false, the reader is still unsafe for malicious input. Since the Clojure reader must be able to construct arbitrary Java objects via their constructors, any constructor can be called from the reader even with read-eval bound to false.

fogus 14y ago
So you wouldn't want to use the reader if safety is needed. Instead you would use the EDN data functions. http://clojure.github.com/clojure/#clojure.edn http://clojure.github.com/clojure/#clojure.edn

drcode 14y ago

Glad I ran across this this week- I would have most likely ended up with a security hole in an app otherwise. (I had thought read-eval=false was an adequate safeguard...)

baq 14y ago

executing code you didn't write is unsecure. news at 11. ...am i missing something? isn't this the same problem as with evaling json in javascript?

praptak 14y ago
No, it would be somewhat analogous a json parser that doesn't only return a passive data struct but also executes arbitrary code.
djpowell 14y ago
Lisps separate the notion of reading strings into data structures, from evaluating those data structures. So it would be easy to assume that read-string was safe to use, but there were actually some lesser-known features in the Clojure reader that allowed code execution.
martinced 14y ago
You're missing that the whole point here is that when read-eval is set to false then, precisely, you're making your intent very clear: you do NOT want to execute code you didn't write. The problem is that there's a hole due to Java constructors being called (even when read-eval is set to false).

puredanger 14y ago

I think this is an excellent write-up of the state of the world with Clojure 1.5: http://clojuredocs.org/clojure_core/clojure.core/read http://clojuredocs.org/clojure_core/clojure.core/read In short, don't use read-eval at all if you want safe reading. Use the clojure.edn alternatives to read data safely in those circumstances.

jfb 14y ago
In addition, I was very happy to see it presented as valid Clojure with explicating commentary, rather than as an information-light "screencast".
willismichael 14y ago
;; 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. Use an edn reader or a different data serialization format. From that comment it seems like the Java constructor loophole has been closed. With that gone, I don't understand why the reader is still dangerous. What am I missing?
- mjw 14y ago
  I was wondering this too, and deliberately tried to break something using the clojure 1.5 reader with read-eval false. (Makes a change from building things all day!). Result: user=> (binding [*read-eval* false] (read-string "``````````x")) OutOfMemoryError Java heap space clojure.lang.RT.cons (RT.java:570)
- bjeanes 14y ago
  There's a huge IRC log that explains the reasoning here. Mostly, it comes down to: you can't trust that libraries haven't changed read-eval back, which means that now instead of being assured that a certain fn can never be harmful, you would need to audit all libraries you use. It's the assumption itself (that read and read-string are safe) what makes it unsafe...

pfraze 14y ago

This is basically what got Ruby with its YAML reader, isn't it? EDIT: I'm not familiar enough with clojure; is read-line supposed to be safe?

martinced 14y ago
It's the same issue as long as you consider that any evaluation of unwanted code is the same issue and if you consider that what happens here with the Java constructors is really "eval'ing" code. Here I'd say it's a bit different: it's kinda the whole of having read-eval set to true to be able to evaluate code. So when read-eval is set to false and yet rogue code happens to be called there's a serious issue. Basically the issue is that under the hood some Java constructors seems to be called and, if I understand the issue correctly, by knowing how Clojure works you can exploit Clojure / the JVM by crafting precisely what you want to pass to Java constructors (which shouldn't be called in the first place). Here as I understand it it's not an "eval" exploit per se: it's Java constructors which are passed carefully crafted input.
benatkin 14y ago
This is read-string, not read-line. It doesn't mean read a string but to read from a string. It reads the first clojure expression from a string. It isn't supposed to be safe, though it can be tempting to treat it as though it's safe. - It's similar to the YAML reader, in that Java objects can be created. See "deftype, defrecord, and constructor calls" here: http://clojure.org/reader http://clojure.org/reader - On the positive side for the clojure reader, it isn't widely described as a serialization format and people are less inclined to use it for user input. This is a pretty important difference. - On the negative side, you can not only instantiate objects, but you can execute code. This is what the example in the article does. You can disable that by setting read-eval. - Also on the negative side, it lacks something quite as secure as YAML's safe_load. You can tell it not to evaluate code, but there isn't yet a way to disable instantiating objects. As puredanger pointed out, EDN is a better alternative that also comes with clojure. It's designed for data. Clojure's code format is a superset of EDN. http://clojure.github.com/clojure/clojure.edn-api.html http://clojure.github.com/clojure/clojure.edn-api.html
stonemetal 14y ago

lucian1900 14y ago

In short, use an explicit serialisation format (edn) instead of read. Which mirrors the advice for JavaScript of using a JSON parser instead of eval.

pjungwir 14y ago

I'm feeling vindicated for this comment now: https://news.ycombinator.com/item?id=5258993 As more people start adopting Lispy languages, it will be important to build up a cultural awareness of what's dangerous. Things like not evaling code is obvious enough (I'd hope), but avoiding read less so. That seems less like "avoid eval" and more like "avoid gets".

djacobs 14y ago
It's true that in general, people need to understand what is dangerous in each language they use. However, as far as I can tell, the problem here has nothing to do with homoiconicity. The "get string" vulnerability, as you call it, comes out of Clojure's (excellent) JVM support, and not from the Lisp side of its family tree. A "read" is not just a read only because reading Java means creating and compiling arbitrary classes and objects. That, in turn, is largely because Java is not homoiconic.
betterunix 14y ago
Keep in mind that this is a Clojure-specific issue. In Common Lisp, eval-suppressing macros should prevent this sort of thing (but I would not rely on that -- it is easy to accidentally create a reader macro that fails to check those variables). Personally, I would only use something like read if I were reading a configuration file i.e. something that only someone with privileged access can modify. I would not even consider using read for arbitrary, untrustworthy data; it is just silly in this day and age. It may make the code look prettier, but the fact that arbitrary data structures can be created and the potential to execute arbitrary code is enough to keep me away.
spacemanaki 14y ago
> That seems less like "avoid eval" and more like "avoid gets". Assuming that by `gets` you mean Ruby's `gets`, this is not quite right. `read` (and `read-string`) are not just generic IO functions. They are parsers for Clojure code. There are plenty of other varied forms of reading input to a string, but `read` does more.
- pjungwir 14y ago

fogus 14y ago

Clojure's reader is designed to provide the types of tasks useful for Lisps; specifically to take a string and convert it to data structures. However, like most Lisps Clojure has some special syntax that does various tasks at read-time like the #= tag that might not be safe to use as a webform processor, but it's critical to the way that Clojure works. In Clojure 1.5 there are EDN functions that handle a subset of the Clojure syntax and parse strings into data types like numbers, maps and vectors (and others). It's meant to deal with data only and not "reader forms". I wrote a Ring middleware to handle EDN data and recently ported it to use the new 1.5 EDN reader. https://github.com/fogus/ring-edn/blob/master/src/ring/middleware/edn.clj#L15 https://github.com/fogus/ring-edn/blob/master/src/ring/middl... It looks almost exactly like the old clojure.core/read-string except it will not execute any dangerous code.

solussd 14y ago
neat!
stouset 14y ago
Anything that can turn strings into symbols is dangerous code. Symbols can't be garbage collected, and an attacker can conduct a denial of service by forcing you to load unlimited numbers of uniquely-named symbols.
- technomancy 14y ago
  Symbols can be GCed as of 1.2.

yawnt 14y ago

this looks like an interesting alternative https://github.com/clojure/tools.reader https://github.com/clojure/tools.reader