bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

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

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


Hi,

I'm surely missing something but wouldn't it be a good thing to tighten
auto-save files mode.  Mostly for remote or sudo files, this seems like
a better default.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Restrict-auto-save-file-mode.patch --]
[-- Type: text/x-patch, Size: 1381 bytes --]

From 0039dd6b5076d3edd61b15f017c27d9424ad559e Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Sat, 18 Mar 2023 16:10:44 +0100
Subject: [PATCH] Restrict auto-save file mode

* src/fileio.c (auto_save_1): Restrict auto-save file to user's
mode only.
---
 src/fileio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/fileio.c b/src/fileio.c
index b80f8d61de4..73c7dc5e063 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -5905,20 +5905,20 @@ auto_save_1 (void)
   struct stat st;
   Lisp_Object modes;
 
-  auto_save_mode_bits = 0666;
+  auto_save_mode_bits = 0600;
 
-  /* Get visited file's mode to become the auto save file's mode.  */
+  /* Get visited file's user mode to become the auto save file's mode.  */
   if (! NILP (BVAR (current_buffer, filename)))
     {
       if (emacs_fstatat (AT_FDCWD, SSDATA (BVAR (current_buffer, filename)),
 			 &st, 0)
 	  == 0)
 	/* But make sure we can overwrite it later!  */
-	auto_save_mode_bits = (st.st_mode | 0600) & 0777;
+	auto_save_mode_bits = (st.st_mode | 0600) & 0700;
       else if (modes = Ffile_modes (BVAR (current_buffer, filename), Qnil),
 	       FIXNUMP (modes))
 	/* Remote files don't cooperate with fstatat.  */
-	auto_save_mode_bits = (XFIXNUM (modes) | 0600) & 0777;
+	auto_save_mode_bits = (XFIXNUM (modes) | 0600) & 0700;
     }
 
   return
-- 
2.39.2


