Re: sudo:: method in tramp possible security issue

Re: sudo:: method in tramp possible security issue

[João Távora]

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

Hello emacs-devel,

The off-list discussion below is about TRAMP's usage of
the /sudo:: method, which surprised me very much recently
because I discovered that it lets any elisp run arbitrary shell
commands with root permissions while the buffer editing a file
with /sudo:: is live.

So in theory you could write malicious elisp code to lay there
hoping to hijack a users system on their first file of
/sudo::/etc/apt/sources.list, for example. Supposing all the user
wanted is to edit that file, starting a full "elisp sudo server" for
the duration of the buffer is clearly overkill and unnecessarily
dangerous for most users.

For me this is a very serious security hole, but apparently
it's part of the contract of the /sudo:: method.

I am arguing for:

1. A sudoedit method that works like `sudo -e`

2. A one-time stern warning the first time that the user uses /sudo::
to explain the security implications to new users.

Michael and I are converging on some possibilities, but I
think it's a good idea to have the rest of emacs-devel speak
their mind.

[Michael you have my new in-line replies below, too]

Thanks,
João


On Tue, Nov 20, 2018 at 1:53 PM Michael Albinus <michael.albinus@gmx.de>
wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> >     For the time being, the Tramp manual says:
> >
> >     --8<---------------cut here---------------start------------->8---
> >     ‘sudo’
> >
> >          Similar to ‘su’ method, ‘sudo’ uses ‘sudo’.  ‘sudo’ must have
> >          sufficient rights to start a shell.
> >     --8<---------------cut here---------------end--------------->8---
> >
> >     It says already that a shell is used, but perhaps we shall
> >     emphasize it more. As I said: manuals are unread ...
> >
> > Very true, and this is why I am suggesting an in-your-face warning.
>
> That would be bossy. People would start to modify the code in order to
> bypass this.
>

Perhaps I didn't explain myself. I'm talking about a one-time thing, like
the warnings for a disabled command. Something that you can answer
"I understand the risks: never ask for this again".

Once sudoedit is offered by Tramp, I expect a respective recommendation
> for Emacs.
>

OK. I believe this is not quite enough. But I'm talking for myself. I think
we should indeed raise the matter publicly.


> > And I don't agree it's like sudo -i, because I don't think there is an
> > easy way for non-root code running before the sudo -i session to
> > inject itself into the root session, or is there? Certainly not as
> > easy as writing some trivial elisp.
>
> Everybody is able to write such code. A simple `shell-command' with
> "sudo" might suffice to run any command with root permissions. Or use
> the interactive `shell', and call "sudo -i" there. Nothing you need
> Tramp for.
>

I was just arguing that the comparison between "sudo -i", meant to be
run in a shell and tramp's launching of a "sudo shell" isn't adequate,
because
there's no easy way for some unprivileged code to stay around
and wait for an opportunity to hijack your interactive "sudo -i"


>
> > /Sudoedit:: would be a really good addition. It would replace all the
> > usage I and quite possibly many others have ever had of /sudo::.
>
> Agreed. If it is possible to implement as proposed.
>
> (Please write the bug report about, in order to discuss it with greater
> audience. Or start a discussion at the emacs-devel ML.)
>

I just did that.

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

Re: sudo:: method in tramp possible security issue

[Paul Eggert]

Is there a simple way to configure Emacs so that it does not use this 
sudo (or sudoedit) feature of Tramp? If not, perhaps there should be one.

I long ago put (setq tramp-mode nil) into my ~/.emacs file because of 
security concerns, so I wouldn't need to selectively disable sudo 
myself. But perhaps others who are less concerned about security (but 
still somewhat concerned) might want it.

Come to think of it, if 'emacs -Q' enables Tramp by default then perhaps 
I should stop using 'emacs -Q'....

Re: sudo:: method in tramp possible security issue

[Stefan Monnier]

> Is there a simple way to configure Emacs so that it does not use this sudo
> (or sudoedit) feature of Tramp?

Yes: use the default config and just don't use Tramp's sudo method.

> I long ago put (setq tramp-mode nil) into my ~/.emacs file because of
> security concerns, so I wouldn't need to selectively disable sudo
> myself.

Tramp is not magical: it can do no more nor less than what an attacker
could do.  So disabling it doesn't buy you anything in this respect, AFAICT.


        Stefan

