[PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie]

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

Hi,
    Here is a patch that improves the ergonomics and thus hopefully
the security for the recent changes to check evaluation for cells. Full
details in the commit message on the patch. Best!
Tom

[-- Attachment #2: 0001-ob-core-add-org-confirm-babel-evaluate-cell-custom-v.patch --]
[-- Type: text/x-patch, Size: 3199 bytes --]

From da3a88e2919ec5bac6328d55eb99b3352f25b9b4 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Sat, 10 Dec 2022 12:11:17 -0800
Subject: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

* lisp/ob-core.el (org-confirm-babel-evaluate-cell): Added to control
execution of cells separate from execution of src blocks, it works in
exactly the same way as org-confirm-babel-evaluate.
* lisp/ob-core.el (org-babel-read): org-confirm-babel-evaluate-cell is
now used to check cells independent of org-confirm-babel-evaluate.

Following the change in 10e857d42859a55b23cd4206ffce3ebd0f678583 it
became extremely annoying to tangle files that make extensive use of
elisp expression in src block #+header: statements.

This commit resolves the issue by making it possible to ignore checks
on cells (the old behavior) without compromising general security for
running src blocks.

This is necessary because there is no easy way to hop swap
org-confirm-babel-evaluate between org-get-src-block-info where
org-babel-read is called and the execution of that src block. It could
probably be done using advice around org-babel-read, but that is a
level of hackery that should be avoided.
---
 lisp/ob-core.el | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index 62b0d3612..14669d940 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -128,6 +128,14 @@ remove code block execution from the `\\[org-ctrl-c-ctrl-c]' keybinding."
 ;; don't allow this variable to be changed through file settings
 (put 'org-confirm-babel-evaluate 'safe-local-variable (lambda (x) (eq x t)))
 
+(defcustom org-confirm-babel-evaluate-cell t
+  "Confirm before evaluating a cell."
+  :group 'org-babel
+  :version "29.1"
+  :type '(choice boolean function))
+;; don't allow this variable to be changed through file settings
+(put 'org-confirm-babel-evaluate-cell 'safe-local-variable (lambda (x) (eq x t)))
+
 (defcustom org-babel-no-eval-on-ctrl-c-ctrl-c nil
   "\\<org-mode-map>\
 Remove code block evaluation from the `\\[org-ctrl-c-ctrl-c]' key binding."
@@ -3180,11 +3188,14 @@ situations in which is it not appropriate."
 		  (string= cell "*this*")))
          ;; Prevent arbitrary function calls.
          (if (and (memq (string-to-char cell) '(?\( ?`))
+                  (if (functionp org-confirm-babel-evaluate-cell)
+                      (funcall org-confirm-babel-evaluate-cell cell)
+                    org-confirm-babel-evaluate-cell)
                   (not (org-babel-confirm-evaluate
-                      ;; See `org-babel-get-src-block-info'.
-                      (list "emacs-lisp" (format "%S" cell)
-                            '((:eval . yes)) nil (format "%S" cell)
-                            nil nil))))
+                        ;; See `org-babel-get-src-block-info'.
+                        (list "emacs-lisp" (format "%S" cell)
+                              '((:eval . yes)) nil (format "%S" cell)
+                              nil nil))))
              ;; Not allowed.
              (user-error "Evaluation of elisp code %S aborted." cell)
 	   (eval (read cell) t)))
-- 
2.37.4

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Max Nikulin]

On 11/12/2022 03:28, Tom Gillespie wrote:
>      Here is a patch that improves the ergonomics and thus hopefully
> the security for the recent changes to check evaluation for cells.

Tom, thank you for the patch. Frankly speaking, I was expecting this 
kind of complains, but I could not suggest any solution. I am not 
familiar with org-babel code, so my comments may be false alarms.

> * lisp/ob-core.el (org-confirm-babel-evaluate-cell): Added to control
> execution of cells separate from execution of src blocks, it works in
> exactly the same way as org-confirm-babel-evaluate.

I am not sure concerning "exactly".

lisp/ob-core.el:248
`org-confirm-babel-evaluate' is called with 2 arguments. In your patch 
`org-confirm-babel-evaluate-cell' has a single argument.

> This commit resolves the issue by making it possible to ignore checks
> on cells (the old behavior) without compromising general security for
> running src blocks.

It seems, you do not change defaults. Could you, please, provide an 
example of configuration that is less annoying, but still safe?

> This is necessary because there is no easy way to hop swap
> org-confirm-babel-evaluate between org-get-src-block-info where
> org-babel-read is called and the execution of that src block. It could
> probably be done using advice around org-babel-read, but that is a
> level of hackery that should be avoided.

I was thinking if it is possible to collect requests to confirm and to 
allow the user to decide for the whole bunch of expressions and code 
blocks. Besides implementation issues, there is a question concerning UI 
that will allow to inspect code to be evaluated.

> diff --git a/lisp/ob-core.el b/lisp/ob-core.el
...> +(defcustom org-confirm-babel-evaluate-cell t
> +  "Confirm before evaluating a cell."
Calling convention for the case of function value is not described. If 
it is really the same as for `org-confirm-babel-evaluate' then this user 
option should be mentioned in the docstring.

> +  :group 'org-babel
> +  :version "29.1"

:package-version instead of :version?

> +  :type '(choice boolean function))
> +;; don't allow this variable to be changed through file settings
> +(put 'org-confirm-babel-evaluate-cell 'safe-local-variable (lambda (x) (eq x t)))

Is there any reason to not use the :safe property of `defcustom'? I see 
that you take definition of `org-confirm-babel-evaluate' as a template 
so I wonder if there is some particular reason or the original code was 
just written before introducing of :safe.

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie]

Hi Max,
   Thank you for the feedback. More replies in lines. Best!
Tom

> I am not sure concerning "exactly".
>
> lisp/ob-core.el:248
> `org-confirm-babel-evaluate' is called with 2 arguments. In your patch
> `org-confirm-babel-evaluate-cell' has a single argument.

You're right, and in point of fact I should have retained the structure
exactly because in other contexts I have thought about ways to use
other languages in contexts like that. At the moment everything is
elisp so I dropped the argument, but that is clearly a mistake.

> It seems, you do not change defaults. Could you, please, provide an
> example of configuration that is less annoying, but still safe?

#+begin_src elisp :results none
(setq-local
 org-confirm-babel-evaluate-cell
 (lambda (lang body)
   (ignore lang)
   (let ((rb (read body)))
     (not ; aka (unless condition t)
      (or
       (member rb
               '((or)
                 (and)
                 ;; add more forms that are known safe here
                 ))
       (and
        (eq (car rb) 'identity)
        (let ((v (cadr rb)))
          (or
           (symbolp v)
           (stringp v)
           (numberp v)
           ))))))))
#+end_src

#+header: :var v1=(or) v2=(and) v3=(identity nil)
#+header: :var v4=(identity default-directory) v5=(identity #o0755)
#+header: :var v6=(identity "not sure why you would want to do this")
#+header: :var v7=(identity (concat "this" "will" "fail"))
#+header: :var v8="reminder that strings are ok"
#+begin_src elisp
(mapcar
 #'list
 (list v1 v2 v3 v4 v5 v6 v7 v8))
#+end_src

> I was thinking if it is possible to collect requests to confirm and to
> allow the user to decide for the whole bunch of expressions and code
> blocks. Besides implementation issues, there is a question concerning UI
> that will allow to inspect code to be evaluated.

Yes, in the example above I thought about including something
with a yes-or-no-p where users could quickly add forms to a
safe list some (defcustom org-known-safe-cells '()) or something
like that. A user could do that with the new machinery, and we
could do the same for the default implementation. I think that
is the next step once we get the basics in place.

> Calling convention for the case of function value is not described. If
> it is really the same as for `org-confirm-babel-evaluate' then this user
> option should be mentioned in the docstring.

When I correct the function signature to actually match
I will make a note in the docstring.

> :package-version instead of :version?

I think because org is part of emacs core we use the emacs version?
I see "24.1" included with other org defcustoms.

> Is there any reason to not use the :safe property of `defcustom'? I see
> that you take definition of `org-confirm-babel-evaluate' as a template
> so I wonder if there is some particular reason or the original code was
> just written before introducing of :safe.

I'm guessing that it was written before :safe, but don't
know for sure. A systematic cleanup of stuff like that
could come after this maybe?

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie]

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

Here is an updated version of the patch with the convention
corrected and the docstring updated for clarity. It would be
great to try to get this into any 9.6 patches before the 29
release, but I'm not sure if that is possible. If it is missing
we are likely to get a lot of messages from unhappy users.
Best!
Tom

[-- Attachment #2: 0001-ob-core-add-org-confirm-babel-evaluate-cell-custom-v.patch --]
[-- Type: text/x-patch, Size: 3281 bytes --]

From 54e468b60f17b54d81edafd8ee9e22311e519793 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Sat, 10 Dec 2022 12:11:17 -0800
Subject: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

* lisp/ob-core.el (org-confirm-babel-evaluate-cell): Added to control
execution of cells separate from execution of src blocks, it works in
exactly the same way as org-confirm-babel-evaluate.
* lisp/ob-core.el (org-babel-read): org-confirm-babel-evaluate-cell is
now used to check cells independent of org-confirm-babel-evaluate.

Following the change in 10e857d42859a55b23cd4206ffce3ebd0f678583 it
became extremely annoying to tangle files that make extensive use of
elisp expression in src block #+header: statements.

This commit resolves the issue by making it possible to ignore checks
on cells (the old behavior) without compromising general security for
running src blocks.

This is necessary because there is no easy way to hop swap
org-confirm-babel-evaluate between org-get-src-block-info where
org-babel-read is called and the execution of that src block. It could
probably be done using advice around org-babel-read, but that is a
level of hackery that should be avoided.
---
 lisp/ob-core.el | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index 62b0d3612..60dabab0a 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -128,6 +128,15 @@ remove code block execution from the `\\[org-ctrl-c-ctrl-c]' keybinding."
 ;; don't allow this variable to be changed through file settings
 (put 'org-confirm-babel-evaluate 'safe-local-variable (lambda (x) (eq x t)))
 
+(defcustom org-confirm-babel-evaluate-cell t
+  "Confirm before evaluating a cell.
+This follows the same conventions as `org-confirm-babel-evaluate'."
+  :group 'org-babel
+  :version "29.1"
+  :type '(choice boolean function))
+;; don't allow this variable to be changed through file settings
+(put 'org-confirm-babel-evaluate-cell 'safe-local-variable (lambda (x) (eq x t)))
+
 (defcustom org-babel-no-eval-on-ctrl-c-ctrl-c nil
   "\\<org-mode-map>\
 Remove code block evaluation from the `\\[org-ctrl-c-ctrl-c]' key binding."
@@ -3180,11 +3189,14 @@ situations in which is it not appropriate."
 		  (string= cell "*this*")))
          ;; Prevent arbitrary function calls.
          (if (and (memq (string-to-char cell) '(?\( ?`))
+                  (if (functionp org-confirm-babel-evaluate-cell)
+                      (funcall org-confirm-babel-evaluate-cell "emacs-lisp" cell)
+                    org-confirm-babel-evaluate-cell)
                   (not (org-babel-confirm-evaluate
-                      ;; See `org-babel-get-src-block-info'.
-                      (list "emacs-lisp" (format "%S" cell)
-                            '((:eval . yes)) nil (format "%S" cell)
-                            nil nil))))
+                        ;; See `org-babel-get-src-block-info'.
+                        (list "emacs-lisp" (format "%S" cell)
+                              '((:eval . yes)) nil (format "%S" cell)
+                              nil nil))))
              ;; Not allowed.
              (user-error "Evaluation of elisp code %S aborted." cell)
 	   (eval (read cell) t)))
