Re: tiff

2019-02-12 Thread Brian May
Hugo Lefeuvre  writes:

>> ++if (0x / tilew < spp) {
>
> I don't really like this patch... it has not been merged yet (the PR has
> been closed, so I guess it will never get merged) and looks more like a
> hack to me.
>
> What if tilew * spp = INT_MAX ?
>
> Then oskew + iskew will still overflow. So this does not fix the issue.

I can't say I really followed this patch... Seems odd using a divide
here. Should I skip this patch for now?
-- 
Brian May 



Re: tiff

2019-02-12 Thread Hugo Lefeuvre
.. follow up of 20190212073152.ga2...@behemoth.owl.eu.com.local

otherwise tests went fine.

one more comment:

> +  * Non-maintainer upload by the LTS Team.
> +  * Fix CVE-2018-19210: NULL pointer dereference
> +There is a NULL pointer dereference in the TIFFWriteDirectorySec function
> +in tif_dirwrite.c that will lead to a denial of service attack, as
> +demonstrated by tiffset.
> +  * Fix CVE-2018-17000: NULL pointer dereference
> +A NULL pointer dereference in the function _TIFFmemcmp at tif_unix.c 
> (called
> +from TIFFWriteDirectoryTagTransferfunction) allows an attacker
> +to cause a denial-of-service through a crafted tiff file. This 
> vulnerability
> +can be triggered by the executable tiffcp.

This patch is actually the one for CVE-2019-7663, which happens to also
fix CVE-2018-17000 (not confirmed by upstream yet?). I suggest to mention
CVE-2019-7663 here. :)

thanks!

Hugo

-- 
Hugo Lefeuvre (hle)|www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C


signature.asc
Description: PGP signature


Re: tiff

2019-02-11 Thread Hugo Lefeuvre
Hi Brian,

I am currently testing the update. I already had a look at the patches.

> diff -Nru tiff-4.0.3/debian/patches/CVE-2018-12900.patch 
> tiff-4.0.3/debian/patches/CVE-2018-12900.patch
> --- tiff-4.0.3/debian/patches/CVE-2018-12900.patch1970-01-01 
> 10:00:00.0 +1000
> +++ tiff-4.0.3/debian/patches/CVE-2018-12900.patch2019-02-08 
> 14:52:01.0 +1100
> @@ -0,0 +1,13 @@
> +--- a/tools/tiffcp.c
>  b/tools/tiffcp.c
> +@@ -1394,6 +1394,10 @@
> + uint32 row;
> + uint16 bps, bytes_per_sample;
> + 
> ++if (0x / tilew < spp) {
> ++TIFFError(TIFFFileName(in), "Error, either TileWidth (%u) or 
> SamplePerPixel (%u) is too large", tilew, spp);
> ++return 0;
> ++}
> + tilebuf = _TIFFmalloc(tilesize);
> + if (tilebuf == 0)
> + return 0;

I don't really like this patch... it has not been merged yet (the PR has
been closed, so I guess it will never get merged) and looks more like a
hack to me.

What if tilew * spp = INT_MAX ?

Then oskew + iskew will still overflow. So this does not fix the issue.

cheers,
Hugo

-- 
Hugo Lefeuvre (hle)|www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C


signature.asc
Description: PGP signature


Re: tiff

2019-02-10 Thread Hugo Lefeuvre
Hi,

> Attached is my proposed patch for tiff in Jessie.

I will be able to test the upload with my usual set of test files tomorrow
if necessary.

> +From d0a842c5dbad2609aed43c701a12ed12461d3405 Mon Sep 17 00:00:00 2001
> +From: Hugo Lefeuvre 
> +Date: Wed, 21 Nov 2018 18:50:34 +0100
> +Subject: [PATCH] tif_dir: unset transferfunction field if necessary

Thanks for getting this patch merged :)

regards,
 Hugo

-- 
Hugo Lefeuvre (hle)|www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C


signature.asc
Description: PGP signature


Re: tiff

2019-02-10 Thread Ola Lundqvist
Hi Brian

I made a brief look and I have nothing to complain about.

// Ola

On Sun, 10 Feb 2019 at 21:55, Brian May  wrote:

> Hello,
>
> Attached is my proposed patch for tiff in Jessie.
>
> Regards
> --
> Brian May 
>


-- 
 --- Inguza Technology AB --- MSc in Information Technology 
/  o...@inguza.comFolkebogatan 26\
|  o...@debian.org   654 68 KARLSTAD|
|  http://inguza.com/Mobile: +46 (0)70-332 1551 |
\  gpg/f.p.: 7090 A92B 18FE 7994 0C36 4FE4 18A1 B1CF 0FE5 3DD9  /
 ---


Re: tiff / CVE-2018-18661

2018-11-15 Thread Brian May
Ola Lundqvist  writes:

> Could it be so that the problem is only reproducible on 32-bit systems?

It also occurred to me that maybe my computer has too much memory to
reproduce an "out of memory" error condition:

=== cut ===
(stretch-i386-default)root@silverfish:/tmp/brian/tmpcvxwwflu/build/i386# 
valgrind tiff2bw /tmp/poc /dev/null
==4412== Memcheck, a memory error detector
==4412== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==4412== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==4412== Command: tiff2bw /tmp/poc /dev/null
==4412== 
TIFFReadDirectory: Warning, Unknown field with tag 292 (0x124) encountered.
TIFFScanlineSize: Integer arithmetic overflow.
TIFFReadDirectory: Cannot handle zero scanline size.
==4412== 
==4412== HEAP SUMMARY:
==4412== in use at exit: 0 bytes in 0 blocks
==4412==   total heap usage: 33 allocs, 33 frees, 3,943 bytes allocated
==4412== 
==4412== All heap blocks were freed -- no leaks are possible
==4412== 
==4412== For counts of detected and suppressed errors, rerun with: -v
==4412== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
(stretch-i386-default)root@silverfish:/tmp/brian/tmpcvxwwflu/build/i386# 
valgrind -v tiff2bw /tmp/poc /dev/null
==4508== Memcheck, a memory error detector
==4508== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==4508== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==4508== Command: tiff2bw /tmp/poc /dev/null
==4508== 
--4508-- Valgrind options:
--4508---v
--4508-- Contents of /proc/version:
--4508--   Linux version 4.17.0-0.bpo.3-amd64 (debian-ker...@lists.debian.org) 
(gcc version 6.3.0 20170516 (Debian 6.3.0-18+deb9u1)) #1 SMP Debian 
4.17.17-1~bpo9+1 (2018-08-27)
--4508-- 
--4508-- Arch and hwcaps: X86, LittleEndian, x86-mmxext-sse1-sse2-sse3-lzcnt
--4508-- Page sizes: currently 4096, max supported 4096
--4508-- Valgrind library directory: /usr/lib/valgrind
--4508-- Reading syms from /usr/bin/tiff2bw
--4508--   Considering 
/usr/lib/debug/.build-id/fa/2a69bd4b42864c37053e0e1fed86b96a26efe1.debug ..
--4508--   .. build-id is valid
--4508-- Reading syms from /lib/i386-linux-gnu/ld-2.24.so
--4508--   Considering 
/usr/lib/debug/.build-id/f1/ae5f623e170729016ce9b68a6c98c5aa2f5cd3.debug ..
--4508--   .. build-id is valid
--4508-- Reading syms from /usr/lib/valgrind/memcheck-x86-linux
--4508--   Considering /usr/lib/valgrind/memcheck-x86-linux ..
--4508--   .. CRC mismatch (computed 903ab9f6 wanted c13efb00)
--4508--   Considering /usr/lib/debug/usr/lib/valgrind/memcheck-x86-linux ..
--4508--   .. CRC is valid
--4508--object doesn't have a dynamic symbol table
--4508-- Scheduler: using generic scheduler lock implementation.
--4508-- Reading suppressions file: /usr/lib/valgrind/default.supp
==4508== embedded gdbserver: reading from 
/tmp/vgdb-pipe-from-vgdb-to-4508-by-root-on-???
==4508== embedded gdbserver: writing to   
/tmp/vgdb-pipe-to-vgdb-from-4508-by-root-on-???
==4508== embedded gdbserver: shared mem   
/tmp/vgdb-pipe-shared-mem-vgdb-4508-by-root-on-???
==4508== 
==4508== TO CONTROL THIS PROCESS USING vgdb (which you probably
==4508== don't want to do, unless you know exactly what you're doing,
==4508== or are doing some strange experiment):
==4508==   /usr/lib/valgrind/../../bin/vgdb --pid=4508 ...command...
==4508== 
==4508== TO DEBUG THIS PROCESS USING GDB: start GDB like this
==4508==   /path/to/gdb tiff2bw
==4508== and then give GDB the following command
==4508==   target remote | /usr/lib/valgrind/../../bin/vgdb --pid=4508
==4508== --pid is optional if only one valgrind process is running
==4508== 
--4508-- REDIR: 0x4019770 (ld-linux.so.2:strlen) redirected to 0x3804a602 
(vgPlain_x86_linux_REDIR_FOR_strlen)
--4508-- REDIR: 0x4019480 (ld-linux.so.2:index) redirected to 0x3804a5dd 
(vgPlain_x86_linux_REDIR_FOR_index)
--4508-- Reading syms from /usr/lib/valgrind/vgpreload_core-x86-linux.so
--4508--   Considering /usr/lib/valgrind/vgpreload_core-x86-linux.so ..
--4508--   .. CRC mismatch (computed a861379f wanted ba578d67)
--4508--   Considering 
/usr/lib/debug/usr/lib/valgrind/vgpreload_core-x86-linux.so ..
--4508--   .. CRC is valid
--4508-- Reading syms from /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so
--4508--   Considering /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so ..
--4508--   .. CRC mismatch (computed ca0375fa wanted fcf2e5ee)
--4508--   Considering 
/usr/lib/debug/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so ..
--4508--   .. CRC is valid
==4508== WARNING: new redirection conflicts with existing -- ignoring it
--4508-- old: 0x04019770 (strlen  ) R-> (.0) 0x3804a602 
vgPlain_x86_linux_REDIR_FOR_strlen
--4508-- new: 0x04019770 (strlen  ) R-> (2007.0) 0x04831430 
strlen
--4508-- Reading syms from /usr/lib/i386-linux-gnu/libtiff.so.5.3.0
--4508--   Considering 
/usr/lib/debug/.build-id/d6/a2dd9aaee2e7257d491a88f3ff5e17035b3772.debug ..
--4508--   .. build-id is valid