Re: sudo:: method in tramp possible security issue

[Paul Eggert]

On 11/20/18 1:18 PM, Stefan Monnier wrote:
> Tramp is not magical: it can do no more nor less than what an attacker
> could do.

Sure, if the attacker has control over my keyboard, or over my display, 
or over the Lisp code that I load and execute. That being said, Tramp 
does make attacks easier, so it has been an easy call for me to disable it.

Re: sudo:: method in tramp possible security issue

[Stefan Monnier]

>> Tramp is not magical: it can do no more nor less than what an attacker
>> could do.
> Sure, if the attacker has control over my keyboard, or over my display, or
> over the Lisp code that I load and execute.  That being said, Tramp does make
> attacks easier, so it has been an easy call for me to disable it.

I don't see in which way you think it makes attacks easier.
Are you thinking if things like file-local variables which may point
to a file like "/sudo:..."?

I'd expect that in most such cases such vars pointing to arbitrary files
would be a risk even without the sudo method, so I'd hope we'd plug
those quickly enough (and yes, the sudo method would make such attacks
worse, indeed).


        Stefan

Re: sudo:: method in tramp possible security issue

[Michael Albinus]

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

> On 11/20/18 1:18 PM, Stefan Monnier wrote:
>> Tramp is not magical: it can do no more nor less than what an attacker
>> could do.
>
> Sure, if the attacker has control over my keyboard, or over my
> display, or over the Lisp code that I load and execute. That being
> said, Tramp does make attacks easier, so it has been an easy call for
> me to disable it.

Tramp's sudo method needs your credentials. If you don't provide them,
Tramp cannot do anything.

Like calling sudo in a terminal.

Best regards, Michael.

Re: sudo:: method in tramp possible security issue

[João Távora]

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

On Tue, Nov 20, 2018 at 10:06 PM Michael Albinus <michael.albinus@gmx.de>
wrote:

> Paul Eggert <eggert@cs.ucla.edu> writes:
>
> > On 11/20/18 1:18 PM, Stefan Monnier wrote:
> >> Tramp is not magical: it can do no more nor less than what an attacker
> >> could do.
> >
> > Sure, if the attacker has control over my keyboard, or over my
> > display, or over the Lisp code that I load and execute. That being
> > said, Tramp does make attacks easier, so it has been an easy call for
> > me to disable it.
>
> Tramp's sudo method needs your credentials. If you don't provide them,
> Tramp cannot do anything.
>
> Like calling sudo in a terminal.


It's not exactly like calling sudo in a terminal, because when you
use sudo you generally:

1. perform a one time action and are back at a non-sudo prompt; OR
2. start an interactive superuser session that easy to identify visually
   and for which there isn't a programmatic way for other programs
   to interfere

In other words, what bothers me the most about the sudo:: method is
the persistent sudo session that makes me vulnerable to attackers, and
to my elisp developing mistakes.  This is why I think a warning makes
sense, or some visual way to identify this vulnerable state.

In contrast, using sudoedit:: should not bring about this vulnerable state.

That being said, if your non-elevated user has already been compromised,
entering sudo credentials into Emacs, where elisp can do whatever, is
probably a very bad idea, regardless of Tramp.

João

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

Re: sudo:: method in tramp possible security issue

[Stefan Monnier]

> In other words, what bothers me the most about the sudo:: method is
> the persistent sudo session that makes me vulnerable to attackers, and
> to my elisp developing mistakes.  This is why I think a warning makes
> sense, or some visual way to identify this vulnerable state.

I guess it all depends on the sudo setup:

Can you run a shell via sudo?  On those machines where I can do that,
I typically do "sudo zsh" and then live happily in my root shell.
But even you don't, after you've used sudo, there's a time window
during which sudo won't ask for your password and during which an
attacker could run "sudo sh" via start-process, regardless of Tramp.

If you can't run a shell via sudo, then Tramp's sudo method won't work
anyway.


        Stefan

Re: sudo:: method in tramp possible security issue

[Michael Albinus]

João Távora <joaotavora@gmail.com> writes:

