Possible make_oidjoins_check Security Issue

On Wed, 2004-10-20 at 06:18, Rod Taylor wrote:

http://secunia.com/advisories/12860/

This seems like a rather inconsequential problem, but it should be
fixed. The first two ideas that come to mind: use temporary files in
$PWD rather than /tmp, or create a subdirectory in /tmp to use for the
temporary files.

-Neil

On Wed, Oct 20, 2004 at 12:52:57PM +1000, Neil Conway wrote:

On Wed, 2004-10-20 at 06:18, Rod Taylor wrote:

http://secunia.com/advisories/12860/

This seems like a rather inconsequential problem, but it should be
fixed. The first two ideas that come to mind: use temporary files in
$PWD rather than /tmp, or create a subdirectory in /tmp to use for the
temporary files.

Better, use mktemp(1). The thread testing script already does it IIRC.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Un poeta es un mundo encerrado en un hombre" (Victor Hugo)

Neil Conway <neilc@samurai.com> writes:

On Wed, 2004-10-20 at 06:18, Rod Taylor wrote:

http://secunia.com/advisories/12860/

This seems like a rather inconsequential problem,

Indeed, since ordinary users have no use for make_oidjoins_check.
It's surely very implausible that anyone would run it as root; and
even if someone did, the attacker cannot control what gets written.

but it should be fixed. The first two ideas that come to mind: use
temporary files in $PWD rather than /tmp, or create a subdirectory in
/tmp to use for the temporary files.

I believe that the subdirectory idea is also vulnerable without great
care.

My inclination so far as the Red Hat packages are concerned is simply to
omit the contrib/findoidjoins files from the installed RPMs.

The patch originally proposed by trustix involved using mktemp(1), which
would be a great fix if mktemp(1) weren't so laughably unportable :-(
But in any case it's hard to see why we are expending RPM distro space
on this script in the first place. I suspect that no one on the planet
except Bruce and myself have ever actually run this script.

regards, tom lane

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:

Better, use mktemp(1). The thread testing script already does it IIRC.

