Error handling in Read/Write/Seek

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Error handling in Read/Write/Seek

Nicolas RUFF
Hello,

Apparently TIFFReadFile / TIFFWriteFile / TIFFSeekFile return byte
count on success, and -1 on error.

I have a pathological TIFF file (generated by a fuzzer) where a
directory seek will actually end up calling TIFFSeekFile(-1,
SEEK_SET). This will pass the SeekOK() test in any case, and result in
an out-of-bound access to tiff_buffer[-1].

The quick-and-dirty fix I have is redefining the following macros:

#define ReadOK(tif, buf, size) \
(TIFFReadFile((tif),(buf),(size))==(size))
#define SeekOK(tif, off) \
(TIFFSeekFile((tif),(off),SEEK_SET)==(off))
#define WriteOK(tif, buf, size) \
(TIFFWriteFile((tif),(buf),(size))==(size))

into:

#define ReadOK(tif, buf, size) \
((TIFFReadFile((tif),(buf),(size))==(size) && (size)!=-1))
#define SeekOK(tif, off) \
((TIFFSeekFile((tif),(off),SEEK_SET)==(off) && (off)!=-1))
#define WriteOK(tif, buf, size) \
((TIFFWriteFile((tif),(buf),(size))==(size) && (size)!=-1))

This approach has two shortcomings:
1. If "size" or "off" is an expression, it will be evaluated one more time.
2. I wonder if a directory entry could be located at exact offset
0xFFFFFFFF in the input file (in which case it would be inaccessible).

The real fix would be some kind of out-of-band error reporting (a la
errno). Such a change would be transparent to all users of ReadOK /
WriteOK / SeekOK macros, but I wonder if there are people in the wild
checking return values on their own.

WDYT?
_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in Read/Write/Seek

Bob Friesenhahn
On Wed, 2 Aug 2017, Nicolas RUFF wrote:
>
> The real fix would be some kind of out-of-band error reporting (a la
> errno). Such a change would be transparent to all users of ReadOK /
> WriteOK / SeekOK macros, but I wonder if there are people in the wild
> checking return values on their own.

This is a private header so there should be no other users of these
macros besides libtiff.

It seems best to block any negative size values from being passed into
these functions in the first place.  Libtiff is not in control of the
I/O functions, so it is best to assure that they are not passed
illegal values which might cause I/O implementations to do very bad
things.

Bob
--
Bob Friesenhahn
[hidden email], http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer,    http://www.GraphicsMagick.org/
_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in Read/Write/Seek

Nicolas RUFF
> It seems best to block any negative size values from being passed into these
> functions in the first place.  Libtiff is not in control of the I/O
> functions, so it is best to assure that they are not passed illegal values
> which might cause I/O implementations to do very bad things.

Not sure to understand what you mean here. Here are all call locations
for SeekOK():
tif_dir.c : 2 times
tif_read.c : 4 times
tif_write.c : 1 time
tif_dirread.c : 3 times
tif_dirwrite.c : 9 times

Do you suggest to add an extra check for (off<0) before each call? If
yes, I can prepare a patch.

Also, is there any chance that TIFFSeekFile() could legitimately be
called with off<0 and whence=SEEK_CUR? I mean, is there any reason to
seek backward in a TIFF file? Looking at the code, the only calls to
TIFFSeekFile(whence=SEEK_CUR) take (dircount*object_size) as an offset
and should therefore always be positive, but I am not super-familiar
with TIFF format(s).

Thank you,
- Nicolas RUFF
_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in Read/Write/Seek

Bob Friesenhahn
On Thu, 3 Aug 2017, Nicolas RUFF wrote:

>> It seems best to block any negative size values from being passed into these
>> functions in the first place.  Libtiff is not in control of the I/O
>> functions, so it is best to assure that they are not passed illegal values
>> which might cause I/O implementations to do very bad things.
>
> Not sure to understand what you mean here. Here are all call locations
> for SeekOK():
> tif_dir.c : 2 times
> tif_read.c : 4 times
> tif_write.c : 1 time
> tif_dirread.c : 3 times
> tif_dirwrite.c : 9 times
>
> Do you suggest to add an extra check for (off<0) before each call? If
> yes, I can prepare a patch.

Yes, I am suggesting that it is best to avoid passing a wrong request
to the I/O functions (which might have been implemented by a library
user or are implemented in a C library on any operating system libtiff
may be compiled on) rather than dealing with a subsquent possible
error return due to the wrong request.  This approach assures
consistent behavior.  It can also produce a better quality (more
appropriate) error message for the library user.

