Re: $TMPDIR bug

[Jörn Engel]

Retry after subscribing to the list. I hope this works and I can
unsubscribe again.

On Wed, Dec 02, 2015 at 03:32:00PM -0800, Jörn Engel wrote:
> Found this when dealing with a "creative" FUSE filesystem. The fuse
> filesystem will assume that everything exists and is a directory, so
> stat works, cd and file creation do not. The fuse filesystem is clearly
> bad, but that is not the point.
>
> Vim will try to mkdir "$TMPDIR/v4sgqHU". If TMPDIR exists in your
> environment, it will mkdir "/tmp/v4sgqHU" or whatever you have set it
> to. If it doesn't exist, it will literally mkdir "$TMPDIR/v4sgqHU".
>
> In most sane environments "$TMPDIR" doesn't exist as vim works just
> fine. Something like:
> stat("$TMPDIR", 0x7fffc90aad60) = -1 ENOENT (No such file or directory)
> stat("/tmp", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=5620, ...}) = 0
> mkdir("/tmp/v3C11LF", 0700) = 0
>
> In my insane fuse environment instead:
> stat("$TMPDIR", {st_mode=S_IFDIR|0555, st_size=4096, ...}) = 0
> gettimeofday({1430937134, 707649}, NULL) = 0
> mkdir("$TMPDIR/v4sgqHU", 0700) = -1 EEXIST (File exists)
> mkdir("$TMPDIR/vvuiqHU", 0700) = -1 EEXIST (File exists)
> mkdir("$TMPDIR/vWvkqHU", 0700) = -1 EEXIST (File exists)
> ...
>
> After about 30s and 250k attempts vim aborts and uses /tmp instead.
>
> The vim bug is to try "$TMPDIR" even though the environment variable
> isn't set. Arguably that could become a security-issue if I can create
> a "$TMPDIR" in a directory I control and trick someone else to open a
> file from that directory. So independently of what praise you might
> want to heap on my fuse-thing, vim is equally at fault here.
>
> Jörn
>
> --
> The key to performance is elegance, not battalions of special cases.
> -- Jon Bentley and Doug McIlroy

Jörn

--
But this is not to say that the main benefit of Linux and other GPL
software is lower-cost. Control is the main benefit--cost is secondary.
-- Bruce Perens

[Jörn Engel]

[Tony Mechelynck]

On Thu, Dec 3, 2015 at 12:39 AM, Jörn Engel <jo...@purestorage.com> wrote:
> Retry after subscribing to the list. I hope this works and I can
> unsubscribe again.

> [...]

Indeed, messages can only make it to the list if they are from
subscribed authors, but in addition, the first time a given author
posts, he needs a human moderator to look at the post, see that it
isn't spam, and whitelist the author. Now those moderators are few in
number and none of them is at the keyboard 24/7 so your first message
may have to wait several hours if it arrives at a time when no one is
looking.

Later answers (from any list subscribers) will usually be just to the
list, on the assumption that the author is subscribed and therefore
gets the list posts. That's why you shouldn't unsubscribe: if you do,
not only you won't see answers to your original post, but you will
have to go through the same rigmarole again if ever you want to post
another message to the list. The only times to unsubscribe would IMHO
be just before you decide to get rid of the email account, or just
after you decide never to use Vim again.


Best regards,
Tony.

[Jörn Engel]

Cannot say I enjoy this policy or that it encourages bug reports. But
then again, this is the first bug I have encountered in 17 years of
using vim, so you must be doing something right.

Thank you and everyone else that made my life better in the past 17
years.

And I will unsubscribe long before I stop using vim. The latter should
be a few weeks before my death, if that long. ;)

Jörn

--
The rabbit runs faster than the fox, because the rabbit is rinning for
his life while the fox is only running for his dinner.
-- Aesop

[Random832]

On 2015-12-02, Jörn Engel <jo...@purestorage.com> wrote:
> The vim bug is to try "$TMPDIR" even though the environment variable
> isn't set. Arguably that could become a security-issue if I can create
> a "$TMPDIR" in a directory I control and trick someone else to open a
> file from that directory.

