bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Jim Porter]

Normally, when running `emacsclient --alternate-editor=""' with no Emacs 
server running, it will run `emacs --daemon' and then connect to it. In 
Emacs 28, it will also issue the following warning:

   Should XDG_RUNTIME_DIR='/run/user/1000' be in the environment?
   (Be careful: XDG_RUNTIME_DIR is security-related.)

However, XDG_RUNTIME_DIR *is* set in my environment, so it shouldn't be 
warning me about it.

I believe this is due to the fix for bug#33847 (see commit 
007744dd0404d6febca88b00c22981cc630fb8c0). That bug asked for 
emacsclient to look in both XDG_RUNTIME_DIR and TMPDIR to find the 
server socket, in order to accommodate the case where `emacs --daemon' 
is started when XDG_RUNTIME_DIR is unset, but *is* set when running 
`emacsclient'.

That works for the issue described in bug#33847, but for users with 
XDG_RUNTIME_DIR set who want `emacs --daemon' to start on demand, the 
change means that emacsclient will look in TMPDIR to find a server 
socket, record that attempt, and then warn about it before finally going 
ahead and starting `emacs --daemon'.

I'm not an expert on XDG_RUNTIME_DIR, but my understanding is that this 
was added to improve security over using TMPDIR. However, as far as I 
can tell, the fix in bug#33847 partially undoes the security improvement 
for users who want to start `emacs --daemon' on demand.

I'm not sure what the fix here is, at least not while ensuring that both 
this case and the case in bug#33847 "just work" without setting some 
option...

(The original bug#33847 is rather long, and I see that similar concerns 
were raised there, so I hope I've summarized this accurately and I'm not 
just misunderstanding what this code is doing.)

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Jim Porter]

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

On 10/21/2021 9:58 PM, Jim Porter wrote:
> Normally, when running `emacsclient --alternate-editor=""' with no Emacs 
> server running, it will run `emacs --daemon' and then connect to it. In 
> Emacs 28, it will also issue the following warning:
> 
>    Should XDG_RUNTIME_DIR='/run/user/1000' be in the environment?
>    (Be careful: XDG_RUNTIME_DIR is security-related.)
> 
> However, XDG_RUNTIME_DIR *is* set in my environment, so it shouldn't be 
> warning me about it.
> 
> I believe this is due to the fix for bug#33847 (see commit 
> 007744dd0404d6febca88b00c22981cc630fb8c0). That bug asked for 
> emacsclient to look in both XDG_RUNTIME_DIR and TMPDIR to find the 
> server socket, in order to accommodate the case where `emacs --daemon' 
> is started when XDG_RUNTIME_DIR is unset, but *is* set when running 
> `emacsclient'.

Attached is a patch that should fix this by skipping the TMPDIR check 
whenever a) we have an alternate editor and b) XDG_RUNTIME_DIR is set. 
This has the benefit of supporting the use case in bug#33847 as well as 
users who start the Emacs daemon on-demand.

The only flaw I can think of with this method is that it would still be 
technically possible to perform a symlink attack against a user who runs 
`emacs --daemon' explicitly with XDG_RUNTIME_DIR set, and then runs 
`emacsclient' without an alternate editor set. However, this would 
require the attacker to be able to kill the `emacs --daemon' process 
somehow so that emacsclient falls back to looking in TMPDIR. I'm not 
sure that's a realistic attack vector, but I thought I'd mention it for 
completeness.

[-- Attachment #2: 0001-Prevent-symlink-attacks-in-emacsclient-when-an-alter.patch --]
[-- Type: text/plain, Size: 1148 bytes --]

From 6b8c7a9881b79254c618356a4dfa257812a6fe5c Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 30 Oct 2021 12:22:02 -0700
Subject: [PATCH] Prevent symlink attacks in emacsclient when an alternate
 editor is set

* lib-src/emacsclient.c (set_local_socket): Don't look in TMPDIR for a
socket if we have an alternate editor and XDG_RUNTIME_DIR is set
(Bug#51327).
---
 lib-src/emacsclient.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib-src/emacsclient.c b/lib-src/emacsclient.c
index cff3cec2a7..1305226056 100644
--- a/lib-src/emacsclient.c
+++ b/lib-src/emacsclient.c
@@ -1466,7 +1466,10 @@ set_local_socket (char const *server_name)
 			 ? connect_socket (AT_FDCWD, sockname, s, 0)
 			 : ENAMETOOLONG);
 	}
