Re: [PATCH] ob-clojure.el: Add support for babashka and nbb backend

[Max Nikulin <manikulin@gmail.com>]

On 14/11/2021 23:30, Daniel Kraus wrote:
> Max Nikulin writes:
>> On 14/11/2021 22:28, Daniel Kraus wrote:
>>> +(defun ob-clojure-escape-quotes (str-val)
>>> +  "Escape quotes for STR-VAL."
>>> +  (replace-regexp-in-string "\"" "\\\"" str-val 'FIXEDCASE 'LITERAL))
>>> +
>>> +(defun ob-clojure-eval-with-babashka (bb expanded)
>>> +  "Evaluate EXPANDED code block using BB (babashka or nbb)."
>>> +  (let ((escaped (ob-clojure-escape-quotes expanded)))
>>> +    (shell-command-to-string
>>> +     (concat bb " -e \"" escaped "\""))))
>>
>> Does not it an open door for security vulnerabilities? Consider a string
>> somewhere in the code: "`echo arbitrary code execution`". Only outer quotes are
>> escaped.
> 
> The escaping is not done for security reasons.
> When I have a babel block like
> 
> #+BEGIN_SRC clojure
> (str "foo" "bar")
> #+END_SRC
> 
> babashka has to be called with
> 
> bb -e "(str \"foo\" \"bar\")"

Enough shell constructs may be interpreted by shell inside double quotes 
before result is passed to bb. I mentioned execution of code inside 
backticks, variable substitutions are mostly undesired as well. I do not 
think, users should escape "$" inside source blocks just because you 
chose incomplete escaping of shell specials.

The following source block must not execute echo and touch

#+begin_src clojure
   (str "`echo $HOME`" "`touch /tmp/pwned`")
#+end_src

Shell should not be used to launch any command unless it is really 
necessary. Arguments should be passed directly to execve(2) system call 
as an array. Combining them into string to pass through shell 
interpreter to parse into argument array again is error prone.

Unfortunately Emacs API related to execution of external processes is 
awkward. In this particular case it encourages usage of the unsafe 
function since there is no convenient helper that accepts binary and 
*list* of arguments and returns output as a string.

So more verbose code is required to invoke bb without intermediate 
interpretation of content of argument string. In my opinion it is better 
than using of more reliable and tested function to escape shell specials.