-- 
2.37.4

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Kyle Meyer]

Tom Gillespie writes:

[...]
>> :package-version instead of :version?
>
> I think because org is part of emacs core we use the emacs version?

Please use :package-version and let the mapping be handled by
customize-package-emacs-version-alist.

> I see "24.1" included with other org defcustoms.

There has a been a good amount of clean up to remove :version and use
:package-version, but, yes, there are still :version instances.  Things
look good for the last Org release, though:

  $ git grep '29\.1'
  lisp/org.el:               ("9.6" . "29.1")))

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie]

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

> Please use :package-version and let the mapping be handled by
> customize-package-emacs-version-alist.

Got it. Here's the updated patch (I think I did it correctly?).

[-- Attachment #2: 0001-ob-core-add-org-confirm-babel-evaluate-cell-custom-v.patch --]
[-- Type: text/x-patch, Size: 3297 bytes --]

From 47a47aa9453a54a4f5f2e9188e2ad072a77c50cb Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Sat, 10 Dec 2022 12:11:17 -0800
Subject: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

* lisp/ob-core.el (org-confirm-babel-evaluate-cell): Added to control
execution of cells separate from execution of src blocks, it works in
exactly the same way as org-confirm-babel-evaluate.
* lisp/ob-core.el (org-babel-read): org-confirm-babel-evaluate-cell is
now used to check cells independent of org-confirm-babel-evaluate.

Following the change in 10e857d42859a55b23cd4206ffce3ebd0f678583 it
became extremely annoying to tangle files that make extensive use of
elisp expression in src block #+header: statements.

This commit resolves the issue by making it possible to ignore checks
on cells (the old behavior) without compromising general security for
running src blocks.

This is necessary because there is no easy way to hop swap
org-confirm-babel-evaluate between org-get-src-block-info where
org-babel-read is called and the execution of that src block. It could
probably be done using advice around org-babel-read, but that is a
level of hackery that should be avoided.
---
 lisp/ob-core.el | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index 62b0d3612..aa73a9cbc 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -128,6 +128,15 @@ remove code block execution from the `\\[org-ctrl-c-ctrl-c]' keybinding."
 ;; don't allow this variable to be changed through file settings
 (put 'org-confirm-babel-evaluate 'safe-local-variable (lambda (x) (eq x t)))
 
+(defcustom org-confirm-babel-evaluate-cell t
+  "Confirm before evaluating a cell.
+This follows the same conventions as `org-confirm-babel-evaluate'."
+  :group 'org-babel
+  :package-version '(Org . "9.6")
+  :type '(choice boolean function))
+;; don't allow this variable to be changed through file settings
+(put 'org-confirm-babel-evaluate-cell 'safe-local-variable (lambda (x) (eq x t)))
+
 (defcustom org-babel-no-eval-on-ctrl-c-ctrl-c nil
   "\\<org-mode-map>\
 Remove code block evaluation from the `\\[org-ctrl-c-ctrl-c]' key binding."
@@ -3180,11 +3189,14 @@ situations in which is it not appropriate."
 		  (string= cell "*this*")))
          ;; Prevent arbitrary function calls.
          (if (and (memq (string-to-char cell) '(?\( ?`))
+                  (if (functionp org-confirm-babel-evaluate-cell)
+                      (funcall org-confirm-babel-evaluate-cell "emacs-lisp" cell)
+                    org-confirm-babel-evaluate-cell)
                   (not (org-babel-confirm-evaluate
-                      ;; See `org-babel-get-src-block-info'.
-                      (list "emacs-lisp" (format "%S" cell)
-                            '((:eval . yes)) nil (format "%S" cell)
-                            nil nil))))
+                        ;; See `org-babel-get-src-block-info'.
+                        (list "emacs-lisp" (format "%S" cell)
+                              '((:eval . yes)) nil (format "%S" cell)
+                              nil nil))))
              ;; Not allowed.
              (user-error "Evaluation of elisp code %S aborted." cell)
 	   (eval (read cell) t)))
-- 
2.37.4

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Tom Gillespie <tgbugs@gmail.com> writes:

> Got it. Here's the updated patch (I think I did it correctly?).

Thanks!
You also need a NEWS entry here.

> * lisp/ob-core.el (org-confirm-babel-evaluate-cell): Added to control
> execution of cells separate from execution of src blocks, it works in
> exactly the same way as org-confirm-babel-evaluate.

Please quote `symbols' in the commit message and separate sentences with
double space.

> +;; don't allow this variable to be changed through file settings
> +(put 'org-confirm-babel-evaluate-cell 'safe-local-variable (lambda (x) (eq x t)))

You can simply use :safe keyword for `defcustom'.

I am also wondering if we should include this into bugfix.
This is a new feature, but it appears to be important enough to be
merged into built-in Org.
Bastien, Kyle, any thoughts?

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie]

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

Hi Ihor,
   Here's the updated patch using :safe, and an additional
patch for the news entry to make it easier to apply the
core change to bugfix if needed. Best!
Tom

> I am also wondering if we should include this into bugfix.

I can vouch for the fact that trying to work around this in
any other way is going to be a massive pain. For example,
it will be hard to use vanilla 29 for various CI types of things
where someone might need to execute a cell but not
want to allow arbitrary codeblocks.

[-- Attachment #2: 0001-ob-core-add-org-confirm-babel-evaluate-cell-custom-v.patch --]
[-- Type: text/x-patch, Size: 3194 bytes --]

From 4a78e1b5ea98dee569ff690037c661ab5c300194 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Sat, 10 Dec 2022 12:11:17 -0800
Subject: [PATCH 1/2] ob-core: add org-confirm-babel-evaluate-cell custom
 variable

* lisp/ob-core.el (org-confirm-babel-evaluate-cell): Added to control
execution of cells separate from execution of src blocks, it works in
exactly the same way as `org-confirm-babel-evaluate'.
* lisp/ob-core.el (org-babel-read): `org-confirm-babel-evaluate-cell'
is used to check cells independent of `org-confirm-babel-evaluate'.

Following the change in 10e857d42859a55b23cd4206ffce3ebd0f678583 it
became extremely annoying to tangle files that make extensive use of
elisp expression in src block #+header: statements.

This commit resolves the issue by making it possible to ignore checks
on cells (the old behavior) without compromising general security for
running src blocks.

This is necessary because there is no easy way to hop swap
`org-confirm-babel-evaluate' between `org-get-src-block-info' where
`org-babel-read' is called and the execution of that src block.  It
could probably be done using advice around `org-babel-read', but that
is a level of hackery that should be avoided.
---
 lisp/ob-core.el | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index 2fa9d8978..d56e47de5 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -128,6 +128,14 @@ remove code block execution from the `\\[org-ctrl-c-ctrl-c]' keybinding."
 ;; don't allow this variable to be changed through file settings
 (put 'org-confirm-babel-evaluate 'safe-local-variable (lambda (x) (eq x t)))
 
+(defcustom org-confirm-babel-evaluate-cell t
+  "Confirm before evaluating a cell.
+This follows the same conventions as `org-confirm-babel-evaluate'."
+  :group 'org-babel
+  :package-version '(Org . "9.6")
+  :type '(choice boolean function)
+  :safe (lambda (x) (eq x t)))
+
 (defcustom org-babel-no-eval-on-ctrl-c-ctrl-c nil
   "\\<org-mode-map>\
 Remove code block evaluation from the `\\[org-ctrl-c-ctrl-c]' key binding."
@@ -3180,11 +3188,14 @@ situations in which is it not appropriate."
 		  (string= cell "*this*")))
          ;; Prevent arbitrary function calls.
          (if (and (memq (string-to-char cell) '(?\( ?`))
+                  (if (functionp org-confirm-babel-evaluate-cell)
+                      (funcall org-confirm-babel-evaluate-cell "emacs-lisp" cell)
+                    org-confirm-babel-evaluate-cell)
                   (not (org-babel-confirm-evaluate
-                      ;; See `org-babel-get-src-block-info'.
-                      (list "emacs-lisp" (format "%S" cell)
-                            '((:eval . yes)) nil (format "%S" cell)
-                            nil nil))))
+                        ;; See `org-babel-get-src-block-info'.
+                        (list "emacs-lisp" (format "%S" cell)
+                              '((:eval . yes)) nil (format "%S" cell)
+                              nil nil))))
              ;; Not allowed.
              (user-error "Evaluation of elisp code %S aborted." cell)
 	   (eval (read cell) t)))
-- 
2.37.4