-      if (sock_status == ENOENT)
+      /* Fall back to checking for a socket in TMPDIR unless we have
+	 an alternate editor and XDG_RUNTIME_DIR is set.  In that
+	 case, we want to bail out and spawn the alternate editor. */
+      if (!(xdg_runtime_dir && alternate_editor) && sock_status == ENOENT)
 	{
 	  char const *tmpdir = egetenv ("TMPDIR");
 	  if (tmpdir)
-- 
2.25.1

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Paul Eggert]

Thanks, this patch looks good to me. You're right that the current 
approach is insecure, and your patch should make it somewhat less insecure.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Stefan Kangas]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Thanks, this patch looks good to me. You're right that the current approach is
> insecure, and your patch should make it somewhat less insecure.

Agreed.  The only question is if this patch should go to emacs-28 or
master?  Perhaps Eli or Lars has an opinion about that.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Tue, 7 Dec 2021 12:26:23 +0100
> Cc: Jim Porter <jporterbugs@gmail.com>, 51327@debbugs.gnu.org
> 
> Paul Eggert <eggert@cs.ucla.edu> writes:
> 
> > Thanks, this patch looks good to me. You're right that the current approach is
> > insecure, and your patch should make it somewhat less insecure.
> 
> Agreed.  The only question is if this patch should go to emacs-28 or
> master?  Perhaps Eli or Lars has an opinion about that.

AFAIU, Ulrich wasn't happy with that patch and proposed an
alternative?

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> Agreed.  The only question is if this patch should go to emacs-28 or
>> master?  Perhaps Eli or Lars has an opinion about that.
>
> AFAIU, Ulrich wasn't happy with that patch and proposed an
> alternative?

You are correct, so it seems like we need to think about this more
closely before taking action.

I linked the relevant emacs-devel thread with more discussion
separately.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Paul Eggert]

On 12/7/21 06:58, Stefan Kangas wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>>> Agreed.  The only question is if this patch should go to emacs-28 or
>>> master?  Perhaps Eli or Lars has an opinion about that.
>>
>> AFAIU, Ulrich wasn't happy with that patch and proposed an
>> alternative?
> 
> You are correct, so it seems like we need to think about this more
> closely before taking action.
> 
> I linked the relevant emacs-devel thread with more discussion
> separately.

Although none of us has done a thorough security audit, I still think 
that looking in TMPDIR first is a security loophole that is exploitable 
in some circumstances.

Ulrich says the loophole is small because Emacs verifies that the 
current user is the socket owner. However, small loopholes can still be 
exploited: for example, an attacker could cause you to think that you're 
connecting to your Emacs when you're really connecting to another of 
your processes, and this could still lead to problems (particularly if 
you're root).

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Jim Porter]

On 12/7/2021 11:03 AM, Paul Eggert wrote:
> Ulrich says the loophole is small because Emacs verifies that the 
> current user is the socket owner. However, small loopholes can still be 
> exploited: for example, an attacker could cause you to think that you're 
> connecting to your Emacs when you're really connecting to another of 
> your processes, and this could still lead to problems (particularly if 
> you're root).

While I understand that Ulrich's goal is for things to Just Work with 
Gentoo's app-emacs/emacs-daemon package (which puts the socket in 
$TMPDIR), it seems there's no way to get that without opening at least a 
small loophole.

When the user is guaranteed to be connecting to an Emacs daemon whose 
socket is in $TMPDIR, it's sufficient on Emacs 27 to just unset 
$XDG_RUNTIME_DIR first. However, from my discussion with Ulrich 
before[1], I believe one of the goals is to look in *both* places for a 
socket to be more flexible, as Emacs 28 currently does.

Doing that by default opens a loophole for all emacsclient users, but 
what about a command-line flag like `emacsclient 
--allow-tmpdir-loophole' and/or an environment variable like 
`EMACS_ALLOW_TMPDIR_LOOPHOLE=1 emacsclient' (with a better name, of 
course)? Then, the default behavior would be free of loopholes[2], but 
Ulrich's case could be achieved by passing that flag when calling 
emacsclient. It might even be possible for Gentoo to enable that for the 
user in the appropriate cases...

That's not as user-/distro-friendly as things just working 
automatically, but maybe it would be a decent compromise? Of course, if 
the loophole is small enough, maybe the current behavior in Emacs 28 is 
ok (aside from the warning message). I'm not an expert on the security 
implications though, so I don't have a strong opinion on which way to go 
here.

[1] https://lists.gnu.org/archive/html/emacs-devel/2021-11/msg00435.html
[2] Well, *known* loopholes...

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Paul Eggert]