Re: tiff / CVE-2018-18661

2018-11-14 Thread Brian May
Brian May  writes:

> I can reproduce CVE-2018-19210. Both on wheezy and stretch. Doesn't

Me getting distributions confused. I tested on Jessie, and I was able to
reproduce the problem. I did not test wheezy.
-- 
Brian May 



Re: tiff / CVE-2018-18661

2018-11-14 Thread Brian May
Brian May  writes:

> Ola Lundqvist  writes:
>
>> Could it be so that the problem is only reproducible on 32-bit
>> systems?
>
> Good point. Will try.

Nope. Can't reproduce i386 build on amd64 kernel. I would be rather
surprised if choice of kernel mattered.

I can reproduce CVE-2018-19210. Both on wheezy and stretch. Doesn't
appear to be any patch available yet. Note when testing this
vulnerabilty, the supplied command will modify the source file, meaning
running the same command one plus times will not crash after the first
time (unless you restore the input file).
-- 
Brian May 



Re: tiff / CVE-2018-18661

2018-11-14 Thread Brian May
Ola Lundqvist  writes:

> Could it be so that the problem is only reproducible on 32-bit
> systems?

Good point. Will try.
-- 
Brian May 



Re: tiff / CVE-2018-18661

2018-11-14 Thread Ola Lundqvist
Hi

Could it be so that the problem is only reproducible on 32-bit systems?

// Ola

On Tue, 13 Nov 2018 at 07:30, Brian May  wrote:

> Ola Lundqvist  writes:
>
> > Interesting. I wonder what the fix do differently in this case. It is a
> > little worrying that it exit with a zero return code, but maybe not
> major.
> > On the other hand, if we cannot reproduce the problem maybe it is not
> worth
> > patching... Hmm.
>
> I tried to reproduce this in a stretch chroot using version
> 4.0.9-1. This version should be vulerable, it is the version mentioned
> in the upstream bug report:
>
> http://bugzilla.maptools.org/show_bug.cgi?id=2819
>
> Still can't reproduce:
>
> (stretch-amd64-default)root@silverfish:/tmp/brian/tmpog1hq_fw/build/amd64#
> tiff2bw /tmp/poc /dev/null
> TIFFReadDirectory: Warning, Unknown field with tag 292 (0x124) encountered.
> LZWDecode: Not enough data at scanline 0 (short 6442004472 bytes).
> TIFFWriteDirectoryTagData: IO error writing tag data.
>
> From upstream bug report:
>
> $ ./tiff2bw poc /dev/null
> TIFFReadDirectory: Warning, Unknown field with tag 292 (0x124) encountered.
> Segmentation fault
>
> I might have missed something, however I can't see any sign of any
> Debian specific changes in 4.0.9-1 either.
> --
> Brian May 
>


-- 
 --- Inguza Technology AB --- MSc in Information Technology 
/  o...@inguza.comFolkebogatan 26\
|  o...@debian.org   654 68 KARLSTAD|
|  http://inguza.com/Mobile: +46 (0)70-332 1551 |
\  gpg/f.p.: 7090 A92B 18FE 7994 0C36 4FE4 18A1 B1CF 0FE5 3DD9  /
 ---


Re: tiff / CVE-2018-18661

2018-11-12 Thread Brian May
Ola Lundqvist  writes:

> Interesting. I wonder what the fix do differently in this case. It is a
> little worrying that it exit with a zero return code, but maybe not major.
> On the other hand, if we cannot reproduce the problem maybe it is not worth
> patching... Hmm.

I tried to reproduce this in a stretch chroot using version
4.0.9-1. This version should be vulerable, it is the version mentioned
in the upstream bug report:

http://bugzilla.maptools.org/show_bug.cgi?id=2819

Still can't reproduce:

(stretch-amd64-default)root@silverfish:/tmp/brian/tmpog1hq_fw/build/amd64# 
tiff2bw /tmp/poc /dev/null
TIFFReadDirectory: Warning, Unknown field with tag 292 (0x124) encountered.
LZWDecode: Not enough data at scanline 0 (short 6442004472 bytes).
TIFFWriteDirectoryTagData: IO error writing tag data.

>From upstream bug report:

$ ./tiff2bw poc /dev/null
TIFFReadDirectory: Warning, Unknown field with tag 292 (0x124) encountered.
Segmentation fault

I might have missed something, however I can't see any sign of any
Debian specific changes in 4.0.9-1 either.
-- 
Brian May 



Re: tiff / CVE-2018-18661

2018-11-12 Thread Ola Lundqvist
Hi Brian

Interesting. I wonder what the fix do differently in this case. It is a
little worrying that it exit with a zero return code, but maybe not major.
On the other hand, if we cannot reproduce the problem maybe it is not worth
patching... Hmm.

// Ola

On Mon, 12 Nov 2018 at 07:24, Brian May  wrote:

> Ola Lundqvist  writes:
>
> > Hi Brian
> >
> > To me it looks like you have been able to reproduce the problem. You
> > clearly get different results with and without the patch indicating
> > that you have in fact triggered the problem. I do not see that you
> > have run the program using a debugger, so are you sure that you did
> > not end up in a crash?
>
> Looks like it it exiting normally to me:
>
> (jessie-amd64-default)root@silverfish:/tmp/brian/tmph6ow42nt/build/amd64#
> gdb tiff2bw
> GNU gdb (Debian 7.7.1+dfsg-5) 7.7.1
> Copyright (C) 2014 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <
> http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "x86_64-linux-gnu".
> Type "show configuration" for configuration details.
> For bug reporting instructions, please see:
> .
> Find the GDB manual and other documentation resources online at:
> .
> For help, type "help".
> Type "apropos word" to search for commands related to "word"...
> Reading symbols from tiff2bw...(no debugging symbols found)...done.
> (gdb) set args /tmp/poc /dev/null
> (gdb) r
> Starting program: /usr/bin/tiff2bw /tmp/poc /dev/null
> TIFFReadDirectory: Warning, Unknown field with tag 292 (0x124) encountered.
> LZWDecode: Not enough data at scanline 0 (short 6442004472 bytes).
> TIFFWriteDirectoryTagData: IO error writing tag data.
> [Inferior 1 (process 31103) exited normally]
> (gdb)
> --
> Brian May 
>


-- 
 --- Inguza Technology AB --- MSc in Information Technology 
