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

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

[Max Nikulin]

Consider the following Org file

---- 8< ----
#+begin_src elisp :results none
   (require 'ob-sqlite)
#+end_src

#+begin_src sqlite :db /tmp/ob.sqlite$(date >/tmp/ob-sqlite-vuln.log)
   select 1
#+end_src
---- >8 ----

Executing of the sqlite code block causes creation of the 
/tmp/ob-sqlite-vuln.log file.

The cause is usage of `org-fill-template' without `shell-quote-argument'.

 From my point of view it is unsafe to open Org files from untrusted 
sources in Emacs in general, so it is not a serious vulnerability. Some 
users may consider shell expansion in file name as a convenient feature. 
However earlier we had a quite similar issue:

lux. [PATCH] Fix ob-latex.el command injection vulnerability. Sat, 18 
Feb 2023 18:08:44 +0800. 
https://list.orgmode.org/tencent_7B48D6A8D4FCDC2DC8DF842B069B715ECE0A@qq.com

that is known as CVE-2023-28617 with high enough score 
"org-babel-execute:latex in ob-latex.el in Org Mode through 9.6.1 for 
GNU Emacs allows attackers to execute arbitrary commands via a file name 
or directory name that contains shell metacharacters."

and caused updates of Emacs in various Linux distributions
https://security-tracker.debian.org/tracker/CVE-2023-28617

As to `org-fill-template', it may be affected by an issue similar to
Maxim Nikulin. greedy substitution in org-open-file. Wed, 20 Jan 2021 
23:08:35 +0700.
https://list.orgmode.org/ru9ki4$t5e$1@ciao.gmane.io

since expansion of a %key may contain %another that might be 
interpolated on next iteration. The function should perform substitution 
during single scan of the passed template.

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

[Ihor Radchenko]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

Max Nikulin <manikulin@gmail.com> writes:

> ---- 8< ----
> #+begin_src elisp :results none
>    (require 'ob-sqlite)
> #+end_src
>
> #+begin_src sqlite :db /tmp/ob.sqlite$(date >/tmp/ob-sqlite-vuln.log)
>    select 1
> #+end_src
> ---- >8 ----
>
> Executing of the sqlite code block causes creation of the 
> /tmp/ob-sqlite-vuln.log file.
>
> The cause is usage of `org-fill-template' without `shell-quote-argument'.

Confirmed.

This is clearly very common.
What do you think about creating a new API to built shell commands and
then using it across all the babel backends?

See the attached tentative diff.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: tentative-shell-escape-api.diff --]
[-- Type: text/x-patch, Size: 3010 bytes --]

diff --git a/lisp/ob-sqlite.el b/lisp/ob-sqlite.el
index 7510e5158..27e495fce 100644
--- a/lisp/ob-sqlite.el
+++ b/lisp/ob-sqlite.el
@@ -77,26 +77,20 @@ (defun org-babel-execute:sqlite (body params)
     (with-temp-buffer
       (insert
        (org-babel-eval
-	(org-fill-template
-	 "%cmd %header %separator %nullvalue %others %csv %db "
-	 (list
-	  (cons "cmd" org-babel-sqlite3-command)
-	  (cons "header" (if headers-p "-header" "-noheader"))
-	  (cons "separator"
-		(if separator (format "-separator %s" separator) ""))
-	  (cons "nullvalue"
-		(if nullvalue (format "-nullvalue %s" nullvalue) ""))
-	  (cons "others"
-		(mapconcat
-		 (lambda (arg) (format "-%s" (substring (symbol-name arg) 1)))
-		 others " "))
-	  ;; for easy table parsing, default header type should be -csv
-	  (cons "csv" (if (or (member :csv others) (member :column others)
-			      (member :line others) (member :list others)
-			      (member :html others) separator)
-			  ""
-			"-csv"))
-          (cons "db" (or db ""))))
+        (org-make-shell-command
+         org-babel-sqlite3-command
+         (if headers-p "-header" "-noheader")
+         (when separator (format "-separator %s" separator))
+         (when nullvalue (format "-nullvalue %s" nullvalue))
+         (mapcar
+	  (lambda (arg) (format "-%s" (substring (symbol-name arg) 1)))
+	  others)
+         ;; for easy table parsing, default header type should be -csv
+         (unless (or (member :csv others) (member :column others)
+		     (member :line others) (member :list others)
+		     (member :html others) separator)
+	   "-csv")
+         db)
 	;; body of the code block
 	(org-babel-expand-body:sqlite body params)))
       (org-babel-result-cond result-params
diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index 442c607d7..3c92c9405 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -1592,6 +1592,33 @@ (defun org-sxhash-safe (obj &optional counter)
 	  (puthash hash obj org-sxhash-objects)
 	  (puthash obj hash org-sxhash-hashes)))))
 
