Clojure

Change *read-eval* default value to false

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Declined
  • Affects Version/s: Release 1.3, Release 1.4, Release 1.5
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • 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.

  1. CLJ-1185.patch
    01/Feb/13 12:57 PM
    7 kB
    Chas Emerick
  2. clj-1153-patch-v2.txt
    02/Feb/13 11:05 AM
    8 kB
    Andy Fingerhut

Activity

Hide
Phil Hagelberg added a comment -

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.

Show
Phil Hagelberg added a comment - 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.
Hide
Soren Macbeth added a comment -

Seconded!

Show
Soren Macbeth added a comment - Seconded!
Hide
Mike Anderson added a comment -

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.

Show
Mike Anderson added a comment - 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.
Hide
Stuart Halloway added a comment -

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

Show
Stuart Halloway added a comment - Fourthed. I hope folks will pitch in and help deal with any breakage.
Hide
Chas Emerick added a comment -

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.

Show
Chas Emerick added a comment - 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.
Hide
Paul Stadig added a comment -

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

Show
Paul Stadig added a comment - 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
Hide
Chas Emerick added a comment -

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.

Show
Chas Emerick added a comment - 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.
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Andy Fingerhut added a comment -

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

Show
Andy Fingerhut added a comment - Also removed my original didn't-fix-enough-things patch from Jan 30 2013.
Hide
Tim McCormack added a comment -

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.)

Show
Tim McCormack added a comment - 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.)
Hide
Rich Hickey added a comment -

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.

Show
Rich Hickey added a comment - 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.

People

Vote (33)
Watch (14)

Dates

  • Created:
    Updated:
    Resolved: