bug#25611: 26.0.50; dired-do-compress unpacks .tgz files

bug#25611: 26.0.50; dired-do-compress unpacks .tgz files

[Mike Kupfer]

Suppose I have a .tar file, I move point to that file in a dired buffer,
and I press "Z".  My file gets converted to a .tgz file.  Okay so far.

If I then press "Z" again, I expect to get the original .tar file back.
Instead, the contents of the .tar file are extracted, and the .tgz file
is left where it was.

First problem: no .tar file.

Second problem: this violates the principle of Least Surprise.  The
prompt just talked about (de)compressing the file, it didn't say
anything about unpacking it.  (Not to mention this is a departure from
earlier behavior of "Z", and it's different behavior than what you'd get
with a plain old text file.)

Corollary to second problem: this can overwrite the user's existing
files, causing the user to lose data.

I can see the utility of the new behavior, but I think it should either
be non-default behavior or bound to a different character in dired.  (I
suppose you could just fix the prompt, but you'd lose the invertibility
of the old "Z" behavior, which seems like a step backward.)

In GNU Emacs 26.0.50.6 (x86_64-unknown-linux-gnu, X toolkit, Xaw scroll bars)
 of 2017-02-02 built on alto
Repository revision: 7cb7a582f44db94292709d35f4f5474f891f03b0
Windowing system distributor 'The X.Org Foundation', version 11.0.11604000
System Description:	Debian GNU/Linux 8.7 (jessie)

Recent messages:
(Shell command succeeded with no output)
Deleting...done
Move: 1 of 1
Move: 1 file
Directory has changed on disk; type g to update Dired
Quit
Type C-x 1 to delete the help window.
tar: This does not look like a tar archive
tar: office-mail-20161212.tar: Not found in archive
tar: Exiting with failure status due to previous errors

Configured using:
 'configure --prefix=/usr/new'

Configured features:
XPM JPEG TIFF GIF PNG SOUND NOTIFY GNUTLS LIBXML2 FREETYPE XFT ZLIB
TOOLKIT_SCROLL_BARS LUCID X11

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

Major mode: Dired by name

Minor modes in effect:
  shell-dirtrack-mode: t
  delete-selection-mode: t
  global-eldoc-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
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow eieio-opt speedbar sb-image ezimage dframe help-fns radix-tree
emacsbug view skeleton misearch multi-isearch sgml-mode dom pcmpl-unix
pcmpl-gnu pp flow-fill gnus-fun gnus-mh sort gnus-async gnus-bcklg
gnus-kill gnus-dup gnus-ml disp-table url-http url-gw url-cache url-auth
url-handlers nnrss xml mm-url url url-proxy url-privacy url-expand
url-methods url-history url-cookie url-domsuf nndraft nnmh
network-stream nsm starttls gnus-agent gnus-srvr gnus-score score-mode
nnvirtual gnus-msg nntp gnus-cache dired-aux dired-x mh-funcs mh-thread
mh-identity mh-letter mh-comp url-util url-parse url-vars org-element
org-rmail org-mhe org-irc org-info org-gnus org-docview doc-view
jka-compr image-mode org-bibtex bibtex org-bbdb org-w3m org org-macro
org-footnote org-pcomplete org-list org-faces org-entities org-version
ob-emacs-lisp ob ob-tangle ob-ref ob-lob ob-table ob-exp org-src ob-keys
ob-comint ob-core ob-eval org-compat org-macs org-loaddefs find-func
cal-menu calendar cal-loaddefs vc-hg qp mm-archive mh-alias crm
mail-extr mh-mime mh-gnus mh-acros mh-show goto-addr thingatpt gnus-cite
gnus-art mm-uu mml2015 mm-view mml-smime smime dig mailcap gnus-sum
gnus-group gnus-undo gnus-start gnus-cloud nnimap nnmail mail-source tls
gnutls utf7 netrc nnoo parse-time gnus-spec gnus-int gnus-win gnus-range
gnus nnheader wid-edit mh-inc hl-line mh-tool-bar mh-seq mh-xface
mh-utils mh-folder which-func imenu mh-scan mh-e mh-compat mh-buffers
mh-loaddefs mdk-mail smtpmail auth-source cl-seq eieio eieio-core
cl-macs eieio-loaddefs sendmail message subr-x puny seq byte-opt
bytecomp byte-compile cl-extra help-mode cconv format-spec rfc822 mml
mml-sec password-cache epa derived epg epg-config gnus-util rmail
rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047
rfc2045 mm-util ietf-drums mail-prsvr mailabbrev mail-utils gmm-utils
mailheader server noutline outline easy-mmode cc-mode cc-fonts cc-guess
cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs cl gv
shell pcomplete comint ansi-color ring xcscope easymenu advice delsel vc
cl-loaddefs pcase cl-lib vc-dispatcher dired dired-loaddefs timeclock
mdk-hacks time-date mule-util tooltip eldoc electric uniquify ediff-hook
vc-hooks lisp-float-type 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 elisp-mode lisp-mode prog-mode register page
menu-bar rfn-eshadow isearch timer select scroll-bar mouse jit-lock
font-lock syntax facemenu font-core term/tty-colors frame cl-generic
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 charscript
case-table epa-hook jka-cmpr-hook help simple abbrev obarray minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote inotify dynamic-setting
font-render-setting x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 334531 42157)
 (symbols 48 44642 1)
 (miscs 40 429 941)
 (strings 32 97784 18745)
 (string-bytes 1 3083356)
 (vectors 16 45101)
 (vector-slots 8 1532783 157208)
 (floats 8 525 644)
 (intervals 56 2775 535)
 (buffers 976 41)
 (heap 1024 70224 44977))

