search

jkitchin / ox-clip

Copy formatted content from org-mode
67 stars 10 forks source link

Improper method to create a temporary file #10

Open maxnikulin opened 3 years ago

maxnikulin commented 3 years ago

Predictable file name in the /tmp directory hardly could be considered as good practice.

https://github.com/jkitchin/ox-clip/blob/master/ox-clip.el#L369

(with-temp-file "/tmp/ox-clip-org.html"

There is at least make-temp-file function, see info '(elisp)Unique File Names', though I do not know if it is the best option.

Another system user could create file or symlink with such name causing permission denied error.

Fortunately, nowadays it is at least unlikely a security vulnerability since fs.protected_symlinks should be turned on by default.

https://www.kernel.org/doc/Documentation/sysctl/fs.txt

A long-standing class of security issues is the symlink-based time-of-check-time-of-use race, most commonly seen in world-writable directories like /tmp. The common method of exploitation of this flaw is to cross privilege boundaries when following a given symlink (i.e. a root process follows a symlink belonging to another user). For a likely incomplete list of hundreds of examples across the years, please see: http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=/tmp

When set to "0", symlink following behavior is unrestricted.

When set to "1" symlinks are permitted to be followed only when outside a sticky world-writable directory, or when the uid of the symlink and follower match, or when the directory owner matches the symlink's owner.

This protection is based on the restrictions in Openwall and grsecurity.

P.S. I like the idea to bypass emacs limitations in respect to clipboard format by using an external utility.

jkitchin commented 3 years ago

I don't have a linux system to test changes on, so pull requests are welcome.

the problem is that you have to construct the xclip command with a file name in it the way it is written. If xclip can read from stdin we can probably avoid that. I don't have a way to verify it though, so I don't want to make that change.

maxnikulin commented 3 years ago

I don't have a linux system to test changes on, so pull requests are welcome.

I am unaware concerning degree of severity of the issue on other operation systems. Generally temporary files should be created with some analogue of POSIX O_EXCL flag causing error if a file having requested name already exists. Otherwise various races and rare but still hard to debug problems could appear. I agree that it could be tedious to carefully handle such cases.

I was curious concerning implementation details when you suggested the package in the mail list message. Earlier I was evaluating feasibility of a similar feature and realized emacs API does not allow to implement it without external tools. The cases when such feature is required are not frequent for me, but next time I will consider a pull request.

the problem is that you have to construct the xclip command with a file name in it the way it is written. If xclip can read from stdin we can probably avoid that. I don't have a way to verify it though, so I don't want to make that change.

xclip could get data from stdin but there is an issue with stdout (that should be explicitly closed) faced by tmux users astrand/xclip#20 "Not closing stdout when setting clipboard from stdin", workaround is described in Arch wiki as well https://wiki.archlinux.org/index.php/tmux#X_clipboard_integration

Ideally command could have a substitution, e.g. optionally quoted %f that is expanded after splitting the string xclip -in -target text/html %f into arguments array. Unfortunately I have not found ready to use function in elisp despite the task is not really rare and users could do something insecure instead.

jkitchin commented 3 years ago

this is only an issue on linux, for using xclip. The other platforms don't use this named tempfile. The command you are looking for is (format "xclip -in -target text/html %s" temp-file-name) in the simplest form.

maxnikulin commented 3 years ago

The command you are looking for is (format "xclip -in -target text/html %s" temp-file-name)

No, I was writing about something like

(mapcar (lambda (arg)
      (format-spec arg '((?f . "/tmp/a.html")
                 (?t . "text/html"))))
    (split-string-and-unquote
     "xclip -verbose -in -type %t -selection clipboard \"%f\""))
  1. Substitutions are performed after string splitting. Temporary directory with spaces and other fancy characters would not be a problem.
  2. User could optionally add quotes that are usually necessary in shell commands but could cause incorrect file name if not stripped when arguments are safely passed as a list.
  3. There is another variant of command that requires 2 parameters. At least Emacs-25 does not support (format "%2$s, %3$s, %%, %1$s" "x" "y" "z") syntax, and it is anyway less convenient than "f" for file and "t" for type convention.

Concerning launching the command without temporary file, pseudoterminal created by start-process is certainly redundant. I have not looked into emacs sources yet, so I have no guess why it decides to kill process immediately if -verbose is not specified (related commit message in ox-clip is not detailed enough too). It seems, if pipe is used, emacs behaves better in respect to xclip