[-- Attachment #3: Type: text/plain, Size: 7650 bytes --]



In GNU Emacs 30.0.50 (build 1, x86_64-unknown-openbsd7.3, cairo version
 1.17.8) of 2023-03-18 built on computer
Repository revision: 4234e204ec0e73211e0041d78460b2c51913a517
Repository branch: mgi/restrict-auto-save
Windowing system distributor 'The X.Org Foundation', version 11.0.12101006
System Description: OpenBSD computer 7.3 GENERIC.MP#1105 amd64

Configured using:
 'configure --prefix=/home/manuel/emacs --bindir=/home/manuel/bin
 --with-x-toolkit=no --without-sound --without-compress-install
 CPPFLAGS=-I/usr/local/include LDFLAGS=-L/usr/local/lib'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LCMS2 LIBOTF LIBXML2 MODULES NOTIFY KQUEUE OLDXMENU PDUMPER PNG RSVG
SQLITE3 THREADS TIFF TREE_SITTER WEBP X11 XDBE XIM XINPUT2 XPM ZLIB

Important settings:
  value of $LC_ALL: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Dired by name

Minor modes in effect:
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  gnus-dired-mode: t
  display-time-mode: t
  display-battery-mode: t
  server-mode: t
  shell-dirtrack-mode: t
  repeat-mode: t
  desktop-save-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  buffer-read-only: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/manuel/.emacs.d/elpa/ef-themes-0.10.0/theme-loaddefs hides /home/manuel/emacs/share/emacs/30.0.50/lisp/theme-loaddefs
/home/manuel/.emacs.d/elpa/transient-0.3.7/transient hides /home/manuel/emacs/share/emacs/30.0.50/lisp/transient

Features:
(shadow sort mail-extr dabbrev emacsbug whitespace magit-patch
tramp-cmds pulse magit-extras face-remap magit-submodule magit-obsolete
magit-blame magit-stash magit-reflog magit-bisect magit-push magit-pull
magit-fetch magit-clone magit-remote magit-commit magit-sequence
magit-notes magit-worktree magit-tag magit-merge magit-branch
magit-reset magit-files magit-refs magit-status magit magit-repos
magit-apply magit-wip magit-log which-func magit-diff git-commit
log-edit add-log magit-core magit-autorevert magit-margin
magit-transient magit-process with-editor magit-mode transient magit-git
magit-section magit-utils dash vc-bzr rfc2104 tramp-cache time-stamp
tramp-sh org-indent idlwave idlwave-help idlw-help emacs-news-mode view
vc-dir ewoc rng-xsd xsd-regexp rng-cmpct rng-nxml rng-valid rng-loc
rng-uri rng-parse nxml-parse rng-match rng-dt rng-util rng-pttrn nxml-ns
nxml-mode nxml-outln nxml-rap nxml-util nxml-enc xmltok pascal vc-cvs
vc-rcs log-view pcvs-util conf-mode css-mode sgml-mode facemenu imenu
vc-hg sh-script smie treesit executable smerge-mode diff oc-basic ol-eww
eww url-queue mm-url ol-rmail ol-mhe ol-irc ol-info ol-gnus nnselect
ol-docview doc-view jka-compr image-mode exif ol-bibtex bibtex ol-bbdb
ol-w3m ol-doi org-link-doi mule-util paredit edmacro autorevert
filenotify vc-git diff-mode vc-svn vc vc-dispatcher bug-reference
gnus-dired time battery cus-load exwm-randr xcb-randr exwm-config ido
exwm exwm-input xcb-keysyms xcb-xkb exwm-manage exwm-floating xcb-cursor
xcb-render exwm-layout exwm-workspace exwm-core xcb-ewmh xcb-icccm xcb
xcb-xproto xcb-types xcb-debug kmacro server modus-operandi-theme
modus-themes ytdious mingus libmpdee reporter edebug debug backtrace
transmission color calc-bin calc-ext calc calc-loaddefs rect calc-macs
supercite regi ebdb-message ebdb-gnus gnus-msg gnus-art mm-uu mml2015
mm-view mml-smime smime gnutls dig gnus-sum shr pixel-fill kinsoku
url-file svg dom gnus-group gnus-undo gnus-start gnus-dbus gnus-cloud
nnimap nnmail mail-source utf7 nnoo gnus-spec gnus-int gnus-range
message sendmail yank-media puny rfc822 mml mml-sec epa epg rfc6068
epg-config mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047
rfc2045 ietf-drums gmm-utils mailheader gnus-win gnus nnheader gnus-util
mail-utils range mm-util mail-prsvr wid-edit ebdb-mua ebdb-com crm
ebdb-format ebdb mailabbrev eieio-opt cl-extra help-mode speedbar
ezimage dframe eieio-base pcase timezone visual-basic-mode cl web-mode
derived disp-table erlang-start smart-tabs-mode skeleton cc-mode
cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars
cc-defs slime-asdf grep slime-tramp tramp tramp-loaddefs trampver
tramp-integration files-x tramp-compat shell parse-time iso8601 ls-lisp
slime-fancy slime-indentation slime-cl-indent cl-indent
slime-trace-dialog slime-fontifying-fu slime-package-fu slime-references
slime-compiler-notes-tree advice slime-scratch slime-presentations
bridge slime-macrostep macrostep slime-mdot-fu slime-enclosing-context
slime-fuzzy slime-fancy-trace slime-fancy-inspector slime-c-p-c
slime-editing-commands slime-autodoc slime-repl slime-parse slime
apropos compile text-property-search etags fileloop xref project
arc-mode archive-mode pp hyperspec thingatpt slime-autoloads org-agenda
org-element org-persist xdg org-id avl-tree generator org-refile org ob
ob-tangle ob-ref ob-lob ob-table ob-exp org-macro org-src ob-comint
org-pcomplete pcomplete comint ansi-osc ansi-color ring org-list
org-footnote org-faces org-entities time-date noutline outline icons
ob-emacs-lisp ob-core ob-eval org-cycle org-table org-keys oc
org-loaddefs find-func ol rx org-fold org-fold-core org-compat
org-version org-macs format-spec appt diary-lib diary-loaddefs cal-menu
calendar cal-loaddefs dired-aux dired-x dired dired-loaddefs
notifications dbus xml repeat easy-mmode desktop frameset osm-autoloads
rust-mode-autoloads ebdb-autoloads compat-autoloads magit-autoloads
debbugs-autoloads git-commit-autoloads magit-section-autoloads
ef-themes-autoloads with-editor-autoloads paredit-autoloads
dash-autoloads ytdious-autoloads transmission-autoloads
transient-autoloads exwm-autoloads hyperbole-autoloads
detached-autoloads info package browse-url url url-proxy url-privacy
url-expand url-methods url-history url-cookie generate-lisp-file
url-domsuf url-util mailcap url-handlers url-parse auth-source cl-seq
eieio eieio-core cl-macs password-cache json subr-x map byte-opt gv
bytecomp byte-compile url-vars cl-loaddefs cl-lib rmc iso-transl tooltip
cconv eldoc paren electric uniquify ediff-hook vc-hooks lisp-float-type
elisp-mode mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd
fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow
isearch easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind kqueue lcms2
dynamic-setting system-font-setting font-render-setting cairo xinput2 x
multi-tty make-network-process emacs)

Memory information:
((conses 16 765135 152535)
 (symbols 48 60399 6)
 (strings 32 197023 9623)
 (string-bytes 1 6230759)
 (vectors 16 117472)
 (vector-slots 8 2355311 71951)
 (floats 8 566 318)
 (intervals 56 21464 629)
 (buffers 984 137))

-- 
Manuel Giraud

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Eli Zaretskii]