/  o...@inguza.comFolkebogatan 26\
|  o...@debian.org   654 68 KARLSTAD|
|  http://inguza.com/Mobile: +46 (0)70-332 1551 |
\  gpg/f.p.: 7090 A92B 18FE 7994 0C36 4FE4 18A1 B1CF 0FE5 3DD9  /
 ---


Re: tiff / CVE-2018-18661

2018-11-11 Thread Brian May
Ola Lundqvist  writes:

> Hi Brian
>
> To me it looks like you have been able to reproduce the problem. You
> clearly get different results with and without the patch indicating
> that you have in fact triggered the problem. I do not see that you
> have run the program using a debugger, so are you sure that you did
> not end up in a crash?

Looks like it it exiting normally to me:

(jessie-amd64-default)root@silverfish:/tmp/brian/tmph6ow42nt/build/amd64# gdb 
tiff2bw  
GNU gdb (Debian 7.7.1+dfsg-5) 7.7.1
Copyright (C) 2014 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:
.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from tiff2bw...(no debugging symbols found)...done.
(gdb) set args /tmp/poc /dev/null
(gdb) r
Starting program: /usr/bin/tiff2bw /tmp/poc /dev/null
TIFFReadDirectory: Warning, Unknown field with tag 292 (0x124) encountered.
LZWDecode: Not enough data at scanline 0 (short 6442004472 bytes).
TIFFWriteDirectoryTagData: IO error writing tag data.
[Inferior 1 (process 31103) exited normally]
(gdb) 
-- 
Brian May 



Re: tiff / CVE-2018-18661

2018-11-10 Thread Ola Lundqvist
Hi Brian

To me it looks like you have been able to reproduce the problem. You
clearly get different results with and without the patch indicating that
you have in fact triggered the problem. I do not see that you have run the
program using a debugger, so are you sure that you did not end up in a
crash?

// Ola

On Thu, 8 Nov 2018 at 07:33, Brian May  wrote:

> I applied the fix for this CVE. Patch attached.
>
>
> However, then I found out I can't reproduce the bug under Debian/Jessie,
> with or without the security update.
>
> Version 4.0.3-12.3+deb8u7 in Jessie+security:
>
> (jessie-i386-default)root@silverfish:/home/brian/tree/debian/lts/packages/tiff/tiff-4.0.3#
> tiff2bw /tmp/poc /dev/null
> TIFFReadDirectory: Warning, Unknown field with tag 292 (0x124) encountered.
> TIFFScanlineSize: Integer arithmetic overflow.
> TIFFReadDirectory: Cannot handle zero scanline size.
> (jessie-i386-default)root@silverfish:/home/brian/tree/debian/lts/packages/tiff/tiff-4.0.3#
> echo $?
> 255
>
>
> 4.0.3-12.3+deb8u8 with patch applied:
>
> (jessie-amd64-default)root@silverfish:/tmp/brian/tmpz5ka6n27/build/amd64#
> tiff2bw /tmp/poc /dev/null
> TIFFReadDirectory: Warning, Unknown field with tag 292 (0x124) encountered.
> LZWDecode: Not enough data at scanline 0 (short 6442004472 bytes).
> TIFFWriteDirectoryTagData: IO error writing tag data.
> (jessie-amd64-default)root@silverfish:/tmp/brian/tmpz5ka6n27/build/amd64#
> echo $?
> 0
>
>
> Diff attached. So I suspect this security issue may have already been
> fixed.
>
> However it looks like this patch might also fixed some out-of-memory
> conditions also. So maybe worth applying regardless.
>
> Kind of troubling that it returns a 0 exit code after the patch.
> --
> Brian May 
> https://linuxpenguins.xyz/brian/
>


-- 
 --- Inguza Technology AB --- MSc in Information Technology 
/  o...@inguza.comFolkebogatan 26\
|  o...@debian.org   654 68 KARLSTAD|
|  http://inguza.com/Mobile: +46 (0)70-332 1551 |
\  gpg/f.p.: 7090 A92B 18FE 7994 0C36 4FE4 18A1 B1CF 0FE5 3DD9  /
 ---


Re: tiff / CVE-2018-15209

2018-08-31 Thread Antoine Beaupré
On 2018-08-29 12:24:30, Brian May wrote:
> Antoine Beaupré  writes:
>
>> Brian, are you sure you're getting those failures in jessie? Which
>> architecture? Here my tests were done in a VirtualBox VM using an up to
>> date Debian jessie amd64 box.
>
> My tests were done in a schroot. Not sure if I used i386 or amd64 now.

Well, as I can't reproduce any issue at all with this, I've left it as
N/A for jessie.

A.
-- 
Secrecy is the keystone to all tyranny. Not force, but secrecy and
censorship.
   -  Robert A. Heinlein



Re: tiff / CVE-2018-15209

2018-08-28 Thread Brian May
Antoine Beaupré  writes:

> Brian, are you sure you're getting those failures in jessie? Which
> architecture? Here my tests were done in a VirtualBox VM using an up to
> date Debian jessie amd64 box.

My tests were done in a schroot. Not sure if I used i386 or amd64 now.
-- 
Brian May 



Re: tiff / CVE-2018-15209

2018-08-27 Thread Antoine Beaupré
On 2018-08-14 17:27:29, Brian May wrote:
> I have been trying to reproduce this bug (buffer overflow), but instead
> I get increasing memory usage until my computer crashes. With versions
> from Jessie, Stretch, and Sid. So maybe another security issue?
>
> I note that CVE-2017-11613 and CVE-2018-5784 can use unbounded
> memory. However these are marked as fixed everywhere but Stretch.
>
> As far as I can tell, the relevant code is:
>
> uint64* newcounts;
>
> ...
>
> newcounts = (uint64*) _TIFFCheckMalloc(tif, nstrips, sizeof (uint64),
> "for chopped \"StripByteCounts\" array");
>
> ...
>
> for (strip = 0; strip < nstrips; strip++) {
> ...
> newcounts[strip] = stripbytes;
> ...
> }
>
> However, I cannot see how this could cause a buffer overflow
> condition. We appear to allocate nstrips uint64, and then use nstrips
> uint64.

I can't reproduce this either in a jessie VM:

[...]
ii  libtiff-tools4.0.3-12.3+deb8u6   amd64  
 TIFF manipulation and conversion tools
ii  libtiff5:amd64   4.0.3-12.3+deb8u6   amd64  
 Tag Image File Format (TIFF) library
vagrant@jessie:~$ valgrind tiff2pdf poc1
==17408== Memcheck, a memory error detector
==17408== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==17408== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==17408== Command: tiff2pdf poc1
==17408== 
TIFFReadDirectoryCheckOrder: Warning, Invalid TIFF directory; tags are not 
sorted in ascending order.
TIFFReadDirectory: Warning, Unknown field with tag 65046 (0xfe16) encountered.
TIFFFetchNormalTag: Warning, IO error during reading of "Software"; tag ignored.
TIFFReadDirectory: Warning, Sum of Photometric type-related color channels and 
ExtraSamples doesn't match SamplesPerPixel. Defining non-color channels as 
ExtraSamples..
TIFFReadDirectory: Warning, Bogus "StripByteCounts" field, ignoring and 
calculating from imagelength.
II*TIFFReadDirectoryCheckOrder: Warning, Invalid TIFF directory; tags are not 
sorted in ascending order.
TIFFReadDirectory: Warning, Unknown field with tag 65046 (0xfe16) encountered.
TIFFFetchNormalTag: Warning, IO error during reading of "Software"; tag ignored.
TIFFReadDirectory: Warning, Sum of Photometric type-related color channels and 
ExtraSamples doesn't match SamplesPerPixel. Defining non-color channels as 
ExtraSamples..
TIFFReadDirectory: Warning, Bogus "StripByteCounts" field, ignoring and 
calculating from imagelength.
TIFFReadDirectoryCheckOrder: Warning, Invalid TIFF directory; tags are not 
sorted in ascending order.
TIFFReadDirectory: Warning, Unknown field with tag 65046 (0xfe16) encountered.
TIFFFetchNormalTag: Warning, IO error during reading of "Software"; tag ignored.
TIFFReadDirectory: Warning, Sum of Photometric type-related color channels and 
ExtraSamples doesn't match SamplesPerPixel. Defining non-color channels as 
ExtraSamples..
TIFFReadDirectory: Warning, Bogus "StripByteCounts" field, ignoring and 
calculating from imagelength.
%PDF-1.1 
%âãÏÓ
1 0 obj
<< 
/Type /Catalog 
/Pages 3 0 R 
>>
endobj
2 0 obj
<< 
/CreationDate (D:20180827145928)
/ModDate (D:20180827145928)
/Producer (libtiff / tiff2pdf - 20120922)
/Title (miniswhite-1c-1b.tiff)
>> 
endobj
3 0 obj
<< 
/Type /Pages 
/Kids [ 4 0 R ] 
/Count 1 
>> 
endobj
TIFFReadDirectoryCheckOrder: Warning, Invalid TIFF directory; tags are not 
sorted in ascending order.
TIFFReadDirectory: Warning, Unknown field with tag 65046 (0xfe16) encountered.
TIFFFetchNormalTag: Warning, IO error during reading of "Software"; tag ignored.
TIFFReadDirectory: Warning, Sum of Photometric type-related color channels and 
ExtraSamples doesn't match SamplesPerPixel. Defining non-color channels as 
ExtraSamples..
TIFFReadDirectory: Warning, Bogus "StripByteCounts" field, ignoring and 
calculating from imagelength.
tiff2pdf: No support for poc1 with 254 samples per pixel.
tiff2pdf: An error occurred creating output PDF file.
==17408== 
==17408== HEAP SUMMARY:
==17408== in use at exit: 0 bytes in 0 blocks
==17408==   total heap usage: 106 allocs, 106 frees, 35,811 bytes allocated
==17408== 
==17408== All heap blocks were freed -- no leaks are possible
==17408== 
==17408== For counts of detected and suppressed errors, rerun with: -v
==17408== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Now, this could be because it's valgrind and not ASAN. But still - if
compiling with ASAN triggers the bug, I fail to see how this affects us:
our package is *not* compiled with it, and, as such, doesn't
misbehave. But just for the fun of it, I *did* try to recompile the
package with ASAN, as per:

