[GRASS5] Re: [Pkg-grass-general] r.terraflow ?

GRASS GIS GRASS GIS developer
1

> > Isn't r.terraflow modul added to grass6 (installed as debian
> > package).
>
> No. There is an outstanding security issue that precludes it from
> being part of the Debian package. (insecure temp files)
>
> See /usr/share/doc/grass/changelog.Debian.gz

What's keeping us from patching r.terraflow then? I'm guessing it's
probably swapping one libc function for another, no?

Not very much, just needs to be changed to use a directory created with
G_tempfile() or tmpfile() instead of /var/tmp/ by default for the
STREAM_DIR= option.

G_tempfile() creates a temporary file in the users' mapset repository,
e.g. $MAPSET/.tmp/$HOSTNAME/12345.0

Just need to remove that file, mkdir something of the same name &
cleanup when done?

G_tempfile() is found in the grass source in lib/gis/tempfile.c

Alternatively & maybe better use tmpfile(). G_tempfile() & usage
rules may be in flux in the near future, please read this thread:
  http://thread.gmane.org/gmane.comp.gis.grass.devel/8065

I had fixed this for other modules to take care of Debian bug #287651,
but didn't touch r.terraflow for two reasons. a) it's optional; b)
the original author is still around. To date no fix from (b) though.

further reading:
  http://www.linuxsecurity.com/content/view/115462/151/#mozTocId316364

Hamish

2

Hamish wrote:

> > > Isn't r.terraflow modul added to grass6 (installed as debian
> > > package).
> >
> > No. There is an outstanding security issue that precludes it from
> > being part of the Debian package. (insecure temp files)
> >
> > See /usr/share/doc/grass/changelog.Debian.gz
>
> What's keeping us from patching r.terraflow then? I'm guessing it's
> probably swapping one libc function for another, no?

Not very much, just needs to be changed to use a directory created with
G_tempfile() or tmpfile() instead of /var/tmp/ by default for the
STREAM_DIR= option.

G_tempfile() creates a temporary file in the users' mapset repository,
e.g. $MAPSET/.tmp/$HOSTNAME/12345.0

Just need to remove that file, mkdir something of the same name &
cleanup when done?

G_tempfile() is found in the grass source in lib/gis/tempfile.c

Alternatively & maybe better use tmpfile(). G_tempfile() & usage
rules may be in flux in the near future, please read this thread:
  http://thread.gmane.org/gmane.comp.gis.grass.devel/8065

The simplest approach is likely to be to use the session directory
/tmp/grass6-<user>-<pid>. That should be writable only by its owner.
So long as that directory is created securely, we don't need to worry
about creating files inside it. At least, not from a security
standpoint; race conditions could still be an issue for background
processes.

--
Glynn Clements <glynn@gclements.plus.com>

3

On Fri, 2005-06-24 at 09:56 +0100, Glynn Clements wrote:

Hamish wrote:

> > > > Isn't r.terraflow modul added to grass6 (installed as debian
> > > > package).
> > >
> > > No. There is an outstanding security issue that precludes it from
> > > being part of the Debian package. (insecure temp files)
> > >
> > > See /usr/share/doc/grass/changelog.Debian.gz
> >
> > What's keeping us from patching r.terraflow then? I'm guessing it's
> > probably swapping one libc function for another, no?
>
>
> Not very much, just needs to be changed to use a directory created with
> G_tempfile() or tmpfile() instead of /var/tmp/ by default for the
> STREAM_DIR= option.
>
> G_tempfile() creates a temporary file in the users' mapset repository,
> e.g. $MAPSET/.tmp/$HOSTNAME/12345.0
>
> Just need to remove that file, mkdir something of the same name &
> cleanup when done?
>
> G_tempfile() is found in the grass source in lib/gis/tempfile.c
>
> Alternatively & maybe better use tmpfile(). G_tempfile() & usage
> rules may be in flux in the near future, please read this thread:
> http://thread.gmane.org/gmane.comp.gis.grass.devel/8065

The simplest approach is likely to be to use the session directory
/tmp/grass6-<user>-<pid>. That should be writable only by its owner.
So long as that directory is created securely, we don't need to worry
about creating files inside it. At least, not from a security
standpoint; race conditions could still be an issue for background
processes.