bug#25611: 26.0.50; dired-do-compress unpacks .tgz files

[Glenn Morris]

Mike Kupfer wrote:

> Suppose I have a .tar file, I move point to that file in a dired buffer,
> and I press "Z".  My file gets converted to a .tgz file.  Okay so far.
>
> If I then press "Z" again, I expect to get the original .tar file back.
> Instead, the contents of the .tar file are extracted, and the .tgz file
> is left where it was.


Looks like this was added in https://debbugs.gnu.org/20384#11 ?
I've cc'd the author of that change.


> First problem: no .tar file.
>
> Second problem: this violates the principle of Least Surprise.  The
> prompt just talked about (de)compressing the file, it didn't say
> anything about unpacking it.  (Not to mention this is a departure from
> earlier behavior of "Z", and it's different behavior than what you'd get
> with a plain old text file.)
>
> Corollary to second problem: this can overwrite the user's existing
> files, causing the user to lose data.
>
> I can see the utility of the new behavior, but I think it should either
> be non-default behavior or bound to a different character in dired.  (I
> suppose you could just fix the prompt, but you'd lose the invertibility
> of the old "Z" behavior, which seems like a step backward.)

bug#25611: 26.0.50; dired-do-compress unpacks .tgz files

[Mike Kupfer]

Glenn Morris wrote:

> Looks like this was added in https://debbugs.gnu.org/20384#11 ?
> I've cc'd the author of that change.

Thanks.

It occurs to me that this could be considered a security vulnerability.
If the .tgz file is (unintentionally) unpacked in $HOME and contains a
.ssh/authorized_keys, that could give an attacker access to the victim's
account.

mike

bug#25611: 26.0.50; dired-do-compress unpacks .tgz files

[Oleh Krehel]

Hi Mike,

> It occurs to me that this could be considered a security vulnerability.
> If the .tgz file is (unintentionally) unpacked in $HOME and contains a
> .ssh/authorized_keys, that could give an attacker access to the victim's
> account.