https://wiki.debian.org/LTS/Development/Asan

And I still can't trigger the bug.

For what it's worth, the original bug report mentions Ubuntu 16.04 and
v4.0.9, compiled with clang:


Re: tiff: CVE-2018-8905: heap-based buffer overflow in LZWDecodeCompat

2018-05-02 Thread Hugo Lefeuvre
> So, what is the solution ?
> 
> First, change
> 
> TIFFReadScanline(in, buf, row, s) < 0
> 
> in tools/tiffcp.c to
> 
> TIFFReadScanline(in, buf, row, s) <= 0
> 
> It is important to understand that 0 is also an error code here. Otherwise,
> change error handling in tif_lzw to return negative numbers.

After taking a fresh look at it, this turns out to be a non solution
because TIFFReadScanline (the function between LZWDecodeCompat and
DECLAREcpFunc) converts 0 return codes to -1s.
 
> Then, there's maybe something to fix in tif_lzw, but I'm not quite sure
> at the moment and I still have to check that.

So, obviously the solution is in tif_lzw. Because of the ignore option,
we must understand the process of exiting not only as returning, but
most importantly as preparing the decoder for being called again with
the same corrupted memory state and the same buggy codes.

If we don't do that the next decoder call will stumble across the
corrupted memory and crash.

There are two things we have to handle: First, make sure the next
decoder call detects that it was previously interrupted and that the
oldcodep is invalid, so we avoid the crash.

Then, we have to be able to skip the buggy error code, because otherwise
we are going to run into an endless loop and we do not want that either.

I suggest to fix both issues via an additional flag in the decoder base
state. This flag would indicate the position where an invalid code can
be found.

Whenever this flag is set, we would fast forward to the next CODE_CLEAR
after the invalid code (according to my understanding of LZW, the whole
set of information between the corrupted code and the next CODE_CLEAR
may be corrupted or risky / hard to recover).

Concerning the Wheezy version, I have inspected the diffs and it is
still pretty unclear to me why the issue isn't reproducible because the
affected code it present. I'm still working on it.

Cheers,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature


Re: tiff: CVE-2018-8905: heap-based buffer overflow in LZWDecodeCompat

2018-04-30 Thread Hugo Lefeuvre
Hi,

> It looks like this buffer overflow is the consequence of an earlier buffer
> overflow in the GetNextCodeCompat macro:

Hum, that was not completely true. But I think I got it now.

The buggy sequence is:

1) Initialy oldcodep points to 0x63201890.
   We get code 0x010c = 268, add it to the code table, print some stuff
   and read the next code.

2) The next code is 0x100, which is CLEAR_CODE, so we set free_entp to
   0x63201820 (= sp->dec_codetab + CODE_FIRST).

   Then, we memset free_entp with a size of
 CSIZE - CODE_FIRST
   = (MAXCODE(BITS_MAX)+1024L)-258
   = (MAXCODE(9)+1024L)-258
   = ((1L<<(9))-1)+1024L-258 = 1277

   This means that everything between 0x63201820 and 0x63201D1D
   will be set to 0. Then we read the next code.

3) Next code is CLEAR_CODE, same as before, nothing changes.

4) Next code is 0x01d6. This is bad because it is too big for a first
   code. At this point we exit 0 without updating the decoder state.

5) Because we exit 0, TIFFReadScanline(in, buf, row, s) < 0 in
   tools/tiffcp.c is false and the whole process is repeated !

6) So, again, oldcodep initially points to 0x63201890. BUT,
   remember, this area was set to 0 by 2) !

   So, well, we read the next code which is 0x010c and add it to the code
   table. This means that for our 0x010c code @codep, codep->next will
   point to the memset-ed area ! This means that codep->next->next == NULL.

7) Overflow, NULL pointer dereference, etc.

So, what is the solution ?

First, change

TIFFReadScanline(in, buf, row, s) < 0

in tools/tiffcp.c to

TIFFReadScanline(in, buf, row, s) <= 0

It is important to understand that 0 is also an error code here. Otherwise,
change error handling in tif_lzw to return negative numbers.

Then, there's maybe something to fix in tif_lzw, but I'm not quite sure
at the moment and I still have to check that.

Now that the issue is clear to me, I'll check the Wheezy code and try to
see why the issue isn't reproducible there.

Cheers,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature


Re: tiff: CVE-2018-8905: heap-based buffer overflow in LZWDecodeCompat

2018-04-22 Thread Hugo Lefeuvre
It looks like this buffer overflow is the consequence of an earlier buffer
overflow in the GetNextCodeCompat macro:

> #define GetNextCodeCompat(sp, bp, code) {   \
>  nextdata |= (unsigned long) *(bp)++ << nextbits;\
>  nextbits += 8;  \
>  if (nextbits < nbits) { \
>  nextdata |= (unsigned long) *(bp)++ << nextbits;\
>  nextbits += 8;  \
>  }   \
>  code = (hcode_t)(nextdata & nbitsmask); \
>  nextdata >>= nbits; \
>  nextbits -= nbits;  \
> }

The raw data buffer is read using the bp pointer without proper bound checking.
At some point, we start to read garbage, store it into the code variable which
is later used to create the codep. This invalid codep later triggers the second
overflow.

So now the question is: Why is this first buffer overflow happening ?

My guess is that the sample is declaring more strips than actually available, or
declares strips with incorrect size. I still have to check that however.

Regards,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature


Re: tiff: CVE-2018-8905: heap-based buffer overflow in LZWDecodeCompat

2018-04-19 Thread Hugo Lefeuvre
Hi,

My current understanding of the problem (based on investigations on
latest master, but also valid for older versions):

The code_t string type is defined as a kind of chained list. Each entry
contains:

. a pointer to the next string entry
. a length field indicating the remaining length of the string

This length field includes the current entry.

. a value field containing the current string character
. the first character of the string

In the affected source code

> do {
> *--tp = codep->value;
> } while ( (codep = codep->next) != NULL );