If it were possible to trick someone to open an existing file in the
temporary directory, that'd be a problem even under normal
circumstances, since /tmp (/var/tmp, /usr/tmp, etc, all typical values
for TMPDIR) is world-writable.

> So independently of what praise you might
> want to heap on my fuse-thing, vim is equally at fault here.

Vim isn't opening a file that exists, it is trying to find a file that
doesn't exist. Which means it can't be tricked into opening a file that
exists.

[Jörn Engel]

On Thu, Dec 03, 2015 at 02:12:08AM +0000, Random832 wrote:
> On 2015-12-02, Jörn Engel <jo...@purestorage.com> wrote:
> > The vim bug is to try "$TMPDIR" even though the environment variable
> > isn't set. Arguably that could become a security-issue if I can create
> > a "$TMPDIR" in a directory I control and trick someone else to open a
> > file from that directory.
>
> If it were possible to trick someone to open an existing file in the
> temporary directory, that'd be a problem even under normal
> circumstances, since /tmp (/var/tmp, /usr/tmp, etc, all typical values
> for TMPDIR) is world-writable.

Fair point.

> > So independently of what praise you might
> > want to heap on my fuse-thing, vim is equally at fault here.
>
> Vim isn't opening a file that exists, it is trying to find a file that
> doesn't exist. Which means it can't be tricked into opening a file that
> exists.

It still is a bug. Using a directory called "$TMPDIR" is silly at best.
The only reason it has survived as long as it has is that the
misbehaviour is typically 2-3 extra syscalls, not 30s of wait time.

Jörn

--
Geld macht nicht glücklich.
Glück macht nicht satt.

[lilydjwg]

On Thu, Dec 03, 2015 at 02:12:08AM +0000, Random832 wrote:

> [...]

>
> Vim isn't opening a file that exists, it is trying to find a file that
> doesn't exist. Which means it can't be tricked into opening a file that
> exists.

So let's try to steal the file content instead.

If a normal user can mount FUSE filesystems with the "allow_other"
option (this can be enabled by root, and can't be enabled per user), the
user can trick another user to put temporary files on a FUSE filesystem
he controls, thus he can know all the data written there.

I didn't try it, but I think this is possible.

--
Best regards,
lilydjwg

[Marius Gedminas]

On Wed, Dec 02, 2015 at 05:24:03PM -0800, Jörn Engel wrote:
> On Thu, Dec 03, 2015 at 01:20:22AM +0100, Tony Mechelynck wrote:
> > On Thu, Dec 3, 2015 at 12:39 AM, Jörn Engel <jo...@purestorage.com> wrote:
> > > Retry after subscribing to the list. I hope this works and I can
> > > unsubscribe again.
> > > [...]

...

> Cannot say I enjoy this policy or that it encourages bug reports.

You can also report bugs at https://github.com/vim/vim/issues

> But then again, this is the first bug I have encountered in 17 years
> of using vim, so you must be doing something right.
>
> Thank you and everyone else that made my life better in the past 17
> years.
>
> And I will unsubscribe long before I stop using vim. The latter should
> be a few weeks before my death, if that long. ;)

BTW you can stop email delivery without unsubscribing, at
https://groups.google.com/forum/#!forum/vim_dev (the my settings button,
second on top-right; then Membership and email settings; then Email
delivery preference: Don't send email updates).

Marius Gedminas
--
"If you think C++ is not overly complicated, just what is a protected abstract
virtual base pure virtual private destructor, and when was the last time you
needed one?"
-- Tom Cargil, _C++_Journal_

[Bram Moolenaar]

Does this patch fix your problem:


--- a/fileio.c 2015-12-03 13:52:48.451584080 +0100
+++ b/fileio.c 2015-12-03 17:31:53.457770134 +0100
@@ -7390,8 +7390,9 @@

