Clojure

clojure.repl/source fails when *read-eval* bound to :unknown

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: Release 1.5
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

clojure.repl/source is broken in Clojure 1.5.1 when *read-eval* is bound to :unknown, since source-fn reads without binding.

user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
RuntimeException Reading disallowed - *read-eval* bound to :unknown  clojure.lang.Util.runtimeException (Util.java:219)

Approach: Throw explicit error stating the cause in this case.

Patch: clj-1176-source-read-eval-3.patch

Screened by: Stuart Sierra

Activity

Hide
Tim McCormack added a comment -

The attached patch just binds *read-eval* to true inside source-fn.

Show
Tim McCormack added a comment - The attached patch just binds *read-eval* to true inside source-fn.
Tim McCormack made changes -
Field Original Value New Value
Attachment 0001-CLJ-1176-Bind-read-eval-true-in-clojure.repl-source-.patch [ 11895 ]
Tim McCormack made changes -
Description clojure.repl/source is broken in Clojure 1.5.0 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

Reproduce:

1. Either set {{:jvm-opts ["-Dclojure.read.eval=unknown"]}} in Leiningen or eval at REPL: {{(alter-var-root #'*read-eval* (constantly :unknown))}}
2. (use 'clojure.repl)
3. (source drop-last)

Expected:

Source of drop-last.

Actual:

RuntimeException Reading disallowed - \*read-eval\* bound to :unknown
clojure.repl/source is broken in Clojure 1.5.0 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

Reproduce:

# Either set {noformat}:jvm-opts ["-Dclojure.read.eval=unknown"]{noformat} in Leiningen or eval at REPL: {noformat}(alter-var-root #'*read-eval* (constantly :unknown)){noformat}
# {{(use 'clojure.repl)}}
# {{(source drop-last)}}

Expected:

Source of drop-last.

Actual:

RuntimeException Reading disallowed - \*read-eval\* bound to :unknown
Andy Fingerhut made changes -
Patch Code [ 10001 ]
Hide
Stuart Halloway added a comment -

Note: Allowing this implies that you trust the data on your classpath. If there are reasons somebody might not, we should reject this patch and people will have to be explicit when calling source.

Show
Stuart Halloway added a comment - Note: Allowing this implies that you trust the data on your classpath. If there are reasons somebody might not, we should reject this patch and people will have to be explicit when calling source.
Stuart Halloway made changes -
Approval Triaged [ 10120 ]
Hide
Tim McCormack added a comment -

Ugh, that's a fair point when it comes to sandboxing. I'll check with the owners of clojurebot and lazybot.

Show
Tim McCormack added a comment - Ugh, that's a fair point when it comes to sandboxing. I'll check with the owners of clojurebot and lazybot.
Rich Hickey made changes -
Approval Triaged [ 10120 ] Vetted [ 10003 ]
Rich Hickey made changes -
Fix Version/s Release 1.6 [ 10157 ]
Hide
Tim McCormack added a comment -

I haven't come up with any scenarios where this is problematic, and I haven't heard anything back from the bot owners. As for sandboxing, clojure.repl can easily be excluded.

Show
Tim McCormack added a comment - I haven't come up with any scenarios where this is problematic, and I haven't heard anything back from the bot owners. As for sandboxing, clojure.repl can easily be excluded.
Gabriel Horner made changes -
Assignee Gabriel Horner [ cldwalker ]
Gabriel Horner made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Assignee Gabriel Horner [ cldwalker ]
Hide
Gabriel Horner added a comment -

Looks good

Show
Gabriel Horner added a comment - Looks good
Hide
Alex Miller added a comment -

Would like to screen this one again. I think it has open questions and is worth a discussion somewhere.

Show
Alex Miller added a comment - Would like to screen this one again. I think it has open questions and is worth a discussion somewhere.
Alex Miller made changes -
Approval Screened [ 10004 ] Vetted [ 10003 ]
Alex Miller made changes -
Labels repl
Hide
Alex Miller added a comment -

To me, this seems like we would be opening a security hole and a cleverly concocted resource could exploit it.

Other alternatives:
1) do nothing (user can always bind read-eval around a call to source if they want to do this safely)
2) add a source-unsafe or other wrapper function that did this
3) change source-fn to use edn/read instead? This may introduce some cases where source using non-edn features could not be read. I'd be ok with that.

Show
Alex Miller added a comment - To me, this seems like we would be opening a security hole and a cleverly concocted resource could exploit it. Other alternatives: 1) do nothing (user can always bind read-eval around a call to source if they want to do this safely) 2) add a source-unsafe or other wrapper function that did this 3) change source-fn to use edn/read instead? This may introduce some cases where source using non-edn features could not be read. I'd be ok with that.
Hide
Andy Fingerhut added a comment -

