Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23548034:
Follow up to serialization validation code (Closed)
|
Created:
7 years, 3 months ago by sugoi1 Modified:
7 years, 2 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionFollow up to serialization validation code
1 ) Added check for bool to make sure is it either 0 or 1 and not garbage
2 ) Added more solid kernel size checks in SkMatrixConvolutionImageFilter
3 ) Make sure array size is validated in SkMergeImageFilter
BUG=
Committed: http://code.google.com/p/skia/source/detail?r=11925
Patch Set 1 #
Total comments: 6
Patch Set 2 : Simplified readBool() #Patch Set 3 : Made class specific switch debug only #Patch Set 4 : Merged in changes from 23021015 #
Total comments: 6
Patch Set 5 : Changed 0xFFFFFFFE for ~1 #
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/23548034/diff/1/src/core/SkFlattenableSeriali... File src/core/SkFlattenableSerialization.cpp (right): https://codereview.chromium.org/23548034/diff/1/src/core/SkFlattenableSeriali... src/core/SkFlattenableSerialization.cpp:27: return reader.readFlattenableT<SkFlattenable>(); This isn't adding any security yet here, but I'll make more specialized cases later on.
https://codereview.chromium.org/23548034/diff/1/include/core/SkFlattenableBuf... File include/core/SkFlattenableBuffers.h (right): https://codereview.chromium.org/23548034/diff/1/include/core/SkFlattenableBuf... include/core/SkFlattenableBuffers.h:115: // Use readFlattenableT to enforce a type check on the flattenable read How does this enforce a type-check?
On 2013/09/13 15:18:40, reed1 wrote: > https://codereview.chromium.org/23548034/diff/1/include/core/SkFlattenableBuf... > File include/core/SkFlattenableBuffers.h (right): > > https://codereview.chromium.org/23548034/diff/1/include/core/SkFlattenableBuf... > include/core/SkFlattenableBuffers.h:115: // Use readFlattenableT to enforce a > type check on the flattenable read > How does this enforce a type-check? It does not, this is step 1 : making sure everything goes through readFlattenableT. Using the template argument, we'll be able to find out if the expected object matched the object about to be deserialized and avoid deserializing unwanted objects instead of having to decide if they are valid only post-deserialization.
On 2013/09/13 15:26:45, sugoi1 wrote: > On 2013/09/13 15:18:40, reed1 wrote: > > > https://codereview.chromium.org/23548034/diff/1/include/core/SkFlattenableBuf... > > File include/core/SkFlattenableBuffers.h (right): > > > > > https://codereview.chromium.org/23548034/diff/1/include/core/SkFlattenableBuf... > > include/core/SkFlattenableBuffers.h:115: // Use readFlattenableT to enforce a > > type check on the flattenable read > > How does this enforce a type-check? > > It does not, this is step 1 : making sure everything goes through > readFlattenableT. Using the template argument, we'll be able to find out if the > expected object matched the object about to be deserialized and avoid > deserializing unwanted objects instead of having to decide if they are valid > only post-deserialization. This change to using the T<> version may or may not be what we end up using for a real runtime check, but I agree that is it not a step backwards. I propose don't document it to claim anything about "checking", since that is definitely misleading at this stage.
https://codereview.chromium.org/23548034/diff/1/src/core/SkValidatingReadBuff... File src/core/SkValidatingReadBuffer.cpp (right): https://codereview.chromium.org/23548034/diff/1/src/core/SkValidatingReadBuff... src/core/SkValidatingReadBuffer.cpp:98: fError |= !boolPtr || (*boolPtr) & 0xFFFFFFFE; Why are we dealing with ptrs and casts here? Why not... uint32_t value = this->readInt(); if (0 != value || 1 != value) { fError = true; } return value; https://codereview.chromium.org/23548034/diff/1/src/pipe/SkGPipeRead.cpp File src/pipe/SkGPipeRead.cpp (right): https://codereview.chromium.org/23548034/diff/1/src/pipe/SkGPipeRead.cpp#newc... src/pipe/SkGPipeRead.cpp:66: static SkFlattenable* read_paintflat(SkFlattenableReadBuffer* reader, unsigned paintFlat) { Seems like a lot of code to just check that paintFlat is < kCount_PaintFlags. Why not just if (paintFlat >= kCount_PaintFlags) { return NULL; } else { return reader->readFlattenableT<SkFlattenable>(); } I understand the future-idea-desire to have a runtime check, but this switch statement accomplishes none of that afaik.
https://codereview.chromium.org/23548034/diff/1/src/core/SkValidatingReadBuff... File src/core/SkValidatingReadBuffer.cpp (right): https://codereview.chromium.org/23548034/diff/1/src/core/SkValidatingReadBuff... src/core/SkValidatingReadBuffer.cpp:98: fError |= !boolPtr || (*boolPtr) & 0xFFFFFFFE; On 2013/09/13 15:42:56, reed1 wrote: > Why are we dealing with ptrs and casts here? Why not... > > uint32_t value = this->readInt(); > if (0 != value || 1 != value) { > fError = true; > } > return value; You're right, I was originally thinking of a more complex case where I wouldn't want to read an entire, larger object, but here, it's already reading the entire int value to perform the check so this is redundant. https://codereview.chromium.org/23548034/diff/1/src/pipe/SkGPipeRead.cpp File src/pipe/SkGPipeRead.cpp (right): https://codereview.chromium.org/23548034/diff/1/src/pipe/SkGPipeRead.cpp#newc... src/pipe/SkGPipeRead.cpp:66: static SkFlattenable* read_paintflat(SkFlattenableReadBuffer* reader, unsigned paintFlat) { On 2013/09/13 15:42:56, reed1 wrote: > Seems like a lot of code to just check that paintFlat is < kCount_PaintFlags. > Why not just > > if (paintFlat >= kCount_PaintFlags) { > return NULL; > } else { > return reader->readFlattenableT<SkFlattenable>(); > } > > I understand the future-idea-desire to have a runtime check, but this switch statement accomplishes none of that afaik. I'm not sure I understand. Each case uses a different class that can be used for type checking. That's not equivalent to treating every case as a generic SkFlattenable. Right now, if someone made a mistake and swapped 2 objects, nothing would detect it.
Since we don't have a fuzzer, IWBN to add unit tests for the different validation checks.
On 2013/09/13 16:53:44, sugoi1 wrote: > https://codereview.chromium.org/23548034/diff/1/src/core/SkValidatingReadBuff... > File src/core/SkValidatingReadBuffer.cpp (right): > > https://codereview.chromium.org/23548034/diff/1/src/core/SkValidatingReadBuff... > src/core/SkValidatingReadBuffer.cpp:98: fError |= !boolPtr || (*boolPtr) & > 0xFFFFFFFE; > On 2013/09/13 15:42:56, reed1 wrote: > > Why are we dealing with ptrs and casts here? Why not... > > > > uint32_t value = this->readInt(); > > if (0 != value || 1 != value) { > > fError = true; > > } > > return value; > > You're right, I was originally thinking of a more complex case where I wouldn't > want to read an entire, larger object, but here, it's already reading the entire > int value to perform the check so this is redundant. > > https://codereview.chromium.org/23548034/diff/1/src/pipe/SkGPipeRead.cpp > File src/pipe/SkGPipeRead.cpp (right): > > https://codereview.chromium.org/23548034/diff/1/src/pipe/SkGPipeRead.cpp#newc... > src/pipe/SkGPipeRead.cpp:66: static SkFlattenable* > read_paintflat(SkFlattenableReadBuffer* reader, unsigned paintFlat) { > On 2013/09/13 15:42:56, reed1 wrote: > > Seems like a lot of code to just check that paintFlat is < kCount_PaintFlags. > > Why not just > > > > if (paintFlat >= kCount_PaintFlags) { > > return NULL; > > } else { > > return reader->readFlattenableT<SkFlattenable>(); > > } > > > > I understand the future-idea-desire to have a runtime check, but this switch > statement accomplishes none of that afaik. > > I'm not sure I understand. Each case uses a different class that can be used for > type checking. That's not equivalent to treating every case as a generic > SkFlattenable. Right now, if someone made a mistake and swapped 2 objects, > nothing would detect it. My understanding is that readFlattenableT<SkImageFilter>() is not performing *any* checks. Is this incorrect?
On 2013/09/13 17:58:41, reed1 wrote: > On 2013/09/13 16:53:44, sugoi1 wrote: > > > https://codereview.chromium.org/23548034/diff/1/src/core/SkValidatingReadBuff... > > File src/core/SkValidatingReadBuffer.cpp (right): > > > > > https://codereview.chromium.org/23548034/diff/1/src/core/SkValidatingReadBuff... > > src/core/SkValidatingReadBuffer.cpp:98: fError |= !boolPtr || (*boolPtr) & > > 0xFFFFFFFE; > > On 2013/09/13 15:42:56, reed1 wrote: > > > Why are we dealing with ptrs and casts here? Why not... > > > > > > uint32_t value = this->readInt(); > > > if (0 != value || 1 != value) { > > > fError = true; > > > } > > > return value; > > > > You're right, I was originally thinking of a more complex case where I > wouldn't > > want to read an entire, larger object, but here, it's already reading the > entire > > int value to perform the check so this is redundant. > > > > https://codereview.chromium.org/23548034/diff/1/src/pipe/SkGPipeRead.cpp > > File src/pipe/SkGPipeRead.cpp (right): > > > > > https://codereview.chromium.org/23548034/diff/1/src/pipe/SkGPipeRead.cpp#newc... > > src/pipe/SkGPipeRead.cpp:66: static SkFlattenable* > > read_paintflat(SkFlattenableReadBuffer* reader, unsigned paintFlat) { > > On 2013/09/13 15:42:56, reed1 wrote: > > > Seems like a lot of code to just check that paintFlat is < > kCount_PaintFlags. > > > Why not just > > > > > > if (paintFlat >= kCount_PaintFlags) { > > > return NULL; > > > } else { > > > return reader->readFlattenableT<SkFlattenable>(); > > > } > > > > > > I understand the future-idea-desire to have a runtime check, but this switch > > statement accomplishes none of that afaik. > > > > I'm not sure I understand. Each case uses a different class that can be used for > > type checking. That's not equivalent to treating every case as a generic > > SkFlattenable. Right now, if someone made a mistake and swapped 2 objects, > > nothing would detect it. > > My understanding is that readFlattenableT<SkImageFilter>() is not performing > *any* checks. Is this incorrect? Right, but the reason for that is that I'm cutting my work into small, easy to review cls instead of doing the whole thing at once. In that respect, I think I have to write the code in a way that will work once the type checking is implemented, otherwise, what would be the point of changing to readFlattenableT if it wasn't going to provide any improvement over readFlattenable ?
On 2013/09/13 18:04:53, sugoi1 wrote: > On 2013/09/13 17:58:41, reed1 wrote: > > On 2013/09/13 16:53:44, sugoi1 wrote: > > > > > > https://codereview.chromium.org/23548034/diff/1/src/core/SkValidatingReadBuff... > > > File src/core/SkValidatingReadBuffer.cpp (right): > > > > > > > > > https://codereview.chromium.org/23548034/diff/1/src/core/SkValidatingReadBuff... > > > src/core/SkValidatingReadBuffer.cpp:98: fError |= !boolPtr || (*boolPtr) & > > > 0xFFFFFFFE; > > > On 2013/09/13 15:42:56, reed1 wrote: > > > > Why are we dealing with ptrs and casts here? Why not... > > > > > > > > uint32_t value = this->readInt(); > > > > if (0 != value || 1 != value) { > > > > fError = true; > > > > } > > > > return value; > > > > > > You're right, I was originally thinking of a more complex case where I > > wouldn't > > > want to read an entire, larger object, but here, it's already reading the > > entire > > > int value to perform the check so this is redundant. > > > > > > https://codereview.chromium.org/23548034/diff/1/src/pipe/SkGPipeRead.cpp > > > File src/pipe/SkGPipeRead.cpp (right): > > > > > > > > > https://codereview.chromium.org/23548034/diff/1/src/pipe/SkGPipeRead.cpp#newc... > > > src/pipe/SkGPipeRead.cpp:66: static SkFlattenable* > > > read_paintflat(SkFlattenableReadBuffer* reader, unsigned paintFlat) { > > > On 2013/09/13 15:42:56, reed1 wrote: > > > > Seems like a lot of code to just check that paintFlat is < > > kCount_PaintFlags. > > > > Why not just > > > > > > > > if (paintFlat >= kCount_PaintFlags) { > > > > return NULL; > > > > } else { > > > > return reader->readFlattenableT<SkFlattenable>(); > > > > } > > > > > > > > I understand the future-idea-desire to have a runtime check, but this > switch > > > statement accomplishes none of that afaik. > > > > > > I'm not sure I understand. Each case uses a different class that can be used > for > > > type checking. That's not equivalent to treating every case as a generic > > > SkFlattenable. Right now, if someone made a mistake and swapped 2 objects, > > > nothing would detect it. > > > > My understanding is that readFlattenableT<SkImageFilter>() is not performing > > *any* checks. Is this incorrect? > > Right, but the reason for that is that I'm cutting my work into small, easy to > review cls instead of doing the whole thing at once. In that respect, I think I > have to write the code in a way that will work once the type checking is > implemented, otherwise, what would be the point of changing to readFlattenableT > if it wasn't going to provide any improvement over readFlattenable ? I kinda agree, so I'm not sure of the value of this change, since we can't verify that the new annotations are in fact correct.
Most of the changes I did not comment on, since you were replacing a static_cast with a template 'cast'. That seems fine (and not any worse). The big switch statement just looks like a gob of typing and possible slow-down, with no benefit. Maybe just a todo/comment reminding us to apply real checking if/when we get it?
On 2013/09/13 18:09:11, reed1 wrote: > Most of the changes I did not comment on, since you were replacing a static_cast > with a template 'cast'. That seems fine (and not any worse). > > The big switch statement just looks like a gob of typing and possible slow-down, > with no benefit. Maybe just a todo/comment reminding us to apply real checking > if/when we get it? The way I see it, there wouldn't be anything else to do here. The type checking code would simply allow us to generate errors and abort read operations that are reading bad data early rather than continuing until the end of a compromised stream. That being said, SkGPipeRead is not currently in my list of high priority things to do, so if it makes you feel better about it, I guess I can postpone and simplify this code until we need a secure version of SkGPipeRead. Another option could be to add type checking in debug only, maybe ? It could be used to detect errors at runtime and changing it so that it works in optimized versions too would then require minimal efforts.
I made the more costly switch() in SkGPipeRead.cpp debug only, to see if this is an acceptable solution. Tests should be ready soon.
Merged this cl with the serialization cl committed today. Only a few validation tests are left in this cl.
lgtm w/ suggestion https://codereview.chromium.org/23548034/diff/22001/src/core/SkValidatingRead... File src/core/SkValidatingReadBuffer.cpp (right): https://codereview.chromium.org/23548034/diff/22001/src/core/SkValidatingRead... src/core/SkValidatingReadBuffer.cpp:49: if (value & 0xFFFFFFFE) { this is correct, but possibly hard for a reviewer to confirm. What about? if (value & ~1) ...
https://codereview.chromium.org/23548034/diff/22001/src/core/SkValidatingRead... File src/core/SkValidatingReadBuffer.cpp (right): https://codereview.chromium.org/23548034/diff/22001/src/core/SkValidatingRead... src/core/SkValidatingReadBuffer.cpp:49: if (value & 0xFFFFFFFE) { On 2013/10/23 17:58:42, reed1 wrote: > this is correct, but possibly hard for a reviewer to confirm. What about? > > if (value & ~1) ... Done.
https://codereview.chromium.org/23548034/diff/22001/src/core/SkValidatingRead... File src/core/SkValidatingReadBuffer.cpp (right): https://codereview.chromium.org/23548034/diff/22001/src/core/SkValidatingRead... src/core/SkValidatingReadBuffer.cpp:47: uint32_t value = this->readInt(); Not new to this patch, but some flavours of writeBool() call write8(), SkWriter::writeBool() calls writeInt() (without explicitly converting to 0, 1). The readers do a readInt(). Kinda weird. https://codereview.chromium.org/23548034/diff/22001/src/effects/SkMatrixConvo... File src/effects/SkMatrixConvolutionImageFilter.cpp (right): https://codereview.chromium.org/23548034/diff/22001/src/effects/SkMatrixConvo... src/effects/SkMatrixConvolutionImageFilter.cpp:71: uint32_t readSize = buffer.readScalarArray(fKernel); Shouldn't we be passing an expected size to readScalarArray() to avoid overflowing fKernel?
https://codereview.chromium.org/23548034/diff/22001/src/core/SkValidatingRead... File src/core/SkValidatingReadBuffer.cpp (right): https://codereview.chromium.org/23548034/diff/22001/src/core/SkValidatingRead... src/core/SkValidatingReadBuffer.cpp:47: uint32_t value = this->readInt(); On 2013/10/23 18:09:35, Stephen White wrote: > Not new to this patch, but some flavours of writeBool() call write8(), > SkWriter::writeBool() calls writeInt() (without explicitly converting to 0, 1). > The readers do a readInt(). Kinda weird. Yeah, I had to check that noone was doing any kind of funny business with the bools to make sure skia never writes anything other than 0 or 1 when writing a bool. https://codereview.chromium.org/23548034/diff/22001/src/effects/SkMatrixConvo... File src/effects/SkMatrixConvolutionImageFilter.cpp (right): https://codereview.chromium.org/23548034/diff/22001/src/effects/SkMatrixConvo... src/effects/SkMatrixConvolutionImageFilter.cpp:71: uint32_t readSize = buffer.readScalarArray(fKernel); On 2013/10/23 18:09:35, Stephen White wrote: > Shouldn't we be passing an expected size to readScalarArray() to avoid overflowing fKernel? Yes, a few of these functions need to be fixed (all the read...Array() functions can overflow the input buffer and readPath, readRegion and readMatrix can overflow the stream). Will do in the next cl.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/23548034/82001
Message was sent while issue was closed.
Change committed as 11925 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