/* expand $TMP, leave room for "/v1100000/999999999" */
expand_env((char_u *)tempdirs[i], itmp, TEMPNAMELEN - 20);
- if (mch_isdir(itmp)) /* directory exists */
+ if (itemp[0] != '$' && mch_isdir(itmp))
{
+ /* directory exists */
# ifdef __EMX__
/* If $TMP contains a forward slash (perhaps using bash or
* tcsh), don't add a backslash, use a forward slash!


--
hundred-and-one symptoms of being an internet addict:
178. You look for an icon to double-click to open your bedroom window.

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[Jörn Engel]

On Thu, Dec 03, 2015 at 05:43:33PM +0100, Bram Moolenaar wrote:
>
> Does this patch fix your problem:
>
>
> --- a/fileio.c 2015-12-03 13:52:48.451584080 +0100
> +++ b/fileio.c 2015-12-03 17:31:53.457770134 +0100
> @@ -7390,8 +7390,9 @@
>
> /* expand $TMP, leave room for "/v1100000/999999999" */
> expand_env((char_u *)tempdirs[i], itmp, TEMPNAMELEN - 20);
> - if (mch_isdir(itmp)) /* directory exists */
> + if (itemp[0] != '$' && mch_isdir(itmp))
> {
> + /* directory exists */
> # ifdef __EMX__
> /* If $TMP contains a forward slash (perhaps using bash or
> * tcsh), don't add a backslash, use a forward slash!

Doesn't compile, s/itemp/itmp/ fixes that. Next I cannot seem to
reproduce the problem with or without the patch. My self-compiled vim
behaves completely different from the distro-supplied vim. Looks like a
day or so to track down the differences, which I don't have.

But fundamentally this patch is still wrong. If I run
export TMPDIR='$TMPDIR' vim
then vim should very much try '$TMPDIR' because I explicitly asked it
to do so. Might be a silly choice, but that is mine to make.

Correct patch seems hard. In my case tempdirs[i] is defined to
"$TMPDIR", "/tmp", ".", "$HOME". We don't want to try "$TMPDIR" or
"$HOME", unless the user explicitly asks us to. We do want to try
"/tmp" and ".". Add the other platforms and things get even messier.

Maybe the patch below is close enough to correct and simple enough to
ship? It doesn't allow TMPDIR='$TMPDIR, but retains
TMPDIR='$SOME_OTHER_DIRECTORY_STARTING_WITH_A_DOLLAR' for those two
users that might have such a monstrosity.

So many bad options to choose from.

Jörn

--
They laughed at Galileo. They laughed at Copernicus. They laughed at
Columbus. But remember, they also laughed at Bozo the Clown.
-- unknown

diff --git a/src/fileio.c b/src/fileio.c
index ded95b728abc..5680c02300c7 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -7390,7 +7390,7 @@ vim_tempname(extra_char, keep)

/* expand $TMP, leave room for "/v1100000/999999999" */
expand_env((char_u *)tempdirs[i], itmp, TEMPNAMELEN - 20);
- if (mch_isdir(itmp)) /* directory exists */

+ if (!(itmp[0] == '$' && strcmp(itmp, tempdirs[i]) == 0) && mch_isdir(itmp))
{

[Bram Moolenaar]

Jörn Engel wrote:

> > Does this patch fix your problem:
> >
> >
> > --- a/fileio.c 2015-12-03 13:52:48.451584080 +0100
> > +++ b/fileio.c 2015-12-03 17:31:53.457770134 +0100
> > @@ -7390,8 +7390,9 @@
> >
> > /* expand $TMP, leave room for "/v1100000/999999999" */
> > expand_env((char_u *)tempdirs[i], itmp, TEMPNAMELEN - 20);
> > - if (mch_isdir(itmp)) /* directory exists */
> > + if (itemp[0] != '$' && mch_isdir(itmp))
> > {
> > + /* directory exists */
> > # ifdef __EMX__
> > /* If $TMP contains a forward slash (perhaps using bash or
> > * tcsh), don't add a backslash, use a forward slash!
>
> Doesn't compile, s/itemp/itmp/ fixes that. Next I cannot seem to
> reproduce the problem with or without the patch. My self-compiled vim
> behaves completely different from the distro-supplied vim. Looks like a
> day or so to track down the differences, which I don't have.
>
> But fundamentally this patch is still wrong. If I run
> export TMPDIR='$TMPDIR' vim
> then vim should very much try '$TMPDIR' because I explicitly asked it
> to do so. Might be a silly choice, but that is mine to make.

The path should start with a slash. And if it doesn't, I don't think
any sane person would have a directory name starting with $.
Let's just say we don't support it.

> Correct patch seems hard. In my case tempdirs[i] is defined to
> "$TMPDIR", "/tmp", ".", "$HOME". We don't want to try "$TMPDIR" or
> "$HOME", unless the user explicitly asks us to. We do want to try
> "/tmp" and ".". Add the other platforms and things get even messier.
>
> Maybe the patch below is close enough to correct and simple enough to
> ship? It doesn't allow TMPDIR='$TMPDIR, but retains
> TMPDIR='$SOME_OTHER_DIRECTORY_STARTING_WITH_A_DOLLAR' for those two
> users that might have such a monstrosity.
>
> So many bad options to choose from.

Well, we could make a version of expand_env() that returns an empty
string if the expansion fails. But I don't think it is worth it.

--
hundred-and-one symptoms of being an internet addict:

186. You overstay in the office so you can have more time surfing the net.

[Jörn Engel]

On Thu, Dec 03, 2015 at 09:02:47PM +0100, Bram Moolenaar wrote:
> Jörn Engel wrote:
>
> > But fundamentally this patch is still wrong. If I run
> > export TMPDIR='$TMPDIR' vim
> > then vim should very much try '$TMPDIR' because I explicitly asked it
> > to do so. Might be a silly choice, but that is mine to make.
>
> The path should start with a slash. And if it doesn't, I don't think
> any sane person would have a directory name starting with $.
> Let's just say we don't support it.

Fair point. Having relative paths to TMPDIR is crazy enough to not
support it.

I like your patch, modulo the typo.

> Well, we could make a version of expand_env() that returns an empty
> string if the expansion fails. But I don't think it is worth it.

Ack.

Jörn

--
Invincibility is in oneself, vulnerability is in the opponent.
-- Sun Tzu

[James McCoy]

On Thu, Dec 03, 2015 at 11:03:19AM -0800, Jörn Engel wrote:
> On Thu, Dec 03, 2015 at 05:43:33PM +0100, Bram Moolenaar wrote:
> >
> > Does this patch fix your problem:
> >
> >
> > --- a/fileio.c 2015-12-03 13:52:48.451584080 +0100
> > +++ b/fileio.c 2015-12-03 17:31:53.457770134 +0100
> > @@ -7390,8 +7390,9 @@
> >
> > /* expand $TMP, leave room for "/v1100000/999999999" */
> > expand_env((char_u *)tempdirs[i], itmp, TEMPNAMELEN - 20);
> > - if (mch_isdir(itmp)) /* directory exists */
> > + if (itemp[0] != '$' && mch_isdir(itmp))
> > {
> > + /* directory exists */
> > # ifdef __EMX__
> > /* If $TMP contains a forward slash (perhaps using bash or
> > * tcsh), don't add a backslash, use a forward slash!
>
> Doesn't compile, s/itemp/itmp/ fixes that. Next I cannot seem to
> reproduce the problem with or without the patch. My self-compiled vim
> behaves completely different from the distro-supplied vim. Looks like a
> day or so to track down the differences, which I don't have.
>
> But fundamentally this patch is still wrong. If I run
> export TMPDIR='$TMPDIR' vim
> then vim should very much try '$TMPDIR' because I explicitly asked it
> to do so. Might be a silly choice, but that is mine to make.

I've always found it rather awkward that Vim tries to be too intelligent
about things that look like environment variables. The behavior should
be consistent.

If $FOO is used in a context where environment variables are expanded,
but that environment variable doesn't exist in the environment, then it
should result in an empty string. The current behavior, although
documented, is just inconsistent and therefore error-prone.

When $FOO exists, its value is substituted, otherwise the literal $FOO
is used. If one wants to literally use $FOO, then the $ should be
escaped.

Cheers,
--
James
GPG Key: 4096R/331BA3DB 2011-12-05 James McCoy <jame...@jamessan.com>