[ANN] Emergency bugfix release: Org mode 9.7.5

[ANN] Emergency bugfix release: Org mode 9.7.5

[Ihor Radchenko]

Dear all,

I just released Org mode 9.7.5 that fixes a critical vulnerability.
The release is coordinated with emergency Emacs 29.4 release.

Please upgrade your Org mode *and* Emacs ASAP.

The vulnerability involves arbitrary Shell code evaluation when
previewing attachments in Emacs MUA (gnus-based: at least, mu4e,
Notmuch, Gnus itself) or when opening third-party Org files. All the
earlier versions of Org mode are affected.

Note that the vulnerability solved in this release has nothing to do
with recent Org 9.6.23 release
(https://list.orgmode.org/871q7zbldp.fsf@localhost/). It existed since
long time ago and was discovered by accident.

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

Re: [ANN] Emergency bugfix release: Org mode 9.7.5

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> Please upgrade your Org mode *and* Emacs ASAP.

*Org mode or Emacs.

The fix is purely in Org code, so upgrading Emacs is only needed when
you want to use built-in Org mode.

Otherwise, it is enough to upgrade Org mode via ELPA (the tarball will
be available soon, after ELPA scripts fetch the latest release tag).

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

Re: [ANN] Emergency bugfix release: Org mode 9.7.5

[Greg Troxel]

(Thanks for fixing and your efforts on org.  I've been an org user since
at least July of 2010.)

Just to be clear, is this the commit that needs applying to emacs
sources, 29.3, 28.x, and so on?  It seems so, but I would rather not
guess.  I'm asking on behalf of pkgsrc, where I am managing the release
process for our 2024Q2 branch, due on 30 June.  Believe it or not we
have 20, 21, 26, 27, 28, 29 and a from-git version.  While some should
be pruned, some people use it on vaxes.   Any idea how far back this
goes?

Thanks,
Greg

commit f4cc61636947b5c2f0afc67174dd369fe3277aa8
Author: Ihor Radchenko <yantar92@posteo.net>
Date:   Tue Jun 18 13:06:44 2024 +0200

    org-link-expand-abbrev: Do not evaluate arbitrary unsafe Elisp code
    
    * lisp/ol.el (org-link-expand-abbrev): Refuse expanding %(...) link
    abbrevs that specify unsafe function.  Instead, display a warning, and
    do not expand the abbrev.  Clear all the text properties from the
    returned link, to avoid any potential vulnerabilities caused by
    properties that may contain arbitrary Elisp.

diff --git a/lisp/ol.el b/lisp/ol.el
index 7a7f4f558..8a556c7b9 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -1152,17 +1152,35 @@ Abbreviations are defined in `org-link-abbrev-alist'."
       (if (not as)
 	  link
 	(setq rpl (cdr as))
-	(cond
-	 ((symbolp rpl) (funcall rpl tag))
-	 ((string-match "%(\\([^)]+\\))" rpl)
-	  (replace-match
-	   (save-match-data
-	     (funcall (intern-soft (match-string 1 rpl)) tag))
-	   t t rpl))
-	 ((string-match "%s" rpl) (replace-match (or tag "") t t rpl))
-	 ((string-match "%h" rpl)
-	  (replace-match (url-hexify-string (or tag "")) t t rpl))
-	 (t (concat rpl tag)))))))
+        ;; Drop any potentially dangerous text properties like
+        ;; `modification-hooks' that may be used as an attack vector.
+        (substring-no-properties
+	 (cond
+	  ((symbolp rpl) (funcall rpl tag))
+	  ((string-match "%(\\([^)]+\\))" rpl)
+           (let ((rpl-fun-symbol (intern-soft (match-string 1 rpl))))
+             ;; Using `unsafep-function' is not quite enough because
+             ;; Emacs considers functions like `genenv' safe, while
+             ;; they can potentially be used to expose private system
+             ;; data to attacker if abbreviated link is clicked.
+             (if (or (eq t (get rpl-fun-symbol 'org-link-abbrev-safe))
+                     (eq t (get rpl-fun-symbol 'pure)))
+                 (replace-match
+	          (save-match-data
+	            (funcall (intern-soft (match-string 1 rpl)) tag))
+	          t t rpl)
+               (org-display-warning
+                (format "Disabling unsafe link abbrev: %s
+You may mark function safe via (put '%s 'org-link-abbrev-safe t)"
+                        rpl (match-string 1 rpl)))
+               (setq org-link-abbrev-alist-local (delete as org-link-abbrev-alist-local)
+                     org-link-abbrev-alist (delete as org-link-abbrev-alist))
+               link
+	       )))
+	  ((string-match "%s" rpl) (replace-match (or tag "") t t rpl))
+	  ((string-match "%h" rpl)
+	   (replace-match (url-hexify-string (or tag "")) t t rpl))
+	  (t (concat rpl tag))))))))
 
 (defun org-link-open (link &optional arg)
   "Open a link object LINK.

Re: [ANN] Emergency bugfix release: Org mode 9.7.5

[Steven Allen]

Greg Troxel <gdt@lexort.com> writes:

> (Thanks for fixing and your efforts on org.  I've been an org user since
> at least July of 2010.)
>
> Just to be clear, is this the commit that needs applying to emacs
> sources, 29.3, 28.x, and so on?

Yes, that's the correct commit.

> It seems so, but I would rather not guess. I'm asking on behalf of
> pkgsrc, where I am managing the release process for our 2024Q2 branch,
> due on 30 June. Believe it or not we have 20, 21, 26, 27, 28, 29 and a
> from-git version. While some should be pruned, some people use it on
> vaxes. Any idea how far back this goes?

It was introduced in org 7.9 (commit [1] from July of 2012). From what I
can tell, it has been present in Emacs since emacs-24.2.

[1]: ef3d4b5965b828e85a535ef3f32999473c6a2a7a 

>
> Thanks,
> Greg
>
> commit f4cc61636947b5c2f0afc67174dd369fe3277aa8
> Author: Ihor Radchenko <yantar92@posteo.net>
> Date:   Tue Jun 18 13:06:44 2024 +0200
>
>     org-link-expand-abbrev: Do not evaluate arbitrary unsafe Elisp code
>     
>     * lisp/ol.el (org-link-expand-abbrev): Refuse expanding %(...) link
>     abbrevs that specify unsafe function.  Instead, display a warning, and
>     do not expand the abbrev.  Clear all the text properties from the
>     returned link, to avoid any potential vulnerabilities caused by
>     properties that may contain arbitrary Elisp.
>
> diff --git a/lisp/ol.el b/lisp/ol.el
> index 7a7f4f558..8a556c7b9 100644
> --- a/lisp/ol.el
> +++ b/lisp/ol.el
> @@ -1152,17 +1152,35 @@ Abbreviations are defined in `org-link-abbrev-alist'."
>        (if (not as)
>  	  link
>  	(setq rpl (cdr as))
> -	(cond
> -	 ((symbolp rpl) (funcall rpl tag))
> -	 ((string-match "%(\\([^)]+\\))" rpl)
> -	  (replace-match
> -	   (save-match-data
> -	     (funcall (intern-soft (match-string 1 rpl)) tag))
> -	   t t rpl))
> -	 ((string-match "%s" rpl) (replace-match (or tag "") t t rpl))
> -	 ((string-match "%h" rpl)
> -	  (replace-match (url-hexify-string (or tag "")) t t rpl))
> -	 (t (concat rpl tag)))))))
> +        ;; Drop any potentially dangerous text properties like
> +        ;; `modification-hooks' that may be used as an attack vector.
> +        (substring-no-properties
> +	 (cond
> +	  ((symbolp rpl) (funcall rpl tag))
> +	  ((string-match "%(\\([^)]+\\))" rpl)
> +           (let ((rpl-fun-symbol (intern-soft (match-string 1 rpl))))
> +             ;; Using `unsafep-function' is not quite enough because
> +             ;; Emacs considers functions like `genenv' safe, while
> +             ;; they can potentially be used to expose private system
> +             ;; data to attacker if abbreviated link is clicked.
> +             (if (or (eq t (get rpl-fun-symbol 'org-link-abbrev-safe))
> +                     (eq t (get rpl-fun-symbol 'pure)))
> +                 (replace-match
> +	          (save-match-data
> +	            (funcall (intern-soft (match-string 1 rpl)) tag))
> +	          t t rpl)
> +               (org-display-warning
> +                (format "Disabling unsafe link abbrev: %s
> +You may mark function safe via (put '%s 'org-link-abbrev-safe t)"
> +                        rpl (match-string 1 rpl)))
> +               (setq org-link-abbrev-alist-local (delete as org-link-abbrev-alist-local)
> +                     org-link-abbrev-alist (delete as org-link-abbrev-alist))
> +               link
> +	       )))
> +	  ((string-match "%s" rpl) (replace-match (or tag "") t t rpl))
> +	  ((string-match "%h" rpl)
> +	   (replace-match (url-hexify-string (or tag "")) t t rpl))
> +	  (t (concat rpl tag))))))))
>  
>  (defun org-link-open (link &optional arg)
>    "Open a link object LINK.

Re: [ANN] Emergency bugfix release: Org mode 9.7.5

[emacs-orgmode]

Ihor Radchenko <yantar92@posteo.net> writes:

> I just released Org mode 9.7.5 that fixes a critical vulnerability.
> The release is coordinated with emergency Emacs 29.4 release.

Thanks for the release and the anouncement.

Will a CVE be released? I am interested if there are mitigating factors
such as using `emacs -nw` (without GUI), thus no possible preview of the
attachments (IIUC).

Best,

Re: [ANN] Emergency bugfix release: Org mode 9.7.5

[Ihor Radchenko]

emacs-orgmode@city17.xyz writes:

> Will a CVE be released?

Should be, I think.
If nobody reports it independently by tomorrow, I will look into how to
request a CVE number myself.

> ... I am interested if there are mitigating factors
> such as using `emacs -nw` (without GUI), thus no possible preview of the
> attachments (IIUC).

AFAIK, previewing attachments is not disabled by "no GUI" - preview in
this context simply means fontification using major mode of the attached
files.

To disable email previews, see `mm-inline-media-tests'.

Note that you cannot easily work around the problem when opening an
actual Org file. You would either have to advice the problematic Org
function, or cherry-pick the relevant commit from the release.

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

Assigned: CVE-2024-39331 (was: [ANN] Emergency bugfix release: Org mode 9.7.5)

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> emacs-orgmode@city17.xyz writes:
>
>> Will a CVE be released?
>
> Should be, I think.
> If nobody reports it independently by tomorrow, I will look into how to
> request a CVE number myself.

https://www.cve.org/CVERecord?id=CVE-2024-39331

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

Re: [ANN] Emergency bugfix release: Org mode 9.7.5

[Bastien Guerry]

Ihor Radchenko <yantar92@posteo.net> writes:

> I just released Org mode 9.7.5 that fixes a critical vulnerability.
> The release is coordinated with emergency Emacs 29.4 release.

Thank you a lot for your diligent and careful work on this!

-- 
 Bastien Guerry

[POLL] We plan to remove #+LINK: ...%(my-function) placeholder from link abbreviation spec (was: [ANN] Emergency bugfix release: Org mode 9.7.5)

[Ihor Radchenko]

Dear all,

> I just released Org mode 9.7.5 that fixes a critical vulnerability.
> The release is coordinated with emergency Emacs 29.4 release.
> ...
> The vulnerability involves arbitrary Shell code evaluation...

In a view of the recent vulnerability, we are considering to remove the
offending feature completely.

For the time being, we restricted %(function) constructs in #+LINK:
... lines to (1) pure functions (no side effects, no access to global
state); (2) functions explicitly marked by the user.

However, while discussing how to approach the vulnerability, we did not
find many examples of using #+LINK: label %(function) in the wild.

If you are actively using #+LINK: keywords with %(...) placeholders or
have any objections to this feature removal, please let us know.

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

Re: [POLL] We plan to remove #+LINK: ...%(my-function) placeholder from link abbreviation spec

[Suhail Singh]

Ihor Radchenko <yantar92@posteo.net> writes:

> If you are actively using #+LINK: keywords with %(...) placeholders or
> have any objections to this feature removal, please let us know.

I do not actively use this feature, however, removing it seems
excessive.  IIUC, it's a useful feature in situations when the tag may
require deterministic, yet non-trivial manipulation.  The current
mechanism of restricting this to functions marked safe by user seems
sufficient.

Am I missing something?  Is the threat model such that it can only be
adequately addressed by removing the feature altogether?

-- 
Suhail

Re: [POLL] We plan to remove #+LINK: ...%(my-function) placeholder from link abbreviation spec

[Steven Allen]

Suhail Singh <suhailsingh247@gmail.com> writes:
> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> If you are actively using #+LINK: keywords with %(...) placeholders or
>> have any objections to this feature removal, please let us know.
>
> I do not actively use this feature, however, removing it seems
> excessive.  IIUC, it's a useful feature in situations when the tag may
> require deterministic, yet non-trivial manipulation.  The current
> mechanism of restricting this to functions marked safe by user seems
> sufficient.
>
> Am I missing something?  Is the threat model such that it can only be
> adequately addressed by removing the feature altogether?

There are two issues:

1. While this feature no longer invokes completely arbitrary code, it
still allows an attacker to call any function marked as "pure" which is
a pretty large attack surface.

2. Making it secure also made it significantly less useful, if it ever
was all that useful. For the %(...) specifier to be useful, you need a
pure/safe function that takes exactly one string argument and produces
the string you need. You can, of course, write that function; but then
you might as well use org-link-abbrev-alist instead of defining a
local #+LINK.

Personally, I'd start by forbidding %(...) placeholders in buffer-local
#+LINK: definitions, they're perfectly safe in org-link-abbrev-alist.

Re: [POLL] We plan to remove #+LINK: ...%(my-function) placeholder from link abbreviation spec

[Suhail Singh]

Steven Allen <steven@stebalien.com> writes:

> 1. While this feature no longer invokes completely arbitrary code, it
> still allows an attacker to call any function marked as "pure" which
> is a pretty large attack surface.

I am struggling to assess this, because it's not clear to me what the
threat model is.  Could you please elaborate?  How are the attacker and
potential victim interacting; what is the attack vector(s); who are the
threat agents and what is their goal that we are trying to guard
against, etc?

> You can, of course, write that function; but then you might as well
> use org-link-abbrev-alist instead of defining a local #+LINK.

Perhaps I misunderstood, I thought the thing being polled was whether or
not to allow org-link-abbrev-alist to have REPLACE (per its docstring)
be a function.  I.e., if %(my-function) is removed, so too would the
ability to have a function in the REPLACE position in
org-link-abbrev-alist.  Did I misunderstand?

-- 
Suhail

Re: [POLL] We plan to remove #+LINK: ...%(my-function) placeholder from link abbreviation spec

[Ihor Radchenko]

Suhail Singh <suhailsingh247@gmail.com> writes:

>> You can, of course, write that function; but then you might as well
>> use org-link-abbrev-alist instead of defining a local #+LINK.
>
> Perhaps I misunderstood, I thought the thing being polled was whether or
> not to allow org-link-abbrev-alist to have REPLACE (per its docstring)
> be a function.  I.e., if %(my-function) is removed, so too would the
> ability to have a function in the REPLACE position in
> org-link-abbrev-alist.  Did I misunderstand?

Yup. I only meant %(...) placeholder constructs.
("linkkey" . REPLACE) where REPLACE is a function symbol will still be
allowed.

What will not be allowed is:

#+LINK: linkkey %(my-function)
and
("linkkey" . "...%(my-function)...") in `org-link-abbrev-alist'.

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

Re: [POLL] We plan to remove #+LINK: ...%(my-function) placeholder from link abbreviation spec

[Suhail Singh]

Ihor Radchenko <yantar92@posteo.net> writes:

> Yup. I only meant %(...) placeholder constructs.  ("linkkey"
> . REPLACE) where REPLACE is a function symbol will still be allowed.

Thank you for confirming.  Please ignore my previous response.

-- 
Suhail

Re: [POLL] We plan to remove #+LINK: ...%(my-function) placeholder from link abbreviation spec

[Steven Allen]

Suhail Singh <suhailsingh247@gmail.com> writes:

> Steven Allen <steven@stebalien.com> writes:
>
>> 1. While this feature no longer invokes completely arbitrary code, it
>> still allows an attacker to call any function marked as "pure" which
>> is a pretty large attack surface.
>
> I am struggling to assess this, because it's not clear to me what the
> threat model is.  Could you please elaborate?  How are the attacker and
> potential victim interacting; what is the attack vector(s); who are the
> threat agents and what is their goal that we are trying to guard
> against, etc?

Scenario: Attacker sends an email containing an inline org-mode part with a
malicious link abbreviation.

The concern is that, e.g., there may b a function _marked_ as pure
that's not actually pure, leaks some information, and/or has a security
vulnerability (e.g., a C function exposed to lisp that's marked as pure
but internally has, e.g., a buffer overflow).

Of course, the actual attack hypothetical. The question being asked here
is: is the %(..) specifier in link abbreviations useful enough to warent
the potential risks.

>> You can, of course, write that function; but then you might as well
>> use org-link-abbrev-alist instead of defining a local #+LINK.
>
> Perhaps I misunderstood, I thought the thing being polled was whether or
> not to allow org-link-abbrev-alist to have REPLACE (per its docstring)
> be a function.  I.e., if %(my-function) is removed, so too would the
> ability to have a function in the REPLACE position in
> org-link-abbrev-alist.  Did I misunderstand?

The question is whether or not %(function) placeholders should be
allowed in #+LINK: lines. It doesn't actually say anything about
allowing them in the global org-link-abbrev-alist. But to be explicit,
there are three options:

1. Allow them in both #+LINK: lines and the global
org-link-abbrev-alist.

2. Allow them in org-link-abbrev-alist only.

3. Remove them entirely.

Re: [POLL] We plan to remove #+LINK: ...%(my-function) placeholder from link abbreviation spec

[Suhail Singh]

Steven Allen <steven@stebalien.com> writes:

> The concern is that, e.g., there may b a function _marked_ as pure
> that's not actually pure, leaks some information, and/or has a
> security vulnerability (e.g., a C function exposed to lisp that's
> marked as pure but internally has, e.g., a buffer overflow).

Are there any functions marked as pure, by default?

> 1. Allow them in both #+LINK: lines and the global
> org-link-abbrev-alist.
>
> 2. Allow them in org-link-abbrev-alist only.
>
> 3. Remove them entirely.

If no functions are marked as pure by default, 1 seems reasonable to me.
If some functions are marked as pure by default (by Emacs / Org mode),
then 2 seems reasonable.  I believe 3 is excessive.

-- 
Suhail

Re: [POLL] We plan to remove #+LINK: ...%(my-function) placeholder from link abbreviation spec

[Steven Allen]

Suhail Singh <suhailsingh247@gmail.com> writes:

> Steven Allen <steven@stebalien.com> writes:
>
>> The concern is that, e.g., there may b a function _marked_ as pure
>> that's not actually pure, leaks some information, and/or has a
>> security vulnerability (e.g., a C function exposed to lisp that's
>> marked as pure but internally has, e.g., a buffer overflow).
>
> Are there any functions marked as pure, by default?
>

Yes. Any function that starts with:

    (declare (pure t) ...

This flag was introduced to allow the byte/native compiler to better
optimize calls to pure functions. It's used here because "pure"
functions should be safe to call.

[POLL] Bug of Feature? Attack vector via deceiving link abbrevs (was: [ANN] Emergency bugfix release: Org mode 9.7.5)

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> I just released Org mode 9.7.5 that fixes a critical vulnerability.
> The release is coordinated with emergency Emacs 29.4 release.

This one is another potential issue (or a feature) we have found while
discussing the main vulnerability.

Currently, one can create an Org file like

#+LINK: https https://fake-gmail-login-page.xyz/
[[https://gmail.com]]

And the "https" link will actually be expanded according to the
abbreviation.  In other words, abbreviations take priority over the link
types in Org mode.

As illustrated above, one can try to trick user into clicking the above
"gmail" link, redirecting to completely different page instead.

On the other hand, I can totally see people making use of the current
behavior to have custom filters for existing link types. For example, to
redirect to archive.org when opening web links.

I am inclined to call this a feature, and leave the current behavior
unchanged, but would like to hear from others first.

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

Re: [POLL] Bug of Feature? Attack vector via deceiving link abbrevs (was: [ANN] Emergency bugfix release: Org mode 9.7.5)

[Steven Allen]

Ihor Radchenko <yantar92@posteo.net> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> I just released Org mode 9.7.5 that fixes a critical vulnerability.
>> The release is coordinated with emergency Emacs 29.4 release.
>
> This one is another potential issue (or a feature) we have found while
> discussing the main vulnerability.
>
> Currently, one can create an Org file like
>
> #+LINK: https https://fake-gmail-login-page.xyz/
> [[https://gmail.com]]

This is no different from:

    [[https://fake-gmail-login-page.xyz][https://gmail.com]]

In both cases, mousing over the link will show you the actual target address.

On the other hand, having different faces for "plain" links (links where
the text in the buffer matches the link target) and special links would
be kind of nice.

Re: [POLL] Bug of Feature? Attack vector via deceiving link abbrevs

[Suhail Singh]

Ihor Radchenko <yantar92@posteo.net> writes:

> On the other hand, I can totally see people making use of the current
> behavior to have custom filters for existing link types.

Yes, I use this currently for redirecting reddit links.  It's certainly
a feature in my opinion.

-- 
Suhail