> Also, is there any chance that TIFFSeekFile() could legitimately be
> called with off<0 and whence=SEEK_CUR? I mean, is there any reason to
> seek backward in a TIFF file? Looking at the code, the only calls to
> TIFFSeekFile(whence=SEEK_CUR) take (dircount*object_size) as an offset
> and should therefore always be positive, but I am not super-familiar
> with TIFF format(s).

I don't know what might happen in the future.  We only know what
libtiff does now.  The seek operation used should be apparent at each
call point.

Bob
--
Bob Friesenhahn
[hidden email], http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer,    http://www.GraphicsMagick.org/
_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in Read/Write/Seek

Even Rouault-2
In reply to this post by Nicolas RUFF

On jeudi 3 août 2017 17:04:55 CEST Nicolas RUFF wrote:

> > It seems best to block any negative size values from being passed into

> > these functions in the first place. Libtiff is not in control of the I/O

> > functions, so it is best to assure that they are not passed illegal

> > values which might cause I/O implementations to do very bad things.

>

> Not sure to understand what you mean here. Here are all call locations

> for SeekOK():

> tif_dir.c : 2 times

> tif_read.c : 4 times

> tif_write.c : 1 time

> tif_dirread.c : 3 times

> tif_dirwrite.c : 9 times

>

> Do you suggest to add an extra check for (off<0) before each call? If

> yes, I can prepare a patch.

 

Before patching *all* those calls, I'd start with just the one (or the few ones) that are needed with your fuzzed file (blindly guessing that the issue is localized and not general...)

 

Opening a ticket in http://bugzilla.maptools.org/enter_bug.cgi with the reproducer file and your proposed patch would be good.

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in Read/Write/Seek

Nicolas RUFF
Thank you all. I filed http://bugzilla.maptools.org/show_bug.cgi?id=2726 as a follow-up. Let's continue this discussion on the bug.

Regards,
- Nicolas RUFF

2017-08-04 17:27 GMT+02:00 Even Rouault <[hidden email]>:

On jeudi 3 août 2017 17:04:55 CEST Nicolas RUFF wrote:

> > It seems best to block any negative size values from being passed into

> > these functions in the first place. Libtiff is not in control of the I/O

> > functions, so it is best to assure that they are not passed illegal

> > values which might cause I/O implementations to do very bad things.

>

> Not sure to understand what you mean here. Here are all call locations

> for SeekOK():

> tif_dir.c : 2 times

> tif_read.c : 4 times

> tif_write.c : 1 time

> tif_dirread.c : 3 times

> tif_dirwrite.c : 9 times

>

> Do you suggest to add an extra check for (off<0) before each call? If

> yes, I can prepare a patch.

 

Before patching *all* those calls, I'd start with just the one (or the few ones) that are needed with your fuzzed file (blindly guessing that the issue is localized and not general...)

 

Opening a ticket in http://bugzilla.maptools.org/enter_bug.cgi with the reproducer file and your proposed patch would be good.

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com



_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in Read/Write/Seek

Nicolas RUFF
Sorry to resurrect this old thread, but I came up with a much simpler patch:

--- libtiff/tiffiop.h    2017-07-04 15:28:42.000000000 +0200
+++ libtiff/tiffiop.h    2017-09-05 13:38:02.305947462 +0200
@@ -239,7 +239,7 @@
 #endif
 #ifndef SeekOK
 #define SeekOK(tif, off) \
-    (TIFFSeekFile((tif),(off),SEEK_SET)==(off))
+    (((off_t)(off) >= 0) && TIFFSeekFile((tif),(off),SEEK_SET)==(off))
 #endif
 #ifndef WriteOK
 #define WriteOK(tif, buf, size) \

The rationale behind is described in http://bugzilla.maptools.org/show_bug.cgi?id=2726#c4

Would you consider this patch for inclusion?

Thank you.

2017-08-07 17:53 GMT+02:00 Nicolas RUFF <[hidden email]>:
Thank you all. I filed http://bugzilla.maptools.org/show_bug.cgi?id=2726 as a follow-up. Let's continue this discussion on the bug.

Regards,
- Nicolas RUFF

2017-08-04 17:27 GMT+02:00 Even Rouault <[hidden email]>:

On jeudi 3 août 2017 17:04:55 CEST Nicolas RUFF wrote:

> > It seems best to block any negative size values from being passed into

> > these functions in the first place. Libtiff is not in control of the I/O

