Ico security issues fix

Ico security issues fix

BUG=skia:3401
BUG=skia:3426
BUG=skia:3441
Committed: https://skia.googlesource.com/skia/+/6e8f9033bb8d4f27a0dae6bc10bfb629f8e3ba0b

[msarett, 2015-03-11 21:55:25]

msarett@google.com changed reviewers:
+ scroggo@google.com

[msarett, 2015-03-11 21:55:25]

This fixes existing Ico security issues and seems to cover all cases.  I can't
claim to be 100% sure that this will catch everything.  The code is quite
convoluted.  An alternate approach would be to call the new Ico decoder.

https://code.google.com/p/skia/issues/detail?id=3401
https://code.google.com/p/skia/issues/detail?id=3426
https://code.google.com/p/skia/issues/detail?id=3441

[scroggo, 2015-03-12 13:25:55]

On 2015/03/11 21:55:25, msarett wrote:
> This fixes existing Ico security issues and seems to cover all cases.  I can't
> claim to be 100% sure that this will catch everything.  The code is quite
> convoluted.  An alternate approach would be to call the new Ico decoder.

Please add tests. The attached issues have corrupt ICO files that you can add to
testing. Add them to resources/invalid_images. You can tack the names of the
files on to the end of the list in BadIcoTest.cpp so we'll test them. The latter
two will confirm that we do not crash, and the first should trigger TSAN/ASAN if
we do some out of bounds reads. (It might be interesting to add the first one as
a separate CL, and do a try to confirm that it does fail.) Once we have the
SkIcoCodec working, we can run the test with it as well.

> 
> https://code.google.com/p/skia/issues/detail?id=3401
> https://code.google.com/p/skia/issues/detail?id=3426
> https://code.google.com/p/skia/issues/detail?id=3441

Please update the issue to include:

BUG=skia:3401
BUG=skia:3426
BUG=skia:3441

This will make it easy to see what bug inspired the change, and will also
automatically update those issues with the CL (once committed).

https://codereview.chromium.org/996173005/diff/40001/src/images/SkImageDecode...
File src/images/SkImageDecoder_libico.cpp (right):

https://codereview.chromium.org/996173005/diff/40001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:86: if (length < 6) {
Why not merge this with the earlier check? We did not do anything interesting
that needs to be between the two.

https://codereview.chromium.org/996173005/diff/40001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:201: int bytesPerColor = 4;
This can be inlined, since it's only used once. (Maybe with a comment that it's
always 4 bytes per color - at least the ones we support...).

https://codereview.chromium.org/996173005/diff/40001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:257: int maxAndOffset = andOffset +
((andLineWidth*(h-1)+(w-1)) >> 3);
nit: This seems like it's the max offset of the alpha byte.

https://codereview.chromium.org/996173005/diff/40001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:280: placePixel(pixelNo, buf, xorOffset, x,
y, w, bm, alphaByte, m, shift, colors);
Here's the part where I get nervous. We pass buf to one of our placePixel
functions. Are they guaranteed not to read off the end off the array?

Maybe so... It looks like each call to read the buffer always starts from
xorOffset + f(pixelNo), (where f is some function). So long as that part is
smaller than maxAndOffset, the read should be safe. (I don't know whether we can
guarantee that.)

[msarett, 2015-03-12 14:46:59]

msarett@google.com changed reviewers:
+ hcm@google.com

[msarett, 2015-03-12 15:58:52]

I have been unable to reproduce to the seg faults, but I think the fact that the
decode succeeds on these invalid files is demonstration of the bug.

Running the new test on the old code:
0 srcs * 4 sinks + 1 tests == 1 tasks
FAILURE: ../../tests/BadIcoTest.cpp:34	!success
FAILURE: ../../tests/BadIcoTest.cpp:34	!success
FAILURE: ../../tests/BadIcoTest.cpp:34	!success
(  60MB     0) 14.6ms	unit test BadIco
Failures:
	../../tests/BadIcoTest.cpp:34	!success
	../../tests/BadIcoTest.cpp:34	!success
	../../tests/BadIcoTest.cpp:34	!success
3 failures

Running the new tests on the new code:
0 srcs * 4 sinks + 1 tests == 1 tasks
(  60MB     0) 308µs	unit test BadIco

https://codereview.chromium.org/996173005/diff/40001/src/images/SkImageDecode...
File src/images/SkImageDecoder_libico.cpp (right):

https://codereview.chromium.org/996173005/diff/40001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:201: int bytesPerColor = 4;
On 2015/03/12 13:25:54, scroggo wrote:
> This can be inlined, since it's only used once. (Maybe with a comment that
it's
> always 4 bytes per color - at least the ones we support...).

