Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#800 closed defect (fixed)

r.random and r.reclass - buffer overflow on long mapset/map names

Reported by: ferrouswheel Owned by: grass-dev@…
Priority: normal Milestone: 6.4.3
Component: Raster Version: svn-develbranch6
Keywords: sprintf, r.reclass, r.random Cc:
CPU: All Platform: All

Description

In r.random/support.c there are sprintf calls which cause buffer overflow errors when the map names and mapsets are too long. I've attached a patch to replace with snprintf. This truncates the messages, but the RECORD_LEN for the History struct is by default only 80.

Attachments (3)

support.c.patch (1.3 KB) - added by ferrouswheel 7 years ago.
reclass.c.diff (1.1 KB) - added by ferrouswheel 7 years ago.
reclass_and_random_fix.diff (2.2 KB) - added by ferrouswheel 6 years ago.
fixes using G_snprintf instead

Download all attachments as: .zip

Change History (17)

Changed 7 years ago by ferrouswheel

Attachment: support.c.patch added

comment:1 Changed 7 years ago by ferrouswheel

Component: defaultRaster
CPU: UnspecifiedAll
Platform: UnspecifiedAll
Version: unspecifiedsvn-develbranch6

comment:2 in reply to:  description ; Changed 7 years ago by glynn

Replying to ferrouswheel:

In r.random/support.c there are sprintf calls which cause buffer overflow errors when the map names and mapsets are too long. I've attached a patch to replace with snprintf.

snprintf() isn't in C89; if you want to use it, you need to add a configure check, and provide an alternate in case it isn't available.

comment:3 Changed 7 years ago by hamish

Hi,

title overflow fixed in 6.5svn with r39679. I notice that a similar problem exists with the (priorly redundant but now relevant) data source metadata. IIRC those are limited to 80 (??) chars currently by gis.h.

the @mapset part can be dropped for starters, and G_command_history() added.

nothing ported to other branches yet.

Hamish

comment:4 Changed 7 years ago by ferrouswheel

Milestone: 6.4.06.5.0
Summary: r.random - buffer overflow on long mapset/map namesr.random and r.reclass - buffer overflow on long mapset/map names

Same issue occurs with r.reclass

I've attached an alternative way resolve it which doesn't use snprintf, but is rather hacky.

Changed 7 years ago by ferrouswheel

Attachment: reclass.c.diff added

comment:5 Changed 7 years ago by ferrouswheel

I wonder if there shouldn't be a wrapper method for updating the History object.

Doing a grep for the offending line:

grep -r sprintf\(hist.dat *

Shows the following modules making unsafe assumptions about map name length: r.buffer, r.carve, r.random, r.recode, r.slope.aspect, r.sunmask, and the simwe modules.

comment:6 in reply to:  2 ; Changed 7 years ago by neteler

Replying to glynn:

Replying to ferrouswheel:

In r.random/support.c there are sprintf calls which cause buffer overflow errors when the map names and mapsets are too long. I've attached a patch to replace with snprintf.

snprintf() isn't in C89; if you want to use it, you need to add a configure check, and provide an alternate in case it isn't available.

We do have G_snprintf() in lib/gis/snprintf.c which is a "private" implementation. Should that be used instead?

Markus

comment:7 in reply to:  6 Changed 7 years ago by glynn

Replying to neteler:

snprintf() isn't in C89; if you want to use it, you need to add a configure check, and provide an alternate in case it isn't available.

We do have G_snprintf() in lib/gis/snprintf.c which is a "private" implementation. Should that be used instead?

Yes. Although vsnprintf() isn't guaranteed to exist, so far no-one has complained about GRASS failing to build, which suggests that platforms lacking vsnprintf() are uncommon.

Changed 6 years ago by ferrouswheel

Attachment: reclass_and_random_fix.diff added

fixes using G_snprintf instead

comment:8 Changed 6 years ago by ferrouswheel

Any chance of my latest simple patch getting applied? It would be most appreciated - thanks!

comment:9 Changed 6 years ago by cmbarton

Just discovered that this also affects r.recode. It crashes with a very confusing buffer overflow error if the file names are too long.

Michael

comment:10 in reply to:  9 Changed 6 years ago by hamish

Replying to cmbarton:

Just discovered that this also affects r.recode. It crashes with a very confusing buffer overflow error if the file names are too long.

probably these two:

recode.c:    sprintf(hist.edhist[0], "recode of raster map %s", name);
recode.c:    sprintf(hist.datsrc_1, "raster map %s", name);

which has a max RECORD_LEN of 80 in grass 6.

Hamish

comment:11 in reply to:  5 Changed 6 years ago by hamish

Keywords: sprintf added

Replying to ferrouswheel:

Doing a grep for the offending line: grep -r sprintf\(hist.dat *

Shows the following modules making unsafe assumptions about map name length: r.buffer, r.carve, r.random, r.recode, r.slope.aspect, r.sunmask, and the simwe modules.

all of these now updated in devbr6. please test. (grass7 is not affected as the 80 column restriction on the history components has been removed)

Hamish

comment:12 Changed 4 years ago by ferrouswheel

In grass64_release, the r.reclass issue is fixed by user "mmetz". But it is not fixed for r.random?

It's been 3 years and it's sad to see the ubuntu packages still contain these simple buffer overflow bugs!

comment:13 in reply to:  12 Changed 4 years ago by hamish

Keywords: r.reclass r.random added
Milestone: 6.5.06.4.3
Resolution: fixed
Status: newclosed

Replying to ferrouswheel:

In grass64_release, the r.reclass issue is fixed by user "mmetz". But it is not fixed for r.random?

I fixed in 19 months ago (r48460); the r.reclass half of r48460 got backported, but the r.random half didn't. Now done in r55628. Trunk was fixed by Glynn a long time ago during the revamp of the hist/ structure.

(see also backport needed(?) for #1082: r.random cover= gets stray data in first and last rows)

Hamish

comment:14 Changed 4 years ago by ferrouswheel

Thanks!

Note: See TracTickets for help on using tickets.