Re: [BUG][SECURITY] ob-sqlite header args allows execution of arbitrary shell commands

[Max Nikulin]

On 22/08/2023 16:46, Ihor Radchenko wrote:
> See the updated version of the patches attached.

Thank you, I do not see apparent issues with code any more. Commit 
message needs an update, apostrophes in the doc string should be 
escaped. Feel free to ignore other comments since there are other issues 
and investing excessive time into polishing of this one is not reasonable.

> Subject: [PATCH 1/2] org-macs: New common API function to quote shell
>   arguments
>
> * lisp/org-macs.el (org-shell-arg-literal): New auxiliary constant.
                      ^^^^^^^^^^^^^^^^^^^^^
You have changed its name.

> (org-make-shell-command): New function that returns shell command
> built from individual shell arguments, escaping them to prevent
> malicious code execution.
...
> +++ b/lisp/org-macs.el
> @@ -1593,6 +1593,46 @@ (defun org-sxhash-safe (obj &optional counter)
>           (puthash hash obj org-sxhash-objects)
>           (puthash obj hash org-sxhash-hashes)))))
>   
> +(defconst org-shell-arg-tag-unescaped (gensym "literal")
> +  "Symbol to be used to mark shell arguments that should not be escaped.
> +See `org-make-shell-command'.")
> +(defun org-make-shell-command (command &rest args)
> +  "Build safe shell command string to run COMMAND with ARGS.
> +
> +The resulting shell command is safe against malicious shell expansion.
> +
> +This function is used to avoid unexpected shell expansion when
> +building shell command using header arguments from Org babel blocks.
> +
> +ARGS can be nil, strings, `(,org-shell-arg-tag-unescaped STRING), or a

add \\= before ` and ', otherwise help formatter makes them "pretty".

> +list of such elements.  For example,
> +
> + (let ((files '(\"a.txt\" \"b.txt\" nil \"$HOME.txt\")))
> +  `(org-make-shell-command \"command\" \"-l\"
> +      \"value with spaces\"
> +      (,org-shell-arg-tag-unescaped \"$HOME\")
> +      (mapcar #'identity files)))

Is `mapcar' necessary here? Anyway `delq' is called on another result of 
`mapcar', so the function should not do any destructive list modification.

An idea that may be ignored: make the constant internal and add
(defsubst org-make-shell-command-unescaped (arg)
 (list org--shell-arg-tag-unescaped arg))

to avoid `, noise in `(,org-shell-arg-tag-unescaped STRING).

> +will shell-escape \"-l\", \"value with spaces\", and each non-nil member of

There is nothing to escape in "-l".

Perhaps it deserves a mention that COMMAND is passed unquoted to be 
suitable for commands with arguments as defcustom user option values. To 
escape it pass nil as fist argument and add COMMAND before ARGS.

> +FILES list, but leave \"$HOME\" to be expanded."

...by shell.

> Subject: [PATCH 2/2] org-babel-execute:sqlite: Fix shell arg expansion
>   vulnerability
>
> -       (org-fill-template

Should an explicit warning be added to `org-fill-template' that enough 
care is required to escape values if it is used to build a shell command?