<< Back to previous view

[CLJ-1153] Change *read-eval* default value to false Created: 30/Jan/13  Updated: 01/Mar/13  Resolved: 09/Feb/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3, Release 1.4, Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Declined Votes: 33
Labels: None

Attachments: Text File clj-1153-patch-v2.txt     Text File CLJ-1185.patch    
Patch: Code

 Description   

Discussion on the security implications of read-eval defaulting to true here: https://groups.google.com/forum/?fromgroups=#!topic/clojure/qUk-bM0JSGc

I'm not sure whether the unit test that needs read-eval true in order to pass is a sign of lots of other code that would break if read-eval defaulted to false.



 Comments   
Comment by Phil Hagelberg [ 31/Jan/13 11:55 AM ]

It's worth noting that there was a breakin to rubygems.org over the weekend that was caused by what amounts to this same issue, but with YAML: https://news.ycombinator.com/item?id=5141138

The current default is both dangerous and undocumented; this needs to change.

Comment by Soren Macbeth [ 31/Jan/13 12:33 PM ]

Seconded!

Comment by Mike Anderson [ 01/Feb/13 12:52 AM ]

Thirded.

For what it's worth, on the topic of breaking changes I'd much rather my old code break than continue to work with an unnoticed security hole.

Comment by Stuart Halloway [ 01/Feb/13 7:21 AM ]

Fourthed. I hope folks will pitch in and help deal with any breakage.

Comment by Chas Emerick [ 01/Feb/13 12:57 PM ]

New patch attached, {{CLJ-1185.patch}}.

This patch eliminates the use of *print-dup* in the compiler entirely. read-string continues to be used to support values that can only be represented via tagged reader literals; I don't think there's any way around that, since the mapping between particular data readers and values' types are buried in individual print-method implementations.

All tests pass (including cases like (eval (list + 1 2 3)) as discussed in the ML thread referenced in the issue description), and a variety of sanity checks around evaluation and compilation tooling (e.g. AOT, nREPL, introspection utilities in Leiningen/Reply/Counterclockwise) all work as expected.

Of course, if this is applied, then any usage of read over trusted content containing #= forms will need to be done with *read-eval* bound to true.

To aid in testing, a Clojure build (1.5.0-RC4) + this patch is available here:

[com.cemerick/clojure "1.5.0-SNAPSHOT"]

Note that you'll have to exclude the standard Clojure dependency from your project in order for this one to take precedence; you can do this by adding

:exclusions [org.clojure/clojure]

to your project.clj.

Comment by Paul Stadig [ 01/Feb/13 2:49 PM ]

I ran the Sonian test base against this patch. I wouldn't say our test cases are comprehensive, but they're not trivial. We talk to databases through JDBC, and we deal with lots of different data types (BigInts, Strings, Dates, and such).

I had to make a few small changes to our code base, because we rely pretty heavily on print-dup to serialize non-user originated data. We have to add corresponding binding blocks when reading to set read-eval to true. Other than that, the tests all passed.

+1

Comment by Chas Emerick [ 01/Feb/13 4:28 PM ]

Thanks to all that have tested the patch! Looks like we've made some progress. I'm going to post to the main Clojure ML now and hopefully get more yay/nay votes.

Comment by Andy Fingerhut [ 02/Feb/13 11:05 AM ]

clj-1153-patch-v2.txt dated Feb 2 2013 is functionally identical to Chas Emerick's CLJ-1185.patch dated Feb 1 2013, and no retesting is needed by anyone because of it.

The only change I have made to his patch is: the doc string for read-eval now says its default value is false.

Comment by Andy Fingerhut [ 02/Feb/13 11:06 AM ]

Also removed my original didn't-fix-enough-things patch from Jan 30 2013.

Comment by Tim McCormack [ 02/Feb/13 3:22 PM ]

I'm bumping the Priority field from its default value to "Major" to reflect recent events. (I personally feel "blocker" would be more appropriate, or at least "critical", but I don't want to step on any toes.)

Comment by Rich Hickey [ 04/Feb/13 7:55 PM ]

https://github.com/clojure/clojure/commit/974a64c06917861b7557fd73154254195b2de548

Comment by Rich Hickey [ 09/Feb/13 9:17 AM ]

This ticket is an excellent example of a terrible ticket.

1) It does not lead with a problem statement. The title takes the form akin to "I want X". It proposes a tactic.

2) The description is woefully inadequate. Here too, no problem statement. Descriptions should point to any discussions, but discussions are long and rambling, and no substitute for a succinct problem statement in the description. Descriptions need to be maintained, with the current strategy and name of best patch.

3) No resolution strategy. Just patches attached. How is a reviewer supposed to assess your patch if you don't even bother stating what the problem is and what your approach will be in fixing it, how that approach does fix it, and what if any tradeoffs are being made?

4) The change being requested would be a breaking change. That should be made extremely clear in the description, and doubles the threshold for quality of motivation and implementation.

5) Any breaking change would have to carefully enumerate what would break and when, what the migration strategy would be etc.

6) The patch breaks the compiler. If you don't understand how the compiler works, and why features are there, you shouldn't submit patches that alter it. The only assessments made in comments are 'it works for me', which, while useful anecdotes, are insufficient.

While the ticket itself was bad, the uncritical rallying behind it was more troubling. This is not how Clojure was built, and will not be how it is maintained.

In the end, the ticket proposed a tactic, and that tactic has been rejected. Read safety has been addressed by other means.

Generated at Sun Nov 23 08:50:16 CST 2014 using JIRA 4.4#649-r158309.