> Date: Sat, 18 Mar 2023 16:18:07 +0100
> From:  Manuel Giraud via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I'm surely missing something but wouldn't it be a good thing to tighten
> auto-save files mode.  Mostly for remote or sudo files, this seems like
> a better default.

That could make it impossible to overwrite the auto-save file later.
So I don't think we should make this change.

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

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

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> That could make it impossible to overwrite the auto-save file later.
> So I don't think we should make this change.

I've tried it but I'm not able to come with such an example.  With this
patch, the auto-save file has always a mode of 0600 even for a file with
a mode of 0006 not own by me.
-- 
Manuel Giraud

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 62260@debbugs.gnu.org
> Date: Sat, 18 Mar 2023 19:39:20 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> > That could make it impossible to overwrite the auto-save file later.
> > So I don't think we should make this change.
> 
> I've tried it but I'm not able to come with such an example.  With this
> patch, the auto-save file has always a mode of 0600 even for a file with
> a mode of 0006 not own by me.

Did you try a different user?

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Eli Zaretskii]

> Cc: 62260@debbugs.gnu.org
> Date: Sat, 18 Mar 2023 20:47:17 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Manuel Giraud <manuel@ledu-giraud.fr>
> > Cc: 62260@debbugs.gnu.org
> > Date: Sat, 18 Mar 2023 19:39:20 +0100
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > [...]
> > 
> > > That could make it impossible to overwrite the auto-save file later.
> > > So I don't think we should make this change.
> > 
> > I've tried it but I'm not able to come with such an example.  With this
> > patch, the auto-save file has always a mode of 0600 even for a file with
> > a mode of 0006 not own by me.
> 
> Did you try a different user?

Actually, the right question is: what exactly did you try?  As you can
see from the code, 0666 value is used only for buffers that don't
visit files.  Did you try with such a buffer, and what exactly did you
try?

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

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

Eli Zaretskii <eliz@gnu.org> writes:

>> I've tried it but I'm not able to come with such an example.  With this
>> patch, the auto-save file has always a mode of 0600 even for a file with
>> a mode of 0006 not own by me.
>
> Did you try a different user?

I've just tried but with sudo over TRAMP.  I had a message about the
lock that I stole and then Emacs created another auto-save file with
TRAMP naming (this file is also mode 0600).

I guess that using TRAMP is cheating here and that I should really be
another user using Emacs and trying to open this same file...  Then I
guess I won't be able to recover from (or write to) this auto-save file.
But does this kind of scenarios appear in real life?
-- 
Manuel Giraud

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Did you try a different user?
>
> Actually, the right question is: what exactly did you try?  As you can
> see from the code, 0666 value is used only for buffers that don't
> visit files.  Did you try with such a buffer, and what exactly did you
> try?

I didn't with a buffer not visiting a file.  I have tried with a file in
/tmp that by default on my system is created with mode 0644.  Then I
write something into this file and do 'M-x do-auto-save'.