There are only two uses of mktemp(1) in our source tree: configure and
config.guess. Both were gotten from elsewhere, and both jump through
some seriously unreadable hoops in order to achieve allegedly-portable
behavior. mktemp(1) is simply not portable :-( ... the Single Unix Spec
refuses to touch it at all ...

regards, tom lane

On Wed, Oct 20, 2004 at 12:31:11AM -0400, Tom Lane wrote:

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:

Better, use mktemp(1). The thread testing script already does it IIRC.

There are only two uses of mktemp(1) in our source tree: configure and
config.guess. Both were gotten from elsewhere, and both jump through
some seriously unreadable hoops in order to achieve allegedly-portable
behavior.

Huh, right. I was remembering mkstemp(3), which is used in the thread
test (which is not a script after all ...)

config.guess usage surely is ugly ...

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... �Qui�n es el machito que tendr�a carnet?" (Mafalda)

Tom Lane wrote:

I suspect that no one on the planet
except Bruce and myself have ever actually run this script.

Then why don't we just remove it? Problem solved ...

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

Tom Lane wrote:

I suspect that no one on the planet
except Bruce and myself have ever actually run this script.

Then why don't we just remove it? Problem solved ...

Because it's a needed maintenance tool. There isn't any particularly
good reason for it to get installed as though it were an interesting
program for users, though, so I think that this is mostly a matter of
poor packaging choices. I am in fact intending to remove the
contrib/findoidjoins files from the set of stuff installed by Red Hat's
RPMs.

I suppose you could make an argument for moving this directory out of
contrib and putting it under src/tools instead, but that seems like more
work (and loss of CVS history) than it's worth.

regards, tom lane

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Tom Lane wrote:

I suspect that no one on the planet
except Bruce and myself have ever actually run this script.

Then why don't we just remove it? Problem solved ...

Because it's a needed maintenance tool. There isn't any particularly
good reason for it to get installed as though it were an interesting
program for users, though, so I think that this is mostly a matter of
poor packaging choices. I am in fact intending to remove the
contrib/findoidjoins files from the set of stuff installed by Red Hat's
RPMs.

I suppose you could make an argument for moving this directory out of
contrib and putting it under src/tools instead, but that seems like more
work (and loss of CVS history) than it's worth.

Then maybe there's a case for removing findoidjoins from WANTED_DIRS in
contrib/Makefile? I agree this issue is so trifling that it's not worth
spending much energy on.

On a very slightly related note, I see that ipcclean (which is a shell
script) is installed on Windows by "make install". Do we want to fix
that or trust the binary packagers to remove it?

cheers

andrew

Tom Lane wrote:

Neil Conway <neilc@samurai.com> writes:

On Wed, 2004-10-20 at 06:18, Rod Taylor wrote:

http://secunia.com/advisories/12860/

This seems like a rather inconsequential problem,

Indeed, since ordinary users have no use for make_oidjoins_check.
It's surely very implausible that anyone would run it as root; and
even if someone did, the attacker cannot control what gets written.

but it should be fixed. The first two ideas that come to mind: use
temporary files in $PWD rather than /tmp, or create a subdirectory in
/tmp to use for the temporary files.

I believe that the subdirectory idea is also vulnerable without great
care.

I believe the proper way to handle this is a new directory under /tmp.
I use this in my local scripts:

TMP=/tmp/$$
OMASK="`umask`"
umask 077
if ! mkdir "$TMP"
then echo "Can't create temporary directory $TMP." 1>&2
exit 1
fi
umask "$OMASK"
unset OMASK

It basically makes sure it creates a new directory under /tmp with a
umask that guarantees no one else can create a file in that directory.
All temp files are accessed as $TMP/XXX.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I believe the proper way to handle this is a new directory under /tmp.

It's definitely not worth the trouble. I looked at what configure does
to make /tmp subdirectories portably, and it is spectacularly ugly
(not to mention long). If make_oidjoins_check were a user-facing tool
that would be one thing, but it isn't ...

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I believe the proper way to handle this is a new directory under /tmp.

It's definitely not worth the trouble. I looked at what configure does
to make /tmp subdirectories portably, and it is spectacularly ugly
(not to mention long). If make_oidjoins_check were a user-facing tool
that would be one thing, but it isn't ...

From a public relations perspective and a code reuse perspective I think
we should create temporary tables securely. The attached applied patch
fixes contrib/findoidjoins/make_oidjoins_check.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I believe the proper way to handle this is a new directory under /tmp.

It's definitely not worth the trouble. I looked at what configure does
to make /tmp subdirectories portably, and it is spectacularly ugly
(not to mention long). If make_oidjoins_check were a user-facing tool
that would be one thing, but it isn't ...

From a public relations perspective and a code reuse perspective I think

we should create temporary tables securely. The attached applied patch

^^^^^^
files

fixes contrib/findoidjoins/make_oidjoins_check.

Sorry, meant temporary files.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

From a public relations perspective and a code reuse perspective I think
we should create temporary tables securely. The attached applied patch
fixes contrib/findoidjoins/make_oidjoins_check.

... and creates issues of its own, such as attempting an rm -rf on
something that it shouldn't. At the very least don't install the trap
until after creating the directory successfully.

I really think this is a waste of time though. The current code creates
the temp files in the current directory, and if the bad guy has write
access on that directory you are already screwed (for instance, what's
to stop him from altering the script file itself to do anything at all
when you run it?). I do not think that putting stuff back into /tmp is
an improvement; that just adds risks where none exist now.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

From a public relations perspective and a code reuse perspective I think
we should create temporary tables securely. The attached applied patch
fixes contrib/findoidjoins/make_oidjoins_check.

... and creates issues of its own, such as attempting an rm -rf on
something that it shouldn't. At the very least don't install the trap
until after creating the directory successfully.

OK, moved.

I really think this is a waste of time though. The current code creates
the temp files in the current directory, and if the bad guy has write
access on that directory you are already screwed (for instance, what's
to stop him from altering the script file itself to do anything at all
when you run it?). I do not think that putting stuff back into /tmp is
an improvement; that just adds risks where none exist now.

My method is secure, and I think we do have to handle this in a way that
addresses the security concerns. It is easy to say no one would run
this under normal use but that isn't really a safe answer for the
security folks, I think.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Thu, 2004-11-04 at 10:07, Bruce Momjian wrote:

My method is secure, and I think we do have to handle this in a way that
addresses the security concerns.

I think Tom's fix adequately addresses the security concerns. Exactly
what is wrong with writing to the current working directory?

It is easy to say no one would run
this under normal use but that isn't really a safe answer for the
security folks, I think.

This is a non-sequitor -- I don't think Tom or anyone else has argued
this.

-Neil

Neil Conway wrote:

On Thu, 2004-11-04 at 10:07, Bruce Momjian wrote:

My method is secure, and I think we do have to handle this in a way that
addresses the security concerns.

I think Tom's fix adequately addresses the security concerns. Exactly
what is wrong with writing to the current working directory?

Because it could be run from a directory where others have write
permission.

It is easy to say no one would run
this under normal use but that isn't really a safe answer for the
security folks, I think.

This is a non-sequitor -- I don't think Tom or anyone else has argued
this.

I remember hearing that from someone. I thought it was Tom.

Bottom line is that the only secure way I have ever heard of for
creating temp files is to create a 077 directory in /tmp and write in
there.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I think Tom's fix adequately addresses the security concerns. Exactly
what is wrong with writing to the current working directory?

Because it could be run from a directory where others have write
permission.

In which case, they could also change the findoidjoins script itself.
I think your fix is *less* secure than what you replaced.

However, I've already wasted more than enough time on this issue...
I'm done arguing about it.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I think Tom's fix adequately addresses the security concerns. Exactly
what is wrong with writing to the current working directory?

Because it could be run from a directory where others have write
permission.

In which case, they could also change the findoidjoins script itself.
I think your fix is *less* secure than what you replaced.

However, I've already wasted more than enough time on this issue...
I'm done arguing about it.

As far as I know, my method is the only secure method. If I am wrong I
would like to know.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Wed, 3 Nov 2004, Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I think Tom's fix adequately addresses the security concerns. Exactly
what is wrong with writing to the current working directory?

Because it could be run from a directory where others have write
permission.

In which case, they could also change the findoidjoins script itself.
I think your fix is *less* secure than what you replaced.

However, I've already wasted more than enough time on this issue...
I'm done arguing about it.

As far as I know, my method is the only secure method. If I am wrong I
would like to know.

I think the problem can really be solved by just removing it from the
distribution. However, one thing I noticed with Bruce's script is that it
does not respect $TMPDIR -- which security conscious admins may be
setting. Solution would be to set TMP=${TMPDIR:-/tmp} before defining the
path to the temporary sub directory.

Thanks,

Gavin