* [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable
@ 2022-12-10 20:28 Tom Gillespie
2022-12-11 2:58 ` Max Nikulin
0 siblings, 1 reply; 49+ messages in thread
From: Tom Gillespie @ 2022-12-10 20:28 UTC (permalink / raw)
To: emacs-orgmode
[-- 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
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-10 20:28 [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable Tom Gillespie @ 2022-12-11 2:58 ` Max Nikulin 2022-12-11 20:27 ` Tom Gillespie 0 siblings, 1 reply; 49+ messages in thread From: Max Nikulin @ 2022-12-11 2:58 UTC (permalink / raw) To: emacs-orgmode 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-11 2:58 ` Max Nikulin @ 2022-12-11 20:27 ` Tom Gillespie 2022-12-11 20:37 ` Tom Gillespie ` (2 more replies) 0 siblings, 3 replies; 49+ messages in thread From: Tom Gillespie @ 2022-12-11 20:27 UTC (permalink / raw) To: Max Nikulin; +Cc: emacs-orgmode 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? ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-11 20:27 ` Tom Gillespie @ 2022-12-11 20:37 ` Tom Gillespie 2022-12-11 20:46 ` Kyle Meyer 2022-12-13 16:15 ` Max Nikulin 2 siblings, 0 replies; 49+ messages in thread From: Tom Gillespie @ 2022-12-11 20:37 UTC (permalink / raw) To: emacs-orgmode [-- 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-11 20:27 ` Tom Gillespie 2022-12-11 20:37 ` Tom Gillespie @ 2022-12-11 20:46 ` Kyle Meyer 2022-12-11 21:08 ` Tom Gillespie 2022-12-13 16:15 ` Max Nikulin 2 siblings, 1 reply; 49+ messages in thread From: Kyle Meyer @ 2022-12-11 20:46 UTC (permalink / raw) To: Tom Gillespie; +Cc: Max Nikulin, emacs-orgmode 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"))) ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-11 20:46 ` Kyle Meyer @ 2022-12-11 21:08 ` Tom Gillespie 2022-12-12 10:20 ` Ihor Radchenko 0 siblings, 1 reply; 49+ messages in thread From: Tom Gillespie @ 2022-12-11 21:08 UTC (permalink / raw) To: Kyle Meyer; +Cc: emacs-orgmode [-- 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-11 21:08 ` Tom Gillespie @ 2022-12-12 10:20 ` Ihor Radchenko 2022-12-13 1:53 ` Tom Gillespie 2022-12-13 4:16 ` Kyle Meyer 0 siblings, 2 replies; 49+ messages in thread From: Ihor Radchenko @ 2022-12-12 10:20 UTC (permalink / raw) To: Tom Gillespie, Bastien; +Cc: Kyle Meyer, emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-12 10:20 ` Ihor Radchenko @ 2022-12-13 1:53 ` Tom Gillespie 2022-12-13 9:03 ` Ihor Radchenko ` (2 more replies) 2022-12-13 4:16 ` Kyle Meyer 1 sibling, 3 replies; 49+ messages in thread From: Tom Gillespie @ 2022-12-13 1:53 UTC (permalink / raw) To: Ihor Radchenko; +Cc: Bastien, Kyle Meyer, emacs-orgmode [-- 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-13 1:53 ` Tom Gillespie @ 2022-12-13 9:03 ` Ihor Radchenko 2022-12-13 16:31 ` Max Nikulin 2022-12-25 11:06 ` Ihor Radchenko 2 siblings, 0 replies; 49+ messages in thread From: Ihor Radchenko @ 2022-12-13 9:03 UTC (permalink / raw) To: Tom Gillespie; +Cc: Bastien, Kyle Meyer, emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-13 1:53 ` Tom Gillespie 2022-12-13 9:03 ` Ihor Radchenko @ 2022-12-13 16:31 ` Max Nikulin 2022-12-13 21:16 ` Tom Gillespie 2022-12-25 11:06 ` Ihor Radchenko 2 siblings, 1 reply; 49+ messages in thread From: Max Nikulin @ 2022-12-13 16:31 UTC (permalink / raw) To: emacs-orgmode 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-13 16:31 ` Max Nikulin @ 2022-12-13 21:16 ` Tom Gillespie 2022-12-14 16:40 ` Max Nikulin 0 siblings, 1 reply; 49+ messages in thread From: Tom Gillespie @ 2022-12-13 21:16 UTC (permalink / raw) To: Max Nikulin; +Cc: emacs-orgmode > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-13 21:16 ` Tom Gillespie @ 2022-12-14 16:40 ` Max Nikulin 2022-12-14 18:24 ` Tom Gillespie 2022-12-15 9:10 ` Ihor Radchenko 0 siblings, 2 replies; 49+ messages in thread From: Max Nikulin @ 2022-12-14 16:40 UTC (permalink / raw) To: emacs-orgmode 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-14 16:40 ` Max Nikulin @ 2022-12-14 18:24 ` Tom Gillespie 2022-12-15 9:18 ` Ihor Radchenko 2022-12-15 9:10 ` Ihor Radchenko 1 sibling, 1 reply; 49+ messages in thread From: Tom Gillespie @ 2022-12-14 18:24 UTC (permalink / raw) To: Max Nikulin; +Cc: emacs-orgmode [-- 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 --] ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-14 18:24 ` Tom Gillespie @ 2022-12-15 9:18 ` Ihor Radchenko 2022-12-15 9:25 ` Tom Gillespie 2022-12-15 9:57 ` tomas 0 siblings, 2 replies; 49+ messages in thread From: Ihor Radchenko @ 2022-12-15 9:18 UTC (permalink / raw) To: Tom Gillespie; +Cc: Max Nikulin, emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-15 9:18 ` Ihor Radchenko @ 2022-12-15 9:25 ` Tom Gillespie 2022-12-15 9:57 ` tomas 1 sibling, 0 replies; 49+ messages in thread From: Tom Gillespie @ 2022-12-15 9:25 UTC (permalink / raw) To: Ihor Radchenko; +Cc: Max Nikulin, emacs-orgmode > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-15 9:18 ` Ihor Radchenko 2022-12-15 9:25 ` Tom Gillespie @ 2022-12-15 9:57 ` tomas 1 sibling, 0 replies; 49+ messages in thread From: tomas @ 2022-12-15 9:57 UTC (permalink / raw) To: emacs-orgmode [-- 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 --] ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-14 16:40 ` Max Nikulin 2022-12-14 18:24 ` Tom Gillespie @ 2022-12-15 9:10 ` Ihor Radchenko 2022-12-15 12:10 ` Max Nikulin 1 sibling, 1 reply; 49+ messages in thread From: Ihor Radchenko @ 2022-12-15 9:10 UTC (permalink / raw) To: Max Nikulin; +Cc: emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-15 9:10 ` Ihor Radchenko @ 2022-12-15 12:10 ` Max Nikulin 2022-12-15 12:25 ` Ihor Radchenko 0 siblings, 1 reply; 49+ messages in thread From: Max Nikulin @ 2022-12-15 12:10 UTC (permalink / raw) To: emacs-orgmode 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-15 12:10 ` Max Nikulin @ 2022-12-15 12:25 ` Ihor Radchenko 2022-12-15 14:46 ` Max Nikulin 0 siblings, 1 reply; 49+ messages in thread From: Ihor Radchenko @ 2022-12-15 12:25 UTC (permalink / raw) To: Max Nikulin; +Cc: emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-15 12:25 ` Ihor Radchenko @ 2022-12-15 14:46 ` Max Nikulin 2022-12-15 21:08 ` Tim Cross 2022-12-18 14:12 ` Ihor Radchenko 0 siblings, 2 replies; 49+ messages in thread From: Max Nikulin @ 2022-12-15 14:46 UTC (permalink / raw) To: emacs-orgmode 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-15 14:46 ` Max Nikulin @ 2022-12-15 21:08 ` Tim Cross 2022-12-16 6:07 ` Ihor Radchenko 2022-12-18 14:12 ` Ihor Radchenko 1 sibling, 1 reply; 49+ messages in thread From: Tim Cross @ 2022-12-15 21:08 UTC (permalink / raw) To: emacs-orgmode 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-15 21:08 ` Tim Cross @ 2022-12-16 6:07 ` Ihor Radchenko 2022-12-16 7:22 ` Tim Cross 0 siblings, 1 reply; 49+ messages in thread From: Ihor Radchenko @ 2022-12-16 6:07 UTC (permalink / raw) To: Tim Cross; +Cc: emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-16 6:07 ` Ihor Radchenko @ 2022-12-16 7:22 ` Tim Cross 2022-12-18 14:19 ` Ihor Radchenko 0 siblings, 1 reply; 49+ messages in thread From: Tim Cross @ 2022-12-16 7:22 UTC (permalink / raw) To: Ihor Radchenko; +Cc: emacs-orgmode 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-16 7:22 ` Tim Cross @ 2022-12-18 14:19 ` Ihor Radchenko 2022-12-18 21:37 ` Tim Cross 0 siblings, 1 reply; 49+ messages in thread From: Ihor Radchenko @ 2022-12-18 14:19 UTC (permalink / raw) To: Tim Cross; +Cc: emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-18 14:19 ` Ihor Radchenko @ 2022-12-18 21:37 ` Tim Cross 2022-12-20 0:00 ` Tom Gillespie 0 siblings, 1 reply; 49+ messages in thread From: Tim Cross @ 2022-12-18 21:37 UTC (permalink / raw) To: Ihor Radchenko; +Cc: emacs-orgmode 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-18 21:37 ` Tim Cross @ 2022-12-20 0:00 ` Tom Gillespie 2022-12-20 0:06 ` Tom Gillespie 0 siblings, 1 reply; 49+ messages in thread From: Tom Gillespie @ 2022-12-20 0:00 UTC (permalink / raw) To: Tim Cross; +Cc: Ihor Radchenko, emacs-orgmode [-- 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 --] ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-20 0:00 ` Tom Gillespie @ 2022-12-20 0:06 ` Tom Gillespie 2022-12-25 11:00 ` Ihor Radchenko 0 siblings, 1 reply; 49+ messages in thread From: Tom Gillespie @ 2022-12-20 0:06 UTC (permalink / raw) To: Tim Cross; +Cc: Ihor Radchenko, emacs-orgmode [-- 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 --] ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-20 0:06 ` Tom Gillespie @ 2022-12-25 11:00 ` Ihor Radchenko 0 siblings, 0 replies; 49+ messages in thread From: Ihor Radchenko @ 2022-12-25 11:00 UTC (permalink / raw) To: Tom Gillespie; +Cc: Tim Cross, emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-15 14:46 ` Max Nikulin 2022-12-15 21:08 ` Tim Cross @ 2022-12-18 14:12 ` Ihor Radchenko 1 sibling, 0 replies; 49+ messages in thread From: Ihor Radchenko @ 2022-12-18 14:12 UTC (permalink / raw) To: Max Nikulin; +Cc: emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-13 1:53 ` Tom Gillespie 2022-12-13 9:03 ` Ihor Radchenko 2022-12-13 16:31 ` Max Nikulin @ 2022-12-25 11:06 ` Ihor Radchenko 2022-12-29 15:58 ` Bastien Guerry 2 siblings, 1 reply; 49+ messages in thread From: Ihor Radchenko @ 2022-12-25 11:06 UTC (permalink / raw) To: Tom Gillespie, Bastien; +Cc: Kyle Meyer, emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-25 11:06 ` Ihor Radchenko @ 2022-12-29 15:58 ` Bastien Guerry 2022-12-29 16:33 ` Max Nikulin 2022-12-29 16:35 ` Ihor Radchenko 0 siblings, 2 replies; 49+ messages in thread From: Bastien Guerry @ 2022-12-29 15:58 UTC (permalink / raw) To: Ihor Radchenko; +Cc: Tom Gillespie, Kyle Meyer, emacs-orgmode 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-29 15:58 ` Bastien Guerry @ 2022-12-29 16:33 ` Max Nikulin 2022-12-29 16:35 ` Ihor Radchenko 1 sibling, 0 replies; 49+ messages in thread From: Max Nikulin @ 2022-12-29 16:33 UTC (permalink / raw) To: emacs-orgmode 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). ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-29 15:58 ` Bastien Guerry 2022-12-29 16:33 ` Max Nikulin @ 2022-12-29 16:35 ` Ihor Radchenko 2022-12-30 8:52 ` Bastien 1 sibling, 1 reply; 49+ messages in thread From: Ihor Radchenko @ 2022-12-29 16:35 UTC (permalink / raw) To: Bastien Guerry; +Cc: Tom Gillespie, Kyle Meyer, emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-29 16:35 ` Ihor Radchenko @ 2022-12-30 8:52 ` Bastien 2022-12-30 11:10 ` Max Nikulin 2022-12-30 17:43 ` Tom Gillespie 0 siblings, 2 replies; 49+ messages in thread From: Bastien @ 2022-12-30 8:52 UTC (permalink / raw) To: Ihor Radchenko; +Cc: Tom Gillespie, Kyle Meyer, emacs-orgmode 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-30 8:52 ` Bastien @ 2022-12-30 11:10 ` Max Nikulin 2022-12-30 17:43 ` Tom Gillespie 1 sibling, 0 replies; 49+ messages in thread From: Max Nikulin @ 2022-12-30 11:10 UTC (permalink / raw) To: emacs-orgmode 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-30 8:52 ` Bastien 2022-12-30 11:10 ` Max Nikulin @ 2022-12-30 17:43 ` Tom Gillespie 2022-12-31 13:48 ` Ihor Radchenko 2023-01-02 15:15 ` Bastien 1 sibling, 2 replies; 49+ messages in thread From: Tom Gillespie @ 2022-12-30 17:43 UTC (permalink / raw) To: Bastien; +Cc: Ihor Radchenko, Kyle Meyer, emacs-orgmode 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-30 17:43 ` Tom Gillespie @ 2022-12-31 13:48 ` Ihor Radchenko 2022-12-31 16:15 ` Tom Gillespie ` (2 more replies) 2023-01-02 15:15 ` Bastien 1 sibling, 3 replies; 49+ messages in thread From: Ihor Radchenko @ 2022-12-31 13:48 UTC (permalink / raw) To: Tom Gillespie; +Cc: Bastien, Kyle Meyer, emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-31 13:48 ` Ihor Radchenko @ 2022-12-31 16:15 ` Tom Gillespie 2023-01-02 8:34 ` [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) Ihor Radchenko 2023-01-02 15:13 ` [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable Bastien Guerry 2 siblings, 0 replies; 49+ messages in thread From: Tom Gillespie @ 2022-12-31 16:15 UTC (permalink / raw) To: Ihor Radchenko; +Cc: Bastien, Kyle Meyer, emacs-orgmode > 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) 2022-12-31 13:48 ` Ihor Radchenko 2022-12-31 16:15 ` Tom Gillespie @ 2023-01-02 8:34 ` Ihor Radchenko 2023-01-02 10:59 ` [SECURITY] Arbitrary code evaluation security in Org Greg Minshall 2023-01-02 19:00 ` [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) Tim Cross 2023-01-02 15:13 ` [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable Bastien Guerry 2 siblings, 2 replies; 49+ messages in thread From: Ihor Radchenko @ 2023-01-02 8:34 UTC (permalink / raw) To: Tom Gillespie; +Cc: Bastien, Kyle Meyer, emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [SECURITY] Arbitrary code evaluation security in Org 2023-01-02 8:34 ` [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) Ihor Radchenko @ 2023-01-02 10:59 ` Greg Minshall 2023-01-03 9:52 ` [SECURITY] Tangling can overwrite arbitrary tangling targets, including important user files (was: [SECURITY] Arbitrary code evaluation security in Org) Ihor Radchenko 2023-01-02 19:00 ` [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) Tim Cross 1 sibling, 1 reply; 49+ messages in thread From: Greg Minshall @ 2023-01-02 10:59 UTC (permalink / raw) To: Ihor Radchenko; +Cc: Tom Gillespie, Bastien, Kyle Meyer, emacs-orgmode 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [SECURITY] Tangling can overwrite arbitrary tangling targets, including important user files (was: [SECURITY] Arbitrary code evaluation security in Org) 2023-01-02 10:59 ` [SECURITY] Arbitrary code evaluation security in Org Greg Minshall @ 2023-01-03 9:52 ` Ihor Radchenko 0 siblings, 0 replies; 49+ messages in thread From: Ihor Radchenko @ 2023-01-03 9:52 UTC (permalink / raw) To: Greg Minshall; +Cc: Tom Gillespie, Bastien, Kyle Meyer, emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) 2023-01-02 8:34 ` [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) Ihor Radchenko 2023-01-02 10:59 ` [SECURITY] Arbitrary code evaluation security in Org Greg Minshall @ 2023-01-02 19:00 ` Tim Cross 2023-01-03 11:00 ` Ihor Radchenko 1 sibling, 1 reply; 49+ messages in thread From: Tim Cross @ 2023-01-02 19:00 UTC (permalink / raw) To: emacs-orgmode 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!". ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) 2023-01-02 19:00 ` [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) Tim Cross @ 2023-01-03 11:00 ` Ihor Radchenko 2023-01-07 13:12 ` Ihor Radchenko 0 siblings, 1 reply; 49+ messages in thread From: Ihor Radchenko @ 2023-01-03 11:00 UTC (permalink / raw) To: Tim Cross; +Cc: emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) 2023-01-03 11:00 ` Ihor Radchenko @ 2023-01-07 13:12 ` Ihor Radchenko 0 siblings, 0 replies; 49+ messages in thread From: Ihor Radchenko @ 2023-01-07 13:12 UTC (permalink / raw) To: Tim Cross; +Cc: emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-31 13:48 ` Ihor Radchenko 2022-12-31 16:15 ` Tom Gillespie 2023-01-02 8:34 ` [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) Ihor Radchenko @ 2023-01-02 15:13 ` Bastien Guerry 2023-01-02 15:17 ` Ihor Radchenko 2 siblings, 1 reply; 49+ messages in thread From: Bastien Guerry @ 2023-01-02 15:13 UTC (permalink / raw) To: Ihor Radchenko; +Cc: Tom Gillespie, Kyle Meyer, emacs-orgmode 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2023-01-02 15:13 ` [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable Bastien Guerry @ 2023-01-02 15:17 ` Ihor Radchenko 0 siblings, 0 replies; 49+ messages in thread From: Ihor Radchenko @ 2023-01-02 15:17 UTC (permalink / raw) To: Bastien Guerry; +Cc: Tom Gillespie, Kyle Meyer, emacs-orgmode 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> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-30 17:43 ` Tom Gillespie 2022-12-31 13:48 ` Ihor Radchenko @ 2023-01-02 15:15 ` Bastien 1 sibling, 0 replies; 49+ messages in thread From: Bastien @ 2023-01-02 15:15 UTC (permalink / raw) To: Tom Gillespie; +Cc: Ihor Radchenko, Kyle Meyer, emacs-orgmode 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-12 10:20 ` Ihor Radchenko 2022-12-13 1:53 ` Tom Gillespie @ 2022-12-13 4:16 ` Kyle Meyer 1 sibling, 0 replies; 49+ messages in thread From: Kyle Meyer @ 2022-12-13 4:16 UTC (permalink / raw) To: Ihor Radchenko; +Cc: Tom Gillespie, Bastien, emacs-orgmode 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable 2022-12-11 20:27 ` Tom Gillespie 2022-12-11 20:37 ` Tom Gillespie 2022-12-11 20:46 ` Kyle Meyer @ 2022-12-13 16:15 ` Max Nikulin 2 siblings, 0 replies; 49+ messages in thread From: Max Nikulin @ 2022-12-13 16:15 UTC (permalink / raw) To: emacs-orgmode 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
end of thread, other threads:[~2023-01-07 13:12 UTC | newest] Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-10 20:28 [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable Tom Gillespie 2022-12-11 2:58 ` Max Nikulin 2022-12-11 20:27 ` Tom Gillespie 2022-12-11 20:37 ` Tom Gillespie 2022-12-11 20:46 ` Kyle Meyer 2022-12-11 21:08 ` Tom Gillespie 2022-12-12 10:20 ` Ihor Radchenko 2022-12-13 1:53 ` Tom Gillespie 2022-12-13 9:03 ` Ihor Radchenko 2022-12-13 16:31 ` Max Nikulin 2022-12-13 21:16 ` Tom Gillespie 2022-12-14 16:40 ` Max Nikulin 2022-12-14 18:24 ` Tom Gillespie 2022-12-15 9:18 ` Ihor Radchenko 2022-12-15 9:25 ` Tom Gillespie 2022-12-15 9:57 ` tomas 2022-12-15 9:10 ` Ihor Radchenko 2022-12-15 12:10 ` Max Nikulin 2022-12-15 12:25 ` Ihor Radchenko 2022-12-15 14:46 ` Max Nikulin 2022-12-15 21:08 ` Tim Cross 2022-12-16 6:07 ` Ihor Radchenko 2022-12-16 7:22 ` Tim Cross 2022-12-18 14:19 ` Ihor Radchenko 2022-12-18 21:37 ` Tim Cross 2022-12-20 0:00 ` Tom Gillespie 2022-12-20 0:06 ` Tom Gillespie 2022-12-25 11:00 ` Ihor Radchenko 2022-12-18 14:12 ` Ihor Radchenko 2022-12-25 11:06 ` Ihor Radchenko 2022-12-29 15:58 ` Bastien Guerry 2022-12-29 16:33 ` Max Nikulin 2022-12-29 16:35 ` Ihor Radchenko 2022-12-30 8:52 ` Bastien 2022-12-30 11:10 ` Max Nikulin 2022-12-30 17:43 ` Tom Gillespie 2022-12-31 13:48 ` Ihor Radchenko 2022-12-31 16:15 ` Tom Gillespie 2023-01-02 8:34 ` [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) Ihor Radchenko 2023-01-02 10:59 ` [SECURITY] Arbitrary code evaluation security in Org Greg Minshall 2023-01-03 9:52 ` [SECURITY] Tangling can overwrite arbitrary tangling targets, including important user files (was: [SECURITY] Arbitrary code evaluation security in Org) Ihor Radchenko 2023-01-02 19:00 ` [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) Tim Cross 2023-01-03 11:00 ` Ihor Radchenko 2023-01-07 13:12 ` Ihor Radchenko 2023-01-02 15:13 ` [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable Bastien Guerry 2023-01-02 15:17 ` Ihor Radchenko 2023-01-02 15:15 ` Bastien 2022-12-13 4:16 ` Kyle Meyer 2022-12-13 16:15 ` Max Nikulin
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).