With this patch, the /tmp directory now contains an auto-save file with
mode 0600.
-- 
Manuel Giraud

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 62260@debbugs.gnu.org
> Date: Sat, 18 Mar 2023 20:22:55 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Did you try a different user?
> >
> > Actually, the right question is: what exactly did you try?  As you can
> > see from the code, 0666 value is used only for buffers that don't
> > visit files.  Did you try with such a buffer, and what exactly did you
> > try?
> 
> I didn't with a buffer not visiting a file.  I have tried with a file in
> /tmp that by default on my system is created with mode 0644.  Then I
> write something into this file and do 'M-x do-auto-save'.
> 
> With this patch, the /tmp directory now contains an auto-save file with
> mode 0600.

When a buffer visits a file, its auto-save file should have the same
mode bits as the file itself (modulo your umask).  I see no need to
affect that part in any case.  If the file itself is not private, why
should its auto-save file be private?  Also, there's
auto-save-visited-mode.

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Eli Zaretskii]

> Cc: 62260@debbugs.gnu.org
> Date: Sat, 18 Mar 2023 22:00:32 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > With this patch, the /tmp directory now contains an auto-save file with
> > mode 0600.
> 
> When a buffer visits a file, its auto-save file should have the same
> mode bits as the file itself (modulo your umask).  I see no need to
> affect that part in any case.  If the file itself is not private, why
> should its auto-save file be private?  Also, there's
> auto-save-visited-mode.

More generally: what problems are you trying to solve here?  If this
code causes some problems, please describe them, and let's see whether
and how we should fix them.

This code is in its present shape for the last 25 years at least, and
in all that time I don't think we ever had any complaints about the
mode bits of the auto-save files.  So if there's no real-life problem
here, I see no reason to change code that is stable for so long.

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 62260@debbugs.gnu.org
>> Date: Sat, 18 Mar 2023 22:00:32 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> 
>> > With this patch, the /tmp directory now contains an auto-save file with
>> > mode 0600.
>> 
>> When a buffer visits a file, its auto-save file should have the same
>> mode bits as the file itself (modulo your umask).  I see no need to
>> affect that part in any case.  If the file itself is not private, why
>> should its auto-save file be private?  Also, there's
>> auto-save-visited-mode.
>
> More generally: what problems are you trying to solve here?  If this
> code causes some problems, please describe them, and let's see whether
> and how we should fix them.

You're right.  I should have start from here.  So it all start with a
recurring message I get from TRAMP whenever I access a root file (via
sudo method) for the first time.  It asks:

  "Autosave file on local temporary directory, do you want to continue?"

I answer "yes" but it seems that it can potentially leak root data
through auto-save files.  Looking at the code that asks this question
(tramp.el:6528), I see that I can set
'tramp-allow-unsafe-temporary-files' to t and I won't see the question
again... but the leakage is still possible.  So I guess what I want is
an option to stop auto-save entirely in those cases.

From there, I've looked at how auto-save work and I ask myself: "maybe
instead of such an option, I could limit others rights on auto-save
files".  And that's how I came up with this patch.
-- 
Manuel Giraud

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 62260@debbugs.gnu.org
> Date: Sun, 19 Mar 2023 12:43:02 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > More generally: what problems are you trying to solve here?  If this
> > code causes some problems, please describe them, and let's see whether
> > and how we should fix them.
> 
> You're right.  I should have start from here.  So it all start with a
> recurring message I get from TRAMP whenever I access a root file (via
> sudo method) for the first time.  It asks:
> 
>   "Autosave file on local temporary directory, do you want to continue?"
> 
> I answer "yes" but it seems that it can potentially leak root data
> through auto-save files.  Looking at the code that asks this question
> (tramp.el:6528), I see that I can set
> 'tramp-allow-unsafe-temporary-files' to t and I won't see the question
> again... but the leakage is still possible.  So I guess what I want is
> an option to stop auto-save entirely in those cases.
> 
> >From there, I've looked at how auto-save work and I ask myself: "maybe
> instead of such an option, I could limit others rights on auto-save
> files".  And that's how I came up with this patch.

