* Re: [elpa] externals-release/org e2366ac283: * lisp/org-persist.el: Do not litter /tmp when native compiling
[not found] ` <20221221185757.74391C00613@vcs2.savannah.gnu.org>
@ 2022-12-21 19:07 ` Stefan Monnier
2022-12-21 19:15 ` Ihor Radchenko
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Monnier @ 2022-12-21 19:07 UTC (permalink / raw)
To: emacs-devel; +Cc: Ihor Radchenko
> +(when (and org-persist--disable-when-emacs-Q
> + ;; FIXME: This is relying on undocumented fact that
> + ;; Emacs sets `user-init-file' to nil when loaded with
> + ;; "-Q" argument.
> + (not user-init-file))
> + (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?
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.
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. ]
Stefan
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [elpa] externals-release/org e2366ac283: * lisp/org-persist.el: Do not litter /tmp when native compiling
2022-12-21 19:07 ` [elpa] externals-release/org e2366ac283: * lisp/org-persist.el: Do not litter /tmp when native compiling Stefan Monnier
@ 2022-12-21 19:15 ` Ihor Radchenko
2022-12-21 19:54 ` Stefan Monnier
0 siblings, 1 reply; 4+ messages in thread
From: Ihor Radchenko @ 2022-12-21 19:15 UTC (permalink / raw)
To: Stefan Monnier; +Cc: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> + (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.
> 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). If we are in that situation, your
scenario is the least of our concerns. Also, do note that this branch of
code is only executed with emacs -Q.
> 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.
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [elpa] externals-release/org e2366ac283: * lisp/org-persist.el: Do not litter /tmp when native compiling
2022-12-21 19:15 ` Ihor Radchenko
@ 2022-12-21 19:54 ` Stefan Monnier
2022-12-25 9:22 ` Ihor Radchenko
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Monnier @ 2022-12-21 19:54 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: emacs-devel
>>> + (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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [elpa] externals-release/org e2366ac283: * lisp/org-persist.el: Do not litter /tmp when native compiling
2022-12-21 19:54 ` Stefan Monnier
@ 2022-12-25 9:22 ` Ihor Radchenko
0 siblings, 0 replies; 4+ messages in thread
From: Ihor Radchenko @ 2022-12-25 9:22 UTC (permalink / raw)
To: Stefan Monnier; +Cc: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> 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` :-(
Fair point.
I now fixed this by creating the directory at the beginning and only
removing it before exiting Emacs.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?h=bugfix&id=987fe173acf6c6fdb4bec5c5c07796d98c6e1983
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-12-25 9:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <167164907706.9937.9724677162932540750@vcs2.savannah.gnu.org>
[not found] ` <20221221185757.74391C00613@vcs2.savannah.gnu.org>
2022-12-21 19:07 ` [elpa] externals-release/org e2366ac283: * lisp/org-persist.el: Do not litter /tmp when native compiling Stefan Monnier
2022-12-21 19:15 ` Ihor Radchenko
2022-12-21 19:54 ` Stefan Monnier
2022-12-25 9:22 ` Ihor Radchenko
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.