>     Tramp's sudo method needs your credentials. If you don't provide
>     them, Tramp cannot do anything.
>
>     Like calling sudo in a terminal.
>
> It's not exactly like calling sudo in a terminal, because when you
> use sudo you generally:
>
> 1. perform a one time action and are back at a non-sudo prompt; OR
> 2. start an interactive superuser session that easy to identify
> visually 
>    and for which there isn't a programmatic way for other programs 
>    to interfere
>
> In other words, what bothers me the most about the sudo:: method is 
> the persistent sudo session that makes me vulnerable to attackers, and
> to my elisp developing mistakes.  This is why I think a warning makes 
> sense, or some visual way to identify this vulnerable state.

There is already a "visual way to identify this state". It is called
tramp-theme, a GNU ELPA package.

This is documented in the Tramp manual, see (info "(tramp) Frequently Asked Questions")
Again, nobody reads the manual :-(

The command `tramp-cleanup-connection' closes any background session for
a Tramp connection, including removing cached passwords. Maybe we shall
call this for sudo/su methods automatically after a given timeout, like
the password expiration for sudo in a terminal. 5 minutes seem to be a
sensible value to me.

> João

Best regards, Michael.

Re: sudo:: method in tramp possible security issue

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

> The command `tramp-cleanup-connection' closes any background session for
> a Tramp connection, including removing cached passwords. Maybe we shall
> call this for sudo/su methods automatically after a given timeout, like
> the password expiration for sudo in a terminal. 5 minutes seem to be a
> sensible value to me.

I give it a try. I've pushed a change to the master branch, which
expires sudo and doas background sessions after 5 minutes. Check the
Tramp manual for details.

Let's see how it goes.

>> João

Best regards, Michael.

Re: sudo:: method in tramp possible security issue

[Filipp Gunbin]

On 21/11/2018 08:41 +0100, Michael Albinus wrote:

> Maybe we shall call this for sudo/su methods automatically after a
> given timeout, like the password expiration for sudo in a terminal. 5
> minutes seem to be a sensible value to me.

Yes, that'd be nice (for root).

Filipp

Re: sudo:: method in tramp possible security issue

[Michael Albinus]

Filipp Gunbin <fgunbin@fastmail.fm> writes:

Hi Filipp,

>> Maybe we shall call this for sudo/su methods automatically after a
>> given timeout, like the password expiration for sudo in a terminal. 5
>> minutes seem to be a sensible value to me.
>
> Yes, that'd be nice (for root).

I have just committed a patch to master, which does it for all "sudo"
connections. If you want it only for root, you could configure it as
such. See the Tramp manual.

> Filipp

Best regards, Michael.

Re: sudo:: method in tramp possible security issue

[John Shahid]

Michael Albinus <michael.albinus@gmx.de> writes:

> The command `tramp-cleanup-connection' closes any background session for
> a Tramp connection, including removing cached passwords. Maybe we shall
> call this for sudo/su methods automatically after a given timeout, like
> the password expiration for sudo in a terminal. 5 minutes seem to be a
> sensible value to me.

Warning, I am not familiar with the internals of tramp, so please
forgive my following comment if it doesn't make sense.

Is there a reason for doing a manual expiration instead of relying on
the default sudo behavior.  If tramp start a new sudo shell for example
to get file attributes, then sudo can take care of caching the password
or asking for it after the configured timeout.  That would consolidate
the configuration in one place (i.e. /etc/sudoers for the timeout) as
well as let users manage the cache (e.g. sudo -k when the user logs out)
the same way they do today.

Re: sudo:: method in tramp possible security issue

[Michael Albinus]

John Shahid <jvshahid@gmail.com> writes:

Hi John,

> Is there a reason for doing a manual expiration instead of relying on
> the default sudo behavior.  If tramp start a new sudo shell for example
> to get file attributes, then sudo can take care of caching the password
> or asking for it after the configured timeout.  That would consolidate
> the configuration in one place (i.e. /etc/sudoers for the timeout) as
> well as let users manage the cache (e.g. sudo -k when the user logs out)
> the same way they do today.

The point is that Tramp (until now) keeps a session open forever. Tramp
doesn't "start a new sudo shell for example to get file attributes".
Therefore, there's no chance that sudo could ask for a password,
again. That's why the new mechanism interrupts the session after the
session timeout, and opening a new one depends on sudo's mechanism for
cached passwords.

Best regards, Michael.

Re: sudo:: method in tramp possible security issue

[John Shahid]

Michael Albinus <michael.albinus@gmx.de> writes:

> John Shahid <jvshahid@gmail.com> writes:
>
> Hi John,
>
>> Is there a reason for doing a manual expiration instead of relying on
>> the default sudo behavior.  If tramp start a new sudo shell for example
>> to get file attributes, then sudo can take care of caching the password
>> or asking for it after the configured timeout.  That would consolidate
>> the configuration in one place (i.e. /etc/sudoers for the timeout) as
>> well as let users manage the cache (e.g. sudo -k when the user logs out)
>> the same way they do today.
>
> The point is that Tramp (until now) keeps a session open forever. Tramp
> doesn't "start a new sudo shell for example to get file attributes".
> Therefore, there's no chance that sudo could ask for a password,
> again. That's why the new mechanism interrupts the session after the
> session timeout, and opening a new one depends on sudo's mechanism for
> cached passwords.

That was the essence of my question.  What is stopping us from starting
a new session as needed instead of keeping one around forever ?

Re: sudo:: method in tramp possible security issue

[Michael Albinus]

John Shahid <jvshahid@gmail.com> writes:

Hi John,

> That was the essence of my question.  What is stopping us from starting
> a new session as needed instead of keeping one around forever ?

Performance. Opening a new sudo session every single information bit
would be awful slow.

We could set a command timeout for the sudo invocation, but nobody
guarantees that it happens while data are transferred.

Best regards, Michael.

Re: sudo:: method in tramp possible security issue

[Michael Albinus]

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

> Is there a simple way to configure Emacs so that it does not use this
> sudo (or sudoedit) feature of Tramp? If not, perhaps there should be
> one.

Remove the corresponding entries from tramp-methods. Something like

(setq tramp-methods (delete (assoc "sudo" tramp-methods) tramp-methods))

Same for "su", and "sudoedit" (once it is added to Tramp). However,
"sudoedit" is intented to just read a file, and save the respective
buffer. No interactive shell under the hood, no backup files, no remote
processes - nothing else but file reading and buffer saving. The idea is
to use a proper emacsclient call for this.

> I long ago put (setq tramp-mode nil) into my ~/.emacs file because of
> security concerns, so I wouldn't need to selectively disable sudo
> myself. But perhaps others who are less concerned about security (but
> still somewhat concerned) might want it.
>
> Come to think of it, if 'emacs -Q' enables Tramp by default then
> perhaps I should stop using 'emacs -Q'....

Tramp manifests itself via autoloads. They are still active when running
'emacs -Q'.

Tramp does already some actions when it detects that emacs has started
with "-Q". For example, it doesn't read the persistency file
"~/.emacs.d/tramp". We could extend this mechanism.

Best regards, Michael.

Re: sudo:: method in tramp possible security issue

[Michael Albinus]

João Távora <joaotavora@gmail.com> writes:

> Hello emacs-devel, 

Hi João,

> The off-list discussion below is about TRAMP's usage of 
> the /sudo:: method, which surprised me very much recently 
> because I discovered that it lets any elisp run arbitrary shell 
> commands with root permissions while the buffer editing a file 
> with /sudo:: is live.  
>
> So in theory you could write malicious elisp code to lay there
> hoping to hijack a users system on their first file of 
> /sudo::/etc/apt/sources.list, for example. Supposing all the user 
> wanted is to edit that file, starting a full "elisp sudo server" for 
> the duration of the buffer is clearly overkill and unnecessarily 
> dangerous for most users.

It isn't overkill. The implementation in Tramp depends on the file name
handler concept, which requires to implement 70 basic functions. How
would it be possible to implement `file-attributes', for example, w/o an
interactive shell with root permissions?

> For me this is a very serious security hole, but apparently
> it's part of the contract of the /sudo:: method.
>
> I am arguing for:
>
> 1. A sudoedit method that works like `sudo -e`

Agreed. It shall basically implement just `insert-file-contents' and
`write-region'. (If possible, I haven't started to investigate in detail).