On 12/7/21 22:57, Jim Porter wrote:
> Doing that by default opens a loophole for all emacsclient users, but 
> what about a command-line flag like `emacsclient 
> --allow-tmpdir-loophole' and/or an environment variable like 
> `EMACS_ALLOW_TMPDIR_LOOPHOLE=1 emacsclient' (with a better name, of 
> course)? Then, the default behavior would be free of loopholes[2], but 
> Ulrich's case could be achieved by passing that flag when calling 
> emacsclient. It might even be possible for Gentoo to enable that for the 
> user in the appropriate cases...

Yes, I think something like this would be OK. The command-line flag 
would be easier to audit.

Not sure whether a last-minute change like this should go into Emacs 28, 
though, even though it's security-relevant. Eli would be a better judge 
of that.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Eli Zaretskii]

> Date: Wed, 8 Dec 2021 11:06:12 -0800
> Cc: 51327@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 12/7/21 22:57, Jim Porter wrote:
> > Doing that by default opens a loophole for all emacsclient users, but 
> > what about a command-line flag like `emacsclient 
> > --allow-tmpdir-loophole' and/or an environment variable like 
> > `EMACS_ALLOW_TMPDIR_LOOPHOLE=1 emacsclient' (with a better name, of 
> > course)? Then, the default behavior would be free of loopholes[2], but 
> > Ulrich's case could be achieved by passing that flag when calling 
> > emacsclient. It might even be possible for Gentoo to enable that for the 
> > user in the appropriate cases...
> 
> Yes, I think something like this would be OK. The command-line flag 
> would be easier to audit.
> 
> Not sure whether a last-minute change like this should go into Emacs 28, 
> though, even though it's security-relevant. Eli would be a better judge 
> of that.

If it's a new command-line argument, and if the participants in this
discussion can live with it as the solution for this problem, I'm okay
with having it on emacs-28.

Thanks.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Wed, 8 Dec 2021 11:06:12 -0800
>> Cc: 51327@debbugs.gnu.org
>> From: Paul Eggert <eggert@cs.ucla.edu>
>>
>> On 12/7/21 22:57, Jim Porter wrote:
>> > Doing that by default opens a loophole for all emacsclient users, but
>> > what about a command-line flag like `emacsclient
>> > --allow-tmpdir-loophole' and/or an environment variable like
>> > `EMACS_ALLOW_TMPDIR_LOOPHOLE=1 emacsclient' (with a better name, of
>> > course)? Then, the default behavior would be free of loopholes[2], but
>> > Ulrich's case could be achieved by passing that flag when calling
>> > emacsclient. It might even be possible for Gentoo to enable that for the
>> > user in the appropriate cases...
>>
>> Yes, I think something like this would be OK. The command-line flag
>> would be easier to audit.
>>
>> Not sure whether a last-minute change like this should go into Emacs 28,
>> though, even though it's security-relevant. Eli would be a better judge
>> of that.
>
> If it's a new command-line argument, and if the participants in this
> discussion can live with it as the solution for this problem, I'm okay
> with having it on emacs-28.

Copying in Ulrich to make sure he's aware of this discussion.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Ulrich Mueller]

>>>>> On Wed, 08 Dec 2021, Stefan Kangas wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>>> Date: Wed, 8 Dec 2021 11:06:12 -0800
>>> Cc: 51327@debbugs.gnu.org
>>> From: Paul Eggert <eggert@cs.ucla.edu>
>>> 
>>> On 12/7/21 22:57, Jim Porter wrote:
>>> > Doing that by default opens a loophole for all emacsclient users, but
>>> > what about a command-line flag like `emacsclient
>>> > --allow-tmpdir-loophole' and/or an environment variable like
>>> > `EMACS_ALLOW_TMPDIR_LOOPHOLE=1 emacsclient' (with a better name, of
>>> > course)? Then, the default behavior would be free of loopholes[2], but
>>> > Ulrich's case could be achieved by passing that flag when calling
>>> > emacsclient. It might even be possible for Gentoo to enable that for the
>>> > user in the appropriate cases...
>>> 
>>> Yes, I think something like this would be OK. The command-line flag
>>> would be easier to audit.
>>> 
>>> Not sure whether a last-minute change like this should go into Emacs 28,
>>> though, even though it's security-relevant. Eli would be a better judge
>>> of that.
>> 
>> If it's a new command-line argument, and if the participants in this
>> discussion can live with it as the solution for this problem, I'm okay
>> with having it on emacs-28.

That's not an acceptable solution, because it will break the existing
workflow of users. Furthermore, it will make users jump through hoops to
achieve functionality which was the default in previous versions.