(setq pr (make-process :name "ox-clip" :command '("xclip" "-in" "-selection" "CLIPBOARD")
             :connection-type 'pipe
             :buffer nil
             :filter (lambda (proc string) (message "%s" string))))
; (process-send-region)
(process-send-string pr "test")
(process-send-eof pr)

I still feel lack of basic operation in emacs interface to processes: close stdin file descriptor and the same for stdout.

jkitchin commented 3 years ago

Can you check out 977562f and let me know if it works ok for you? I think it solves this issue.

Also thanks for the tips above, I didn't know that format could do that! I also never remember the format-spec. It seems so old fashioned compared to things like s-format.

maxnikulin commented 3 years ago

Please, do not hurry.

To make it working (to some extent) I have to modify the code

diff --git a/ox-clip.el b/ox-clip.el
index 6316f5d..37a4b1d 100644
--- a/ox-clip.el
+++ b/ox-clip.el
@@ -66,14 +66,14 @@
           (file-name-directory (or load-file-name (locate-library "ox-clip")))))
   "Absolute path to html-clip-w32.py."
   :group 'ox-clip
-  :type string)
+  :type 'string)

 (defcustom ox-clip-osx-cmd
   "textutil -inputencoding UTF-8 -stdin -format html -convert rtf -stdout | pbcopy"
   "Command to copy formatted text on osX."
   :group 'ox-clip
-  :type string)
+  :type 'string)

 (defcustom ox-clip-linux-cmd
@@ -399,8 +399,8 @@ R1 and R2 define the selected region."
                ox-clip-osx-cmd)))
            ((eq system-type 'gnu/linux)
             ;; For some reason shell-command on region does not work with xclip.
-           (let ((tmpfile (make-temp-file "ox-clip-" nil ".html"
-                                          (with-current-buffer buf (buffer-string)))))
+           (let ((tmpfile (make-temp-file "ox-clip-" nil ".html")))
+             (with-current-buffer buf (append-to-file nil nil tmpfile))
               (apply
                'start-process "ox-clip" "*ox-clip*"
                (split-string (format-spec ox-clip-linux-cmd

Problems:

jkitchin commented 3 years ago

The quotes were my mistake, thanks for reporting those.

I quoted the filename, which should make it work with spaces I think.

What happens with the tmpfile code? according to (make-temp-file PREFIX &optional DIR-FLAG SUFFIX TEXT) the original version should work.

for removal of the tmp file, maybe the command could be "xclip -verbose -i \"%f\" -t text/html -selection clipboard && rm \"%f\""?

I think I have set the noquery flag. Can you check?

maxnikulin commented 3 years ago

My variant: https://github.com/maxnikulin/ox-clip/commit/f04aa7e5f977e0c7b0007d17f959e225fa690c97 However I have not tested images (it requires some packages that are not currently installed).

I do not think, it is what you were going to achieve:

(split-string
 (format-spec "xclip -verbose -i \"%f\" -t text/html  -selection clipboard"
          '((?f . "/home/ubuntu/My Temporary Documents/ox-clip-249439DA.html")))
 " ")
("xclip" "-verbose" "-i" "\"/home/ubuntu/My" "Temporary" "Documents/ox-clip-249439DA.html\"" "-t" "text/html" "" "-selection" "clipboard")
etc/NEWS.26:1490:** New optional argument TEXT in 'make-temp-file'.

emacs-25.2 is shipped with Ubuntu-18.04 bionic, so fourth argument does not work for me.

I do not like idea to use shell where simple and robust exec should be enough. Anyway, temporary file likely should be removed even if xclip fails for some reason, so shell commands should be separated by ; rather than &&

maxnikulin commented 3 years ago

Why are you trying to avoid splitting command into arguments before applying substitution? File names could be tricky. Not so tricky as shell commands, but still... I agree that setting TMPDIR to some path with quote characters in folder name is shooting a foot, but I am afraid that such code might be copy-pasted to a function where arbitrary directory created by a user could be involved. E.g. find and xargs tools (often used together to process a lot of files) have special options to separate file names using null-byte character to safely communicate through a pipe. That allows to handle files with e.g. newlines in their names. Linux kernel system call execve (that actually executes a binary behind start-process and make-process) accept arguments as separate array elements. It is much more safe and reliable to not mix file name with other arguments when it is possible.

maxnikulin commented 1 year ago

Since this issue is not closed, I decided to leave the following comment here.

xclip.el uses process-send-string to avoid a temporary file, see