+(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.
+
+ARGS can be nil, strings, (literal STRING), or a list of such elements.
+Strings will be quoted with `shell-quote-argument' while
+(literal STRING) will be used without quoting.
+nil values will be ignored."
+  (concat
+   command (when command " ")
+   (mapconcat
+    #'identity
+    (delq
+     nil
+     (mapcar
+      (lambda (str-def)
+        (pcase str-def
+          (`(or nil "") nil)
+          ((pred stringp) (shell-quote-argument str-def))
+          (`(literal ,(and (pred stringp) str))
+           str)
+          ((pred listp) (apply #'org-make-shell-command nil str-def))
+          (_ (error "Unknown ARG specification: %S" str-def))))
+      args))
+    " ")))
+
 (defun org-compile-file (source process ext &optional err-msg log-buf spec)
   "Compile a SOURCE file using PROCESS.
 

[-- Attachment #3: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

[Max Nikulin]

On 13/08/2023 14:52, Ihor Radchenko wrote:
> What do you think about creating a new API to built shell commands and
> then using it across all the babel backends?

I support the idea in general, but not its particular implementation as 
`org-make-shell-command' in your patch.

It does not address the issue I raised.

#+begin_src sqlite :db '(literal "/tmp/ob.sqlite$(date 
 >/tmp/ob-sqlite-vuln.log)")
   select 1
#+end_src

still executes a shell command without user prompt. Moreover for 
org-babel such value does not look like as something that may be 
evaluated, it is just a list. So the proposed syntax is more explicit 
(and I like it), but it does not prevent unsolicited execution of shell 
command.

I would consider some way to specify whether COMMAND should be quoted as 
well. Path to an executable may contain a space or other special 
character at least for some shells. On the other hand it is more usual 
case to specify some arguments to the command.

I am unsure if a note should be added to the `org-fill-template' 
docstring that the function should not be used for building shell commands.

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

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> On 13/08/2023 14:52, Ihor Radchenko wrote:
>> What do you think about creating a new API to built shell commands and
>> then using it across all the babel backends?
>
> I support the idea in general, but not its particular implementation as 
> `org-make-shell-command' in your patch.
>
> It does not address the issue I raised.
>
> #+begin_src sqlite :db '(literal "/tmp/ob.sqlite$(date 
>  >/tmp/ob-sqlite-vuln.log)")
>    select 1
> #+end_src

Handling lisp values in header arguments is much more general issue not
tied to ob-sql or even to running shell commands.

It should be addressed alongside with https://orgmode.org/list/87edsd5o89.fsf@localhost

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

[Max Nikulin]

On 18/08/2023 15:43, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> #+begin_src sqlite :db '(literal "/tmp/ob.sqlite$(date
>>   >/tmp/ob-sqlite-vuln.log)")
>>     select 1
>> #+end_src
> 
> Handling lisp values in header arguments is much more general issue not
> tied to ob-sql or even to running shell commands.
> 
> It should be addressed alongside with https://orgmode.org/list/87edsd5o89.fsf@localhost

Ihor, this is a list, not an expression to be evaluated. There are some 
conditions to avoid user prompts for strings, lists, etc. They are 
considered safe.

This particular case is handled namely by ob-sqlite and the proposed 
function in org-macs.

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

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> Ihor, this is a list, not an expression to be evaluated. There are some 
> conditions to avoid user prompts for strings, lists, etc. They are 
> considered safe.
>
> This particular case is handled namely by ob-sqlite and the proposed 
> function in org-macs.

Do you have any ideas how to work around the deliberately constructed
header argument values like in your example?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

[Max Nikulin]

On 18/08/2023 18:05, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> Ihor, this is a list, not an expression to be evaluated. There are some
>> conditions to avoid user prompts for strings, lists, etc. They are
>> considered safe.
>>
>> This particular case is handled namely by ob-sqlite and the proposed
>> function in org-macs.
> 
> Do you have any ideas how to work around the deliberately constructed
> header argument values like in your example?

Perhaps `gensym' may be used to create a symbol that can not appear in a 
document. I am unsure if the following `pcase' variant may be improved