So, can we please think about a better solution, and not knee-jerk
something half-baked into Emacs 28, like checking for yet another
environment variable?

Even reverting to the Emacs 27 behaviour would be better than what has
been suggested above: In Emacs 27, you can set EMACS_SOCKET_NAME to make
things work. There's no advantage in introducing yet another variable,
which would only complicate things.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Jim Porter]

On 12/8/2021 1:56 PM, Ulrich Mueller wrote:
> That's not an acceptable solution, because it will break the existing
> workflow of users. Furthermore, it will make users jump through hoops to
> achieve functionality which was the default in previous versions.
> 
> So, can we please think about a better solution, and not knee-jerk
> something half-baked into Emacs 28, like checking for yet another
> environment variable?
> 
> Even reverting to the Emacs 27 behaviour would be better than what has
> been suggested above: In Emacs 27, you can set EMACS_SOCKET_NAME to make
> things work. There's no advantage in introducing yet another variable,
> which would only complicate things.

Given the goal to release Emacs 28.1 soon, I'm not sure there's time to 
come up with (and be confident in) a better solution for 28. In that 
case, I guess the available options are:

* If the security issue is considered minor enough, keep the current 
Emacs 28 behavior and silence the warning.

* Otherwise, revert to the Emacs 27 behavior and come up with a better 
solution for Emacs 29.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Paul Eggert]

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

On 12/8/21 13:56, Ulrich Mueller wrote:
> So, can we please think about a better solution, and not knee-jerk
> something half-baked into Emacs 28, like checking for yet another
> environment variable?

Agreed, now is not the time for significant innovation. It could be that 
any significant improvement will have to wait for Emacs 29.


> Even reverting to the Emacs 27 behaviour would be better than what has
> been suggested above: In Emacs 27, you can set EMACS_SOCKET_NAME to make
> things work.

Setting EMACS_SOCKET_NAME to an absolute file name should work just as 
well in emacs-28 as it did in Emacs 27, because in that case the code 
doesn't consult either XDG_RUNTIME_DIR or TMPDIR. So if that's an 
adequate solution, we should be able to continue with that.


> Given the goal to release Emacs 28.1 soon, I'm not sure there's time to come up with (and be confident in) a better solution for 28. In that case, I guess the available options are:
> 
> * If the security issue is considered minor enough, keep the current Emacs 28 behavior and silence the warning.

I'd rather not silence the warning as there is a real issue here 
(admittedly obscure...).


> * Otherwise, revert to the Emacs 27 behavior and come up with a better solution for Emacs 29. 

That sounds better, if the EMACS_SOCKET_NAME approach is good enough. 
Proposed patch attached. This patch attempts to be reasonably minimal vs 
what's in emacs-28 now (as opposed to cleaning up this somewhat-messy area).

[-- Attachment #2: 0001-emacsclient-takes-more-care-about-XDG_RUNTIME_DIR.patch --]
[-- Type: text/x-patch, Size: 1196 bytes --]

From 2cb6087f2b797d4ec21d27bc531998e07848797f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 8 Dec 2021 15:37:39 -0800
Subject: [PATCH] emacsclient takes more care about XDG_RUNTIME_DIR

* lib-src/emacsclient.c (set_local_socket): Revert to the Emacs 27
behavior of not trying TMPDIR if XDG_RUNTIME_DIR is set.
This is one of the suggestions made by Jim Porter and
independently by Ulrich Mueller in Bug#51327.
---
 lib-src/emacsclient.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib-src/emacsclient.c b/lib-src/emacsclient.c
index cff3cec2a7..d11fd88c45 100644
--- a/lib-src/emacsclient.c
+++ b/lib-src/emacsclient.c
@@ -1456,7 +1456,6 @@ set_local_socket (char const *server_name)
   else
     {
       /* socket_name is a file name component.  */
-      sock_status = ENOENT;
       char const *xdg_runtime_dir = egetenv ("XDG_RUNTIME_DIR");
       if (xdg_runtime_dir)
 	{
@@ -1466,7 +1465,7 @@ set_local_socket (char const *server_name)
 			 ? connect_socket (AT_FDCWD, sockname, s, 0)
 			 : ENAMETOOLONG);
 	}
-      if (sock_status == ENOENT)
+      else
 	{
 	  char const *tmpdir = egetenv ("TMPDIR");
 	  if (tmpdir)
-- 
2.32.0

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Ulrich Mueller]

>>>>> On Thu, 09 Dec 2021, Paul Eggert wrote:

>> * Otherwise, revert to the Emacs 27 behavior and come up with a
>> better solution for Emacs 29.

> That sounds better, if the EMACS_SOCKET_NAME approach is good enough.
> Proposed patch attached. This patch attempts to be reasonably minimal
> vs what's in emacs-28 now (as opposed to cleaning up this
> somewhat-messy area).