[-- Attachment #3: 0002-etc-ORG-NEWS-Add-entry-for-org-confirm-babel-evaluat.patch --]
[-- Type: text/x-patch, Size: 1784 bytes --]

From 03aad0a73acfca05245a01e83bae3609e6d3ed04 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Mon, 12 Dec 2022 17:45:14 -0800
Subject: [PATCH 2/2] * etc/ORG-NEWS: Add entry for
 `org-confirm-babel-evaluate-cell'.

---
 etc/ORG-NEWS | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 5d5e726e0..16ff5ba67 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -12,6 +12,42 @@ See the end of the file for license conditions.
 Please send Org bug reports to mailto:emacs-orgmode@gnu.org.
 
 * Version 9.7 (not released yet)
+
+** New options
+*** A new custom setting ~org-confirm-babel-evaluate-cell~ to control confirming execution of cells
+
+Following recent changes to ~org-babel-read~ it became annoying to
+tangle files that make extensive use of elisp expression in src
+block #+header: statements.
+
+~org-confirm-babel-evaluate-cell~ resolves the issue by making it
+possible to ignore checks on cells (the old behavior) without
+compromising general security for running src blocks.
+
+It works in the same way as ~org-confirm-babel-evaluate~, accepting a
+boolean or a function of two arguments (lang body).
+
+Here is an example that works for ~(and)~, ~(or)~, and simple calls to
+~(identity ...)~.
+
+#+begin_src emacs-lisp
+(setq-local
+ org-confirm-babel-evaluate-cell
+ (lambda (lang body)
+   (ignore lang)
+   (let ((rb (read body)))
+     (not ; aka (unless condition t)
+      (or
+       (member rb '((or) (and)))
+       (and
+        (eq (car rb) 'identity)
+        (let ((v (cadr rb)))
+          (or
+           (symbolp v)
+           (stringp v)
+           (numberp v)))))))))
+#+end_src
+
 * Version 9.6
 
 ** Important announcements and breaking changes
-- 
2.37.4

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Tom Gillespie <tgbugs@gmail.com> writes:

> Hi Ihor,
>    Here's the updated patch using :safe, and an additional
> patch for the news entry to make it easier to apply the
> core change to bugfix if needed. Best!

LGTM.

I will wait a few more days in case if other contributors have something
to say and then apply onto bugfix.

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Max Nikulin]

On 13/12/2022 08:53, Tom Gillespie wrote:
> +(defcustom org-confirm-babel-evaluate-cell t
> +  "Confirm before evaluating a cell.
> +This follows the same conventions as `org-confirm-babel-evaluate'."

Will it be clear to users what "cell" means in this context?

Am I wrong that, roughly speaking, the effective value is (and 
org-confirm-babel-evaluate-cell org-confirm-babel-evaluate)? Should it 
be described in the docstring? It seems e.g. :eval query affects the 
latter, but not the former. If it is so then it is too complicated for 
me to feel firm ground while reasoning on security.

I have not decided if it is feasible to add some property to the INFO 
argument passed to `org-babel-confirm-evaluate' and to put there 
condition which variable should determine result.

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie]

> Will it be clear to users what "cell" means in this context?

I assume the language was originally chosen
with tables in mind, but I think it is clear? The
one issue is that using org-babel-confirm-evaluate
doesn't use the word "cell" in the yes-or-no-p prompt.

> Am I wrong that, roughly speaking, the effective value is (and
> org-confirm-babel-evaluate-cell org-confirm-babel-evaluate)?

You are correct. Only in the case that
org-confirm-babel-evaluate-cell is or evaluates to t will
the yes-or-no-p be displayed _IF_ org-confirm-babel-evaluate
is or evaluates to t. org-babel-confirm-evaluate (NOT
org-confirm-babel-evaluate) is the function that issues
the yes-or-no-p.

> Should it be described in the docstring?

Maybe? In the current implementation the value of
org-confirm-babel-evaluate takes precedence, which
I think is the correct behavior. If someone has told
us that they blindly want to execute all code, but
not cells, we should not change the default behavior,
and thus we should execute all cells and blocks

> It seems e.g. :eval query affects the latter, but not the former.

:eval has no impact on cells. In fact in some files I use
:eval (and this-file-done-loading "never") on some blocks that
(setq this-file-done-loading) at the end to achieve only-once
behavior.

> If it is so then it is too complicated for me to feel firm ground
> while reasoning on security.

:eval has never, and cannot impact evaluation of cells, because
it is specifically used to control evaluation of blocks. Similarly
:eval has no interaction with any of the confirm-babel-evaluate
machinery.

I see one possible point of confusion which is that he :eval that
you see in the code of org-babel-read is a hack to create fake
info for a cell.

Are you maybe looking for a file level :eval-cell option/property
that would allow you to declare that you should NEVER run
cells at all and also never prompt? Basically this is an option
that says "treat all cells as if they and their header property
are the default value".

I'm fairly certain that implementing such a thing is a bad
idea, for example if someone sets :dir using a cell and
it is overwritten they could execute code in the wrong
environment, which can lead to nasty security issues.
Thus the only global option for cells that we could safely
implement would have the semantics of "treat all cells
as if they are (error)".

> I have not decided if it is feasible to add some property to the INFO
> argument passed to `org-babel-confirm-evaluate' and to put there
> condition which variable should determine result.

Having done so before, I would rather not touch
org-babel-confirm-evaluate if at all possible. The only
disadvantage is that for now when users are prompted
they will not be informed that it is a cell, however from
the short nature of cells the will hopefully be able to
figure it out.

The fact that org already jumps to the location of
the cell in the file helps a lot with this. Obviously
this doesn't help in a batch context, but if a user
is hitting this in batch mode they are in trouble
anyway.

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Max Nikulin]

Tom, does not the following allow to achieve the same without your patch?

#+begin_src elisp :results none
   (setq-local
    org-confirm-babel-evaluate
    (lambda (lang body)
      (not
       (and
        (member lang '("elisp" "emacs-lisp"))
        (let ((rb (read body)))
	 (or
	  (member rb
		  '((or)
		    (and)
		    ;; add more forms that are known safe here
		    ))
	  (and
	   (eq (car rb) 'identity)
	   (let ((v (cadr rb)))
	     (or
	      (symbolp v)
	      (stringp v)
	      (numberp v)
	      )))))))))
#+end_src

I know, it does not work, but I think it is due to (format "%S" cell) 
instead of passing cell directly in

-                            '((:eval . yes)) nil (format "%S" cell)

My point is that if some expression is safe for a variable value then it 
is safe for the source block body.

On 14/12/2022 04:16, Tom Gillespie wrote:
>> Will it be clear to users what "cell" means in this context?
> 
> I assume the language was originally chosen
> with tables in mind, but I think it is clear? The
> one issue is that using org-babel-confirm-evaluate
> doesn't use the word "cell" in the yes-or-no-p prompt.

Have you ever seen the prompt for a table?

I suppose, tables are the most prominent security issue related to 
unsolicited code execution:

Max Nikulin to emacs-orgmode. Re: [BUG][Security] begin_src :var 
evaluated before the prompt to confirm execution. Fri, 28 Oct 2022 
11:11:18 +0700. https://list.orgmode.org/tjfkp7$ggm$1@ciao.gmane.io

I am still in doubts if

10e857d42 2022-10-28 11:09:50 +0800 Ihor Radchenko: org-babel-read: Obey 
`org-confirm-babel-evaluate'

was an unambiguous improvement. Perhaps it just forces more users to set 
`org-confirm-babel-evaluate' to nil compromising their security to more 
severe degree.

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie]

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

