[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [elpa] externals-release/org e2366ac283: * lisp/org-persist.el: Do n
From: |
Stefan Monnier |
Subject: |
Re: [elpa] externals-release/org e2366ac283: * lisp/org-persist.el: Do not litter /tmp when native compiling |
Date: |
Wed, 21 Dec 2022 14:54:05 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
>>> + (setq org-persist-directory
>>> + (make-temp-file "org-persist-" 'dir))
>>> + ;; We don't need the temp directory to exist.
>>> + ;; `org-persist-write-all' will refrain from creating and writing to the
>>> dir if
>>> + ;; none exists yet.
>>> + (delete-directory org-persist-directory))
>>
>> Could we skip the "create dir" so we can skip the `delete-directory`
>> part as well?
>
> It would be wonderful, but
>
> There is a race condition between calling make-temp-name and
> later creating the file, which opens all kinds of security holes.
> For that reason, you should normally use make-temp-file instead.
>
> which scared me.
By keeping `org-persist-directory` but deleting the dir, you basically
turn the narrow window of the race condition into an arbitrarily
long window, so your code is worse than using `make-temp-name` :-(
>> Also, this smells like a security hole: after `delete-directory`, with
>> the /tmp setup as mode #o1777, some other user can come and create
>> a directory at that location, so that our `org-persist-directory` will
>> then point to their directory (where they may be able to read the
>> things we write) or they can replace it with a link to a directory of
>> our own tricking us into overwriting some of our own files.
>
> This also means that other user has access to Elisp machine state (the
> value of org-persist-directory).
Not really. The usual way to exploit the race condition in
`make-temp-name` is to predict the name that will be chosen (or trick
the code into choosing a particular value, e.g. by creating lost of
entries with names /tmp/org-persist-NNNNN so your Emacs's choice of free
spot is correspondingly reduced).
But, with your code it's even easier, it can just monitor the /tmp
directory to see which dir was created and then deleted and it will then
know the value of your `org-persist-directory`.
>> IOW, I suggest we use something like
>>
>> (setq org-persist-directory "/non-existing")
>> or
>> (setq org-persist-directory (make-temp-name "/non-existing"))
>>
>> [ Not sure if there is a standard such name that's "guaranteed" not to
>> exist. ]
>
> If you confirm that it is safe to use `make-temp-name' here, I will go
> with your suggestion.
The safety of this code relies on the fact that / is presumed to be
read-only (except for root), contrary to /tmp which is world-writable.
Stefan