I could live with this solution for now.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Eli Zaretskii]

> From: Ulrich Mueller <ulm@gentoo.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Paul Eggert <eggert@cs.ucla.edu>,
>   jporterbugs@gmail.com,  51327@debbugs.gnu.org
> Date: Wed, 08 Dec 2021 22:56:24 +0100
> 
> >> If it's a new command-line argument, and if the participants in this
> >> discussion can live with it as the solution for this problem, I'm okay
> >> with having it on emacs-28.
> 
> That's not an acceptable solution, because it will break the existing
> workflow of users. Furthermore, it will make users jump through hoops to
> achieve functionality which was the default in previous versions.
> 
> So, can we please think about a better solution, and not knee-jerk
> something half-baked into Emacs 28, like checking for yet another
> environment variable?
> 
> Even reverting to the Emacs 27 behaviour would be better than what has
> been suggested above: In Emacs 27, you can set EMACS_SOCKET_NAME to make
> things work. There's no advantage in introducing yet another variable,
> which would only complicate things.

If you guys can arrive at something that you-all can live with, we can
make some progress, maybe even for Emacs 28.  But if everyone will
just keep saying that any solution except the one he/she suggested is
unacceptable, we have a stalemate, and that's not good for anyone.

So my suggestion to all the participants is to abandon the dogmas and
try to find a compromise which will be acceptable.  Bonus points for
finding a compromise that is simple enough to fix this on the release
branch.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Ulrich Mueller]

>>>>> On Thu, 09 Dec 2021, Eli Zaretskii wrote:

> If you guys can arrive at something that you-all can live with, we can
> make some progress, maybe even for Emacs 28.  But if everyone will
> just keep saying that any solution except the one he/she suggested is
> unacceptable, we have a stalemate, and that's not good for anyone.

> So my suggestion to all the participants is to abandon the dogmas and
> try to find a compromise which will be acceptable.  Bonus points for
> finding a compromise that is simple enough to fix this on the release
> branch.

See my earlier message from 00:19 UTC today?
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51327#78

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Paul Eggert]

On 12/8/21 23:44, Ulrich Mueller wrote:
> See my earlier message from 00:19 UTC today?
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51327#78

Yes, and since the latest proposal[1] does not add any environment 
variables or command-line options, and so is even simpler and 
less-intrusive than what Eli already OKed[2], I assume he'll be fine 
with it once he finds the time to read it.

[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51327#75
[2] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51327#63

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Eli Zaretskii]

> Date: Thu, 9 Dec 2021 09:12:47 -0800
> Cc: stefan@marxist.se, jporterbugs@gmail.com, 51327@debbugs.gnu.org,
>  Eli Zaretskii <eliz@gnu.org>
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 12/8/21 23:44, Ulrich Mueller wrote:
> > See my earlier message from 00:19 UTC today?
> > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51327#78
> 
> Yes, and since the latest proposal[1] does not add any environment 
> variables or command-line options, and so is even simpler and 
> less-intrusive than what Eli already OKed[2], I assume he'll be fine 
> with it once he finds the time to read it.

I've read it when you posted it, I'm just waiting for all the
participants to agree to that fix.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Jim Porter]

On 12/9/2021 10:34 AM, Eli Zaretskii wrote:
>> Date: Thu, 9 Dec 2021 09:12:47 -0800
>> Cc: stefan@marxist.se, jporterbugs@gmail.com, 51327@debbugs.gnu.org,
>>   Eli Zaretskii <eliz@gnu.org>
>> From: Paul Eggert <eggert@cs.ucla.edu>
>>
>> On 12/8/21 23:44, Ulrich Mueller wrote:
>>> See my earlier message from 00:19 UTC today?
>>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51327#78
>>
>> Yes, and since the latest proposal[1] does not add any environment
>> variables or command-line options, and so is even simpler and
>> less-intrusive than what Eli already OKed[2], I assume he'll be fine
>> with it once he finds the time to read it.
> 
> I've read it when you posted it, I'm just waiting for all the
> participants to agree to that fix.

I looked over the patch and tried it out briefly, and I'm ok with it 
too. As mentioned previously, I'm no expert on the security 
implications, but this does resolve my (perhaps slightly paranoid) 
concern about symlink attacks.

Hopefully we can come up with a longer term solution that makes it 
easier for Gentoo users to get things set up correctly, but I'm not sure 
off-hand what the best strategy there would be...

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Paul Eggert]