we read such a string and put the values into a buffer (in reverse order, but,
well, it doesn't matter).

Here we assume that the string always has the size it claims to have in its
length field.

But it's not the case with the reproducer. There is a list that claims to have
only one character but defines two, so we continue to read and write to the
buffer even if we are already OOB.

So now the question is: why is that happening ? Is it directly user input or is
coming from an earlier bug in the source code ?

Even if I can't reproduce the issue with the original reproducer, the
versions in Wheezy, Jessie and Stretch are likely to be affected because
the affected code is present.

I'm going to continue my investigations, try to prepare a poc for older
versions, prepare a patch for latest master and backport it to Wheezy
(+ Jessie & Stretch if needed).

Regards,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature


Re: tiff: CVE-2018-8905: heap-based buffer overflow in LZWDecodeCompat

2018-04-15 Thread Hugo Lefeuvre
Hi,

I have successfully reproduced this issue in latest upstream master
branch and Buster but couldn't reproduce it neither in Wheezy nor in
Jessie or Stretch.

I am going to take a closer look, try to prepare a patch and declare
Wheezy, Jessie and Stretch unaffected if appropriate.

Regards,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature


Re: tiff updates

2018-04-11 Thread Antoine Beaupré
On 2018-03-22 09:19:31, Hugo Lefeuvre wrote:
> * Start working on tiff and tiff3:
>
>   - Investigate, debug/prepare and test patch for CVE-2018-7456 (git master
> version). This issue was very long to debug because it required me
> to have a good understanding of the TIFF standard which I had to
> read carefully, and also of the TIFF codebase, which I had to study
> extensively. I have submitted my patch to upstream, not sure it's
> ready yet but I feel like I understand the problem well enough
> to finish it and backport the patch to Wheezy and Jessie.
>
> You can find most of my work on the debian-lts mailing list and
> upstream bug report.
>
> During my investigations I bumped across another, older issue which
> I still have to investigate in master (it never got a CVE assigned and
> I'm not even sure that upstream heard about it, it got probably
> fixed 'by chance').

Hello Hugo!

I see you have the `tiff` package claimed in dla-needed.txt, but not
`tiff3`. I suspect both are fairly similar issues and that you intended
to work on both, so I figured it might be better to claim both next
time.

But I haven't seen new activity on the packages since then, do you need
a review of the patches you submitted in March? Or for someone to carry
this work forward?

Thanks!

-- 
They say that time changes things, but you actually have to change
them yourself.   - Andy Warhol



Re: tiff / CVE-2018-7456

2018-03-20 Thread Hugo Lefeuvre
Hi,

> Thanks for the feedback ! I'll write the remaining part and submit it to
> upstream.

You can find a first draft in attachment.

While it's already a working solution (at least, I think), there's
still place for improvements.

* I'm not sure that the check it well positioned in the source code
* I still fear some redundancy with some other similar (but irrelevant in our
  case) code at libtiff/tif_getimage.c:326

Also, I've double checked and RGB is the only case to handle. Palette
should have SamplePerPixel = 1, AFAIK BlackIsZero/WhiteIsZero should not
specify this field and YCbCr should always have SamplePerPixel = 3.

Feedback welcome.

Cheers,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
diff --git a/libtiff/tif_dirread.c b/libtiff/tif_dirread.c
index 6baa7b31..d9295800 100644
--- a/libtiff/tif_dirread.c
+++ b/libtiff/tif_dirread.c
@@ -4024,6 +4024,26 @@ TIFFReadDirectory(TIFF* tif)
 			}
 		}
 	}
+
+	/*
+	 * Make sure all non-color channels are extrasamples.
+ * If it's not the case, define them as such.
+	 */
+if (tif->tif_dir.td_photometric == PHOTOMETRIC_RGB &&
+tif->tif_dir.td_samplesperpixel - tif->tif_dir.td_extrasamples > 3) {
+		TIFFWarningExt(tif->tif_clientdata,module, "More than 3 color channels in "
+"RGB image, defining non-standard channels as extrasamples");
+
+int old_extrasamples = tif->tif_dir.td_extrasamples;
+tif->tif_dir.td_extrasamples += ((tif->tif_dir.td_samplesperpixel - tif->tif_dir.td_extrasamples) - 3);
+
+// sampleinfo should contain information relative to these new extra samples
+uint16 new_sampleinfo[tif->tif_dir.td_extrasamples];
+memset(new_sampleinfo, 0, tif->tif_dir.td_extrasamples);
+memcpy(new_sampleinfo, tif->tif_dir.td_sampleinfo, old_extrasamples * sizeof(uint16));
+_TIFFsetShortArray(>tif_dir.td_sampleinfo, new_sampleinfo, tif->tif_dir.td_extrasamples);
+}
+
 	/*
 	 * Verify Palette image has a Colormap.
 	 */
diff --git a/libtiff/tif_print.c b/libtiff/tif_print.c
index 8deceb2b..5f9a676e 100644
--- a/libtiff/tif_print.c
+++ b/libtiff/tif_print.c
@@ -544,7 +544,7 @@ TIFFPrintDirectory(TIFF* tif, FILE* fd, long flags)
 uint16 i;
 fprintf(fd, "%2ld: %5u",
 l, td->td_transferfunction[0][l]);
-for (i = 1; i < td->td_samplesperpixel; i++)
+for (i = 1; i < td->td_samplesperpixel - td->td_extrasamples; i++)
 	fprintf(fd, " %5u",
 	td->td_transferfunction[i][l]);
 fputc('\n', fd);


signature.asc
Description: PGP signature


Re: tiff / CVE-2018-7456

2018-03-18 Thread Hugo Lefeuvre
Hi Brian,

> > So, in fact it may very well be that the size of the TransferFunction table
> > is always at most 3 rows and this definition is right.
> 
> Is the code that loads the transfer function safe? Is there any
> possibility of tricking the loading function to try and set the 4th row?

Hum, actually if I understand the specification well, the transfer
function doesn't care about extra samples and supported
PhotometricInterpretations have at most (standard spp) = 3. So, the 4th
(or in general n-th row with n > 3) row is just nonsense and should be
considered as the result of unchecked SamplesPerPixel / ExtraSamples
values.

(only supposing, handle with care :))

Cheers,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature


Re: tiff / CVE-2018-7456

2018-03-18 Thread Hugo Lefeuvre
> Seems good to me. I would suggest sending a patch upstream, see what
> they think.

Thanks for the feedback ! I'll write the remaining part and submit it to
upstream.
 
> Also I tend to think some some of assertion might be a good idea,
> something that aborts if
> 
> (td->td_samplesperpixel - td->td_extrasamples) > 3
> 
> As aborting early is probably better then screwing up memory.
> 
> For reference, the next value in the struct is:
> 
> typedef struct {
> [...]
> 
> /* Colorimetry parameters */
>   uint16* td_transferfunction[3];
>   float*  td_refblackwhite;
> 
> [...]
> } TIFFDirectory;
> 
> So I am guessing when you access td_transferfunction[3], you are
> actually accessing td_refblackwhite, which - surprise surprise - happens
> to be NULL.

While we could certainly do that, I'm not 100% convinced it would be a
good idea, mostly because it would involve a magic number (3), which
we should IMO try to avoid.

Also, if td is spec-compliant, then

td->td_samplesperpixel - td->td_extrasamples

should never be greater than 3 (unless the spec evolves, which is fine
anyways as long as the size of td_transferfunction is re-defined)
because otherwise it would break the assumption

td->td_extrasamples = td->td_samplesperpixel - (standard spp)

which we expect to be guaranteed by the callee.

But, right, something like

/* Should be guaranted by callee */
(td->td_samplesperpixel - td->td_extrasamples) > TD_TRANSFER_FUNCTION_SIZE

and also

typedef struct {
   [...]

   /* Colorimetry parameters */
   uint16* td_transferfunction[TD_TRANSFER_FUNCTION_SIZE];
   float*  td_refblackwhite;

   [...]
} TIFFDirectory;

could clearly be a good idea, at least because it would make the
patch/code clearer on what we are trying to do and what assumption we
make.

Thanks !

Cheers,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature


Re: tiff / CVE-2018-7456

2018-03-17 Thread Brian May
Hugo Lefeuvre  writes:

> So, after some debugging on CVE-2018-7456, I start to get the feeling to
> understand the issue.
>
> Here are my conclusions for the moment:
>
> * In any case, the transfer function should not care about other
>   channels defined by ExtraSamples (see 2nd and 3rd paragraphs of
>   TransferFunction entry, page 84 of the specification), so something
>   like the following patch should be a first step:
>
> --- a/libtiff/tif_print.c 2018-03-17 21:56:47.0 +0100
> +++ b/libtiff/tif_print.c 2018-03-17 22:05:58.446092016 +0100
> @@ -546,7 +546,7 @@
>   uint16 i;
>   fprintf(fd, "%2ld: %5u",
>   l, td->td_transferfunction[0][l]);
> - for (i = 1; i < td->td_samplesperpixel; i++)
> + for (i = 1; i < td->td_samplesperpixel - 
> td->td_extrasamples; i++)
>   fprintf(fd, " %5u",
>   td->td_transferfunction[i][l]);
>   fputc('\n', fd);
>
> But it's not enough. Why ?
>
> * I discovered that td->td_samplesperpixel can have arbitrary size while
>   td->td_extrasamples stays 0. It shouldn't be the case ! For instance
>   the specification doesn't allow RGB with 4 channels and no ExtraSamples.
>   And while it doesn't make any sense, libtiff won't notice it.
>
>   I even tried RGB with 8 channels and no ExtraSamples and it worked too.
>
>   So, what should be done ?
>
>   For each PhotometricInterpretation type there is a 'standard' samples
>   per pixel value (that is the samples per pixel value without extra samples:
>   3 for RGB, etc). Let's name it (standard spp).
>
>   So, what we should do is: If the actual td->td_samplesperpixel is higher
>   than (standard spp), then we should assume that td->td_extrasamples is
>   td->td_samplesperpixel - (standard spp), even if no ExtraSamples field
>   was specified OR if the specified td->td_extrasamples was smaller.