So this is limited to Tramp and how it handles auto-saving?  Adding
Michael, in case he has ideas for how to solve this issue.

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi,

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: 62260@debbugs.gnu.org
>> Date: Sun, 19 Mar 2023 12:43:02 +0100
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> > More generally: what problems are you trying to solve here?  If this
>> > code causes some problems, please describe them, and let's see whether
>> > and how we should fix them.
>>
>> You're right.  I should have start from here.  So it all start with a
>> recurring message I get from TRAMP whenever I access a root file (via
>> sudo method) for the first time.  It asks:
>>
>>   "Autosave file on local temporary directory, do you want to continue?"
>>
>> I answer "yes" but it seems that it can potentially leak root data
>> through auto-save files.  Looking at the code that asks this question
>> (tramp.el:6528), I see that I can set
>> 'tramp-allow-unsafe-temporary-files' to t and I won't see the question
>> again... but the leakage is still possible.  So I guess what I want is
>> an option to stop auto-save entirely in those cases.
>>
>> >From there, I've looked at how auto-save work and I ask myself: "maybe
>> instead of such an option, I could limit others rights on auto-save
>> files".  And that's how I came up with this patch.
>
> So this is limited to Tramp and how it handles auto-saving?  Adding
> Michael, in case he has ideas for how to solve this issue.

Read the Tramp manual (info "(tramp) Auto-save File Lock and Backup")
You can use auto-save-file-name-transforms or tramp-auto-save-directory
in order to change the location of auto-saved files. This is the
recommended way to protect sensible data.

Tramp has no influence on the permissions of the auto-saved file.

There's also bug#57395 with a related (but not identical) topic.

Best regards, Michael.

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

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

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

[...]

>> So this is limited to Tramp and how it handles auto-saving?  Adding
>> Michael, in case he has ideas for how to solve this issue.
>
> Read the Tramp manual (info "(tramp) Auto-save File Lock and Backup")
> You can use auto-save-file-name-transforms or tramp-auto-save-directory
> in order to change the location of auto-saved files. This is the
> recommended way to protect sensible data.

Hi Michael,

Thanks for your inputs.  I have tried with
'auto-save-file-name-transforms' like this:

(add-to-list 'auto-save-file-name-transforms
    '("\\`/\\(?:doas\\|ksu\\|su\\(?:do\\)?\\):.*\\'" "/dev/null" nil))

But now, auto-save tries to create "/dev/#null#".  How can I fix it to
"/dev/null"?
-- 
Manuel Giraud

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,  62260@debbugs.gnu.org
> Date: Sun, 19 Mar 2023 13:40:29 +0100
> 
> Tramp has no influence on the permissions of the auto-saved file.

My reading of the code in auto_save_1 is that the permission modes of
the auto-saved files for remote files are determined from their
original files, by calling file-modes (which I believe Tramp
implements?).  See the 'else' branch of this part of auto_save_1:

  /* Get visited file's mode to become the auto save file's mode.  */
  if (! NILP (BVAR (current_buffer, filename)))
    {
      if (emacs_fstatat (AT_FDCWD, SSDATA (BVAR (current_buffer, filename)),
			 &st, 0)
	  == 0)
	/* But make sure we can overwrite it later!  */
	auto_save_mode_bits = (st.st_mode | 0600) & 0777;
      else if (modes = Ffile_modes (BVAR (current_buffer, filename), Qnil),
	       FIXNUMP (modes))
	/* Remote files don't cooperate with fstatat.  */
	auto_save_mode_bits = (XFIXNUM (modes) | 0600) & 0777;
    }

If by "Tramp has no influence on the permissions of the auto-saved
file" you mean the permissions are determined by the file, not by
Tramp, then I agree.  This simply follows what we do with local files,
just by calling an Emacs primitive instead of fstatat.

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

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

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

> There's also bug#57395 with a related (but not identical) topic.

I've read bug#57395 and it is related of what I try to achieve.  It is
even more precise about which files are dangerous and which are not.
-- 
Manuel Giraud

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Michael Albinus]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> Hi Michael,

Hi Manuel,