On 12/9/21 10:34, Eli Zaretskii wrote:
> I've read it when you posted it, I'm just waiting for all the
> participants to agree to that fix.

Ulrich agreed to it in 
<https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51327#78>.

Jim said it was one of the "available options" in 
<https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51327#72>.

These are the two other major participants as far as I know - did I miss 
somebody, or are we waiting for something more definitive from Jim?

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Eli Zaretskii]

> Date: Thu, 9 Dec 2021 11:48:05 -0800
> Cc: ulm@gentoo.org, stefan@marxist.se, jporterbugs@gmail.com,
>  51327@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 12/9/21 10:34, Eli Zaretskii wrote:
> > I've read it when you posted it, I'm just waiting for all the
> > participants to agree to that fix.
> 
> Ulrich agreed to it in 
> <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51327#78>.
> 
> Jim said it was one of the "available options" in 
> <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51327#72>.
> 
> These are the two other major participants as far as I know - did I miss 
> somebody, or are we waiting for something more definitive from Jim?

I did, and he just produced that.  So please go ahead and install on
the release branch.

Thanks.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Paul Eggert]

On 12/9/21 11:57, Eli Zaretskii wrote:
> please go ahead and install on
> the release branch.

Done.

Not closing the bug report as the bug hasn't been fixed (that can wait 
until Emacs 29).

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Not closing the bug report as the bug hasn't been fixed (that can wait
> until Emacs 29).

I'm not sure what the status is here now -- looking at the emacsclient.c
code, it seems like there's been some changes in this area, but it looks
like the problems discussed here may still be present?

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Paul Eggert via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 9/10/22 00:01, Lars Ingebrigtsen wrote:
> looking at the emacsclient.c
> code, it seems like there's been some changes in this area, but it looks
> like the problems discussed here may still be present?

Yes, that's my impression as well.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on-demand

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

Can we find a true expert to consult about this problem?

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on demand

[Ulrich Mueller]

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

If I understand this report correctly, the problem is just the spurious
warning about XDG_RUNTIME_DIR?

Instead of changing the functionality (which breaks other use cases, see
my message to emacs-devel), wouldn't it make more sense to just suppress
the warning if the variable is set? As in attached patch?