Maybe this is well known to everyone already, but in case not, doing a require or use on a namespace containing the following function on a Unix-like system to create and/or update the last modification time of the file bartleby.txt. If you remove that file, and then do (source badfn) while read-eval is bound to true, you can see that it will do the shell command again. Obviously much more dangerous side effects could be performed instead of that.

(ns bad.fn)

(defn badfn [x]
  (let [y [#=(clojure.java.shell/sh "touch" "bartleby.txt")]]
    x))

Avoiding that behavior in source-fn, yet still showing the source code, would require a different implementation of read other than clojure.core/read and clojure.edn/read.

Show
Andy Fingerhut added a comment - Maybe this is well known to everyone already, but in case not, doing a require or use on a namespace containing the following function on a Unix-like system to create and/or update the last modification time of the file bartleby.txt. If you remove that file, and then do (source badfn) while read-eval is bound to true, you can see that it will do the shell command again. Obviously much more dangerous side effects could be performed instead of that.
(ns bad.fn)

(defn badfn [x]
  (let [y [#=(clojure.java.shell/sh "touch" "bartleby.txt")]]
    x))
Avoiding that behavior in source-fn, yet still showing the source code, would require a different implementation of read other than clojure.core/read and clojure.edn/read.
Hide
Alex Miller added a comment -

Based on comments on the mailing list, most people are not concerned about this from a security point of view. I'm going to let this one through and Rich can decide further.

Show
Alex Miller added a comment - Based on comments on the mailing list, most people are not concerned about this from a security point of view. I'm going to let this one through and Rich can decide further.
Alex Miller made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Description clojure.repl/source is broken in Clojure 1.5.0 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

Reproduce:

# Either set {noformat}:jvm-opts ["-Dclojure.read.eval=unknown"]{noformat} in Leiningen or eval at REPL: {noformat}(alter-var-root #'*read-eval* (constantly :unknown)){noformat}
# {{(use 'clojure.repl)}}
# {{(source drop-last)}}

Expected:

Source of drop-last.

Actual:

RuntimeException Reading disallowed - \*read-eval\* bound to :unknown
clojure.repl/source is broken in Clojure 1.5.1 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

{code}
user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: source in this context, compiling:(NO_SOURCE_PATH:1:1)
{code}

*Approach:* Have source temporarily bind *read-eval* to true while reading so it works regardless of the actual *read-eval* state.

*Patch:* 0001-CLJ-1176-Bind-read-eval-true-in-clojure.repl-source-.patch

*Screened by:* Alex Miller - my only question is whether this is ok from a security point of view. General response from questions on clojure-dev was that this was not a concern for the clojure.repl namespace which can be disallowed in sensitive environments.
Hide
Rich Hickey added a comment -

If you haven't set read-eval and you need to read-eval, then you'd better set it, right? We're not going to do that for you. The only patch that will be accepted for this is one that generates a better error message.

Show
Rich Hickey added a comment - If you haven't set read-eval and you need to read-eval, then you'd better set it, right? We're not going to do that for you. The only patch that will be accepted for this is one that generates a better error message.
Rich Hickey made changes -
Approval Screened [ 10004 ] Incomplete [ 10006 ]
Hide
Alex Miller added a comment -

Updated with new patch that detects and throws an error if calling source with read-eval is false.

Show
Alex Miller added a comment - Updated with new patch that detects and throws an error if calling source with read-eval is false.
Alex Miller made changes -
Patch Code [ 10001 ] Code and Test [ 10002 ]
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
Description clojure.repl/source is broken in Clojure 1.5.1 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

{code}
user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: source in this context, compiling:(NO_SOURCE_PATH:1:1)
{code}

*Approach:* Have source temporarily bind *read-eval* to true while reading so it works regardless of the actual *read-eval* state.

*Patch:* 0001-CLJ-1176-Bind-read-eval-true-in-clojure.repl-source-.patch

*Screened by:* Alex Miller - my only question is whether this is ok from a security point of view. General response from questions on clojure-dev was that this was not a concern for the clojure.repl namespace which can be disallowed in sensitive environments.
clojure.repl/source is broken in Clojure 1.5.1 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

{code}
user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: source in this context, compiling:(NO_SOURCE_PATH:1:1)
{code}

*Approach:* Throw explicit error stating the cause in this case.

*Patch:* clj-1176-source-read-eval-2.patch

*Screened by:*
Attachment clj-1176-source-read-eval-2.patch [ 12570 ]
Hide
Stuart Sierra added a comment - - edited

Screened OK.

Note that this only changes the error message when read-eval is bound to false, not when it is bound to :unknown.

Show
Stuart Sierra added a comment - - edited Screened OK. Note that this only changes the error message when read-eval is bound to false, not when it is bound to :unknown.
Stuart Sierra made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Stuart Sierra made changes -
Description clojure.repl/source is broken in Clojure 1.5.1 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

{code}
user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: source in this context, compiling:(NO_SOURCE_PATH:1:1)
{code}

*Approach:* Throw explicit error stating the cause in this case.

*Patch:* clj-1176-source-read-eval-2.patch

*Screened by:*
clojure.repl/source is broken in Clojure 1.5.1 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

{code}
user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: source in this context, compiling:(NO_SOURCE_PATH:1:1)
{code}

*Approach:* Throw explicit error stating the cause in this case.

*Patch:* clj-1176-source-read-eval-2.patch

*Screened by:* Stuart Sierra
Hide
Stuart Sierra added a comment - - edited

Changed my mind: resetting to 'incomplete'.

This patch doesn't fix the situation in the original report. To improve the error message, it should handle the :unknown case.

If *read-eval* is false, then source still works as long as the source form doesn't contain #=

Show
Stuart Sierra added a comment - - edited Changed my mind: resetting to 'incomplete'. This patch doesn't fix the situation in the original report. To improve the error message, it should handle the :unknown case. If *read-eval* is false, then source still works as long as the source form doesn't contain #=
Stuart Sierra made changes -
Approval Screened [ 10004 ] Incomplete [ 10006 ]
Description clojure.repl/source is broken in Clojure 1.5.1 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

{code}
user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: source in this context, compiling:(NO_SOURCE_PATH:1:1)
{code}

*Approach:* Throw explicit error stating the cause in this case.

*Patch:* clj-1176-source-read-eval-2.patch

*Screened by:* Stuart Sierra
clojure.repl/source is broken in Clojure 1.5.1 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

{code}
user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
RuntimeException Reading disallowed - *read-eval* bound to :unknown clojure.lang.Util.runtimeException (Util.java:219)
{code}

*Approach:* Throw explicit error stating the cause in this case.

*Patch:* clj-1176-source-read-eval-2.patch

*Screened by:*
Hide
Alex Miller added a comment -

Stuart - totally good catch. Things did not work how I thought they worked! I have updated the patch.

Show
Alex Miller added a comment - Stuart - totally good catch. Things did not work how I thought they worked! I have updated the patch.
Alex Miller made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
Description clojure.repl/source is broken in Clojure 1.5.1 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

{code}
user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
RuntimeException Reading disallowed - *read-eval* bound to :unknown clojure.lang.Util.runtimeException (Util.java:219)
{code}

*Approach:* Throw explicit error stating the cause in this case.

*Patch:* clj-1176-source-read-eval-2.patch

*Screened by:*
clojure.repl/source is broken in Clojure 1.5.1 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

{code}
user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
RuntimeException Reading disallowed - *read-eval* bound to :unknown clojure.lang.Util.runtimeException (Util.java:219)
{code}

*Approach:* Throw explicit error stating the cause in this case.

*Patch:* clj-1176-source-read-eval-3.patch

*Screened by:*
Attachment clj-1176-source-read-eval-3.patch [ 12706 ]
Hide
Stuart Sierra added a comment -

Screened ✔

Show
Stuart Sierra added a comment - Screened ✔
Stuart Sierra made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Description clojure.repl/source is broken in Clojure 1.5.1 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

{code}
user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
RuntimeException Reading disallowed - *read-eval* bound to :unknown clojure.lang.Util.runtimeException (Util.java:219)
{code}

*Approach:* Throw explicit error stating the cause in this case.

*Patch:* clj-1176-source-read-eval-3.patch

*Screened by:*
clojure.repl/source is broken in Clojure 1.5.1 when \*read-eval\* is bound to :unknown, since source-fn reads without binding.

{code}
user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
RuntimeException Reading disallowed - *read-eval* bound to :unknown clojure.lang.Util.runtimeException (Util.java:219)
{code}

*Approach:* Throw explicit error stating the cause in this case.

*Patch:* clj-1176-source-read-eval-3.patch

*Screened by:* Stuart Sierra
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: