Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
996173005:
Ico security issues fix (Closed)
DescriptionIco security issues fix
BUG=skia:3401
BUG=skia:3426
BUG=skia:3441
Committed: https://skia.googlesource.com/skia/+/6e8f9033bb8d4f27a0dae6bc10bfb629f8e3ba0b
Patch Set 1 #Patch Set 2 : Security fix for Ico #Patch Set 3 : Security fix for Ico #
Total comments: 7
Patch Set 4 : Added new tests and improved checks #
Total comments: 14
Patch Set 5 : Fixed debug block #Patch Set 6 : Fixed casting #Patch Set 7 : actually fixed it this time #
Messages
Total messages: 23 (9 generated)
msarett@google.com changed reviewers: + scroggo@google.com
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
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@google.com changed reviewers: + hcm@google.com
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.
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.
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.
Sorry, I thought this uploaded with the comments from yesterday.
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.
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.
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/996173005/80001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by msarett@google.com
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/996173005/100001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by msarett@google.com
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/996173005/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/6e8f9033bb8d4f27a0dae6bc10bfb629f8e3ba0b |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