Sure.  FWIW, 4 shows up in the code later.  We could alternately replace these
later uses of 4 with bytesPerColor, but I think it's best to avoid wasting time
trying to improve this code.

https://codereview.chromium.org/996173005/diff/40001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:257: int maxAndOffset = andOffset +
((andLineWidth*(h-1)+(w-1)) >> 3);
On 2015/03/12 13:25:54, scroggo wrote:
> nit: This seems like it's the max offset of the alpha byte.

The AND mask is a 1 bit mask for each pixel indicating transparent or opaque. 
It comes after the XOR mask (actual pixel data).  I should make this more clear.
 I think the designer interprets all AND bytes as "alpha bytes".

https://codereview.chromium.org/996173005/diff/40001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:280: placePixel(pixelNo, buf, xorOffset, x,
y, w, bm, alphaByte, m, shift, colors);
On 2015/03/12 13:25:54, scroggo wrote:
> Here's the part where I get nervous. We pass buf to one of our placePixel
> functions. Are they guaranteed not to read off the end off the array?
> 
> Maybe so... It looks like each call to read the buffer always starts from
> xorOffset + f(pixelNo), (where f is some function). So long as that part is
> smaller than maxAndOffset, the read should be safe. (I don't know whether we
can
> guarantee that.)

The XOR offset SHOULD always be less than the AND offset.  I have performed a
more thorough look at the code, and I feel that this will always be the case,
given the calculations.  I am adding an SkASSERT that clarifies this assumption.

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
File src/images/SkImageDecoder_libico.cpp (right):

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:153: // For ico images, only a byte is used
to store each dimension
The original decoder does not make this check.  I added this code because 0
values of w or h would mess up the bounds checking below.  We could
alternatively return kFailure here.

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:262: // OR mask in the bmp.  If we check
that the largest AND offset is safe,
This is a type I meant XOR not OR.  Will fix in next draft.

[scroggo, 2015-03-12 21:19:29]

On 2015/03/12 15:58:52, msarett wrote:
> I have been unable to reproduce to the seg faults,

Weird. Not really a bug after all then?

> but I think the fact that the
> decode succeeds on these invalid files is demonstration of the bug.

Agreed. At least part of it. It doesn't demonstrate that we necessarily read
memory we shouldn't have (does the seg fault come from reading random data, so
it's non-deterministic?).

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
File src/images/SkImageDecoder_libico.cpp (right):

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:113: SkASSERT(w >= 0 && h >= 0);
Is this because readByte is guaranteed to return a non-negative number? We might
need to instead return kFailure.

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:153: // For ico images, only a byte is used
to store each dimension
On 2015/03/12 15:58:52, msarett wrote:
> The original decoder does not make this check.  I added this code because 0
> values of w or h would mess up the bounds checking below.  We could
> alternatively return kFailure here.