> 2. A one-time stern warning the first time that the user uses /sudo:: 
> to explain the security implications to new users.

Here I'm not convinced. I agree that it must be said more prominent in
the Tramp manual, that an interactive session with root permissions is
running in the background, but I believe it would be too bossy to tell
users they shall not use "/sudo::". It is like telling something like
this to users, who call sudo in a terminal. Are there such warnings,
somewhere?

> Michael and I are converging on some possibilities, but I
> think it's a good idea to have the rest of emacs-devel speak
> their mind.
>
> Thanks,
> João

Best regards, Michael.

Re: sudo:: method in tramp possible security issue

[João Távora]

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

On Tue, Nov 20, 2018 at 10:30 PM Michael Albinus <michael.albinus@gmx.de>
wrote:

>
> It isn't overkill. The implementation in Tramp depends on the file name
> handler concept, which requires to implement 70 basic functions. How
> would it be possible to implement `file-attributes', for example, w/o an
> interactive shell with root permissions?
>

Sorry, I meant overkill for most **uses**.  By which I mean 100%
of uses that **I** have ever had for /sudo::, which is to quickly
edit a system file. Of course there are other uses that other
users might want.


> Here I'm not convinced. I agree that it must be said more prominent in
> the Tramp manual, that an interactive session with root permissions is
> running in the background, but I believe it would be too bossy to tell
> users they shall not use "/sudo::". It is like telling something like
> this to users, who call sudo in a terminal. Are there such warnings,
> somewhere?
>

I've explained that I don't think it is the same to run sudo in a
terminal and inside emacs.  And I'm not suggesting "/sudo::
considered dangerous, thou shalt not use it!", just a one time
summary explanation of what is about to happen.

Alternatively, how would you feel about adding something to
the mode-line clearly showing that there is an ongoing
superuser session going on?

For me, mentioning it in the manual isn't very useful, because
few people read the manual, fine as it may be :-)

João Távora

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

Re: sudo:: method in tramp possible security issue

[Michael Albinus]

João Távora <joaotavora@gmail.com> writes:

> Alternatively, how would you feel about adding something to 
> the mode-line clearly showing that there is an ongoing 
> superuser session going on?

As said the other mail, this is provided by the GNU ELPA package
tramp-theme. When writing this package I decided against making it the
default, because people are very idiosyncratic about the modeline.

> For me, mentioning it in the manual isn't very useful, because
> few people read the manual, fine as it may be :-)

Maintaining Tramp for ~15 years, I'm often feeling that writing
something into the manual is superfluous. For almost every explanation
there, I still get request to repeat the description in the mailing list
/ on stackexchange / you name it.

> João Távora

Best regards, Michael.

Tramp sudoedit method (was: sudo:: method in tramp possible security issue)

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

Hi,

>>> Right, but would it run emacs-lisp code with elevated privileges?
>>> Can't we just make a sudoedit method that just invokes the sudoedit
>>> program or "sudo -e"?  This is what I thought sudo:: did in the first place.
>>
>> All of Tramp basically works by starting an `sh` process (via ssh, sudo,
>> you name it) and then sending it commands to retrieve meta-info about
>> files, contents of file, etc...
>
> Right. But maybe we could offer a Tramp method "sudoedit", which behaves
> differently. If somebody opens "C-x C-f /sudoedit::/etc/passwd", Tramp
> starts in the background the process "env EDITOR=emacsclient sudoedit
> /etc/passwd" (plus fiddling with `server-start', password handling, and
> alike). Nothing else but just editing the file, and saving it, would be
> possible.
>
> Don't know what it means for implementation, because visiting a file is
> more than just calling `insert-file-contents'. Handlers for all the
> other magic file operations should return proper results, w/o running a
> shell under root permissions.

I gave it a try. I could make `insert-file-contents' and `write-region'
run based on emacsclient, but it wasn't mature enough. So I have
implemented the "sudoedit" method by applying a respective sudo call for
everything. It isn't using the sudoedit command any longer, but I kept
the method name, because it behaves similar. If it confuses people too
much, we could change the name.

Contrary to the "sudo" method, it is applicable only to the local host,
and it cannot be used in multi-hop methods. Furthermore, it has a worse
performance than "sudo", especially for large directories like "/etc",
visited with `dired'. Maybe this could be improved by a native
implementation of `directory-files-and-attributes' and `insert-directory'.

But it keeps the security promise, not to run a shell in the background
with other user credentials, which could be attacked by malicious code.

Pushed to master. What do people think?

>>         Stefan

Best regards, Michael.