[-- Attachment #2: 0001-Suppress-a-spurious-warning-in-emacsclient.patch --]
[-- Type: text/plain, Size: 1755 bytes --]

From 8b13b4ce6b5c5998ea8dc6db0c1021f2beba41aa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ulrich=20M=C3=BCller?= <ulm@gentoo.org>
Date: Fri, 5 Nov 2021 11:30:28 +0100
Subject: [PATCH] Suppress a spurious warning in emacsclient

* lib-src/emacsclient.c (set_local_socket): Suppress warning about
unset XDG_RUNTIME_DIR if the variable is actually set.
(Bug#51327)
---
 lib-src/emacsclient.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib-src/emacsclient.c b/lib-src/emacsclient.c
index 0e800dd7e8..d812ae6bc8 100644
--- a/lib-src/emacsclient.c
+++ b/lib-src/emacsclient.c
@@ -1447,6 +1447,7 @@ set_local_socket (char const *server_name)
   int tmpdirlen = -1;
   int socknamelen = -1;
   uid_t uid = geteuid ();
+  char const *xdg_runtime_dir = NULL;
   bool tmpdir_used = false;
   int s = cloexec_socket (AF_UNIX, SOCK_STREAM, 0);
   if (s < 0)
@@ -1468,7 +1469,7 @@ set_local_socket (char const *server_name)
     {
       /* socket_name is a file name component.  */
       sock_status = ENOENT;
-      char const *xdg_runtime_dir = egetenv ("XDG_RUNTIME_DIR");
+      xdg_runtime_dir = egetenv ("XDG_RUNTIME_DIR");
       if (xdg_runtime_dir)
 	{
 	  socknamelen = snprintf (sockname, socknamesize, "%s/emacs/%s",
@@ -1559,7 +1560,8 @@ set_local_socket (char const *server_name)
 	  int sockdirnamelen = snprintf (sockdirname, sizeof sockdirname,
 					 "/run/user/%"PRIuMAX, id);
 	  if (0 <= sockdirnamelen && sockdirnamelen < sizeof sockdirname
-	      && faccessat (AT_FDCWD, sockdirname, X_OK, AT_EACCESS) == 0)
+	      && faccessat (AT_FDCWD, sockdirname, X_OK, AT_EACCESS) == 0
+	      && !xdg_runtime_dir)
 	    message
 	      (true,
 	       ("%s: Should XDG_RUNTIME_DIR='%s' be in the environment?\n"
-- 
2.33.1

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on demand

[Jim Porter]

On 11/5/2021 3:38 AM, Ulrich Mueller wrote:
> If I understand this report correctly, the problem is just the spurious
> warning about XDG_RUNTIME_DIR?
> 
> Instead of changing the functionality (which breaks other use cases, see
> my message to emacs-devel), wouldn't it make more sense to just suppress
> the warning if the variable is set? As in attached patch?

It's not just a spurious warning; the warning is telling the user about 
a real problem, though the wording is a bit confusing for this 
particular case. If a user calls `emacsclient --alternate-editor=""' 
with XDG_RUNTIME_DIR set and no Emacs server running, emacsclient will 
check in both XDG_RUNTIME_DIR and TMPDIR to find the server socket 
before giving up and starting the daemon.

Since XDG_RUNTIME_DIR exists (at least in part) to prevent symlink 
attacks, Emacs should try to avoid checking TMPDIR in order to avoid 
this vulnerability. Emacs 27 is secure in this regard, since it *never* 
checks TMPDIR if XDG_RUNTIME_DIR is set. However, that behavior caused 
the problems described in bug#33847. The patch I posted is a compromise 
that restores the secure behavior for users who set the alternate editor 
and want to start the Emacs daemon on demand (it's not perfect though; 
see my reply in emacs-devel).

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on demand

[Ulrich Mueller]

Can someone please explain to me how an exploit on the _client_ side
would look like?

When starting the server, I can believe that there may be some surface
for a symlink attack. But once the daemon is running? What is the
security issue for the client checking TMPDIR?

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on demand

[Jim Porter]

(Cc'ing Paul Eggert, who can probably answer more confidently than me.)

On 11/5/2021 11:05 AM, Ulrich Mueller wrote:
> Can someone please explain to me how an exploit on the _client_ side
> would look like?
> 
> When starting the server, I can believe that there may be some surface
> for a symlink attack. But once the daemon is running? What is the
> security issue for the client checking TMPDIR?

I'm not an expert on this kind of attack, but my understanding is that 
it could go something like this:

  1. Attacker runs `evil-daemon' which puts its socket in /tmp/evil
  2. Attacker runs `ln -s /tmp/evil /tmp/emacs1000/server'
  3. User runs `emacsclient --alternate-editor=""'
  4. emacsclient doesn't see a socket in XDG_RUNTIME_DIR, checks TMPDIR
  5. emacsclient connects to evil-daemon

The evil-daemon probably can't get access to the user's files, but might 
be able to trick a user into entering some secret. I'll let others chime 
in too though, since like I said, I'm not an expert.

If I'm wrong and this isn't an a problem, then I agree that all we need 
to do here is silence the warning.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on demand

[Ulrich Mueller]

>>>>> On Fri, 05 Nov 2021, Jim Porter wrote:

> (Cc'ing Paul Eggert, who can probably answer more confidently than me.)
> On 11/5/2021 11:05 AM, Ulrich Mueller wrote:
>> Can someone please explain to me how an exploit on the _client_ side
>> would look like?

>> When starting the server, I can believe that there may be some
>> surface for a symlink attack. But once the daemon is running? What is
>> the security issue for the client checking TMPDIR?

> I'm not an expert on this kind of attack, but my understanding is that
> it could go something like this:

>  1. Attacker runs `evil-daemon' which puts its socket in /tmp/evil
>  2. Attacker runs `ln -s /tmp/evil /tmp/emacs1000/server'

Right, and IIUC this must be carefully timed to exploit some race
condition between permission checking and creating the socket. I am not
an expert on this either.

>  3. User runs `emacsclient --alternate-editor=""'
>  4. emacsclient doesn't see a socket in XDG_RUNTIME_DIR, checks TMPDIR
>  5. emacsclient connects to evil-daemon

Note that after locating the socket, emacsclient will double check for
sane permissions. That is, correct user id and _no_ write permission for
either group or others. That's why I think that there's little attack
surface on the client side, once the socket has been created.

> The evil-daemon probably can't get access to the user's files, but
> might be able to trick a user into entering some secret. I'll let
> others chime in too though, since like I said, I'm not an expert.

> If I'm wrong and this isn't an a problem, then I agree that all we
> need to do here is silence the warning.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on demand

[Ulrich Mueller]

>>>>> On Fri, 05 Nov 2021, Ulrich Mueller wrote:

>>>>> On Fri, 05 Nov 2021, Jim Porter wrote:
>> I'm not an expert on this kind of attack, but my understanding is that
>> it could go something like this:

>> 1. Attacker runs `evil-daemon' which puts its socket in /tmp/evil
>> 2. Attacker runs `ln -s /tmp/evil /tmp/emacs1000/server'

> Right, and IIUC this must be carefully timed to exploit some race
> condition between permission checking and creating the socket. I am
> not an expert on this either.

Thinking about it some more, when you always start the daemon with
XDG_RUNTIME_DIR present, there won't be a /tmp/emacs1000/server (at
least not one with correct user and permissions), and I don't believe
that a symlink attack would be possible.

OTOH, when you start the daemon without XDG_RUNTIME_DIR, then the socket
will be created in /tmp, but in that case you'd want the client to find
it there.

>> 3. User runs `emacsclient --alternate-editor=""'
>> 4. emacsclient doesn't see a socket in XDG_RUNTIME_DIR, checks TMPDIR
>> 5. emacsclient connects to evil-daemon

See above, unless the daemon was started without XDG*, there won't be
any socket in TMPDIR.

> Note that after locating the socket, emacsclient will double check for
> sane permissions. That is, correct user id and _no_ write permission for
> either group or others. That's why I think that there's little attack
> surface on the client side, once the socket has been created.

>> The evil-daemon probably can't get access to the user's files, but
>> might be able to trick a user into entering some secret. I'll let
>> others chime in too though, since like I said, I'm not an expert.

>> If I'm wrong and this isn't an a problem, then I agree that all we
>> need to do here is silence the warning.

The core issue is that /run/user/${UID}/ is transient and will disappear
after logout. So if you start an Emacs daemon then its process will
persist after logout, but its socket file will be gone and it will no
longer be possible to connect. This may not be a security issue, but it
may cause loss of data.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on demand

[Jim Porter]

On 11/11/2021 5:04 AM, Ulrich Mueller wrote:
>>>>>> On Fri, 05 Nov 2021, Ulrich Mueller wrote:
> 
>>>>>> On Fri, 05 Nov 2021, Jim Porter wrote:
>>> I'm not an expert on this kind of attack, but my understanding is that
>>> it could go something like this:
> 
>>> 1. Attacker runs `evil-daemon' which puts its socket in /tmp/evil
>>> 2. Attacker runs `ln -s /tmp/evil /tmp/emacs1000/server'
> 
>> Right, and IIUC this must be carefully timed to exploit some race
>> condition between permission checking and creating the socket. I am
>> not an expert on this either.
> 
> Thinking about it some more, when you always start the daemon with
> XDG_RUNTIME_DIR present, there won't be a /tmp/emacs1000/server (at
> least not one with correct user and permissions), and I don't believe
> that a symlink attack would be possible.
> 
> OTOH, when you start the daemon without XDG_RUNTIME_DIR, then the socket
> will be created in /tmp, but in that case you'd want the client to find
> it there.

The case I'm concerned about is when the daemon *hasn't* been started 
yet by the time emacsclient is called. In that case, emacsclient checks 
both XDG_RUNTIME_DIR and TMPDIR before giving up and starting the 
daemon. In this case, that means that even on a system where Emacs only 
uses XDG_RUNTIME_DIR in practice, it'll still search TMPDIR the first 
time when looking for the (non-existent) daemon. The question then is 
whether it's safe for the emacsclient to look in TMPDIR to confirm that 
no daemon already exists.

It's possible that this behavior is perfectly safe, but the way the code 
is currently written (plus Paul Eggert's reply in this bug) seem to 
indicate that it's vulnerable to attack. If it really is vulnerable, 
then I think it should be fixed; if it's safe, then just eliminating the 
warning is sufficient of course.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on demand

[Paul Eggert]

On 11/11/21 09:06, Jim Porter wrote:
> It's possible that this behavior is perfectly safe, but the way the code 
> is currently written (plus Paul Eggert's reply in this bug) seem to 
> indicate that it's vulnerable to attack.

Yes, that's indeed the worry.

bug#51327: 28.0.60; emacsclient warns about XDG_RUNTIME_DIR when starting daemon on demand

[Stefan Kangas]

Ulrich Mueller <ulm@gentoo.org> writes:

> If I understand this report correctly, the problem is just the spurious
> warning about XDG_RUNTIME_DIR?
>
> Instead of changing the functionality (which breaks other use cases, see
> my message to emacs-devel), wouldn't it make more sense to just suppress
> the warning if the variable is set? As in attached patch?

The discussion is here:
https://lists.gnu.org/archive/html/emacs-devel/2021-11/msg00394.html