The file is uncompressed into a directory with the same name. So the
file would have to be ~/.ssh.tar.gz. If a user presses "Z" on that
file, it's pretty clear what will happen, same as with "C" on e.g. an
`authorized_keys' file somewhere.

regards,
Oleh

bug#25611: 26.0.50; dired-do-compress unpacks .tgz files

[Mike Kupfer]

Hi Oleh,

Oleh Krehel wrote:

> > It occurs to me that this could be considered a security vulnerability.
> > If the .tgz file is (unintentionally) unpacked in $HOME and contains a
> > .ssh/authorized_keys, that could give an attacker access to the victim's
> > account.
> 
> The file is uncompressed into a directory with the same name. So the
> file would have to be ~/.ssh.tar.gz. If a user presses "Z" on that
> file, it's pretty clear what will happen, same as with "C" on e.g. an
> `authorized_keys' file somewhere.

That might be the intended usage, but my testing[1] shows that there's
no enforcement.  I created by hand a Desktop.tgz by doing

    tar cf Desktop.tar Desktop .ssh/known_hosts

and then compressing Desktop.tar.  (I don't use an authorized_keys file
on the system that I ran the test on.)  I moved Desktop.tgz to a temp
directory and then pressed "Z" on it.  It unpacked Desktop okay, but it
also created .ssh/known_hosts.

I also tried editing one of the files in <temp_dir>/Desktop and redoing
"Z" on Desktop.tgz.  That silently overwrote my change.

So I think two changes are needed: one to eliminate the security risk,
the second to protect against accidental data loss.

The security risk would be closed by ensuring that foo.<suffix> only
unpacks into "foo".  This could be done by checking the table of
contents of the tar file and erroring out if anything is amiss.  Another
approach would be to invoke tar as "tar xf ... foo".  The first approach
gives better feedback to the user if there is something amiss with the
tar file, but it'll take more code.  (GNU tar, at least, protects
against things like foo/../.ssh/mumble; I don't know about other
variants of tar.)

To protect against accidental data loss, I recommend erroring out if
"foo" already exists, or asking the user for confirmation before
proceeding.

regards,
mike

[1] Emacs master, changeset 18c47695 from 21 February, running on Debian
stable.

bug#25611: 26.0.50; dired-do-compress unpacks .tgz files

[Glenn Morris]

Mike Kupfer wrote:

> It occurs to me that this could be considered a security vulnerability.

I'm not sure I agree, but regardless I do not expect a command that
"compresses or uncompresses files" to extract a tar file. These should
be separate steps/commands IMO. Exactly as you said in your initial
report.

bug#25611: 26.0.50; dired-do-compress unpacks .tgz files

[Mike Kupfer]

Glenn Morris wrote:

> [...] I do not expect a command that
> "compresses or uncompresses files" to extract a tar file. These should
> be separate steps/commands IMO. Exactly as you said in your initial
> report.

That would be my preference, too.  I'm just not sure how much of that
preference comes from mental inertia. :-) That's why my later emails
focused on the data loss and security angles.

If we do keep the extraction functionality, the docstring for
dired-do-compress definitely needs updating.

cheers,
mike

bug#25611: 26.0.50; dired-do-compress unpacks .tgz files

[Mike Kupfer]

Mike Kupfer wrote:

> If we do keep the extraction functionality, the docstring for
> dired-do-compress definitely needs updating.

Sorry, that should have been "docstring and prompt".

mike

bug#25611: 26.0.50; dired-do-compress unpacks .tgz files

[Glenn Morris]

Comments from rms:

http://lists.gnu.org/r/emacs-devel/2018-11/msg00268.html

    It does not literally make sense to "decompress" an archive.  Any kind
    of archive.  That stretches the meaning of "decompress".

    Such stretching might be ok, if it led to no confusion or conflict.
    However, what we see is that it does lead to confusion.  So Z should
    not unpack archives -- not even zip archives.

    Instead, we should have a different letter for unpacking archives.
    I suggest moving dired-do-find-regexp to F
    ad defining A to unpacking an archive.