> Thanks for your inputs.  I have tried with
> 'auto-save-file-name-transforms' like this:
>
> (add-to-list 'auto-save-file-name-transforms
>     '("\\`/\\(?:doas\\|ksu\\|su\\(?:do\\)?\\):.*\\'" "/dev/null" nil))
>
> But now, auto-save tries to create "/dev/#null#".  How can I fix it to
> "/dev/null"?

auto-save-file-name-transforms does not know the meaning of "/dev/null",
it handles it like an ordinary file name. The following code snippet
shall do what you want instead (untested):

--8<---------------cut here---------------start------------->8---
(defun my-find-file-hook ()
  (and buffer-file-name
       (member (file-remote-p buffer-file-name 'method) '("doas" "ksu" "su" "sudo"))
       (auto-save-mode 0)))
(add-hook 'find-file-hook 'my-find-file-hook)
--8<---------------cut here---------------end--------------->8---

@Eli: We have already remote-file-name-inhibit-auto-save-visited. Shall
we define another user option remote-file-name-inhibit-auto-save, which
could be used similarly?

Best regards, Michael.

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> If by "Tramp has no influence on the permissions of the auto-saved
> file" you mean the permissions are determined by the file, not by
> Tramp, then I agree.  This simply follows what we do with local files,
> just by calling an Emacs primitive instead of fstatat.

That's what I mean, yes.

Best regards, Michael.

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Michael Albinus]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

Hi Manual,

>> There's also bug#57395 with a related (but not identical) topic.
>
> I've read bug#57395 and it is related of what I try to achieve.  It is
> even more precise about which files are dangerous and which are not.

Thanks for the confirmation. I'll merge both bugs then.

Best regards, Michael.

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  62260@debbugs.gnu.org
> Date: Mon, 20 Mar 2023 12:41:34 +0100
> 
> Manuel Giraud <manuel@ledu-giraud.fr> writes:
> 
> > Thanks for your inputs.  I have tried with
> > 'auto-save-file-name-transforms' like this:
> >
> > (add-to-list 'auto-save-file-name-transforms
> >     '("\\`/\\(?:doas\\|ksu\\|su\\(?:do\\)?\\):.*\\'" "/dev/null" nil))
> >
> > But now, auto-save tries to create "/dev/#null#".  How can I fix it to
> > "/dev/null"?
> 
> auto-save-file-name-transforms does not know the meaning of "/dev/null",
> it handles it like an ordinary file name. The following code snippet
> shall do what you want instead (untested):
> 
> --8<---------------cut here---------------start------------->8---
> (defun my-find-file-hook ()
>   (and buffer-file-name
>        (member (file-remote-p buffer-file-name 'method) '("doas" "ksu" "su" "sudo"))
>        (auto-save-mode 0)))
> (add-hook 'find-file-hook 'my-find-file-hook)
> --8<---------------cut here---------------end--------------->8---
> 
> @Eli: We have already remote-file-name-inhibit-auto-save-visited. Shall
> we define another user option remote-file-name-inhibit-auto-save, which
> could be used similarly?

Fine by me, but this is not for emacs-29.

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> @Eli: We have already remote-file-name-inhibit-auto-save-visited. Shall
>> we define another user option remote-file-name-inhibit-auto-save, which
>> could be used similarly?
>
> Fine by me, but this is not for emacs-29.

Sure.

Best regards, Michael.

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

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

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

[...]

> (defun my-find-file-hook ()
>   (and buffer-file-name
>        (member (file-remote-p buffer-file-name 'method) '("doas" "ksu" "su" "sudo"))
>        (auto-save-mode 0)))
> (add-hook 'find-file-hook 'my-find-file-hook)

Hi Michael,

Thanks for this method, I'll try that.

> @Eli: We have already remote-file-name-inhibit-auto-save-visited. Shall
> we define another user option remote-file-name-inhibit-auto-save, which
> could be used similarly?

It is a good idea and will be more simple for the end user than the hook
you proposed above.  That said, I think an important aspect of bug#57395
is to consider what remote/sudo file is "dangerous" and what isn't.

So maybe, it would be better to have an option
'dangerous-file-name-inhibit-auto-save' and have a "piece of code" (so
easy to say 😅) that check whether a file is a dangerous one or not…  Or
maybe it is over engineering things.

Best regards,
-- 
Manuel Giraud

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

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

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

Hi Michael,

What do you think of the attached patch?

I think it fixes point 2 and 4 of bug#57395.  IMO point 3 (unsafe
non-root-owned file) is not really solvable in a remote setup: local and
remote uid can be any numbers.

What I'd like to do then is to work on point 1.  For this, my idea is to
make 'tramp-allow-unsafe-temporary-files' a three states variable with
the following possible values:

          - 'ask (default value): Prompt the user "Autosave file on
            local temporary directory, do you want to continue?"

          - nil: auto-save-mode should be disable on this file (same
            behaviour when answering "no" to the prompt)
            
          - t: auto-save-mode is on as usual (same behaviour when
            answering "yes" to the prompt)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Narrow-unsafe-auto-save.patch --]
[-- Type: text/x-patch, Size: 2102 bytes --]

From 2eaf3b2ef59868a349af8d5a1a1132ef1d1cbbe2 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Wed, 22 Mar 2023 10:46:23 +0100
Subject: [PATCH] Narrow unsafe auto-save

* lisp/net/tramp.el (tramp-dangerous-auto-save-p): New function to
determine dangerouness of an auto-save.
(tramp-handle-make-auto-save-file-name): Use it.
---
 lisp/net/tramp.el | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index 6eff5b2ca60..c3ee0ae06a6 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -6474,6 +6474,21 @@ tramp-delete-temp-file-function
 	    (remove-hook 'kill-buffer-hook
 			 #'tramp-delete-temp-file-function)))
 
+(defun tramp-dangerous-auto-save-p (filename autosave)
+  (let ((attributes (file-attributes filename 'integer))
+        (modes (file-modes filename 'nofollow)))
+    (and
+     ;; a file own by root and rwx only by root...
+     (and (= (or (file-attribute-user-id attributes)
+	         tramp-unknown-id-integer)
+	     tramp-root-id-integer)
+          (= modes (logand modes #o700)))
+     ;; ... into world readable autosave temporary
+     (and (file-in-directory-p autosave temporary-file-directory)
+          (/= (logand (file-modes temporary-file-directory 'nofollow)
+                      #o006)
+              0)))))
+
 (defun tramp-handle-make-auto-save-file-name ()
   "Like `make-auto-save-file-name' for Tramp files.
 Returns a file name in `tramp-auto-save-directory' for autosaving
@@ -6516,11 +6531,7 @@ tramp-handle-make-auto-save-file-name
 	;; Protect against security hole.
 	(when (and (not tramp-allow-unsafe-temporary-files)
 		   auto-save-default
-		   (file-in-directory-p result temporary-file-directory)
-		   (= (or (file-attribute-user-id
-			   (file-attributes filename 'integer))
-			  tramp-unknown-id-integer)
-		      tramp-root-id-integer)
+                   (tramp-dangerous-auto-save-p filename result)
 		   (not (with-tramp-connection-property
 			    (tramp-get-process v) "unsafe-temporary-file"
 			  (yes-or-no-p
-- 
2.39.2


[-- Attachment #3: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Michael Albinus]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> Hi Michael,

Hi Manuel,

> What do you think of the attached patch?

Honestly, I haven't had enough time to review it in detail. My gut
feeling tells me that it looks to complicate and too much hard-coded,
but I will need to review. I hope I could do it next days.

> I think it fixes point 2 and 4 of bug#57395.  IMO point 3 (unsafe
> non-root-owned file) is not really solvable in a remote setup: local and
> remote uid can be any numbers.
>
> What I'd like to do then is to work on point 1.  For this, my idea is to
> make 'tramp-allow-unsafe-temporary-files' a three states variable with
> the following possible values:
>
>           - 'ask (default value): Prompt the user "Autosave file on
>             local temporary directory, do you want to continue?"
>
>           - nil: auto-save-mode should be disable on this file (same
>             behaviour when answering "no" to the prompt)
>             
>           - t: auto-save-mode is on as usual (same behaviour when
>             answering "yes" to the prompt)

Please take into account, that tramp-allow-unsafe-temporary-files is not
only for auto-save files. It is used also for backup and file locks. See
functions tramp-handle-find-backup-file-name, tramp-handle-lock-file and
tramp-handle-make-auto-save-file-name.

For the time being, I have added the possibility to suppress auto-save
for remote files. The Tramp manual says now:

--8<---------------cut here---------------start------------->8---
        − Keep auto-save files local.  This is already the default
          configuration in Emacs, don’t change it.  If you want to
          disable auto-saving for remote files at all, set
          ‘remote-file-name-inhibit-auto-save’ to ‘t’, but think about
          the consequences!

          If you want to disable auto-saving just for selected
          connections, for example due to security considerations, use
          connection-local variables in order to set
          ‘buffer-auto-save-file-name’.  If you, for example, want to
          disable auto-saving for all ‘sudo’ connections, apply the
          following code.  *Note (emacs)Connection Variables::.

               (connection-local-set-profile-variables
                 'my-auto-save-profile
                 '((buffer-auto-save-file-name . nil)))

               (connection-local-set-profiles
                '(:application tramp :protocol "sudo")
                'my-auto-save-profile)
--8<---------------cut here---------------end--------------->8---

Pushed to master.

Best regards, Michael.

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

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

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

> Manuel Giraud <manuel@ledu-giraud.fr> writes:
>
>> Hi Michael,
>
> Hi Manuel,
>
>> What do you think of the attached patch?
>
> Honestly, I haven't had enough time to review it in detail. My gut
> feeling tells me that it looks to complicate and too much hard-coded,
> but I will need to review. I hope I could do it next days.

Hi Michael,

Ok, I understand that.  My patch is a bit hard-coded you're right but I
think it will reduce false positive triggering the user prompt about
unsafe auto-save.

[...]

> Please take into account, that tramp-allow-unsafe-temporary-files is not
> only for auto-save files. It is used also for backup and file locks. See
> functions tramp-handle-find-backup-file-name, tramp-handle-lock-file and
> tramp-handle-make-auto-save-file-name.

Ok so it might not be possible to have this three state
'tramp-allow-unsafe-temporary-files' I propose.

> For the time being, I have added the possibility to suppress auto-save
> for remote files. The Tramp manual says now:

Thanks.  That looks great.  IIUC, with this, one can disable only
auto-save (not locking nor backup) on a per connection basis.  I guess
that it could replace what I was proposing then.
-- 
Manuel Giraud

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

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

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

[...]

> For the time being, I have added the possibility to suppress auto-save
> for remote files. The Tramp manual says now:
>
>         − Keep auto-save files local.  This is already the default
>           configuration in Emacs, don’t change it.  If you want to
>           disable auto-saving for remote files at all, set
>           ‘remote-file-name-inhibit-auto-save’ to ‘t’, but think about
>           the consequences!
>
>           If you want to disable auto-saving just for selected
>           connections, for example due to security considerations, use
>           connection-local variables in order to set
>           ‘buffer-auto-save-file-name’.  If you, for example, want to
>           disable auto-saving for all ‘sudo’ connections, apply the
>           following code.  *Note (emacs)Connection Variables::.
>
>                (connection-local-set-profile-variables
>                  'my-auto-save-profile
>                  '((buffer-auto-save-file-name . nil)))
>
>                (connection-local-set-profiles
>                 '(:application tramp :protocol "sudo")
>                 'my-auto-save-profile)

Hi again Michael,

Just a quick "heads-up".  I've tried this new feature and it works
great.  But I still think we could reduce the false positives (as
enumerated by Trent) that are triggering the user prompt with my patch
or something along the line.

There is also still the issue of what should be done when the user
answers "no" to this prompt.  Should we not open the file? Disable
auto-save for this file?

Best regards,
-- 
Manuel Giraud

bug#62260: 30.0.50; [PATCH] Restrict auto-save file mode

[Michael Albinus]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> Hi again Michael,

Hi Manuel,

> Just a quick "heads-up".  I've tried this new feature and it works
> great.  But I still think we could reduce the false positives (as
> enumerated by Trent) that are triggering the user prompt with my patch
> or something along the line.
>
> There is also still the issue of what should be done when the user
> answers "no" to this prompt.  Should we not open the file? Disable
> auto-save for this file?

Yes and yes, these problems must be solved. But I need another free time
slot in order to work on this.

> Best regards,

Best regards, Michael.