If images in the wild use 0 to mean 256, we should include this.

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:273: SkDEBUGCODE(
nit: Typically we use SkDEBUGCODE for one line. For something like this, we use

#ifdef SK_DEBUG

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:294: return kFailure;
This is strange. We (sort of) do something different in debug. But from your
assert, I see we should never reach here. Maybe this is the right thing to do.

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:298: SkASSERT(maxXOROffset < andOffset);
Can you include this inside the SkDEBUGCODE/SK_DEBUG check? It works in both
debug and release here, but I had to do a double take.

[msarett, 2015-03-12 22:18:23]

Yes I'm 99% sure that the non-deterministic seg faults and the leak occur when
reading outside the size of buf.

I'm not 100% because I haven't figured out how to reproduce them with ASAN.  I
will do that tomorrow.

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
File src/images/SkImageDecoder_libico.cpp (right):

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:113: SkASSERT(w >= 0 && h >= 0);
On 2015/03/12 21:19:29, scroggo wrote:
> Is this because readByte is guaranteed to return a non-negative number? We
might
> need to instead return kFailure.

It is because readByte is guaranteed to return a non-negative number.  It
returns an unsigned char.

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:153: // For ico images, only a byte is used
to store each dimension
On 2015/03/12 21:19:29, scroggo wrote:
> On 2015/03/12 15:58:52, msarett wrote:
> > The original decoder does not make this check.  I added this code because 0
> > values of w or h would mess up the bounds checking below.  We could
> > alternatively return kFailure here.
> 
> If images in the wild use 0 to mean 256, we should include this.

This is how images in the wild are stored.  I will leave this.

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:273: SkDEBUGCODE(
On 2015/03/12 21:19:29, scroggo wrote:
> nit: Typically we use SkDEBUGCODE for one line. For something like this, we
use
> 
> #ifdef SK_DEBUG

Done.

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:294: return kFailure;
On 2015/03/12 21:19:29, scroggo wrote:
> This is strange. We (sort of) do something different in debug. But from your
> assert, I see we should never reach here. Maybe this is the right thing to do.

Yes we should never reach here.  I will defer to you on what you think is the
right thing to style-wise.  I have no idea.

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:298: SkASSERT(maxXOROffset < andOffset);
On 2015/03/12 21:19:29, scroggo wrote:
> Can you include this inside the SkDEBUGCODE/SK_DEBUG check? It works in both
> debug and release here, but I had to do a double take.

Yes that will make it easier to understand.

[msarett, 2015-03-13 12:18:07]

Sorry, I thought this uploaded with the comments from yesterday.

[msarett, 2015-03-13 13:19:49]

The address sanitizer fails on each of the three new test images.

=================================================================
==28498== ERROR: AddressSanitizer: heap-buffer-overflow on address
0x600600c7d6f7 at pc 0x7f21a2a2fc96 bp 0x7fffd28933e0 sp 0x7fffd28933d8
READ of size 1 at 0x600600c7d6f7 thread T0

ASAN:SIGSEGV
=================================================================
==28645== ERROR: AddressSanitizer: SEGV on unknown address 0x600600ca1ab0 (pc
0x7fb08b999284 sp 0x7fff151a4200 bp 0x7fff151a4490 T0)

ASAN:SIGSEGV
=================================================================
==28710== ERROR: AddressSanitizer: SEGV on unknown address 0x600c00098eba (pc
0x7fa9906e6284 sp 0x7fff6561b390 bp 0x7fff6561b620 T0)

The CL eliminates these failures.

[scroggo, 2015-03-13 13:43:12]

On 2015/03/13 13:19:49, msarett wrote:
> The address sanitizer fails on each of the three new test images.
> 
> =================================================================
> ==28498== ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x600600c7d6f7 at pc 0x7f21a2a2fc96 bp 0x7fffd28933e0 sp 0x7fffd28933d8
> READ of size 1 at 0x600600c7d6f7 thread T0
> 
> ASAN:SIGSEGV
> =================================================================
> ==28645== ERROR: AddressSanitizer: SEGV on unknown address 0x600600ca1ab0 (pc
> 0x7fb08b999284 sp 0x7fff151a4200 bp 0x7fff151a4490 T0)
> 
> ASAN:SIGSEGV
> =================================================================
> ==28710== ERROR: AddressSanitizer: SEGV on unknown address 0x600c00098eba (pc
> 0x7fa9906e6284 sp 0x7fff6561b390 bp 0x7fff6561b620 T0)
> 
> The CL eliminates these failures.

Great! lgtm

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
File src/images/SkImageDecoder_libico.cpp (right):

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:113: SkASSERT(w >= 0 && h >= 0);
On 2015/03/12 22:18:22, msarett wrote:
> On 2015/03/12 21:19:29, scroggo wrote:
> > Is this because readByte is guaranteed to return a non-negative number? We
> might
> > need to instead return kFailure.
> 
> It is because readByte is guaranteed to return a non-negative number.  It
> returns an unsigned char.

Ah, got it, because buf is defined as pointer to unsigned char.

https://codereview.chromium.org/996173005/diff/60001/src/images/SkImageDecode...
src/images/SkImageDecoder_libico.cpp:294: return kFailure;
On 2015/03/12 22:18:22, msarett wrote:
> On 2015/03/12 21:19:29, scroggo wrote:
> > This is strange. We (sort of) do something different in debug. But from your
> > assert, I see we should never reach here. Maybe this is the right thing to
do.
> 
> Yes we should never reach here.  I will defer to you on what you think is the
> right thing to style-wise.  I have no idea.

I think it's fine as is.

[msarett, 2015-03-13 13:50:06]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-03-13 13:50:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/996173005/80001

[commit-bot: I haz the power, 2015-03-13 14:10:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-13 14:10:18]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[msarett, 2015-03-13 14:30:00]

The CQ bit was checked by msarett@google.com

[msarett, 2015-03-13 14:30:00]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/996173005/#ps100001
(title: "Fixed casting")

[commit-bot: I haz the power, 2015-03-13 14:30:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/996173005/100001

[commit-bot: I haz the power, 2015-03-13 14:47:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-13 14:47:35]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[msarett, 2015-03-13 15:00:01]

The CQ bit was checked by msarett@google.com

[msarett, 2015-03-13 15:00:02]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/996173005/#ps120001
(title: "actually fixed it this time")

[commit-bot: I haz the power, 2015-03-13 15:00:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/996173005/120001

[commit-bot: I haz the power, 2015-03-13 15:07:05]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/6e8f9033bb8d4f27a0dae6bc10bfb629f8e3ba0b