Seems good to me. I would suggest sending a patch upstream, see what
they think.

Also I tend to think some some of assertion might be a good idea,
something that aborts if

(td->td_samplesperpixel - td->td_extrasamples) > 3

As aborting early is probably better then screwing up memory.


For reference, the next value in the struct is:

typedef struct {
[...]

/* Colorimetry parameters */
uint16* td_transferfunction[3];
float*  td_refblackwhite;

[...]
} TIFFDirectory;

So I am guessing when you access td_transferfunction[3], you are
actually accessing td_refblackwhite, which - surprise surprise - happens
to be NULL.
-- 
Brian May 



Re: tiff / CVE-2018-7456

2018-03-17 Thread Brian May
Hugo Lefeuvre  writes:

> Sure ? To me it looked like three entries are provided, but the fourth
> (td->td_transferfunction[3]) isn't.

I was about to write justification for how I was absolutely sure of
this.

Then I realized the loop is:

for (i = 1; i < td->td_samplesperpixel; i++)

i.e. it starts at index 1, not index 0 as I had expected. Oops. So the
third iteration is actually referencing the 4th item.

Out by one error. Sorry.
-- 
Brian May 



Re: tiff / CVE-2018-7456

2018-03-17 Thread Brian May
Hugo Lefeuvre  writes:

> So, in fact it may very well be that the size of the TransferFunction table
> is always at most 3 rows and this definition is right.

Is the code that loads the transfer function safe? Is there any
possibility of tricking the loading function to try and set the 4th row?
-- 
Brian May 



Re: tiff / CVE-2018-7456

2018-03-17 Thread Hugo Lefeuvre
So, after some debugging on CVE-2018-7456, I start to get the feeling to
understand the issue.

Here are my conclusions for the moment:

* In any case, the transfer function should not care about other
  channels defined by ExtraSamples (see 2nd and 3rd paragraphs of
  TransferFunction entry, page 84 of the specification), so something
  like the following patch should be a first step:

--- a/libtiff/tif_print.c   2018-03-17 21:56:47.0 +0100
+++ b/libtiff/tif_print.c   2018-03-17 22:05:58.446092016 +0100
@@ -546,7 +546,7 @@
uint16 i;
fprintf(fd, "%2ld: %5u",
l, td->td_transferfunction[0][l]);
-   for (i = 1; i < td->td_samplesperpixel; i++)
+   for (i = 1; i < td->td_samplesperpixel - 
td->td_extrasamples; i++)
fprintf(fd, " %5u",
td->td_transferfunction[i][l]);
fputc('\n', fd);

But it's not enough. Why ?

* I discovered that td->td_samplesperpixel can have arbitrary size while
  td->td_extrasamples stays 0. It shouldn't be the case ! For instance
  the specification doesn't allow RGB with 4 channels and no ExtraSamples.
  And while it doesn't make any sense, libtiff won't notice it.

  I even tried RGB with 8 channels and no ExtraSamples and it worked too.

  So, what should be done ?

  For each PhotometricInterpretation type there is a 'standard' samples
  per pixel value (that is the samples per pixel value without extra samples:
  3 for RGB, etc). Let's name it (standard spp).

  So, what we should do is: If the actual td->td_samplesperpixel is higher
  than (standard spp), then we should assume that td->td_extrasamples is
  td->td_samplesperpixel - (standard spp), even if no ExtraSamples field
  was specified OR if the specified td->td_extrasamples was smaller.

  Obviously we should also take care of filling appropriate
  td->td_sampleinfo entries.

  For example, if an RGB image has td->td_samplesperpixel = 4, then
  td->td_extrasamples should be set to 1 (with arbitrary
  td->td_sampleinfo entry for example 0 - Unspecified data).

Does it work now ?

I think so ! I didn't write the second part of the patch and will wait
for feedback. But you can convince yourself that it doesn't crash anymore
by modifying the sample to add an ExtraSamples = 1 field (using
"tiffset -s 338 1 0 $(sample)" for example).

Link to the specification:
https://www.itu.int/itudoc/itu-t/com16/tiff-fx/docs/tiff6.pdf

Regards,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature


Re: tiff / CVE-2018-7456

2018-03-16 Thread Hugo Lefeuvre
Hi Brian,

> > Under certain conditions, the td->td_transferfunction table might not
> > have the excepted size, that is it may not have the excepted number of
> > samples per pixel (td->td_samplesperpixel). In this case for example,
> > the table is only 3 rows large while td->td_samplesperpixel is 4. Then,
> > the program segfaults when it comes to td->td_transferfunction[3][0].
> 
> The faulty test case is where the table is suppose to have three
> entries, but only two entries are provided.

Sure ? To me it looked like three entries are provided, but the fourth
(td->td_transferfunction[3]) isn't.

$ ASAN_OPTIONS=abort_on_error=1 gdb tiffinfo
[snip]
(gdb) r -c ../1-tiffinfo-c-null
Starting program: /usr/local/bin/tiffinfo -c ../1-tiffinfo-c-null
[Thread debugging using libthread_db enabled]
Using host libthread_db library
"/lib/x86_64-linux-gnu/libthread_db.so.1".
TIFFReadDirectoryCheckOrder: Warning, Invalid TIFF directory; tags are
not sorted in ascending order.
TIFFReadDirectory: Warning, Unknown field with tag 314 (0x13a)
encountered.
TIFFReadDirectory: Warning, Unknown field with tag 54034 (0xd312)
encountered.
TIFFFetchNormalTag: Warning, Incorrect count for "YResolution"; tag
ignored.
TIFF Directory at offset 0x767fe (485374)
  Image Width: 1024 Image Length: 768
  Resolution: 2, 0 (unitless)
  Bits/Sample: 8
  Compression Scheme: LZW
  Photometric Interpretation: RGB color
  Samples/Pixel: 4
  Planar Configuration: single image plane
  Transfer Function:
 
Program received signal SIGSEGV, Segmentation fault.
0x76bb1da1 in TIFFPrintDirectory (tif=0x61900080,
fd=0x75ed8600 <_IO_2_1_stdout_>, flags=6) at tif_print.c:551
551 td->td_transferfunction[i][l]);
(gdb) l
546 uint16 i;
547 fprintf(fd, "%2ld: %5u",
548 l, td->td_transferfunction[0][l]);
549 for (i = 1; i < td->td_samplesperpixel; i++)
550 fprintf(fd, " %5u",
551 td->td_transferfunction[i][l]);
552 fputc('\n', fd);
553 }
554 } else
555 fprintf(fd, "(present)\n");
(gdb) p td->td_transferfunction
$1 = {0x61500580, 0x61500800, 0x61500a80}

> The defintion of td_transferfunction is:
> 
> typedef struct {
> ...
> uint16* td_transferfunction[3];
> ...
> } TIFFDirectory;
>
> My assumption was that the list would be NULL terminated. In practise it
> is NULL terminated (might be accidental due to newly allocated memory
> being initialized to 0), but I should double check this.

I think we should really avoid relying on NULL termination for this
patch, because nothing guarantees us that this fourth element has been
initialized to 0 at any moment, right ?
 
> However, as this table is only 3 entries long (huh? Is that hardcoded
> value really safe here?), so it cannot be null terminated for the case
> where there are three tables.

Hum well, actually the specification states:

The TransferFunction can be applied to images with a
PhotometricInterpretation value of RGB, Palette, YCbCr, WhiteIsZero,
and BlackIsZero. The TransferFunction is not used with other
PhotometricInterpretation types.

TIFF 6.0 Specification, p84[0]

Palette => td->td_samplesperpixel = 1
YCbCr => td->td_samplesperpixel = 3
WhiteIsZero => td->td_samplesperpixel = 1
RGB => td->td_samplesperpixel = 3 (unless ExtraSamples specified, we
   will take a look at it later)