> > functions, so it is best to assure that they are not passed illegal

> > values which might cause I/O implementations to do very bad things.

>

> Not sure to understand what you mean here. Here are all call locations

> for SeekOK():

> tif_dir.c : 2 times

> tif_read.c : 4 times

> tif_write.c : 1 time

> tif_dirread.c : 3 times

> tif_dirwrite.c : 9 times

>

> Do you suggest to add an extra check for (off<0) before each call? If

> yes, I can prepare a patch.

 

Before patching *all* those calls, I'd start with just the one (or the few ones) that are needed with your fuzzed file (blindly guessing that the issue is localized and not general...)

 

Opening a ticket in http://bugzilla.maptools.org/enter_bug.cgi with the reproducer file and your proposed patch would be good.

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com




_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in Read/Write/Seek

Even Rouault-2

On mercredi 6 septembre 2017 09:48:18 CEST Nicolas RUFF wrote:

> Sorry to resurrect this old thread, but I came up with a much simpler patch:

>

> --- libtiff/tiffiop.h 2017-07-04 15:28:42.000000000 +0200

> +++ libtiff/tiffiop.h 2017-09-05 13:38:02.305947462 +0200

> @@ -239,7 +239,7 @@

> #endif

> #ifndef SeekOK

> #define SeekOK(tif, off) \

> - (TIFFSeekFile((tif),(off),SEEK_SET)==(off))

> + (((off_t)(off) >= 0) && TIFFSeekFile((tif),(off),SEEK_SET)==(off))

> #endif

> #ifndef WriteOK

> #define WriteOK(tif, buf, size) \

>

> The rationale behind is described in

> http://bugzilla.maptools.org/show_bug.cgi?id=2726#c4

>

> Would you consider this patch for inclusion?

 

One potential issue I can see is that we use off_t here, which isn't necessarily what is use in the implementation of the file system callbacks. For example tif_unix.c uses _TIFF_off_t. I'm wondering for example if off_t might not be 32 bit only in some circumstances when evaluated in libtiff non-IO code.

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in Read/Write/Seek

Nicolas RUFF
> One potential issue I can see is that we use off_t here, which isn't
> necessarily what is use in the implementation of the file system callbacks.
> For example tif_unix.c uses _TIFF_off_t. I'm wondering for example if off_t
> might not be 32 bit only in some circumstances when evaluated in libtiff
> non-IO code.

Indeed off_t is 32-bit on 32-bit Linux systems.

TIFFSeekProc is typedef'ed to (thandle_t, toff_t, int).

And toff_t is an alias for uint64, regardless of the platform (from tiffio.h).

tif_unix.c implementation adheres to this type definition:
_tiffSeekProc(thandle_t fd, uint64 off, int whence)

So the proper patch should be:

- (TIFFSeekFile((tif),(off),SEEK_SET)==(off))
+ (((int64)(off) >= 0) && TIFFSeekFile((tif),(off),SEEK_SET)==(off))

Thank you for catching this!
_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in Read/Write/Seek

Even Rouault-2

Hi,

 

> So the proper patch should be:

>

> - (TIFFSeekFile((tif),(off),SEEK_SET)==(off))

> + (((int64)(off) >= 0) && TIFFSeekFile((tif),(off),SEEK_SET)==(off))

>

 

I've applied a variation of your proposal, which avoids some compiler warning when off is of type uint32 (then a cast to int64 always result in a >= 0 value), which avoids undefined behaviour when casting a too large uint64 to int64 and which avoids the (existing) issue if off where the result of a function with a side effect.

 

 

Index: libtiff/tif_aux.c

===================================================================

RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tif_aux.c,v

retrieving revision 1.29

diff -u -r1.29 tif_aux.c

--- libtiff/tif_aux.c 11 Nov 2016 20:45:53 -0000 1.29

+++ libtiff/tif_aux.c 7 Sep 2017 14:00:39 -0000

@@ -359,6 +359,13 @@

}

}

+int _TIFFSeekOK(TIFF* tif, toff_t off)

+{

+ /* Huge offsets, expecially -1 / UINT64_MAX, can cause issues */

+ /* See http://bugzilla.maptools.org/show_bug.cgi?id=2726 */

+ return off <= (~(uint64)0)/2 && TIFFSeekFile(tif,off,SEEK_SET)==off;

+}

+

/* vim: set ts=8 sts=8 sw=8 noet: */