Interestingly, as I was looking through my notes in
orgstrap, I see my past self had found a macro
org-babel-one-header-arg-safe-p which pointed to
defconst org-babel-safe-header-args, but that is
a const and not really user configurable. Of course
the user could cl-setf on it, but it also only checks
strings and has no ability to e.g. see if the value of
(symbol-function #'identity) has changed since the
check function was defined. Two examples.

(let ((old-identity (symbol-function #'identity)))
  (cl-letf* (((symbol-function #'identity)
              (lambda (x) (message "there") x)))
    (identity 'hello)
    (equal (symbol-function #'identity) old-identity)))

(let ((old-and (symbol-function #'and)))
  (cl-letf* (((symbol-function #'old-andf) old-and)
             ((symbol-function #'and)
              (lambda (&rest args)
                (message "oops %s" args)
                (old-andf args))))
    (list
     (and)
     (and 1 2 3)
     (equal (symbol-function #'and) old-and))))

> Tom, does not the following allow to achieve the same without your patch?

It works if I have a closed set of things I want to allow but not if I
want to set it to nil to e.g. restore the old behavior (worse for
security but not as bad as setting ocbe to nil), e.g. if I'm
under duress and need to get something that used to work to
work again without the risk of automatically running dangerous
code, (e.g. blocks that might rm something).

> I know, it does not work, but I think it is due to (format "%S" cell)
> instead of passing cell directly in
>
> -                            '((:eval . yes)) nil (format "%S" cell)
>
> My point is that if some expression is safe for a variable value then it
> is safe for the source block body.

There is another use case here, which I alluded to in the previous
comment, which is that sometimes ocbe is the last line of defense
against running dangerous code. Ideally users would have set
:eval never on blocks like that to be sure, but if they don't you
don't want someone already trying to get something to work
to get too much to work.

Again, this is focused on the ocbec -> nil case.

> Have you ever seen the prompt for a table?

Err ... maybe? So the answer is probably no.

> I suppose, tables are the most prominent security issue related to
> unsolicited code execution:

For me it would be arbitrary expressions in the headers
of source blocks. Hard to know which one is more prevalent
across the population of org users.

> Max Nikulin to emacs-orgmode. Re: [BUG][Security] begin_src :var
> evaluated before the prompt to confirm execution. Fri, 28 Oct 2022
> 11:11:18 +0700. https://list.orgmode.org/tjfkp7$ggm$1@ciao.gmane.io
>
> I am still in doubts if
>
> 10e857d42 2022-10-28 11:09:50 +0800 Ihor Radchenko: org-babel-read: Obey
> `org-confirm-babel-evaluate'
>
> was an unambiguous improvement. Perhaps it just forces more users to set
> `org-confirm-babel-evaluate' to nil compromising their security to more
> severe degree.

Heh. It is always a hard balance to strike. In the context of that
thread having a variable that would find-file-literally for untrusted
org files by default might be useful.

Again, it is a pain. I can tell you from experience having written
the system that does something similar for orgstrap. There is
no safe way other than a user-maintained whitelist based on
file hashes, everything else can be spoofed in one way or another.

I suspect that once we have the machinery in this patch in
place we can look for some sane defaults. Note that the example
function we keep passing around isn't quite good enough because
someone could probably figure out how to rewrite the identity
function so we would need to make sure that it had not changed
since emacs was loaded (unlikely, but if I can image it someone
could surely do it).

[-- Attachment #2: Type: text/html, Size: 4437 bytes --]

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Tom Gillespie <tgbugs@gmail.com> writes:

> Interestingly, as I was looking through my notes in
> orgstrap, I see my past self had found a macro
> org-babel-one-header-arg-safe-p which pointed to
> defconst org-babel-safe-header-args, but that is
> a const and not really user configurable.

It's purpose is also different.
I am generally skeptical about our ability to provide universal way to
judge if a given sexp is safe or not. It should be left to the user.

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie]

> It's purpose is also different.
> I am generally skeptical about our ability to provide universal way to
> judge if a given sexp is safe or not. It should be left to the user.

I am in complete agreement.

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[tomas]

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

On Thu, Dec 15, 2022 at 09:18:14AM +0000, Ihor Radchenko wrote:
> Tom Gillespie <tgbugs@gmail.com> writes:

[...]

> I am generally skeptical about our ability to provide universal way to
> judge if a given sexp is safe or not. It should be left to the user.

This might live in the middle of some thicket deep in the intersection
of Undecidability Plains and Ill-Definedness Swamps. Crocodiles don't
dare near there and nobody knows whether there are Dragons.

Or something :-)

Cheers
-- 
t

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> I am still in doubts if
>
> 10e857d42 2022-10-28 11:09:50 +0800 Ihor Radchenko: org-babel-read: Obey 
> `org-confirm-babel-evaluate'
>
> was an unambiguous improvement. Perhaps it just forces more users to set 
> `org-confirm-babel-evaluate' to nil compromising their security to more 
> severe degree.

Should we then extend `org-babel-check-evaluate' to accept "All" answer
in the coming bugfix release?
Then, users may conveniently allow evaluation without a push to disable
the prompt altogether.

In future release, we may go for more powerful prompt as discussed in
https://orgmode.org/list/8735cyxonl.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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Max Nikulin]

On 15/12/2022 16:10, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> I am still in doubts if
>>
>> 10e857d42 2022-10-28 11:09:50 +0800 Ihor Radchenko: org-babel-read: Obey
>> `org-confirm-babel-evaluate'
>>
>> was an unambiguous improvement. Perhaps it just forces more users to set
>> `org-confirm-babel-evaluate' to nil compromising their security to more
>> severe degree.
> 
> Should we then extend `org-babel-check-evaluate' to accept "All" answer
> in the coming bugfix release?

I would consider reverting the commit causing user prompt for every 
variable. I believe, there should be single prompt on attempt to execute 
a source block. I admit it is not easy to implement.

Main purpose of the new patch is to allow old behavior. Unfortunately it 
adds more complexity to logic around user prompts and classifying some 
expressions as safe.

I am not comfortable with attempts to consider Org as a format for web 
browser similar to HTML: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=58774
Features great for personal notebooks and authoring of documents are 
disaster for documents from non-trusted sources.

In particular, I consider the following reaction as unreasonably 
optimistic. I am afraid, a lot of work is required to achieve such goal.

https://list.orgmode.org/Y1uFDWOjZb85lk+3@protected.localdomain
Re: [BUG][Security] begin_src :var evaluated before the prompt to 
confirm execution
On 28/10/2022 14:30, Jean Louis wrote:
> * Ihor Radchenko [2022-10-28 06:19]:
>> Jean Louis writes:
>>> * Max Nikulin [2022-10-27 06:21]:
>>>> Expected result:
>>>> No code from the Org buffer and linked files is executed prior to
>>>> confirmation from the user.
>>>
>>> Should that be or is it a general policy for Org mode?
>>
>> Yes, it is a general policy.
>> Org should not execute arbitrary Elisp without confirmation, unless the
>> user customizes the confirmation query to non-default.
> 
> That is nice to know. It opens doors for browsing Org files within Emacs.

On 15/12/2022 16:10, Ihor Radchenko wrote:
> In future release, we may go for more powerful prompt as discussed in
> https://orgmode.org/list/8735cyxonl.fsf@localhost

Single prompt for whole bunch of code related to particular block was 
not discussed in that thread, that time the issue was not as sever as 
now. By the way, is it reliable to use (buffer-file-name 
(buffer-base-buffer)) in `org-confirm-babel-evaluate' to determine if 
some file resides in a "safe" directory? It may be discussed in that thread.

I believe that :var code is equally dangerous to the source block body. 
However while nobody pushes Org as a web browser format, it is better to 
implement a transparent and consistent approach to prevention of 
non-trusted code execution.

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> Should we then extend `org-babel-check-evaluate' to accept "All" answer
>> in the coming bugfix release?
>
> I would consider reverting the commit causing user prompt for every 
> variable.

I disagree. If anything, we can set the default value of
`org-confirm-babel-evaluate-cell' to nil and apply this patch.

Then, we can get the old behaviour back yet allowing concerned users to
have more security.

> ... I believe, there should be single prompt on attempt to execute 
> a source block. I admit it is not easy to implement.

This patch does not only affect src blocks. It affects all the users of
`org-babel-read'.

Note that my suggestion about "All" may as well include "All in current
block/Yes". It should not be too hard to implement, I think.

> I am not comfortable with attempts to consider Org as a format for web 
> browser similar to HTML: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=58774
> Features great for personal notebooks and authoring of documents are 
> disaster for documents from non-trusted sources.
>
> In particular, I consider the following reaction as unreasonably 
> optimistic. I am afraid, a lot of work is required to achieve such goal.
>
> https://list.orgmode.org/Y1uFDWOjZb85lk+3@protected.localdomain
> Re: [BUG][Security] begin_src :var evaluated before the prompt to 
> confirm execution

How is it related to the current discussion?
The purpose of the security feature discussed here is not for web
browsers or anything like that.

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Max Nikulin]

On 15/12/2022 19:25, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> I would consider reverting the commit causing user prompt for every
>> variable.
> 
> I disagree. If anything, we can set the default value of
> `org-confirm-babel-evaluate-cell' to nil and apply this patch.
> 
> Then, we can get the old behaviour back yet allowing concerned users to
> have more security.

I am leaving it up to you. Form my point of view it will be dead code 
that increases complexity with no practical outcome. Unfortunately 
setting `org-confirm-babel-evaluate-cell' to anything other than nil 
results in annoyance rather than security.

Perhaps advising `org-babel-execute-src-block' with `y-or-n-p' is a 
better treatment for my paranoia.

> This patch does not only affect src blocks. It affects all the users of
> `org-babel-read'.

Mostly it is called with INHIBIT-LISP-EVAL set to t.

>> https://list.orgmode.org/Y1uFDWOjZb85lk+3@protected.localdomain
>> Re: [BUG][Security] begin_src :var evaluated before the prompt to
>> confirm execution
> 
> How is it related to the current discussion?
> The purpose of the security feature discussed here is not for web
> browsers or anything like that.

I am not going to add malicious source blocks to my private org files. 
For some code it is better to have a prompt, but generally the issue is 
not excessively important. I tend to inspect org files fetched from net 
in some other application at first (browser, less, or vim).

Accidental evaluation of code is a real danger for those who insist on 
opening links to org file directly in emacs or even propose to use org 
as a kind of browser. I raised the security issue in response to 
passionate messages demanding direct ways to work with org files from 
the web. I decided to remind the context with hope that it would help to 
reevaluate severity of the issue.

I do not have a better proposal, but I think I see movement in a wrong 
direction.

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tim Cross]

Max Nikulin <manikulin@gmail.com> writes:

> On 15/12/2022 19:25, Ihor Radchenko wrote:
>> Max Nikulin writes:
>> 
>>> I would consider reverting the commit causing user prompt for every
>>> variable.
>> I disagree. If anything, we can set the default value of
>> `org-confirm-babel-evaluate-cell' to nil and apply this patch.
>> Then, we can get the old behaviour back yet allowing concerned users to
>> have more security.
>
> I am leaving it up to you. Form my point of view it will be dead code that increases
> complexity with no practical outcome. Unfortunately setting
> `org-confirm-babel-evaluate-cell' to anything other than nil results in annoyance rather
> than security.
>
> Perhaps advising `org-babel-execute-src-block' with `y-or-n-p' is a better treatment for
> my paranoia.
>
>> This patch does not only affect src blocks. It affects all the users of
>> `org-babel-read'.
>
> Mostly it is called with INHIBIT-LISP-EVAL set to t.
>
>>> https://list.orgmode.org/Y1uFDWOjZb85lk+3@protected.localdomain
>>> Re: [BUG][Security] begin_src :var evaluated before the prompt to
>>> confirm execution
>> How is it related to the current discussion?
>> The purpose of the security feature discussed here is not for web
>> browsers or anything like that.
>
> I am not going to add malicious source blocks to my private org files. For some code it is
> better to have a prompt, but generally the issue is not excessively important. I tend to
> inspect org files fetched from net in some other application at first (browser, less, or
> vim).
>

While I would argue that is a good practice, it isn't always practicable
for all users. For example, Emacs together with Emacspeak is my primary
accessible interface. While I could use a browser, it would be severely
inconvenient and frustrating.

I have to admit I also have some concerns here. These may or may not be
well founded. My biggest concern is that we don't seem to have a clear
security model. It feels like we are responding to security threats in a
piecemeal and reactive manner and without any clear model. As a result,
there is a risk we will end up with a complex solution where we don't
fully understand how all the separate pieces interact. This could result
in a complex configuration with a low level of confidence, two of
the worst attributes to have when it comes to security.




> Accidental evaluation of code is a real danger for those who insist on opening links to
> org file directly in emacs or even propose to use org as a kind of browser. I raised the
> security issue in response to passionate messages demanding direct ways to work with org
> files from the web. I decided to remind the context with hope that it would help to
> reevaluate severity of the issue.
>

My concern here is that given the power of link configuration, source
blocks, local variables, .dir-local, evaluated block header code etc, it
is extremely difficult to actually know when code will be executed.

One thing I do feel is a risk is that if we don't get the right balance,
the questions about code evaluation may fall to the level of annoying
noise which people get rid of by simply enabling all code evaluation
without question. Obviously, this would be a very bad security decision,
but as we know from years of experience, users will frequently
prioritise convenience over security. If they are unnecessarily 'nagged'
regarding code evaluation, I suspect this is what will happen (noting
that unnecessary is very subjective). 

> I do not have a better proposal, but I think I see movement in a wrong direction.

I'm not sure if I have a better proposal either. I'm not even sure if
this is solely an org issue. It could be that Emacs itself needs a
clearer or better understood and explicit security model. This seems
particularly relevant given the growth in support for downloading,
installing and running code from packages distributed by repositories
with no assessment or vetting of code. I find it quite inconsistent that
when I download and install a new theme, I have to explicitly mark it as
'safe', but I can download a package from melpa, elpa etc with no
additional checks or without having to agree to allow access to whatever
service or data it wants. 

I do wonder if it would be a good idea to try and document when org will
evaluate code in org files. This would include not just babel block
evaluation, but also elisp evaluation in table formulas, block header
arguments, file option arguments and possibly other subtle cases. This
may enable us to see if we have the granularity of controls correct or
identify inconsistencies and omissions. This information might then be
useful in defining a security model which could then identify what
controls are actually necessary and how to implement them to provide a
more straight-forward configuration for end users. It could also provide
valuable input into what additional tests may be necessary to ensure
things are working as expected. 

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Tim Cross <theophilusx@gmail.com> writes:

> I do wonder if it would be a good idea to try and document when org will
> evaluate code in org files. This would include not just babel block
> evaluation, but also elisp evaluation in table formulas, block header
> arguments, file option arguments and possibly other subtle cases. This
> may enable us to see if we have the granularity of controls correct or
> identify inconsistencies and omissions. This information might then be
> useful in defining a security model which could then identify what
> controls are actually necessary and how to implement them to provide a
> more straight-forward configuration for end users. It could also provide
> valuable input into what additional tests may be necessary to ensure
> things are working as expected. 

17.13 Code Evaluation and Security Issues

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tim Cross]

Ihor Radchenko <yantar92@posteo.net> writes:

> Tim Cross <theophilusx@gmail.com> writes:
>
>> I do wonder if it would be a good idea to try and document when org will
>> evaluate code in org files. This would include not just babel block
>> evaluation, but also elisp evaluation in table formulas, block header
>> arguments, file option arguments and possibly other subtle cases. This
>> may enable us to see if we have the granularity of controls correct or
>> identify inconsistencies and omissions. This information might then be
>> useful in defining a security model which could then identify what
>> controls are actually necessary and how to implement them to provide a
>> more straight-forward configuration for end users. It could also provide
>> valuable input into what additional tests may be necessary to ensure
>> things are working as expected. 
>
> 17.13 Code Evaluation and Security Issues

That page is a good start. However, I think it highlights why not having
a clear model limits the utility of the options being provided.

Consider this scenario -

I am a reasonably experienced Emacs user, but wouldn't describe myself
as proficient with elisp. I can do some basic stuff and I have an
init.el file which I basically understand, although there are some
borrowed bits which to be honest, I don't fully understand.

I use org mode a lot. I really like literate programming and find the
whole babel stuff really cool. I also make extensive use of tables and
the spreadsheet capability to track statistics on my projects and use
gnuplot, ditta and plantuml to generate plot, diagrams etc from that
data. Obviously, I trust all the code in these files as I wrote it!

On occasion, I get org files from colleagues. While I do basically trust
my colleagues, I don't want to just blindly allow execution of the code
in these files. I would like to check what the code does before agreeing
to allow it to run.

On very rare occasions, I get an org file from an untrusted source. In
this situation, I want iron clad assurances none of the code from this
file is executed.

Based on the information in section 17.13, how do I configure my Emacs
so that

1. All the code in the files I wrote just runs and doesn't bother me with
annoying execute questions.

2. Code in files from colleagues is shown to me and I'm asked if it
should be executed. Once I say yes, I don't want to be bothered about it
again i.e. next time I open that file, I want org mode to know I trust
it.

3. Absolutely no code in files which are not from the two preceding
sources is to be executed unless I explicitly approve it. 

How does this fictitious user achieve this configuration? Is it a
reasonable expectation that most users will be able to write the elisp
necessary to achieve this model?

It feels like what we currently have is a selection of disconnect knobs
which the user can tweak, but with no over-arching mechanism to help
manage these settings for various scenarios.

Finally, are we 100% certain that these different code evaluation
circumstances are the only ones? Can we be certain there isn't areas
where options or variables are set which couldn't be used to evaluate
code (similar to be previously identified execution of code in block
headers which occurred before the checks on code evaluation?)?  

It also feels like the approach we have taken here is almost a throwback
to a past era where he had a lot more trust. What we seem to have is
protection against accidental execution of code rather than protection
against code with malicious intent which has been crafted to be
difficult to spot and deliberately takes advantages of small 'holes' or
over-sight in the controls supplied.

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Tim Cross <theophilusx@gmail.com> writes:

> Based on the information in section 17.13, how do I configure my Emacs
> so that
>
> 1. All the code in the files I wrote just runs and doesn't bother me with
> annoying execute questions.
>
> 2. Code in files from colleagues is shown to me and I'm asked if it
> should be executed. Once I say yes, I don't want to be bothered about it
> again i.e. next time I open that file, I want org mode to know I trust
> it.
>
> 3. Absolutely no code in files which are not from the two preceding
> sources is to be executed unless I explicitly approve it. 

Not yet, but I hope that we can integrate the approach we use in
`org-safe-remote-resources' and `org--confirm-resource-safe'.

> It feels like what we currently have is a selection of disconnect knobs
> which the user can tweak, but with no over-arching mechanism to help
> manage these settings for various scenarios.

Agree. I hope that we can slowly work towards connecting these knobs.

> Finally, are we 100% certain that these different code evaluation
> circumstances are the only ones? Can we be certain there isn't areas
> where options or variables are set which couldn't be used to evaluate
> code (similar to be previously identified execution of code in block
> headers which occurred before the checks on code evaluation?)?  

No, we can't. But it is true for any software that allows third-party
code, not just for Org or even Emacs.

And we cannot use the more universal sandbox approach either. Not in
Emacs.

> It also feels like the approach we have taken here is almost a throwback
> to a past era where he had a lot more trust. What we seem to have is
> protection against accidental execution of code rather than protection
> against code with malicious intent which has been crafted to be
> difficult to spot and deliberately takes advantages of small 'holes' or
> over-sight in the controls supplied.

I do not think that we can do anything about crafted code in Emacs other
than displaying that code and letting the user decide.

And I haven't seen any better solution, except anti-virus scanners that
are constantly fighting new malicious code.

At least, we do show the code. It is already better than "yes/no"
dialogue you get when running random executable on Windows.

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tim Cross]

Ihor Radchenko <yantar92@posteo.net> writes:

> Tim Cross <theophilusx@gmail.com> writes:
>
>> Based on the information in section 17.13, how do I configure my Emacs
>> so that
>>
>> 1. All the code in the files I wrote just runs and doesn't bother me with
>> annoying execute questions.
>>
>> 2. Code in files from colleagues is shown to me and I'm asked if it
>> should be executed. Once I say yes, I don't want to be bothered about it
>> again i.e. next time I open that file, I want org mode to know I trust
>> it.
>>
>> 3. Absolutely no code in files which are not from the two preceding
>> sources is to be executed unless I explicitly approve it. 
>
> Not yet, but I hope that we can integrate the approach we use in
> `org-safe-remote-resources' and `org--confirm-resource-safe'.
>
>> It feels like what we currently have is a selection of disconnect knobs
>> which the user can tweak, but with no over-arching mechanism to help
>> manage these settings for various scenarios.
>
> Agree. I hope that we can slowly work towards connecting these knobs.
>
>> Finally, are we 100% certain that these different code evaluation
>> circumstances are the only ones? Can we be certain there isn't areas
>> where options or variables are set which couldn't be used to evaluate
>> code (similar to be previously identified execution of code in block
>> headers which occurred before the checks on code evaluation?)?  
>
> No, we can't. But it is true for any software that allows third-party
> code, not just for Org or even Emacs.
>

Yes, you are right there are no guarantees. However, I do feel that if
we are going to add this security layer, we also need to perform some
form of audit and documentation of the specific points where org
constructs will/can trigger evaluation of included code. As the bug with
rich text demonstrated, there will likely be corner cases we
miss. However, we should at least try to identify as many as possible
and document them (and include automated tests when possible).

One of the downsides regarding improved security controls is that it
raises the level of expectations. We need to try and ensure we meet
those expectations. What we really need are some good ethical hackers to
help us identify potential 'hot spots'. The ability to do this
effectively does involve a particular type of mind set and skills not
necessarily common amongst most users. 

> And we cannot use the more universal sandbox approach either. Not in
> Emacs.
>
>> It also feels like the approach we have taken here is almost a throwback
>> to a past era where he had a lot more trust. What we seem to have is
>> protection against accidental execution of code rather than protection
>> against code with malicious intent which has been crafted to be
>> difficult to spot and deliberately takes advantages of small 'holes' or
>> over-sight in the controls supplied.
>
> I do not think that we can do anything about crafted code in Emacs other
> than displaying that code and letting the user decide.
>

Agreed. What we need next is the ability to track those decisions so
that the user isn't nagged about the same things every time and the
ability to set different defaults based on some preference (such as
source/origin, location, etc. 

> And I haven't seen any better solution, except anti-virus scanners that
> are constantly fighting new malicious code.
>
> At least, we do show the code. It is already better than "yes/no"
> dialogue you get when running random executable on Windows.

Agree. The question is whether we actually do provide that y-n and
display in sufficient situations. 

Given the additional information you provided, I'm somewhat less
concerned.

These 'knobs' definitely do need some form of additional abstraction
which will more easily allow end users to set a basic policy regarding
the treatment of different org files or org files from different
sources. I suspect, in the long-term, we are likely to need some type of
file 'cookie' i.e. a local-variable setting which will either set the
policy for that file or set the origin/source/trust level etc so that whatever
security policy the user has defined can be applied. IN some ways,
similar to the 'cookie' placed into your custom variables when you say
you trust some new theme.

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie]

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

By the way, while we're on bugfixes. I just noticed that (format "%S" cell)
is being used to create the fake body for info. This is incorrect because
cell is already a string at this point and this makes the behavior
inconsistent with the rest of org babel as a result. Fix below. Best,
Tom


The fix is to pass

(list "emacs-lisp" cell
                              '((:eval . yes)) nil cell
                              nil nil)

[-- Attachment #2: Type: text/html, Size: 740 bytes --]

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie]

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

One more correction. The source of the issue is that the two values in the
list need to be different, one for the message and one for the actual test.
Best,
Tom

(list "emacs-lisp" cell
                              '((:eval . yes)) nil (format "%S" cell)
                              nil nil)

[-- Attachment #2: Type: text/html, Size: 546 bytes --]

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Tom Gillespie <tgbugs@gmail.com> writes:

> One more correction. The source of the issue is that the two values in the
> list need to be different, one for the message and one for the actual test.
> Best,
> Tom
>
> (list "emacs-lisp" cell
>                               '((:eval . yes)) nil (format "%S" cell)
>                               nil nil)

Makes sense. "%s" for printed string is even better.
Fixed on bugfix.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=7f2f73c41

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> I disagree. If anything, we can set the default value of
>> `org-confirm-babel-evaluate-cell' to nil and apply this patch.
>> 
>> Then, we can get the old behaviour back yet allowing concerned users to
>> have more security.
>
> I am leaving it up to you. Form my point of view it will be dead code 
> that increases complexity with no practical outcome. Unfortunately 
> setting `org-confirm-babel-evaluate-cell' to anything other than nil 
> results in annoyance rather than security.

To clarify, I do not consider this patch to be the end state of security
handling. I do plan to extend the code evaluation query in a way that it
can be used to allow "All" in current command call, session, + mark
files as safe, similar to `org--confirm-resource-safe'.

Just not yet in this release.

So, I don't think that the code is going to be dead.

The new variable is also in line with the existing
`org-link-elisp-confirm-function', `org-link-shell-confirm-function',
and `org-confirm-babel-evaluate'.

> Perhaps advising `org-babel-execute-src-block' with `y-or-n-p' is a 
> better treatment for my paranoia.

Emm. No? The main issue with `y-or-n-p' is that it treats <SPC> as
"yes". I have pressed <SPC> painstakingly in prompts too many times to
consider y-or-n-p safe.

>> This patch does not only affect src blocks. It affects all the users of
>> `org-babel-read'.
>
> Mostly it is called with INHIBIT-LISP-EVAL set to t.

No, not really. From skimming through grep buffer, most of the calls are
not inhibiting `eval'.

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Tom Gillespie <tgbugs@gmail.com> writes:

> From 4a78e1b5ea98dee569ff690037c661ab5c300194 Mon Sep 17 00:00:00 2001
> From: Tom Gillespie <tgbugs@gmail.com>
> Date: Sat, 10 Dec 2022 12:11:17 -0800
> Subject: [PATCH 1/2] ob-core: add org-confirm-babel-evaluate-cell custom
>  variable

Bastien, may you please take a look at this discussion?

As pointed by this patch, the current behaviour of asking users about
code evaluation every single time will increase the odds to set
`org-confirm-babel-evaluate' to t and never come back.

We may not want to do it.

Instead, we may apply this patch, but using an opposite default value of
this new variable - do not confirm.
The end result after applying will then be the same as in the previous
release.

Or we may apply this patch + extend the confirmation dialogue to accept
"All" answer to confirm execution for the duration of `this-command'.

WDYT?

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Bastien Guerry]

Ihor Radchenko <yantar92@posteo.net> writes:

> Tom Gillespie <tgbugs@gmail.com> writes:
>
>> From 4a78e1b5ea98dee569ff690037c661ab5c300194 Mon Sep 17 00:00:00 2001
>> From: Tom Gillespie <tgbugs@gmail.com>
>> Date: Sat, 10 Dec 2022 12:11:17 -0800
>> Subject: [PATCH 1/2] ob-core: add org-confirm-babel-evaluate-cell custom
>>  variable
>
> Bastien, may you please take a look at this discussion?

I've skimmed through the discussion but I'm not entirely clear about
the situation.

Has the situation changed between 9.5 and 9.6?  Tom first message
seems to suggest it did, but etc/ORG-NEWS does not say.

Whether it changed or not, what is the problem in 9.6?

How does the patch solves this problem?

Is it a temporary change while we wait for a better change?

In particular, I'm not sure I understand why this should be added to
9.6.1---I'm not opposed to it, I just try to understand.

Also, org-confirm-babel-evaluate-table-cell seems more explicit than
org-confirm-babel-evaluate-cell.

Thanks!

-- 
 Bastien

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Max Nikulin]

On 29/12/2022 22:58, Bastien Guerry wrote:
>>> From: Tom Gillespie
>>> Date: Sat, 10 Dec 2022 12:11:17 -0800
>>> Subject: [PATCH 1/2] ob-core: add org-confirm-babel-evaluate-cell custom
>>>   variable
...
> Has the situation changed between 9.5 and 9.6?

Yes, it has. In 9.6 C-c C-c for

#+begin_src elisp :var v1=(identity #o755) v2=(identity #o444) 
v3=(identity #o600)
   (list v1 v2 v3)
#+end_src

issues 4 queries if elisp code should be executed (3 variables and the 
src block body). In 9.5 variable values evaluated before the single 
prompt (so "no" did not prevent execution of some code).

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Bastien Guerry <bzg@gnu.org> writes:

> I've skimmed through the discussion but I'm not entirely clear about
> the situation.
>
> Has the situation changed between 9.5 and 9.6?  Tom first message
> seems to suggest it did, but etc/ORG-NEWS does not say.

I considered this change as a bugfix. Though it was more user-facing
than I anticipated.

What changed: The prompt previously displayed on code block evaluation
is now also displayed when expanding header arguments:

#+begin_src emacs-lisp :var x = (message "pwned!")
(concat x "foo")
#+end_src

Before Org 9.6:
1. "pwned!" is displayed
2. Query to evaluate code block is displayed

Org 9.6:
1. Query to evaluate (message "pwned!") is displayed
2. If confirmed, it is evaluated
3. Query to evaluate the whole code block is displayed

> Whether it changed or not, what is the problem in 9.6?

The problem is that Org now displays more queries.

> How does the patch solves this problem?

It allows disabling these new queries about lisp evaluation outside
code blocks without disabling code block eval confirmation completely.

I later suggested disabling the queries by default - mimicking the pre
9.6 behaviour yet keeping the ability for concerned users enable the
extra confirmation.

> Is it a temporary change while we wait for a better change?

Yes. Ideally, we need to improve the code evaluation query. It should
allow confirming evaluation in bulk and add some code blocks/files to
whitelist. Similar to `org--confirm-resource-safe'.

> In particular, I'm not sure I understand why this should be added to
> 9.6.1---I'm not opposed to it, I just try to understand.

A concern have been expressed that more queries may annoy users and
drive them towards setting `org-confirm-babel-evaluate' to nil globally.
Upon doing this, the future more flexible security queries may be not
used by such users.

> Also, org-confirm-babel-evaluate-table-cell seems more explicit than
> org-confirm-babel-evaluate-cell.

But it will not be accurate. The query is now displayed upon executing
`org-babel-read' -- cell refers to Elisp code "cell" here. Not to a
table cell.

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Bastien]

Thanks a lot for the detailed answer, very helpful.

> What changed: The prompt previously displayed on code block evaluation
> is now also displayed when expanding header arguments:

I see nothing in etc/ORG-NEWS that describes this change: am I missing
something?

>> Whether it changed or not, what is the problem in 9.6?
>
> The problem is that Org now displays more queries.
>
>> How does the patch solves this problem?
>
> It allows disabling these new queries about lisp evaluation outside
> code blocks without disabling code block eval confirmation completely.

Is there a real use-case for this?  

It looks like the patch fixes the "too many queries" problem by
providing a setup that is similar to what was the previous default
(i.e. only asking about code block evaluation when
org-confirm-babel-evaluate-cell is set to nil.)

But are we sure that users need to confirm header args evaluation
separately from code blocks evaluation?

IOW I have difficulties understanding when these would be relevant:

(setq org-confirm-babel-evaluate-cell nil)
(setq org-confirm-babel-evaluate t)

> I later suggested disabling the queries by default - mimicking the pre
> 9.6 behaviour yet keeping the ability for concerned users enable the
> extra confirmation.

I think that's the best route: providing, optionnally, more, while not
annoying users who don't want more.

> Yes. Ideally, we need to improve the code evaluation query. It should
> allow confirming evaluation in bulk and add some code blocks/files to
> whitelist. Similar to `org--confirm-resource-safe'.

I see, indeed.

> A concern have been expressed that more queries may annoy users and
> drive them towards setting `org-confirm-babel-evaluate' to nil globally.
> Upon doing this, the future more flexible security queries may be not
> used by such users.

The future more flexible security queries will be made visibile enough
so that users currently setting `org-confirm-babel-evaluate' to nil
will *want* to have a look at it.

>> Also, org-confirm-babel-evaluate-table-cell seems more explicit than
>> org-confirm-babel-evaluate-cell.
>
> But it will not be accurate. The query is now displayed upon executing
> `org-babel-read' -- cell refers to Elisp code "cell" here. Not to a
> table cell.

I find "cell" confusing here (as Max said earlier in the thread).  It
either refers to a table cell or, in Elisp jargon, a "cons cell" (or a
"function cell").

What about modifying `org-confirm-babel-evaluate' to allow these values:

- t      : confirm header vars *and* code blocks
- 'code  : confirm code blocks
- 'vars  : confirm vars
- nil    : don't confirm

and set the default value to 'code, while allowing concerned users to
set it to `t' -- until we have a better system for evaluation query.

WDYT?

-- 
 Bastien

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Max Nikulin]

On 30/12/2022 15:52, Bastien wrote:
> But are we sure that users need to confirm header args evaluation
> separately from code blocks evaluation?

I do not think we need to confirm header arguments *separately*, but 
they should not be executed before asking users. It is not easy to 
implement request for header arguments in another way.

Commit

10e857d42 2022-10-28 11:09:50 +0800 Ihor Radchenko: org-babel-read: Obey 
`org-confirm-babel-evaluate'

was a reaction to

Max Nikulin. [BUG][Security] begin_src :var evaluated before the prompt 
to confirm execution. Thu, 27 Oct 2022 10:18:05 +0700. 
https://list.orgmode.org/tjct9e$179u$1@ciao.gmane.io

The latter partially was caused by demand to open arbitrary Org files 
downloaded from net in Emacs.

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=58774
[WISH]: Let us make EWW browse WWW Org files correctly

Accidental unsolicited code execution due to unintentional C-c C-c may 
be rather dangerous, it does not depend if it is source block body, 
header arguments, or table formula (the latter may still be activated by 
just TAB). Org-9.5 behavior is not ideal but at least acceptable for 
most of trusted (e.g. private) Org files.

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie]

Hi Bastien,
   In short, we need two variables due to the case where
a user wants to set nil for all vars and a function for code.
More replies in line. Best!
Tom

> I see nothing in etc/ORG-NEWS that describes this change: am I missing
> something?

Looks like any mention of the change is missing to me as well.

> >> Whether it changed or not, what is the problem in 9.6?
> >
> > The problem is that Org now displays more queries.
> >
> >> How does the patch solves this problem?
> >
> > It allows disabling these new queries about lisp evaluation outside
> > code blocks without disabling code block eval confirmation completely.

> Is there a real use-case for this?

Yes. To give one example. I have many files that need to run hundreds
of cells at tangling/export time. I have reviewed all the code in the cells
and know patterns that are safe and wont' burn me. I do not want to
allow execution of code blocks blindly during tangling and confirm eval
is a secondary line of defense against running those blocks during
export. I want to allow the cells to run uninhibited as they did before
this change, so that I don't have to fight with org during the tangling
process.

As it is right now the change has completely broken various CI
pipelines for me, and if these changes do not get in to emacs 29
in time I will be unable to use emacs 29 for CI until they are in the
base image without having to resort to insane hacks and massive
additional configuration changes to work around the issues discussed
in the loading multiple org versions thread.

> It looks like the patch fixes the "too many queries" problem by
> providing a setup that is similar to what was the previous default
> (i.e. only asking about code block evaluation when
> org-confirm-babel-evaluate-cell is set to nil.)

This interpretation is not quite right. org-confirm-babel-evaluate-cell has
no interaction with evaluating code blocks, only with evaluating cells.
When org-confirm-babel-evaluate-cell is nil or evaluates to nil then the
org-babel-confirm-evaluate code runs on a fake block that is created
out of the cell. (This is where there is still a bug that I mentioned up
thread.)

> But are we sure that users need to confirm header args evaluation
> separately from code blocks evaluation?

I am sure that in my workflow I want them split, especially for
global nil/t behavior. I need to think a bit more about your
suggestion to add more values to org-confirm-babel-evaluate
to see whether it might work and what the relative complexity
would be.

It seems to me that duplicating the variable is certainly easier
to maintain, if not to explain to those who do not use babel
regularly. I would be interested to hear from other babel users
their thoughts on the design, because it seems obvious to me,
but again I wrote the code so am the last person whose view
on the clarity of the should be considered. It seems that it
was not clear to Max initially.

> IOW I have difficulties understanding when these would be relevant:

> (setq org-confirm-babel-evaluate-cell nil)

Anywhere that a parenthesized elisp expression occurs
that is not in #+begin_src block.

> (setq org-confirm-babel-evaluate t)

Only inside #+begin_src blocks.

> I think that's the best route: providing, optionnally, more, while not
> annoying users who don't want more.

I think this is probably the right default as well.

> > Yes. Ideally, we need to improve the code evaluation query. It should
> > allow confirming evaluation in bulk and add some code blocks/files to
> > whitelist. Similar to `org--confirm-resource-safe'.
>
> I see, indeed.

Improving general code auditing prior to evaluation of blocks would
be greatly appreciated. The fighting between the minibuffer and
the primary buffer for priority and the fact that you often cannot
see that code that will be run is a significant issue. However, this is
orthogonal to the issue at hand.

> > A concern have been expressed that more queries may annoy users and
> > drive them towards setting `org-confirm-babel-evaluate' to nil globally.
> > Upon doing this, the future more flexible security queries may be not
> > used by such users.
>
> The future more flexible security queries will be made visibile enough
> so that users currently setting `org-confirm-babel-evaluate' to nil
> will *want* to have a look at it.

Yep. We need to get the fundamentals in place to enable those
workflows. I'm too paranoid to simply set that variable to nil, even
when I'm only running code that I wrote, but I suppose that is not
the case for everyone.

> I find "cell" confusing here (as Max said earlier in the thread).  It
> either refers to a table cell or, in Elisp jargon, a "cons cell" (or a
> "function cell").

I also dislike the name cell, but i'm not sure what to call it instead.
I mentioned "parenthized elisp expression" above, since strictly
speaking they aren't closures, they are any valid elisp sexpression.

In the context of org-outside-emacs I imagine a day when those
top level expressions might also be written in some other language
and there would be some way to indicate at a file or section level
or perhaps even single block expression level what language a given
cell should be interpreted as.

In the context of naming this means that "parentized expression"
might not be general enough. Maybe embedded-code or
embedded-expression? org-confirm-babel-evaluate-embedded-expressions?

> What about modifying `org-confirm-babel-evaluate' to allow these values:
>
> - t      : confirm header vars *and* code blocks
> - 'code  : confirm code blocks
> - 'vars  : confirm vars
> - nil    : don't confirm
>
> and set the default value to 'code, while allowing concerned users to
> set it to `t' -- until we have a better system for evaluation query.
>
> WDYT?

In short this is not a viable solution because there is no way
to compose nil for embedded expressions with a function
for blocks. We really do not want to change the function
signature for the function to also have to accept whether
it is a block or an embedded expression. That will break
code for everyone.

The solution I have proposed keeps blocks and embedded
expressions orthogonal precisely to avoid these kinds of
major disruptions.

Better for users to  be confused about how to use a variable
but be able to learn about it or ask about it than for all users
of a feature to have to suffer major breakages to their existing
workflows and have to rewrite existing code to work around
something that supposedly increased security.

Having two separate variables is the only way to retain
backward compatibility and give granular control over
evaluation of code blocks and embedded expressions.

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Tom Gillespie <tgbugs@gmail.com> writes:

>> What about modifying `org-confirm-babel-evaluate' to allow these values:
>>
>> - t      : confirm header vars *and* code blocks
>> - 'code  : confirm code blocks
>> - 'vars  : confirm vars
>> - nil    : don't confirm
>>
>> and set the default value to 'code, while allowing concerned users to
>> set it to `t' -- until we have a better system for evaluation query.
>>
>> WDYT?
>
> In short this is not a viable solution because there is no way
> to compose nil for embedded expressions with a function
> for blocks. We really do not want to change the function
> signature for the function to also have to accept whether
> it is a block or an embedded expression. That will break
> code for everyone.

Agree. The value of `org-confirm-babel-evaluate' can be a function. If
we do not change the function signature, there will be no way to provide
the code block vs. code cell granularity we are trying to handle.

We may, however, make `org-confirm-babel-evaluate' function value accept
an extra third argument - context ('code or 'vars). This will retain the
required flexibility without introducing an extra variable.

P.S. Considering intense discussion around the topic, what about
reverting my commit from the release? We can then re-consider the whole
design and apply something more elaborate later.

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie]

> P.S. Considering intense discussion around the topic, what about
> reverting my commit from the release? We can then re-consider the whole
> design and apply something more elaborate later.

I was actually going to mention that in my previous
message but forgot. I think that given the potential
for disruption and our desire to do a good job on the
design of the new system I think it would make sense
to revert the commit from the release so that we don't
rush in trying to fix something under pressure without
taking the time we need to consider all the angles.

Best!
Tom

[SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable)

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> P.S. Considering intense discussion around the topic, what about
> reverting my commit from the release? We can then re-consider the whole
> design and apply something more elaborate later.

I now reverted the discussed commit.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=345402148

We need to come up with a uniform security system where all the code
evaluation is (1) easy to control via variables and user queries; (2)
not too annoying for users; (3) provides the necessary context for users
to decide if a code is safe to run.

Before we continue the discussion, I will try to summarize the current
state of Org manual and code wrt code evaluation of the code coming from
Org documents (including downloaded Org files).

In the manual we now have
17.13 Code Evaluation and Security Issues

This section talks about

1. Source block bodies and `org-confirm-babel-evaluate'
2. shell/elisp links and `org-link-shell-confirm-function' +
   `org-link-elisp-confirm-function'.

   Notably, `org-link-elisp-skip-confirm-regexp' is not mentioned in the
   manual.

3. Table cells, with no way to query or disable execution.

In reality, we have many more places in the code where arbitrary text
from Org files can be evaluated.

I have marked some places in the above commit with FIXME.

From my audit of Org sources the following scenarios may lead to
arbitrary code execution:

1. Code block bodies
2. Elisp sexps in header args. Notably, not only `org-babel-read' is
   responsible for evaluating header args. :results and :exports are
   evaluated independently.
3. Table cells
4. "eval" macros during expansion
5. Diary sexps
   
To the best of my knowledge, this list should be complete. Though I
would appreciate someone double-checking.

--------------------
According to the above:

- `org-confirm-babel-evaluate' allows either using
  `org-babel-confirm-evaluate' prompt (when t), not asking at all (when
  nil), or using arbitrary prompt function.
- `org-link-shell-confirm-function' + `org-link-elisp-confirm-function'
  are similar, except defaulting to `yes-or-no-p'.
- `org-link-elisp-skip-confirm-regexp' extends indiscriminate function
  queries to also skip certain (single) regexp.

The situation is not ideal.

We have similar, but more detailed system for downloading remote files:

- `org-safe-remote-resources' allows users to define certain files/urls
  that are known to be safe
- `org-resource-download-policy' provides choice between "always query",
  "query unless safe", "never download", and "always download"

Also, `org--confirm-resource-safe', unlike `org-babel-confirm-evaluate'
allows manipulating `org-safe-remote-resources' interactively by marking
current URL or URL domain safe for future.

------------------------
What we can do

1. Introduce a new customization `org-confirm-evaluate-safe-regexps'
   listing regexps that are considered safe or cons cells
   (src-body/header-arg/table/macro/diary . regexp)

2. Introduce a new customization `org-confirm-evaluate' that can be set
   to t/nil/prompt/safe/[function]/[alist]:
   - t/safe :: to display default prompt unless the code block in buffer is
               marked safe
   - prompt :: always ask (the prompt will then not display options to
               add current sexp to safe list)
   - [function] :: custom function that returns t/safe/prompt/[alist]
   - [alist] :: (src-body/header-arg/table/macro/diary/t .
                 t/safe/prompt/function)
                 (t . <value>) stands for default value.

3. The default prompt will mimic `org--confirm-resource-safe',
   additionally accepting Y/N to accept/decline all the evaluation in
   current command.

This system will be used across Org for code evaluation. Old variables
will be supported, but obsoleted.

WDYT?

-- 
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: [SECURITY] Arbitrary code evaluation security in Org

[Greg Minshall]

Ihor,

thanks for this.

one additional item (i don't *think* we discussed this before; apologies
if i'm forgetting): tangling.  if one is prompted to "merely" tangle ...
----
#+begin_src sh :tangle /var/tmp/foo.org.tangled
  echo 'hi!'
#+end_src
----

one could imagine more sinister scenarios for destination, content.

i don't really know what, how much, to do.  possibly just an option,
defaulting to =nil=, allowing tangle to write a file outside the subtree
that holds the .org file being tangled.

cheers, Greg

[SECURITY] Tangling can overwrite arbitrary tangling targets, including important user files (was: [SECURITY] Arbitrary code evaluation security in Org)

[Ihor Radchenko]

Greg Minshall <minshall@umich.edu> writes:

> one additional item (i don't *think* we discussed this before; apologies
> if i'm forgetting): tangling.  if one is prompted to "merely" tangle ...
> ----
> #+begin_src sh :tangle /var/tmp/foo.org.tangled
>   echo 'hi!'
> #+end_src
> ----
>
> one could imagine more sinister scenarios for destination, content.
>
> i don't really know what, how much, to do.  possibly just an option,
> defaulting to =nil=, allowing tangle to write a file outside the subtree
> that holds the .org file being tangled.

Good point. Though not directly related to code execution.

In this particular case, we might be able to utilize Emacs' file
dialogues. For example, `write-file' can ask about overwriting an
existing file.

-- 
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: [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable)

[Tim Cross]

Ihor Radchenko <yantar92@posteo.net> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> P.S. Considering intense discussion around the topic, what about
>> reverting my commit from the release? We can then re-consider the whole
>> design and apply something more elaborate later.
>
> I now reverted the discussed commit.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=345402148
>
> We need to come up with a uniform security system where all the code
> evaluation is (1) easy to control via variables and user queries; (2)
> not too annoying for users; (3) provides the necessary context for users
> to decide if a code is safe to run.
>
> Before we continue the discussion, I will try to summarize the current
> state of Org manual and code wrt code evaluation of the code coming from
> Org documents (including downloaded Org files).
>
> In the manual we now have
> 17.13 Code Evaluation and Security Issues
>
> This section talks about
>
> 1. Source block bodies and `org-confirm-babel-evaluate'
> 2. shell/elisp links and `org-link-shell-confirm-function' +
>    `org-link-elisp-confirm-function'.
>
>    Notably, `org-link-elisp-skip-confirm-regexp' is not mentioned in the
>    manual.
>
> 3. Table cells, with no way to query or disable execution.
>
> In reality, we have many more places in the code where arbitrary text
> from Org files can be evaluated.
>
> I have marked some places in the above commit with FIXME.
>
> From my audit of Org sources the following scenarios may lead to
> arbitrary code execution:
>
> 1. Code block bodies
> 2. Elisp sexps in header args. Notably, not only `org-babel-read' is
>    responsible for evaluating header args. :results and :exports are
>    evaluated independently.
> 3. Table cells
> 4. "eval" macros during expansion
> 5. Diary sexps
>    
> To the best of my knowledge, this list should be complete. Though I
> would appreciate someone double-checking.
>
> --------------------
> According to the above:
>
> - `org-confirm-babel-evaluate' allows either using
>   `org-babel-confirm-evaluate' prompt (when t), not asking at all (when
>   nil), or using arbitrary prompt function.
> - `org-link-shell-confirm-function' + `org-link-elisp-confirm-function'
>   are similar, except defaulting to `yes-or-no-p'.
> - `org-link-elisp-skip-confirm-regexp' extends indiscriminate function
>   queries to also skip certain (single) regexp.
>
> The situation is not ideal.
>
> We have similar, but more detailed system for downloading remote files:
>
> - `org-safe-remote-resources' allows users to define certain files/urls
>   that are known to be safe
> - `org-resource-download-policy' provides choice between "always query",
>   "query unless safe", "never download", and "always download"
>
> Also, `org--confirm-resource-safe', unlike `org-babel-confirm-evaluate'
> allows manipulating `org-safe-remote-resources' interactively by marking
> current URL or URL domain safe for future.
>
> ------------------------
> What we can do
>
> 1. Introduce a new customization `org-confirm-evaluate-safe-regexps'
>    listing regexps that are considered safe or cons cells
>    (src-body/header-arg/table/macro/diary . regexp)
>
> 2. Introduce a new customization `org-confirm-evaluate' that can be set
>    to t/nil/prompt/safe/[function]/[alist]:
>    - t/safe :: to display default prompt unless the code block in buffer is
>                marked safe
>    - prompt :: always ask (the prompt will then not display options to
>                add current sexp to safe list)
>    - [function] :: custom function that returns t/safe/prompt/[alist]
>    - [alist] :: (src-body/header-arg/table/macro/diary/t .
>                  t/safe/prompt/function)
>                  (t . <value>) stands for default value.
>
> 3. The default prompt will mimic `org--confirm-resource-safe',
>    additionally accepting Y/N to accept/decline all the evaluation in
>    current command.
>
> This system will be used across Org for code evaluation. Old variables
> will be supported, but obsoleted.
>
> WDYT?

Hi Ihor,

this looks promising. However, there is a lot to be considered here and
it requires some thought before any meaningful feedback can be provided.

The big challenge here will be in getting sufficient fine grained
control that all use cases can be catered for while also providing a
high level interface suitable for the 'general' case which is both
reasonably easy to understand at the user level and flexible enough for
a default configuration.

For many users who don't share org files, who work on a single user
desktop and who implicitly trust the files they are working with, it is
unlikely they want to be bothered by any of this. It should 'just
work'. I suspect this is the majority. Others will have some sharing of
documents - perhaps in a work group, a class or some form of team
activity. The trust here is likely fairly high but perhaps not
absolute. There is probably a need for some choice on execution on a per
file basis. Finally, there are those org files which are from unknown
and therefore untrusted sources - email messages, cloned git
repositories, Emacs packages, etc. Most people likely don't want these
executed by default and want to be asked or be able to block by default.

The point is, we probably need to consider not only what variables are
required, but also some infrastructure for managing those variables
based on some form of document classification or trust. You touch on
this with some of what has been outlined, but it focuses on the original
data source. That information can be lost once a file has been
retrieved. However, the trust level of that file likely doesn't change
just because it now resides 'locally'.

I do wonder if the idea of a document classification model and some form
of heuristic algorithms to handle default document classification might
be useful. The idea is similar to other security systems which classify
documents/files and tags those files with their classification whereby
other services (email, http, sftp, etc) implement policies which impose
restrictions on files based on the tag. While this is a different use
case, I do wonder if such idea might help here e.g. the default setting
for various variables controlling code execution are set based on the
tag of the file, which in turn is set based on the heuristic for that
file. For example, *.org files created in Emacs by the user/owner might
have the highest 'trust' tag while those which originate in Email or
from external links might have the lowest unless the link/source is
flagged as trustworthy.

We could have a mechanism whereby you define a trust 'tag' which in turn
defines the various variables associated with control of code
execution. We would then define a mechanism for defining a 'rule' to
classify org files, allowing users to define their own heuristics and
define a base set of tags and rules which define default behaviour.

The tags could be a header variable or an emacs file local variable
etc. There could be rules triggered for files opened without a tag or a
mechanism defined for a default tag (or perhaps a function which could,
as one option, ask the user when the file is first opened and no tag
exists etc). Of course, the devil is in the details. For example, how do
we handle the case where you and I are sharing a file and we want
different tags? (we would likely need some form of simple 'signing' for
tags and ability to have multiple tags per file or forced 'review' of
tags not originating form the user etc).

Somewhat related, if Timothy is reading this thread, I wonder if some
questions in the next Emacs survey might be useful (or if an org
specific survey would be useful) in order to get some real data on user
use patterns and data sources etc. It is possible that the use case for
org files with untrusted code is so small that none of this is actually
necessary and we could just handle it by making the risks very explicit
and leave the user to decide on how they want to mitigate the
risks. There is one school of thought which would state that if you are
unable to define a reliable and understandable security model which
works, your better off just educating the user base and leaving it to
them to manage in whatever manner is best suitable for their
situation. A model which is hard to understand or which doens't work
runs the risk of creating a false sense of security and may do more harm
than no model and a big red sticker which says "Beware of the tiger!". 

Re: [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable)

[Ihor Radchenko]

Tim Cross <theophilusx@gmail.com> writes:

>> 1. Introduce a new customization `org-confirm-evaluate-safe-regexps'
>>    listing regexps that are considered safe or cons cells
>>    (src-body/header-arg/table/macro/diary . regexp)
>>
>> 2. Introduce a new customization `org-confirm-evaluate' that can be set
>>    to t/nil/prompt/safe/[function]/[alist]:
>>    - t/safe :: to display default prompt unless the code block in buffer is
>>                marked safe
>>    - prompt :: always ask (the prompt will then not display options to
>>                add current sexp to safe list)
>>    - [function] :: custom function that returns t/safe/prompt/[alist]
>>    - [alist] :: (src-body/header-arg/table/macro/diary/t .
>>                  t/safe/prompt/function)
>>                  (t . <value>) stands for default value.
>>
>> 3. The default prompt will mimic `org--confirm-resource-safe',
>>    additionally accepting Y/N to accept/decline all the evaluation in
>>    current command.
>>
>> This system will be used across Org for code evaluation. Old variables
>> will be supported, but obsoleted.
>>
>> WDYT?
>
> For many users who don't share org files, who work on a single user
> desktop and who implicitly trust the files they are working with, it is
> unlikely they want to be bothered by any of this. It should 'just
> work'. I suspect this is the majority. Others will have some sharing of
> documents - perhaps in a work group, a class or some form of team
> activity. The trust here is likely fairly high but perhaps not
> absolute. There is probably a need for some choice on execution on a per
> file basis. Finally, there are those org files which are from unknown
> and therefore untrusted sources - email messages, cloned git
> repositories, Emacs packages, etc. Most people likely don't want these
> executed by default and want to be asked or be able to block by default.

> The point is, we probably need to consider not only what variables are
> required, but also some infrastructure for managing those variables
> based on some form of document classification or trust. You touch on
> this with some of what has been outlined, but it focuses on the original
> data source. That information can be lost once a file has been
> retrieved. However, the trust level of that file likely doesn't change
> just because it now resides 'locally'.

Oops. I think I forgot to describe another point I was thinking about.

REGEXP in `org-confirm-evaluate-safe-regexps' may also be a cons cell
(SOURCE . REGEXP). SOURCE can be a file path, a directory containing the
file, or file contents hash. This way, we can mark whole files or
directories as safe. Or just specific code blocks in files or
directories.

> I do wonder if the idea of a document classification model and some form
> of heuristic algorithms to handle default document classification might
> be useful.

I do not think that we need to go in this direction.
I doubt that we are qualified to get the heuristics right.
Such things should either be maintained in Emacs core or not provided at
all to not create false sense of security.

Look at Emacs' approach to file-local variables.
There are no heuristics there.

-- 
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: [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable)

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

>> I do wonder if the idea of a document classification model and some form
>> of heuristic algorithms to handle default document classification might
>> be useful.
>
> I do not think that we need to go in this direction.
> I doubt that we are qualified to get the heuristics right.
> Such things should either be maintained in Emacs core or not provided at
> all to not create false sense of security.

And I was wrong.

There is `unsafep' and `safe-functions' customization, which we can
utilize.

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Bastien Guerry]

Ihor Radchenko <yantar92@posteo.net> writes:

> We may, however, make `org-confirm-babel-evaluate' function value accept
> an extra third argument - context ('code or 'vars). This will retain the
> required flexibility without introducing an extra variable.

Yes, this is an interesting possibility.

> P.S. Considering intense discussion around the topic, what about
> reverting my commit from the release? We can then re-consider the whole
> design and apply something more elaborate later.

I see you did the revert already: this is fine by me.

Thanks,

-- 
 Bastien

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Ihor Radchenko]

Bastien Guerry <bzg@gnu.org> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> We may, however, make `org-confirm-babel-evaluate' function value accept
>> an extra third argument - context ('code or 'vars). This will retain the
>> required flexibility without introducing an extra variable.
>
> Yes, this is an interesting possibility.

See my other email where I suggested a more elaborate approach.


>> P.S. Considering intense discussion around the topic, what about
>> reverting my commit from the release? We can then re-consider the whole
>> design and apply something more elaborate later.
>
> I see you did the revert already: this is fine by me.

👍

-- 
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: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Bastien]

Hi Tom,

thanks a lot for the detailed explanations: I get your point and I
understand the need.  I think the revert is a good move, though, as
the solution was not really good enough.

Thanks!

-- 
 Bastien

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Kyle Meyer]

Ihor Radchenko writes:

> I am also wondering if we should include this into bugfix.
> This is a new feature, but it appears to be important enough to be
> merged into built-in Org.
> Bastien, Kyle, any thoughts?

Sounds reasonable to me.

Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Max Nikulin]

On 12/12/2022 03:27, Tom Gillespie wrote:
> 
> #+begin_src elisp :results none
> (setq-local
>   org-confirm-babel-evaluate-cell
>   (lambda (lang body)
>     (ignore lang)
>     (let ((rb (read body)))
>       (not ; aka (unless condition t)
>        (or
>         (member rb
>                 '((or)
>                   (and)
>                   ;; add more forms that are known safe here
>                   ))

Thank you, Tom. At first I thought you managed to define a function that 
treats particular directories as safe (to discriminate own files and 
files having non-trusted origins). However in such case you would not 
need additional user option and `org-confirm-babel-evaluate' would be 
enough.

Walking through the passed expression to prove that it has no side 
effects is an interesting and challenging problem.

And frankly speaking, I was confused and believed that it is 
responsibility of the new function to issue e.g. `yes-or-no-p' prompt. 
Now I see that it is `org-babel-confirm-evaluate' that shows such prompt.