So, in fact it may very well be that the size of the TransferFunction table
is always at most 3 rows and this definition is right.

However, there is still the case of RGB where we may still have
td->td_samplesperpixel > 3 (this is our case, here td->td_samplesperpixel = 4
and td->td_photometric = 2 which is RGB).

There are two possibilities
(1) ExtraSamples = 1 => Associated alpha data (with pre-multiplied color)
(2) ExtraSamples = 2 => Unassociated alpha data

I didn't have time to read all information about these two cases, but at
a first glance I noticed something interesting.

The specification states:

Since the ExtraSamples field is independent of other fields, this
scheme permits alpha information to be stored in whatever organization
is appropriate. In particular, components can be stored packed
(PlanarConfiguration=1); [... snip] However, if this scheme is used,
TIFF readers must not derive the SamplesPerPixel from the value of the
PhotometricInterpretation field (e.g., if RGB, then SamplesPerPixel is
3).

TIFF 6.0 Specification, p78[0]

In our case, while components are stored packed (because td->td_planarconfig = 
1),
the td->td_samplesperpixel is still 4 and not 3, so the specification isn't
really respected, right ?

> However, it still could be null terminated for less then three entries.

Maybe, but if I don't misunderstand the problem, the bug is not affecting
us if td->td_samplesperpixel 

Re: tiff / CVE-2018-7456

2018-03-16 Thread Brian May
Hugo Lefeuvre  writes:

> Under certain conditions, the td->td_transferfunction table might not
> have the excepted size, that is it may not have the excepted number of
> samples per pixel (td->td_samplesperpixel). In this case for example,
> the table is only 3 rows large while td->td_samplesperpixel is 4. Then,
> the program segfaults when it comes to td->td_transferfunction[3][0].

The faulty test case is where the table is suppose to have three
entries, but only two entries are provided.

The defintion of td_transferfunction is:

typedef struct {
...
uint16* td_transferfunction[3];
...
} TIFFDirectory;

My assumption was that the list would be NULL terminated. In practise it
is NULL terminated (might be accidental due to newly allocated memory
being initialized to 0), but I should double check this.

However, as this table is only 3 entries long (huh? Is that hardcoded
value really safe here?), so it cannot be null terminated for the case
where there are three tables. However, it still could be null terminated
for less then three entries.

I am out of time for this month, and can't see where td_transferfunction
is read at quick glance. I will resume this next month, unless somebody
has taken over.
-- 
Brian May 



Re: tiff / CVE-2018-7456

2018-03-15 Thread Ben Hutchings
On Thu, 2018-03-15 at 16:55 +0100, Hugo Lefeuvre wrote:
[...]
> * My understanding of the problem:
> 
> Under certain conditions, the td->td_transferfunction table might not
> have the excepted size, that is it may not have the excepted number of
> samples per pixel (td->td_samplesperpixel). In this case for example,
> the table is only 3 rows large while td->td_samplesperpixel is 4. Then,
> the program segfaults when it comes to td->td_transferfunction[3][0].
> 
> * My understanding of your patch:
> 
> You are introducing a loop which verifies that td->td_transferfunction
> has the excepted number of samples per pixel by looking at the pointer
> to each row and making sure it is not NULL (which would mean that it is
> out of bounds). If it is NULL, you 'disable' the actual loop by setting
> n to 0.
> 
> * My understanding of why it might not work with -O1 or -O2, etc.:
> 
> You are assuming that td->td_transferfunction[i] is out of bounds if and
> only if it is NULL, which is AFAIK undefined behavior (I couldn't find
> any information about it, not 100% sure). While it might be true with
> -O0 / some compilers, it might not always be the case.

Absolutely, out-of-bounds access has undefined behaviour.  The compiler
will (in general) optimise on the assumption that the input program
never attempts to do anything that's specified as having undefined
behaviour, which can have extremely weird results if it does.

> If it's not the case, then the if body isn't executed and tiff might
> still crash.
> 
> Also, I'm not completely sure this is the best way to solve this problem.
> 
> In fact, while this kind of patch would help avoiding a crash, tiff would
> still be broken in some way because we wouldn't really have fixed the original
> problem (the fact that the td->td_transferfunction table has an unexpected
> size).
> 
> Wouldn't it be better to catch the issue earlier, for example when building
> the td->td_transferfunction table or when defining td->td_samplesperpixel,
> in order to make this inconsistent state impossible ?

Yes.

Ben.

> (Please correct me if I'm wrong !)
> 
> Hope this helps ! :)

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.



signature.asc
Description: This is a digitally signed message part


Re: tiff / CVE-2018-7456

2018-03-15 Thread Hugo Lefeuvre
Hi Brian,

> I attempted to fix CVE-2018-7456 issue in tiff, for the version in
> stretch. My patch is below. But curiously my patch only works if I
> enable the commented out call to fprintf or use -O0 instead of the
> default -O2 (-O1 also fails). Otherwise the if condition never gets
> executed, and it segfaults later on with a null pointer error when
> trying to access the same pointer.
> 
> To me, this seems like some sort of weird compiler optimization
> error. Does this make sense?

We already had this kind of nasty optimization-triggered bugs in the
past[0], it was quite long to fix but very interesting in the end. :)

Just to avoid duplicate work: I'll take a look at it this afternoon.

Cheers,
 Hugo

[0] https://lists.debian.org/debian-lts/2017/03/msg00213.html

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature


Re: tiff and CVE-2016-10095

2017-06-06 Thread Guido Günther
Hi Raphael,
On Tue, Jun 06, 2017 at 12:05:14PM +0200, Raphael Hertzog wrote:
> Hi,
> 
> On Fri, 02 Jun 2017, Guido Günther wrote:
> > > but it's not worth arguing and providing that in jessie might be useful 
> > > for
> > > building building custom tools still.
> > 
> > But then again the fix for this should be in Wheezy already as far as I
> > can tell. Raphael (since you provided the upstream patches for ths), can
> > you confirm?
> 
> I looked quickly at the upstream patch that got added. While it's based
> on some of my code, the approach retained by upstream is really different
> to what I did.
> 
> The real fix of most CVE for me was to add CODEC-specific tags to the
> global table so that they are known and treated correctly
> (0042-Make-more-tag-fields-known-to-TIFFReadDirectoryFindF.patch). The
> _TIFFCheckFieldIsValidForCodec() function that I added was used to filter
> out tags during write that were invalid in the context of the
> CODEC in use (this was done to fix a regression introduced by my former
> fix).
> 
> Now upstream reused my _TIFFCheckFieldIsValidForCodec() but he uses
> it during "read" of pictures and not during write and he did not add the
> CODEC-specific tags to the global list of known tags.
> 
> So while I believe that we are covered in terms of already report CVE,
> I also believe that it would be sane to replace our own fixes by
> upstream's fix and confirm that the already fixed CVE are still
> properly fixed.

Thanks for having a look. So the current status is fine, we treat wheezy
as affected but wait until more urgent issues pile up.
Cheers,
 -- Guido



Re: tiff and CVE-2016-10095

2017-06-06 Thread Raphael Hertzog
Hi,

On Fri, 02 Jun 2017, Guido Günther wrote:
> > but it's not worth arguing and providing that in jessie might be useful for
> > building building custom tools still.
> 
> But then again the fix for this should be in Wheezy already as far as I
> can tell. Raphael (since you provided the upstream patches for ths), can
> you confirm?

I looked quickly at the upstream patch that got added. While it's based
on some of my code, the approach retained by upstream is really different
to what I did.

The real fix of most CVE for me was to add CODEC-specific tags to the
global table so that they are known and treated correctly
(0042-Make-more-tag-fields-known-to-TIFFReadDirectoryFindF.patch). The
_TIFFCheckFieldIsValidForCodec() function that I added was used to filter
out tags during write that were invalid in the context of the
CODEC in use (this was done to fix a regression introduced by my former
fix).

Now upstream reused my _TIFFCheckFieldIsValidForCodec() but he uses
it during "read" of pictures and not during write and he did not add the
CODEC-specific tags to the global list of known tags.

So while I believe that we are covered in terms of already report CVE,
I also believe that it would be sane to replace our own fixes by
upstream's fix and confirm that the already fixed CVE are still
properly fixed.

Cheers,
-- 
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: https://www.freexian.com/services/debian-lts.html
Learn to master Debian: https://debian-handbook.info/get/