I'm working on it. I need some sleep, so I'll finish it up tomorrow and
post a patch.

Oddly, enough, I was just about to post a message about handling /tmp.
Creating a dir as you stated above answered it.

While I'm at it, should I create 'char *G_mktemp(int global)' and 'int
G_rmtemp(char *path)' functions?

--
Brad Douglas <rez@touchofmadness.com>

4

> > > What's keeping us from patching r.terraflow then? I'm guessing
> > > it's probably swapping one libc function for another, no?

..

> > Not very much, just needs to be changed to use a directory created
> > with G_tempfile() or tmpfile() instead of /var/tmp/ by default for
> > the STREAM_DIR= option.

..

> The simplest approach is likely to be to use the session directory
> /tmp/grass6-<user>-<pid>. That should be writable only by its owner.
>
> So long as that directory is created securely, we don't need to
> worry about creating files inside it. At least, not from a security
> standpoint; race conditions could still be an issue for background
> processes.

I'm working on it. I need some sleep, so I'll finish it up tomorrow
and post a patch.

Oddly, enough, I was just about to post a message about handling /tmp.
Creating a dir as you stated above answered it.

While I'm at it, should I create 'char *G_mktemp(int global)' and 'int
G_rmtemp(char *path)' functions?

You don't need to create that dir, GRASS does that automatically on
startup and removes it on exit. You just have to point to it.

in the shell:
GRASS_TMPDIR="`dirname $GISRC`/"

Slight problem where it doesn't get cleaned up if you start from the GUI
and hit "Exit" from the startup screen or make a new location by EPSG
number. Perhaps it shouldn't make the dir until after you select a
mapset?

(setup & teardown of tmp dir are in lib/init/init.sh)

Hamish

5

On Sat, 2005-06-25 at 17:04 +1200, Hamish wrote:

> While I'm at it, should I create 'char *G_mktemp(int global)' and 'int
> G_rmtemp(char *path)' functions?

You don't need to create that dir, GRASS does that automatically on
startup and removes it on exit. You just have to point to it.

The above functions wouldn't create the dir, but the temp file within.
Essentially "macro" functions to save a few lines of code here and there
and provides a wrapper for G_tempfile() and G_local_tempfile().
Completely optional, but I think it's a good idea...especially for the
average module programmer who does not [need to] intimately know the
GRASS API.

It is a large change, but will not affect existing modules as the old
functions will still be exported, but I would recommend depreciating
them in the next major release and eventually removing them.

in the shell:
GRASS_TMPDIR="`dirname $GISRC`/"

Slight problem where it doesn't get cleaned up if you start from the GUI
and hit "Exit" from the startup screen or make a new location by EPSG
number. Perhaps it shouldn't make the dir until after you select a
mapset?

(setup & teardown of tmp dir are in lib/init/init.sh)

/tmp should not be referenced directly -- it is not portable. It should
be setup from the environment. Why is that commented out in init.sh?

When I wrote the original email, I was looking at _get_make_sock_path().
I had not ventured into the shell scripts. Thank you for the insight.

Having the dir already setup by init.sh does greatly simplify things for
me, although _get_make_sock_path() needs fixing.

--
Brad Douglas <rez@touchofmadness.com>

6

> (setup & teardown of tmp dir are in lib/init/init.sh)

/tmp should not be referenced directly -- it is not portable. It should
be setup from the environment. Why is that commented out in init.sh?

see this thread from feb.

http://thread.gmane.org/gmane.comp.gis.grass.devel/6613

Hamish

7

Brad Douglas wrote:

I'm working on it. I need some sleep, so I'll finish it up tomorrow and
post a patch.

Oddly, enough, I was just about to post a message about handling /tmp.
Creating a dir as you stated above answered it.

While I'm at it, should I create 'char *G_mktemp(int global)' and 'int
G_rmtemp(char *path)' functions?

The former should be the new G_tempfile(), once G_tempfile() has been
renamed. It doesn't need a parameter; it should determine a unique
name on its own.

The latter is unnecessary; you can just use remove().

--
Glynn Clements <glynn@gclements.plus.com>