/*

* Local Variables:

Index: libtiff/tiffiop.h

===================================================================

RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tiffiop.h,v

retrieving revision 1.94

diff -u -r1.94 tiffiop.h

--- libtiff/tiffiop.h 4 Jul 2017 13:28:42 -0000 1.94

+++ libtiff/tiffiop.h 7 Sep 2017 14:00:39 -0000

@@ -238,8 +238,7 @@

(TIFFReadFile((tif),(buf),(size))==(size))

#endif

#ifndef SeekOK

-#define SeekOK(tif, off) \

- (TIFFSeekFile((tif),(off),SEEK_SET)==(off))

+#define SeekOK(tif, off) _TIFFSeekOK(tif, off)

#endif

#ifndef WriteOK

#define WriteOK(tif, buf, size) \

@@ -384,6 +383,7 @@

_TIFFReadTileAndAllocBuffer(TIFF* tif,

void **buf, tmsize_t bufsizetoalloc,

uint32 x, uint32 y, uint32 z, uint16 s);

+extern int _TIFFSeekOK(TIFF* tif, toff_t off);

extern int TIFFInitDumpMode(TIFF*, int);

#ifdef PACKBITS_SUPPORT

 

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in Read/Write/Seek

Nicolas RUFF
Awesome, thank you!

I'll resume fuzzing and keep you posted.


2017-09-07 16:06 GMT+02:00 Even Rouault <[hidden email]>:

> Hi,
>
>
>
>> So the proper patch should be:
>
>>
>
>> - (TIFFSeekFile((tif),(off),SEEK_SET)==(off))
>
>> + (((int64)(off) >= 0) && TIFFSeekFile((tif),(off),SEEK_SET)==(off))
>
>>
>
>
>
> I've applied a variation of your proposal, which avoids some compiler
> warning when off is of type uint32 (then a cast to int64 always result in a
>>= 0 value), which avoids undefined behaviour when casting a too large
> uint64 to int64 and which avoids the (existing) issue if off where the
> result of a function with a side effect.
>
>
>
>
>
> Index: libtiff/tif_aux.c
>
> ===================================================================
>
> RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tif_aux.c,v
>
> retrieving revision 1.29
>
> diff -u -r1.29 tif_aux.c
>
> --- libtiff/tif_aux.c 11 Nov 2016 20:45:53 -0000 1.29
>
> +++ libtiff/tif_aux.c 7 Sep 2017 14:00:39 -0000
>
> @@ -359,6 +359,13 @@
>
> }
>
> }
>
> +int _TIFFSeekOK(TIFF* tif, toff_t off)
>
> +{
>
> + /* Huge offsets, expecially -1 / UINT64_MAX, can cause issues */
>
> + /* See http://bugzilla.maptools.org/show_bug.cgi?id=2726 */
>
> + return off <= (~(uint64)0)/2 && TIFFSeekFile(tif,off,SEEK_SET)==off;
>
> +}
>
> +
>
> /* vim: set ts=8 sts=8 sw=8 noet: */
>
> /*
>
> * Local Variables:
>
> Index: libtiff/tiffiop.h
>
> ===================================================================
>
> RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tiffiop.h,v
>
> retrieving revision 1.94
>
> diff -u -r1.94 tiffiop.h
>
> --- libtiff/tiffiop.h 4 Jul 2017 13:28:42 -0000 1.94
>
> +++ libtiff/tiffiop.h 7 Sep 2017 14:00:39 -0000
>
> @@ -238,8 +238,7 @@
>
> (TIFFReadFile((tif),(buf),(size))==(size))
>
> #endif
>
> #ifndef SeekOK
>
> -#define SeekOK(tif, off) \
>
> - (TIFFSeekFile((tif),(off),SEEK_SET)==(off))
>
> +#define SeekOK(tif, off) _TIFFSeekOK(tif, off)
>
> #endif
>
> #ifndef WriteOK
>
> #define WriteOK(tif, buf, size) \
>
> @@ -384,6 +383,7 @@
>
> _TIFFReadTileAndAllocBuffer(TIFF* tif,
>
> void **buf, tmsize_t bufsizetoalloc,
>
> uint32 x, uint32 y, uint32 z, uint16 s);
>
> +extern int _TIFFSeekOK(TIFF* tif, toff_t off);
>
> extern int TIFFInitDumpMode(TIFF*, int);
>
> #ifdef PACKBITS_SUPPORT
>
>
>
>
>
> Even
>
>
>
> --
>
> Spatialys - Geospatial professional services
>
> http://www.spatialys.com
_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/