(`(,(and s (guard (eq s ob-literal-symbol))) ,(and (pred stringp) str))
  str)

for

;; or ob-shell-argument-literal-symbol
(defconst ob-literal-symbol (gensym "literal"))

I hope, list values can not be used to bypass escaping with such 
approach. It is still possible to use evaluated expressions, but user 
prompt for such cases should be fixed anyway.

P.S. Babel backends should be consistent in respect to treating options 
for header arguments:
- use as is
- expand ~user and $VAR
- allow any shell expression

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

[Ihor Radchenko]

[-- Attachment #1: Type: text/plain, Size: 576 bytes --]

Max Nikulin <manikulin@gmail.com> writes:

>> Do you have any ideas how to work around the deliberately constructed
>> header argument values like in your example?
>
> Perhaps `gensym' may be used to create a symbol that can not appear in a 
> document. I am unsure if the following `pcase' variant may be improved
> ...
> ;; or ob-shell-argument-literal-symbol
> (defconst ob-literal-symbol (gensym "literal"))

Good idea.
I am attaching tentative fix that uses the proposed approach.
Not yet merging - need to go through other babel backends and make them
use the new API.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-macs-New-common-API-function-to-quote-shell-argu.patch --]
[-- Type: text/x-patch, Size: 2241 bytes --]

From dfc03c0330b96ff4fbe14df39ba895427b8fd004 Mon Sep 17 00:00:00 2001
Message-ID: <dfc03c0330b96ff4fbe14df39ba895427b8fd004.1692601432.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Mon, 21 Aug 2023 09:57:50 +0300
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.
(org-make-shell-command): New function that returns shell command
built from individual shell arguments, escaping them to prevent
malicious code execution.

Link: https://orgmode.org/list/ub549k$q11$1@ciao.gmane.io
---
 lisp/org-macs.el | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index 907e8bed7..95af9e45e 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -1593,6 +1593,37 @@ (defun org-sxhash-safe (obj &optional counter)
 	  (puthash hash obj org-sxhash-objects)
 	  (puthash obj hash org-sxhash-hashes)))))
 
+(defconst org-shell-arg-literal (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.
+
+ARGS can be nil, strings, (LITERAL STRING), or a list of
+such elements.  LITERAL must be the value of `org-shell-arg-literal'.
+
+Strings will be quoted with `shell-quote-argument' while \(literal
+STRING) will be used without quoting.  nil values will be ignored."
+  (concat
+   command (when command " ")
+   (mapconcat
+    #'identity
+    (delq
+     nil
+     (mapcar
+      (lambda (str-def)
+        (pcase str-def
+          (`(or nil "") nil)
+          ((pred stringp) (shell-quote-argument str-def))
+          (`(,(pred (eq org-shell-arg-literal)) ,(and (pred stringp) str))
+           str)
+          ((pred listp) (apply #'org-make-shell-command nil str-def))
+          (_ (error "Unknown ARG specification: %S" str-def))))
+      args))
+    " ")))
+
 (defun org-compile-file (source process ext &optional err-msg log-buf spec)
   "Compile a SOURCE file using PROCESS.
 
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-org-babel-execute-sqlite-Fix-shell-arg-expansion-vul.patch --]
[-- Type: text/x-patch, Size: 2633 bytes --]

From d7a8dd47aa06e715b6bb213914d43f973c6cb413 Mon Sep 17 00:00:00 2001
Message-ID: <d7a8dd47aa06e715b6bb213914d43f973c6cb413.1692601432.git.yantar92@posteo.net>
In-Reply-To: <dfc03c0330b96ff4fbe14df39ba895427b8fd004.1692601432.git.yantar92@posteo.net>
References: <dfc03c0330b96ff4fbe14df39ba895427b8fd004.1692601432.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Mon, 21 Aug 2023 09:59:12 +0300
Subject: [PATCH 2/2] org-babel-execute:sqlite: Fix shell arg expansion
 vulnerability

* lisp/ob-sqlite.el (org-babel-execute:sqlite): Use
`org-make-shell-command' to escape the strings taken from Org file.
This will prevent abusing shell expansion.

Reported-by: Max Nikulin <manikulin@gmail.com>
Link: https://orgmode.org/list/ub549k$q11$1@ciao.gmane.io
---
 lisp/ob-sqlite.el | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/lisp/ob-sqlite.el b/lisp/ob-sqlite.el
index 7510e5158..27e495fce 100644
--- a/lisp/ob-sqlite.el
+++ b/lisp/ob-sqlite.el
@@ -77,26 +77,20 @@ (defun org-babel-execute:sqlite (body params)
     (with-temp-buffer
       (insert
        (org-babel-eval
-	(org-fill-template
-	 "%cmd %header %separator %nullvalue %others %csv %db "
-	 (list
-	  (cons "cmd" org-babel-sqlite3-command)
-	  (cons "header" (if headers-p "-header" "-noheader"))
-	  (cons "separator"
-		(if separator (format "-separator %s" separator) ""))
-	  (cons "nullvalue"
-		(if nullvalue (format "-nullvalue %s" nullvalue) ""))
-	  (cons "others"
-		(mapconcat
-		 (lambda (arg) (format "-%s" (substring (symbol-name arg) 1)))
-		 others " "))
-	  ;; for easy table parsing, default header type should be -csv
-	  (cons "csv" (if (or (member :csv others) (member :column others)
-			      (member :line others) (member :list others)
-			      (member :html others) separator)
-			  ""
-			"-csv"))
-          (cons "db" (or db ""))))
+        (org-make-shell-command
+         org-babel-sqlite3-command
+         (if headers-p "-header" "-noheader")
+         (when separator (format "-separator %s" separator))
+         (when nullvalue (format "-nullvalue %s" nullvalue))
+         (mapcar
+	  (lambda (arg) (format "-%s" (substring (symbol-name arg) 1)))
+	  others)
+         ;; for easy table parsing, default header type should be -csv
+         (unless (or (member :csv others) (member :column others)
+		     (member :line others) (member :list others)
+		     (member :html others) separator)
+	   "-csv")
+         db)
 	;; body of the code block
 	(org-babel-expand-body:sqlite body params)))
       (org-babel-result-cond result-params
-- 
2.41.0


[-- Attachment #4: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

[Max Nikulin]

On 21/08/2023 14:04, Ihor Radchenko wrote:
> +(defconst org-shell-arg-literal (gensym "literal")

(opinion) Perhaps a better name exists. Maybe 
org-shell-arg-tag-unescaped (or unquoted)

> +  "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.
> +
> +ARGS can be nil, strings, (LITERAL STRING), or a list of

(opinion) I would give an example

     `(,ob-shell-arg-literal STRING)

to avoid "LITERAL" that is confusing from my point of view. Perhaps it 
is better to describe its purpose more clearly: prevent raw shell 
constructs in ob header arguments in Org documents unless they appear in 
evaluated expressions.

> +such elements.  LITERAL must be the value of `org-shell-arg-literal'.
> +
> +Strings will be quoted with `shell-quote-argument' while \(literal
> +STRING) will be used without quoting.  nil values will be ignored."
> +  (concat
> +   command (when command " ")
> +   (mapconcat
> +    #'identity
> +    (delq
> +     nil
> +     (mapcar
> +      (lambda (str-def)
> +        (pcase str-def
> +          (`(or nil "") nil)

An empty string may be an important argument. E.g.

     read -r -d "" var

allows to read values separated by null character (\0), e.g. from output 
of find -print0. That is why I would leave just nil.

> +         (when separator (format "-separator %s" separator))

Sorry, I made a typo previous time. It would not work. -separator and 
the separator character must *not* be combined into single argument. 
Sqlite does not support it.

     (and separator `("-separator" ,separator))

or (format "%s" separator) if it may have a type other than string.

I hope, this approach does not have unnoticed flaws.

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

[Ihor Radchenko]

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]

Max Nikulin <manikulin@gmail.com> writes:

> On 21/08/2023 14:04, Ihor Radchenko wrote:
>> +(defconst org-shell-arg-literal (gensym "literal")
>
> (opinion) Perhaps a better name exists. Maybe 
> org-shell-arg-tag-unescaped (or unquoted)
> ...

See the updated version of the patches attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-macs-New-common-API-function-to-quote-shell-argu.patch --]
[-- Type: text/x-patch, Size: 2580 bytes --]

From 6909d6165df11bbc256a334488d37ce0ef98523e Mon Sep 17 00:00:00 2001
Message-ID: <6909d6165df11bbc256a334488d37ce0ef98523e.1692697539.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Mon, 21 Aug 2023 09:57:50 +0300
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.
(org-make-shell-command): New function that returns shell command
built from individual shell arguments, escaping them to prevent
malicious code execution.

Link: https://orgmode.org/list/ub549k$q11$1@ciao.gmane.io
---
 lisp/org-macs.el | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index 907e8bed7..73f8b59f9 100644
--- a/lisp/org-macs.el
+++ 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
+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)))
+
+will shell-escape \"-l\", \"value with spaces\", and each non-nil member of
+FILES list, but leave \"$HOME\" to be expanded."
+  (concat
+   command (when command " ")
+   (mapconcat
+    #'identity
+    (delq
+     nil
+     (mapcar
+      (lambda (str-def)
+        (pcase str-def
+          (`nil nil)
+          ((pred stringp) (shell-quote-argument str-def))
+          (`(,(pred (eq org-shell-arg-tag-unescaped)) ,(and (pred stringp) str))
+           str)
+          ((pred listp) (apply #'org-make-shell-command nil str-def))
+          (_ (error "Unknown ARG specification: %S" str-def))))
+      args))
+    " ")))
+
 (defun org-compile-file (source process ext &optional err-msg log-buf spec)
   "Compile a SOURCE file using PROCESS.
 
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-org-babel-execute-sqlite-Fix-shell-arg-expansion-vul.patch --]
[-- Type: text/x-patch, Size: 2623 bytes --]

From db0300d18b7d2986eddd4869b73f5702fb429e93 Mon Sep 17 00:00:00 2001
Message-ID: <db0300d18b7d2986eddd4869b73f5702fb429e93.1692697539.git.yantar92@posteo.net>
In-Reply-To: <6909d6165df11bbc256a334488d37ce0ef98523e.1692697539.git.yantar92@posteo.net>
References: <6909d6165df11bbc256a334488d37ce0ef98523e.1692697539.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Mon, 21 Aug 2023 09:59:12 +0300
Subject: [PATCH 2/2] org-babel-execute:sqlite: Fix shell arg expansion
 vulnerability

* lisp/ob-sqlite.el (org-babel-execute:sqlite): Use
`org-make-shell-command' to escape the strings taken from Org file.
This will prevent abusing shell expansion.

Reported-by: Max Nikulin <manikulin@gmail.com>
Link: https://orgmode.org/list/ub549k$q11$1@ciao.gmane.io
---
 lisp/ob-sqlite.el | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/lisp/ob-sqlite.el b/lisp/ob-sqlite.el
index 7510e5158..027f0a72d 100644
--- a/lisp/ob-sqlite.el
+++ b/lisp/ob-sqlite.el
@@ -77,26 +77,20 @@ (defun org-babel-execute:sqlite (body params)
     (with-temp-buffer
       (insert
        (org-babel-eval
-	(org-fill-template
-	 "%cmd %header %separator %nullvalue %others %csv %db "
-	 (list
-	  (cons "cmd" org-babel-sqlite3-command)
-	  (cons "header" (if headers-p "-header" "-noheader"))
-	  (cons "separator"
-		(if separator (format "-separator %s" separator) ""))
-	  (cons "nullvalue"
-		(if nullvalue (format "-nullvalue %s" nullvalue) ""))
-	  (cons "others"
-		(mapconcat
-		 (lambda (arg) (format "-%s" (substring (symbol-name arg) 1)))
-		 others " "))
-	  ;; for easy table parsing, default header type should be -csv
-	  (cons "csv" (if (or (member :csv others) (member :column others)
-			      (member :line others) (member :list others)
-			      (member :html others) separator)
-			  ""
-			"-csv"))
-          (cons "db" (or db ""))))
+        (org-make-shell-command
+         org-babel-sqlite3-command
+         (if headers-p "-header" "-noheader")
+         (when separator (list "-separator" separator))
+         (when nullvalue (list "-nullvalue" nullvalue))
+         (mapcar
+	  (lambda (arg) (format "-%s" (substring (symbol-name arg) 1)))
+	  others)
+         ;; for easy table parsing, default header type should be -csv
+         (unless (or (member :csv others) (member :column others)
+		     (member :line others) (member :list others)
+		     (member :html others) separator)
+	   "-csv")
+         db)
 	;; body of the code block
 	(org-babel-expand-body:sqlite body params)))
       (org-babel-result-cond result-params
-- 
2.41.0


[-- Attachment #4: Type: text/plain, Size: 225 bytes --]



-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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?

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

[Ihor Radchenko]

[-- Attachment #1: Type: text/plain, Size: 2251 bytes --]

Max Nikulin <manikulin@gmail.com> writes:

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

Thanks for the feedback!
I have updated the patch, except for the comments I reply to below.

>> +  `(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.

The idea was to highlight that `files' is a list.
I now changed this to

 files ; list variable

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

Good idea. I also converted `org-make-shell-command' into defsubst that
cannot be reliably adviced. To reduce attack vectors further.

>> +will shell-escape \"-l\", \"value with spaces\", and each non-nil member of
>
> There is nothing to escape in "-l".

I deliberately list all the arguments, detailing which are escaped and
which are not.

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

>> -	(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?

I don't think so. `org-fill-template' is usually not used to build shell
command. ob-sqlite is the only instance of such use in Org code. Other
backends use different Elisp means to build shell command strings. So,
adding warning to `org-fill-template' docstring will not achieve much.

The new version of the org-macs patch attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-macs-New-common-API-function-to-quote-shell-argu.patch --]
[-- Type: text/x-patch, Size: 3070 bytes --]

From 9e0128b205f568795d8c4688a7a94c175b1b2007 Mon Sep 17 00:00:00 2001
Message-ID: <9e0128b205f568795d8c4688a7a94c175b1b2007.1693295856.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Mon, 21 Aug 2023 09:57:50 +0300
Subject: [PATCH] org-macs: New common API function to quote shell arguments

* lisp/org-macs.el (org-shell-arg-tag-unescaped): New auxiliary
constant.
(org-make-shell-command): New function that returns shell command
built from individual shell arguments, escaping them to prevent
malicious code execution.

Link: https://orgmode.org/list/ub549k$q11$1@ciao.gmane.io
---
 lisp/org-macs.el | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index 907e8bed7..6bcd393ce 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -1593,6 +1593,57 @@ (defun org-sxhash-safe (obj &optional counter)
 	  (puthash hash obj org-sxhash-objects)
 	  (puthash obj hash org-sxhash-hashes)))))
 
+;; We use `gensym' to avoid malicious code know in advance the symbol
+;; used to prevent escaping.
+(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'.")
+;; We are deliberately using `defsubst' below, to make it harder to
+;; advice this function.
+(defsubst org-shell-arg-unescaped (string-arg)
+  "Mark STRING-ARG argument to be unescaped in `org-make-shell-command'."
+  (list org-shell-arg-tag-unescaped string-arg))
+(defsubst 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, the return value of (org-shell-arg-unescaped
+STRING), or a 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-unescaped \"$HOME\")
+     files ; list variable
+     ))
+
+will shell-escape \"-l\", \"value with spaces\", and each non-nil member of
+FILES list, but leave \"$HOME\" to be shell-expanded.
+
+COMMAND itself can contain shell expansion constructs - no escaping
+will be performed."
+  (concat
+   command (when command " ")
+   (mapconcat
+    #'identity
+    (delq
+     nil
+     (mapcar
+      (lambda (str-def)
+        (pcase str-def
+          (`nil nil)
+          ((pred stringp) (shell-quote-argument str-def))
+          (`(,(pred (eq org-shell-arg-tag-unescaped)) ,(and (pred stringp) str))
+           str)
+          ((pred listp) (apply #'org-make-shell-command nil str-def))
+          (_ (error "Unknown ARG specification: %S" str-def))))
+      args))
+    " ")))
+
 (defun org-compile-file (source process ext &optional err-msg log-buf spec)
   "Compile a SOURCE file using PROCESS.
 
-- 
2.42.0


[-- Attachment #3: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

[SECURITY] Shell expansion of babel header args (was: [BUG][SECURITY] ob-sqlite header args allows execution of arbitrary shell commands)

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> P.S. Babel backends should be consistent in respect to treating options 
> for header arguments:
> - use as is
> - expand ~user and $VAR
> - allow any shell expression

We cannot generally know which header arg values can or cannot be
shell-expanded. It is something only individual babel backends can know.

However, there are frequently used header arguments like :cmd, where it
does make sense to allow shell expansion. But we may need to safeguard
them behind user prompt for safety, similar to what has to be done for
Elisp evaluation.

We can allow backends to specify "safety" of the header argument value
similar to how we now define the allowed values in
`org-babel-common-header-args-w-values'. Then, babel can prompt for user
confirmation every time "unsafe" argument value is encountered.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

[Max Nikulin]

On 13/08/2023 14:52, Ihor Radchenko wrote:
> +         (when separator (format "-separator %s" separator))

It should be escaped as a whole. It seems sqlite3 command line arguments 
parser does not support -separator=| variant, so two words should be 
quoted separately. Try :separator |

sqlite3: Error: unknown option: -separator |
Use -help for a list of options.
[ Babel evaluation exited with code 1 ]