Re: tiff and CVE-2016-10095

2017-06-02 Thread Salvatore Bonaccorso
Hi Guido,

On Fri, Jun 02, 2017 at 12:29:29PM +0200, Guido Günther wrote:
> On Fri, Jun 02, 2017 at 11:02:06AM +0200, Moritz Muehlenhoff wrote:
> > On Fri, Jun 02, 2017 at 10:25:29AM +0200, Guido Günther wrote:
> > > Hi Moritz,
> > > I'm trying to figure out the reasoning for @51764. This marks tiff as
> > > affected by CVE-2016-10095. However from the upstream bug and the
> > > changes we made in wheezy it looks like the changes we made already are
> > > sufficient to fix the issue. Do you have a hint why you think this is
> > > not the case?
> > 
> > CVE-2016-10095 is the generic fix for the API. I'm not sure why that 
> > received 
> > a CVE ID, since it's not a vulnerability per se (which are in the call 
> > sites),
> > but it's not worth arguing and providing that in jessie might be useful for
> > building building custom tools still.
> 
> But then again the fix for this should be in Wheezy already as far as I
> can tell. Raphael (since you provided the upstream patches for ths), can
> you confirm?

The upload by Lazslo to unstable contains the following, which is upstream's
changelog enty:

* libtiff/tif_dirinfo.c, tif_dirread.c: add 
_TIFFCheckFieldIsValidForCodec(),
and use it in TIFFReadDirectory() so as to ignore fields whose tag is a
codec-specified tag but this codec is not enabled. This avoids 
TIFFGetField()
to behave differently depending on whether the codec is enabled or not, and
thus can avoid stack based buffer overflows in a number of TIFF utilities
such as tiffsplit, tiffcmp, thumbnail, etc.
Patch derived from 0063-Handle-properly-CODEC-specific-tags.patch
(http://bugzilla.maptools.org/show_bug.cgi?id=2580) by Raphaël Hertzog.
Fixes:
http://bugzilla.maptools.org/show_bug.cgi?id=2580
http://bugzilla.maptools.org/show_bug.cgi?id=2693
http://bugzilla.maptools.org/show_bug.cgi?id=2625 (CVE-2016-10095)
http://bugzilla.maptools.org/show_bug.cgi?id=2564 (CVE-2015-7554)
http://bugzilla.maptools.org/show_bug.cgi?id=2561 (CVE-2016-5318)
http://bugzilla.maptools.org/show_bug.cgi?id=2499 (CVE-2014-8128)
http://bugzilla.maptools.org/show_bug.cgi?id=2441
http://bugzilla.maptools.org/show_bug.cgi?id=2433

I have not cross-checked, to what is applied to wheezy and fully correspond to
Raphael's 0063-Handle-properly-CODEC-specific-tags.patch. If so then yes it
would be fixed already in wheezy.

Regards,
Salvatore



Re: tiff and CVE-2016-10095

2017-06-02 Thread Guido Günther
On Fri, Jun 02, 2017 at 11:02:06AM +0200, Moritz Muehlenhoff wrote:
> On Fri, Jun 02, 2017 at 10:25:29AM +0200, Guido Günther wrote:
> > Hi Moritz,
> > I'm trying to figure out the reasoning for @51764. This marks tiff as
> > affected by CVE-2016-10095. However from the upstream bug and the
> > changes we made in wheezy it looks like the changes we made already are
> > sufficient to fix the issue. Do you have a hint why you think this is
> > not the case?
> 
> CVE-2016-10095 is the generic fix for the API. I'm not sure why that received 
> a CVE ID, since it's not a vulnerability per se (which are in the call sites),
> but it's not worth arguing and providing that in jessie might be useful for
> building building custom tools still.

But then again the fix for this should be in Wheezy already as far as I
can tell. Raphael (since you provided the upstream patches for ths), can
you confirm?
Cheers,
  -- Guido



Re: tiff and CVE-2016-10095

2017-06-02 Thread Moritz Muehlenhoff
On Fri, Jun 02, 2017 at 10:25:29AM +0200, Guido Günther wrote:
> Hi Moritz,
> I'm trying to figure out the reasoning for @51764. This marks tiff as
> affected by CVE-2016-10095. However from the upstream bug and the
> changes we made in wheezy it looks like the changes we made already are
> sufficient to fix the issue. Do you have a hint why you think this is
> not the case?

CVE-2016-10095 is the generic fix for the API. I'm not sure why that received 
a CVE ID, since it's not a vulnerability per se (which are in the call sites),
but it's not worth arguing and providing that in jessie might be useful for
building building custom tools still.

Cheers,
Moritz



Re: tiff / tiff3 / CVE-2015-7554 / CVE-2016-5318

2016-09-15 Thread Brian May
Raphael Hertzog  writes:

> I agree on all this but somehow I have the feeling that we can still
> do better for example by blacklisting tags that are known to use a single
> extension and refusing to handle them as custom
>
> My problem is that I'm not sure that we have a comprehensive list of such
> tags. In particular not one that is available in some structure. In other
> words, we would have to build up that list and hardcode it into the code.
>
> We could build that list from all the tags which are used in CopyField
> (as opposed to CopyField2 or CopyField3) in the various tools. That would
> be a good start IMO.

You are probably right.

However I am still a bit confused with this. According to
http://bugzilla.maptools.org/show_bug.cgi?id=2564:

"However, `field_passcount' is always assigned TRUE if the tag is
processed by `_TIFFCreateAnonField()'.  This happens on unsuccessful
invocations of `TIFFReadDirectoryFindFieldInfo()'"

Oh, wait, only happens on *unsuccessful* invocations of
`TIFFReadDirectoryFindFieldInfo()' - oops - missed that detail.

What does the TIFFReadDirectoryFindFieldInfo function do? What
situations is TIFFReadDirectoryFindFieldInfo unsuccessful?

> In any case, this is really a hack to mitigate the issue that should be
> fixed with a better API IMO. (Feel free to share my comments with
> upstream)

Definitely. Having a C function take a variable number of arguments that
can vary depending on runtime state sounds like a recipe for disaster to
me.

You could perhaps mitigate by requiring an extra parameter that declares
the number of options you are parsing, however I think the chances of
getting it wrong are high.
-- 
Brian May 



Re: tiff / tiff3 / CVE-2015-7554 / CVE-2016-5318

2016-09-15 Thread Brian May
Salvatore Bonaccorso  writes:

> Minor comment: if you are sure that those are duplicates you might try
> to contact MITRE to made them aware.

I was just going based on what others have said, e.g. in the linked
reports. Would hope that one of them has already contacted MITRE...
-- 
Brian May 



Re: tiff / tiff3 / CVE-2015-7554 / CVE-2016-5318

2016-09-14 Thread Raphael Hertzog
Hi,

On Wed, 14 Sep 2016, Brian May wrote:
> CVE-2015-7554 / http://bugzilla.maptools.org/show_bug.cgi?id=2564
> 
> Duplicate:
> 
> CVE-2016-5318 / http://bugzilla.maptools.org/show_bug.cgi?id=2561
> 
> What would be considered an acceptable fix here? It looks like a proper
> fix is not available without changing the API due to limitations in the
> stdarg.h API. Plus IMHO the TIFFGetField API looks badly designed and
> prone to error considering these known limitations.

(I spent a lot of time on tiff3 issues yesterday.)

I agree on all this but somehow I have the feeling that we can still
do better for example by blacklisting tags that are known to use a single
extension and refusing to handle them as custom

My problem is that I'm not sure that we have a comprehensive list of such
tags. In particular not one that is available in some structure. In other
words, we would have to build up that list and hardcode it into the code.

We could build that list from all the tags which are used in CopyField
(as opposed to CopyField2 or CopyField3) in the various tools. That would
be a good start IMO.

In any case, this is really a hack to mitigate the issue that should be
fixed with a better API IMO. (Feel free to share my comments with
upstream)

> There is a fix for the tiffsplit client program:
> 
> http://bugzilla.maptools.org/show_bug.cgi?id=2564#c2
> 
> Is it worth trying to fix tiffsplit (like Redhat), and maybe somehow
> documenting somewhere (e.g. the DSA/DLA) that the scope of the fix is
> restricted?

To me it looks like the RedHat patch fixes the issue with the specificly
crafted file... but you can just recreate the same problem with
another (non-standard) tiff tag and their patch is then useless. 

Cheers,
-- 
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: http://www.freexian.com/services/debian-lts.html
Learn to master Debian: http://debian-handbook.info/get/