emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Tim Cross <theophilusx@gmail.com>
To: emacs-orgmode@gnu.org
Subject: Re: [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable)
Date: Tue, 03 Jan 2023 06:00:45 +1100	[thread overview]
Message-ID: <86tu18d811.fsf@gmail.com> (raw)
In-Reply-To: <87edsd5o89.fsf@localhost>

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.

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

  parent reply	other threads:[~2023-01-02 19:58 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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                           ` Tim Cross [this message]
2023-01-03 11:00                             ` [SECURITY] Arbitrary code evaluation security in Org (was: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable) 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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=86tu18d811.fsf@gmail.com \
    --to=theophilusx@gmail.com \
    --cc=emacs-orgmode@gnu.org \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox


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