Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22799007:
I'm investigating how to make the IPC transfer a bit more secure on the (Closed)
|
Created:
7 years, 4 months ago by sugoi1 Modified:
7 years, 3 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionThis adds a new SkImageFilter fuzzer in skia.
BUG=
Committed: http://code.google.com/p/skia/source/detail?r=11395
Patch Set 1 #
Total comments: 3
Patch Set 2 : New fuzzer added #
Total comments: 30
Patch Set 3 : Fixing comments #Patch Set 4 : Windows error fix #Patch Set 5 : Minor tweaks #
Total comments: 1
Patch Set 6 : Down to fuzzer only #Patch Set 7 : #Patch Set 8 : Fixing windows warnings as error #Patch Set 9 : Removed unwanted file #
Messages
Total messages: 30 (0 generated)
At this point, I just want some input about how this should be done. I've thought about a few solutions, but nothing seems "clean" enough yet. https://codereview.chromium.org/22799007/diff/1/include/core/SkFlattenable.h File include/core/SkFlattenable.h (right): https://codereview.chromium.org/22799007/diff/1/include/core/SkFlattenable.h#... include/core/SkFlattenable.h:48: enum Type { FLATTENABLE, IMAGE_FILTER }; This, of course, could be extended to other types. https://codereview.chromium.org/22799007/diff/1/src/core/SkOrderedReadBuffer.cpp File src/core/SkOrderedReadBuffer.cpp (right): https://codereview.chromium.org/22799007/diff/1/src/core/SkOrderedReadBuffer.... src/core/SkOrderedReadBuffer.cpp:295: At this point, if possible, I'd like to be able to reset the factory to NULL if the factory isn't of the right "type". For example, this function could have some sort of mask or filter as input which only allows certain factory functions to be used. That may be overkill, but why read objects if we know we're going to discard them later ?
It does seem like we'll need some way to validate that the types of objects we deserialize are of the types that we expected, but I don't have much useful to say about whether faux-RTTI is the way to go. Maybe Leon or Mike would have some insight. Perhaps test-driven development might be a useful approach: start by writing a "malicious" serializer that creates objects of the wrong types, arrays of the wrong length, etc and try to detect that they're caught. That could be evolved into a full fuzzer, or just serve as a way to validate assumptions until the full fuzzer is written.
Here's the new fuzzer. It doesn't cover serialization yet, but it covers random filter DAGs. It's not too aggressive at generating really bad data for filters for now, but it did catch a few small things, which are also fixed here. The follow-up should contain serialization code and increasingly aggressive "randomness" to push the limits of what the filters can handle. https://codereview.chromium.org/22799007/diff/1/src/core/SkOrderedReadBuffer.cpp File src/core/SkOrderedReadBuffer.cpp (right): https://codereview.chromium.org/22799007/diff/1/src/core/SkOrderedReadBuffer.... src/core/SkOrderedReadBuffer.cpp:291: if (NULL == factory || NULL == SkFlattenable::FactoryToName(factory)) { I quickly realized that using SkFlattenable::FactoryToName() wasn't going to work since not all flattenable objects use the Registrar interface, but I think we need some way of not simply reading and calling a function pointer, for security reasons.
https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... File samplecode/SampleFilterFuzz.cpp (right): https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... samplecode/SampleFilterFuzz.cpp:33: static bool return_large = false; Shouldn't we also be randomizing these values, or removing them? They seem to always choose one code path. https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... samplecode/SampleFilterFuzz.cpp:218: SkLightingImageFilter::CreateSpotLitDiffuse(SkPoint3(0, 0, 0), Maybe we should have a make_point3 that we could use here? https://codereview.chromium.org/22799007/diff/7001/src/effects/SkBicubicImage... File src/effects/SkBicubicImageFilter.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/effects/SkBicubicImage... src/effects/SkBicubicImageFilter.cpp:102: if (dstIRect.isEmpty()) { In the belt-and-suspenders department, we should also find out who is dereferencing the pixels of this zero-size bitmap downstream, and fix that too. https://codereview.chromium.org/22799007/diff/7001/src/effects/SkLightingImag... File src/effects/SkLightingImageFilter.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/effects/SkLightingImag... src/effects/SkLightingImageFilter.cpp:49: void getClampedColor(const SkPoint3 color, U8CPU& r8, U8CPU& g8, U8CPU& b8) { Personal style, but I'm not a fan of non-const refs. Make it a pointer arg, and it's easier to see from the call site what gets modified. In fact, I'd just use SkClampMax on each component in the the SkPackARGB32() call rather than rolling your own, and do away with this function entirely. https://codereview.chromium.org/22799007/diff/7001/src/effects/SkLightingImag... src/effects/SkLightingImageFilter.cpp:101: return SkPackARGB32(r > g ? (r > b ? r : b) : (g > b ? g : b), r, g, b); Could we not simply clamp the alpha after doing maxComponent()? Kind of ugly to do it all inline here.
https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... File samplecode/SampleFilterFuzz.cpp (right): https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... samplecode/SampleFilterFuzz.cpp:33: static bool return_large = false; On 2013/08/21 20:49:26, Stephen White wrote: > Shouldn't we also be randomizing these values, or removing them? They seem to > always choose one code path. I'd like to add more "randomness" in the next version and handle more cases properly. https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... samplecode/SampleFilterFuzz.cpp:218: SkLightingImageFilter::CreateSpotLitDiffuse(SkPoint3(0, 0, 0), On 2013/08/21 20:49:26, Stephen White wrote: > Maybe we should have a make_point3 that we could use here? I had some issue with having this value randomized. When random origin will be possible here, I'll need more validation code in the spotlight code. I decided to leave it constant for now and handle this case later. https://codereview.chromium.org/22799007/diff/7001/src/effects/SkBicubicImage... File src/effects/SkBicubicImageFilter.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/effects/SkBicubicImage... src/effects/SkBicubicImageFilter.cpp:102: if (dstIRect.isEmpty()) { On 2013/08/21 20:49:26, Stephen White wrote: > In the belt-and-suspenders department, we should also find out who is > dereferencing the pixels of this zero-size bitmap downstream, and fix that too. Isn't returning false enough ? It's pretty much the same behavior as failing to allocate the pixels, a few lines below. And the input isn't 0 sized, it's been scaled down to anything less than 0.5 in any dimension. https://codereview.chromium.org/22799007/diff/7001/src/effects/SkLightingImag... File src/effects/SkLightingImageFilter.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/effects/SkLightingImag... src/effects/SkLightingImageFilter.cpp:49: void getClampedColor(const SkPoint3 color, U8CPU& r8, U8CPU& g8, U8CPU& b8) { On 2013/08/21 20:49:26, Stephen White wrote: > Personal style, but I'm not a fan of non-const refs. Make it a pointer arg, and > it's easier to see from the call site what gets modified. > > In fact, I'd just use SkClampMax on each component in the the SkPackARGB32() > call rather than rolling your own, and do away with this function entirely. Ok, will do. https://codereview.chromium.org/22799007/diff/7001/src/effects/SkLightingImag... src/effects/SkLightingImageFilter.cpp:101: return SkPackARGB32(r > g ? (r > b ? r : b) : (g > b ? g : b), r, g, b); On 2013/08/21 20:49:26, Stephen White wrote: > Could we not simply clamp the alpha after doing maxComponent()? Kind of ugly to > do it all inline here. Sure, I'll just add the clamping code here.
https://codereview.chromium.org/22799007/diff/7001/src/effects/SkBicubicImage... File src/effects/SkBicubicImageFilter.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/effects/SkBicubicImage... src/effects/SkBicubicImageFilter.cpp:102: if (dstIRect.isEmpty()) { On 2013/08/21 21:12:09, sugoi wrote: > On 2013/08/21 20:49:26, Stephen White wrote: > > In the belt-and-suspenders department, we should also find out who is > > dereferencing the pixels of this zero-size bitmap downstream, and fix that > too. > > Isn't returning false enough ? It's pretty much the same behavior as failing to > allocate the pixels, a few lines below. And the input isn't 0 sized, it's been > scaled down to anything less than 0.5 in any dimension. OK, maybe I'm confused. I thought you said at one point that if you allowed this (0x0 alloc), the allocation actually "succeeded", but malloc returned you something you couldn't deref, that blew up in a downstream image filter. That downstream image filter sounds like it's got a bug, if it's trying to deref a 0x0 buffer.
https://codereview.chromium.org/22799007/diff/7001/src/effects/SkBicubicImage... File src/effects/SkBicubicImageFilter.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/effects/SkBicubicImage... src/effects/SkBicubicImageFilter.cpp:102: if (dstIRect.isEmpty()) { On 2013/08/21 21:23:00, Stephen White wrote: > On 2013/08/21 21:12:09, sugoi wrote: > > On 2013/08/21 20:49:26, Stephen White wrote: > > > In the belt-and-suspenders department, we should also find out who is > > > dereferencing the pixels of this zero-size bitmap downstream, and fix that > > too. > > > > Isn't returning false enough ? It's pretty much the same behavior as failing to > > allocate the pixels, a few lines below. And the input isn't 0 sized, it's been > > scaled down to anything less than 0.5 in any dimension. > > OK, maybe I'm confused. I thought you said at one point that if you allowed this > (0x0 alloc), the allocation actually "succeeded", but malloc returned you > something you couldn't deref, that blew up in a downstream image filter. That > downstream image filter sounds like it's got a bug, if it's trying to deref a > 0x0 buffer. Ah, ok, I see what you mean. Since the size is 0 x 0, there's no point in trying to access anything. I'll try to find out where (there must be multiple occurrences of this) we dereference a pointer before actually entering a loop, which is potentially accessing invalid memory.
> It does seem like we'll need some way to validate that the types of objects we > deserialize are of the types that we expected, but I don't have much useful to > say about whether faux-RTTI is the way to go. Maybe Leon or Mike would have some > insight. I don't have any insight on ensuring it's the right type. Mike Klein (cc'ed) likes to talk about the wonders of protocol buffers. The fuzzer sample seems like a good start for testing. Should it also input complete garbage data? (As in, not even something that looks like it could be an image filter) https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... File samplecode/SampleFilterFuzz.cpp (right): https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... samplecode/SampleFilterFuzz.cpp:3: * Copyright 2011 Google Inc. 2013 https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... samplecode/SampleFilterFuzz.cpp:118: canvas.drawText(str, strlen(str), SkIntToScalar(kBitmapSize/8), SkIntToScalar(kBitmapSize/4), paint); 100 chars https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... samplecode/SampleFilterFuzz.cpp:228: SkPerlinNoiseShader::CreateFractalNoise(make_scalar(true), make_scalar(true), R(10), make_scalar()) : 100 chars https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... samplecode/SampleFilterFuzz.cpp:229: SkPerlinNoiseShader::CreateTubulence(make_scalar(true), make_scalar(true), R(10), make_scalar())); 100 chars https://codereview.chromium.org/22799007/diff/7001/src/core/SkFlattenableSeri... File src/core/SkFlattenableSerialization.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/core/SkFlattenableSeri... src/core/SkFlattenableSerialization.cpp:35: || !flattenable->getFactory() I'm trying to understand how this could return NULL. It would never have been deserialized properly if getFactory returned NULL, right? Isn't it also redundant with isA? https://codereview.chromium.org/22799007/diff/7001/src/core/SkFlattenableSeri... src/core/SkFlattenableSerialization.cpp:37: || !flattenable->isA(SkFlattenable::IMAGE_FILTER)) If the unflattening created an SkImageFilter, won't this return true even if it was wrong to create an SkImageFilter? https://codereview.chromium.org/22799007/diff/7001/src/effects/SkLightingImag... File src/effects/SkLightingImageFilter.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/effects/SkLightingImag... src/effects/SkLightingImageFilter.cpp:49: void getClampedColor(const SkPoint3 color, U8CPU& r8, U8CPU& g8, U8CPU& b8) { On 2013/08/21 20:49:26, Stephen White wrote: > Personal style, but I'm not a fan of non-const refs. Make it a pointer arg, and > it's easier to see from the call site what gets modified. That's actually in the Skia style guide, so it's more than your personal preference ;)
On 2013/08/21 23:25:27, scroggo wrote: > The fuzzer sample seems like a good start for testing. Should it also input > complete garbage data? (As in, not even something that looks like it could be an > image filter) Absolutely. When I add the serialization/deserialization portion of the test to the fuzzer, the first test will be random bit flips in the data stream, which should yield garbage data most of the time. This first version only tests filter creation, without using serialization. https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... File samplecode/SampleFilterFuzz.cpp (right): https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... samplecode/SampleFilterFuzz.cpp:3: * Copyright 2011 Google Inc. On 2013/08/21 23:25:27, scroggo wrote: > 2013 Done. https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... samplecode/SampleFilterFuzz.cpp:118: canvas.drawText(str, strlen(str), SkIntToScalar(kBitmapSize/8), SkIntToScalar(kBitmapSize/4), paint); On 2013/08/21 23:25:27, scroggo wrote: > 100 chars Done. https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... samplecode/SampleFilterFuzz.cpp:228: SkPerlinNoiseShader::CreateFractalNoise(make_scalar(true), make_scalar(true), R(10), make_scalar()) : On 2013/08/21 23:25:27, scroggo wrote: > 100 chars Done. https://codereview.chromium.org/22799007/diff/7001/samplecode/SampleFilterFuz... samplecode/SampleFilterFuzz.cpp:229: SkPerlinNoiseShader::CreateTubulence(make_scalar(true), make_scalar(true), R(10), make_scalar())); On 2013/08/21 23:25:27, scroggo wrote: > 100 chars Done. https://codereview.chromium.org/22799007/diff/7001/src/core/SkFlattenableSeri... File src/core/SkFlattenableSerialization.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/core/SkFlattenableSeri... src/core/SkFlattenableSerialization.cpp:35: || !flattenable->getFactory() On 2013/08/21 23:25:27, scroggo wrote: > I'm trying to understand how this could return NULL. It would never have been deserialized properly if getFactory returned NULL, right? > > Isn't it also redundant with isA? Well, my original thought was that, if someone would manage to call another function (not a factory) to construct a malicious object here, then the getFactory() call would fail. But you're right, since I added the isA, this part is no longer necessary. https://codereview.chromium.org/22799007/diff/7001/src/core/SkFlattenableSeri... src/core/SkFlattenableSerialization.cpp:37: || !flattenable->isA(SkFlattenable::IMAGE_FILTER)) On 2013/08/21 23:25:27, scroggo wrote: > If the unflattening created an SkImageFilter, won't this return true even if it was wrong to create an SkImageFilter? How would I know it was wrong to create it ? I have no other information than the input stream, AFAIK. https://codereview.chromium.org/22799007/diff/7001/src/effects/SkLightingImag... File src/effects/SkLightingImageFilter.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/effects/SkLightingImag... src/effects/SkLightingImageFilter.cpp:49: void getClampedColor(const SkPoint3 color, U8CPU& r8, U8CPU& g8, U8CPU& b8) { On 2013/08/21 20:49:26, Stephen White wrote: > Personal style, but I'm not a fan of non-const refs. Make it a pointer arg, and > it's easier to see from the call site what gets modified. > > In fact, I'd just use SkClampMax on each component in the the SkPackARGB32() > call rather than rolling your own, and do away with this function entirely. Done. https://codereview.chromium.org/22799007/diff/7001/src/effects/SkLightingImag... src/effects/SkLightingImageFilter.cpp:101: return SkPackARGB32(r > g ? (r > b ? r : b) : (g > b ? g : b), r, g, b); On 2013/08/21 20:49:26, Stephen White wrote: > Could we not simply clamp the alpha after doing maxComponent()? Kind of ugly to > do it all inline here. Done.
https://codereview.chromium.org/22799007/diff/7001/src/core/SkFlattenableSeri... File src/core/SkFlattenableSerialization.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/core/SkFlattenableSeri... src/core/SkFlattenableSerialization.cpp:37: || !flattenable->isA(SkFlattenable::IMAGE_FILTER)) On 2013/08/22 15:41:00, sugoi1 wrote: > On 2013/08/21 23:25:27, scroggo wrote: > > If the unflattening created an SkImageFilter, won't this return true even if > it was wrong to create an SkImageFilter? > > How would I know it was wrong to create it ? I have no other information than > the input stream, AFAIK. My point is that if you read the stream incorrectly, and created an SkImageFilter, this check would not tell you that you were wrong. What kind of error does this avoid?
https://codereview.chromium.org/22799007/diff/7001/src/core/SkFlattenableSeri... File src/core/SkFlattenableSerialization.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/core/SkFlattenableSeri... src/core/SkFlattenableSerialization.cpp:37: || !flattenable->isA(SkFlattenable::IMAGE_FILTER)) On 2013/08/22 17:59:01, scroggo wrote: > On 2013/08/22 15:41:00, sugoi1 wrote: > > On 2013/08/21 23:25:27, scroggo wrote: > > > If the unflattening created an SkImageFilter, won't this return true even if > > it was wrong to create an SkImageFilter? > > > > How would I know it was wrong to create it ? I have no other information than > > the input stream, AFAIK. > > My point is that if you read the stream incorrectly, and created an > SkImageFilter, this check would not tell you that you were wrong. What kind of > error does this avoid? When this function is called, it verifies that what's supposed to be an SkImageFilter object or tree is indeed that and only that. No other SkFlattenables are allowed to appear inside this tree somehow (in case someone hijacked the stream to create something else, possible maliciously). The deserialization does not prevent anyone from creating an SkData object or an SkRasterizer object, for example.
https://codereview.chromium.org/22799007/diff/7001/src/core/SkFlattenableSeri... File src/core/SkFlattenableSerialization.cpp (right): https://codereview.chromium.org/22799007/diff/7001/src/core/SkFlattenableSeri... src/core/SkFlattenableSerialization.cpp:37: || !flattenable->isA(SkFlattenable::IMAGE_FILTER)) On 2013/08/22 18:14:53, sugoi1 wrote: > On 2013/08/22 17:59:01, scroggo wrote: > > On 2013/08/22 15:41:00, sugoi1 wrote: > > > On 2013/08/21 23:25:27, scroggo wrote: > > > > If the unflattening created an SkImageFilter, won't this return true even > if > > > it was wrong to create an SkImageFilter? > > > > > > How would I know it was wrong to create it ? I have no other information > than > > > the input stream, AFAIK. > > > > My point is that if you read the stream incorrectly, and created an > > SkImageFilter, this check would not tell you that you were wrong. What kind of > > error does this avoid? > > When this function is called, it verifies that what's supposed to be an > SkImageFilter object or tree is indeed that and only that. No other > SkFlattenables are allowed to appear inside this tree somehow (in case someone > hijacked the stream to create something else, possible maliciously). The > deserialization does not prevent anyone from creating an SkData object or an > SkRasterizer object, for example. Ah, okay. Thanks for the explanation.
On 2013/08/21 23:25:27, scroggo wrote: > > It does seem like we'll need some way to validate that the types of objects we > > deserialize are of the types that we expected, but I don't have much useful to > > say about whether faux-RTTI is the way to go. Maybe Leon or Mike would have some > > insight. > > I don't have any insight on ensuring it's the right type. Mike Klein (cc'ed) > likes to talk about the wonders of protocol buffers. @mtklein: so, do you have any insight for me as to how to do this properly ? Specifically, you can look at : SkImageFilter.h : https://codereview.chromium.org/22799007/patch/24001/33004 SkFlattenable.h : https://codereview.chromium.org/22799007/patch/24001/33002 The code is very simple and I'd like to know if there's a better way to accomplish this. Thanks.
On 2013/08/26 16:56:11, sugoi1 wrote: > On 2013/08/21 23:25:27, scroggo wrote: > > > It does seem like we'll need some way to validate that the types of objects > we > > > deserialize are of the types that we expected, but I don't have much useful > to > > > say about whether faux-RTTI is the way to go. Maybe Leon or Mike would have > some > > > insight. > > > > I don't have any insight on ensuring it's the right type. Mike Klein (cc'ed) > > likes to talk about the wonders of protocol buffers. > > @mtklein: so, do you have any insight for me as to how to do this properly ? > Specifically, you can look at : > SkImageFilter.h : https://codereview.chromium.org/22799007/patch/24001/33004 > SkFlattenable.h : https://codereview.chromium.org/22799007/patch/24001/33002 > The code is very simple and I'd like to know if there's a better way to > accomplish this. Thanks. So, bear with me while I probably repeat some questions. Are we talking about security as in coding-and-type-safety, security as in detecting malformed byte streams instead of blindly reading, or security as in safe-against-malicious-dudes? It's pretty clear to me that in the last case, if we don't trust the flattened byte stream, we just shouldn't decode it, no matter if it claims to be type IMAGE_FILTER or not. The only way I know how to trust byte streams is to cryptographically sign them. If we're talking about security as in being able to detect malformed byte streams and bail out, I think we're probably mostly there. Again, tagging the type isn't sufficient, but it sure helps. For this we need to be really thorough about knowing exactly how many bytes we have left in the byte stream and how many we're going to read. I think for the most part we check these, but a comprehensive look at the lower level writers and readers we encode to and decode from is probably in order. My guess is that we'll want to convert a bunch of debug-only asserts into code paths that can fail in release mode code too. If we're talking about making it hard for us to screw up when coding, say, reading a shader with readFlattenable() and casting it accidentally to SkImageFilter* instead of SkShader*, I do think type tagging can help us. When decoding flattenables, we mostly use readFlattenableT, which does that SkFlattenable -> SkSomeConcreteType cast for us. Personally, I'd add the check there. If I were writing it, I'd make the API look like this: on SkFlattenable: enum Type { <all possibilities> }; virtual SkFlattenable::Type type() const = 0; on each concrete class: static bool TypeMatches(const SkFlattenable& flattenable) { return flattenable.type() == MyType; } SkFlattenable::Type type() const { return MyType; } and add this somewhere (SkFlattenable.h?) template <typename T> T* flattenable_cast(SkFlattenable* flattenable) { if (flattenable == NULL) return NULL; if (T::TypeMatches(*flattenable)) return static_cast<T*>(flattenable); return NULL; } then use flattenable_cast inside readFlattenableT and other places that need to do the same sort of thing. If we're feeling super paranoid maybe we write it as, template <typename T> bool SK_WARN_UNUSED_RESULT flattenable_cast(SkFlattenable*, T**) { ... } returning a success bool which must be checked. Modulo any major brainfarts, this will basically give us RTTI for SkFlattenables without enabling it globally, and without storing any extra data on class instances or in the flattened bytes. We're basically setting up that Type enum to be isomorphic to the vtables of the SkFlattenable descendants. Are any of these what you're trying to accomplish?
On 2013/08/27 16:04:56, mtklein wrote: > So, bear with me while I probably repeat some questions. Are we talking about > security as in coding-and-type-safety, security as in detecting malformed byte > streams instead of blindly reading, or security as in > safe-against-malicious-dudes? All of the above, but mostly 2 and 3. > The only way I know how to trust byte streams is to cryptographically sign them. Any examples of this ? Is this easily doable ? The context of this is that this stream is sent as a blob of data between processes over IPC using a ParamTraits attribute in the chromium source code. I don't know what level of security is already provided by chromium's inter-process communication. You can look for SkSerializeFlattenable() / SkDeserializeFlattenable() in SkFlattenableSerialization.cpp for the skia side code, but it's very simple. I had discussions with palmer@ about security and, essentially, Skia's job is to prevent someone from using the stream to perform something malicious, specifically inside the deserialization code, so, AFAIK, the bulk of the work is to add sanity checks and try to prevent the creation of the wrong type of objects (like, in this example, we are expecting SkImageFilter objects only) and numerical problems, like integer overflow. > If we're talking about security as in being able to detect malformed byte > streams and bail out, I think we're probably mostly there. Again, tagging the > type isn't sufficient, but it sure helps. For this we need to be really > thorough about knowing exactly how many bytes we have left in the byte stream > and how many we're going to read. I think for the most part we check these, but > a comprehensive look at the lower level writers and readers we encode to and > decode from is probably in order. My guess is that we'll want to convert a > bunch of debug-only asserts into code paths that can fail in release mode code > too. We're already looking into this. A first draft is available here : https://codereview.chromium.org/23021015/ Thanks for the sample code, I will surely use it in the next iterations.
On 2013/08/27 17:21:53, sugoi1 wrote: > On 2013/08/27 16:04:56, mtklein wrote: > > So, bear with me while I probably repeat some questions. Are we talking about > > security as in coding-and-type-safety, security as in detecting malformed byte > > streams instead of blindly reading, or security as in > > safe-against-malicious-dudes? > > All of the above, but mostly 2 and 3. > > > The only way I know how to trust byte streams is to cryptographically sign > them. > > Any examples of this ? Is this easily doable ? > The context of this is that this stream is sent as a blob of data between > processes over IPC using a ParamTraits attribute in the chromium source code. > I don't know what level of security is already provided by chromium's > inter-process communication. > You can look for SkSerializeFlattenable() / SkDeserializeFlattenable() in > SkFlattenableSerialization.cpp for the skia side code, but it's very simple. > I had discussions with palmer@ about security and, essentially, Skia's job is to > prevent someone from using the stream to perform something malicious, > specifically inside the deserialization code, so, AFAIK, the bulk of the work is > to add sanity checks and try to prevent the creation of the wrong type of > objects (like, in this example, we are expecting SkImageFilter objects only) and > numerical problems, like integer overflow. > > > If we're talking about security as in being able to detect malformed byte > > streams and bail out, I think we're probably mostly there. Again, tagging the > > type isn't sufficient, but it sure helps. For this we need to be really > > thorough about knowing exactly how many bytes we have left in the byte stream > > and how many we're going to read. I think for the most part we check these, > but > > a comprehensive look at the lower level writers and readers we encode to and > > decode from is probably in order. My guess is that we'll want to convert a > > bunch of debug-only asserts into code paths that can fail in release mode code > > too. > > We're already looking into this. A first draft is available here : > https://codereview.chromium.org/23021015/ > Thanks for the sample code, I will surely use it in the next iterations. So, to speak only a little hyperbolicly, if our goal is real trust-no-one security, the entire design of SkFlattenable need to be scrapped and redone. Take a look at SkOrderedReadBuffer::readFlattenable(). It's a little hard to follow because there are a few options there, but the gist is: decode(bytes): function_pointer = decodeFunctionPointer(); length = decodeUint32 start = current_byte_index flattenable = function_pointer(*this) check current_byte_index - start == length return flattenable Isn't this a ridiculously huge security hole? If someone can rewrite that stream, they can literally call any code they want just by writing its address as the first part of the stream. The other options in that first if-statement just use an index into a (predetermined?) table of function pointers, which at least we know aren't malicious but can still be totally bogus and I'm sure somehow abused. Note this is also extremely convenient code if we're not worried about malice. The convenience is why it's this way at all. This mechanism is how readFlattenable() can decode the pointer to the right concrete type without us passing its type somehow to readFlattenable. Notice that readFlattenableT only does the superficial cast to the right pointer type, but the underlying data must already have been decoded into that concrete type by that point. We at least need to push all these checks down into SkOrderedReadBuffer where we're dealing with the bytes stream. By the time we nominally have a SkFlattenable*, it's too late. Maybe I'm missing something?
On 2013/08/27 17:51:29, mtklein wrote: > So, to speak only a little hyperbolicly, if our goal is real trust-no-one > security, the entire design of SkFlattenable need to be scrapped and redone. > Take a look at SkOrderedReadBuffer::readFlattenable(). It's a little hard to > follow because there are a few options there, but the gist is: > > decode(bytes): > function_pointer = decodeFunctionPointer(); > length = decodeUint32 > start = current_byte_index > flattenable = function_pointer(*this) > check current_byte_index - start == length > return flattenable > > Isn't this a ridiculously huge security hole? Yes, but we have to close it. > The other options in that first if-statement > just use an index into a (predetermined?) table of function pointers, which at > least we know aren't malicious but can still be totally bogus and I'm sure > somehow abused. I think this is already how it's supposed to work. The function_pointer that's written and read back is only used when we are not doing cross-process serialization. Otherwise, the function pointer needs to be taken from a table. For example, the flattenable function pointer (or factory) can be sent/received as a string and decoded using SkFlattenable::NameToFactory(). As added safety, to the very least, SkFlattenableReadBuffer::readFunctionPtr() should NEVER be allowed to be used when the cross-process flag is on.
Minor tweaks. I fixed a comment by scroggo@ I had forgotten to fix about removing the factory check in SkValidateImageFilterRec() and I ordered include files alphabetically in SampeFilterFuzz.cpp. There will be a follow up, which I'm already working on, where serialization will be included into the fuzzer. Any issues left with this initial version of the fuzzer ?
LGTM, but IWBN to find out who is dereferencing that 0x0 result formerly produced by SkBicubicImageFilter.
On 2013/08/28 14:46:50, Stephen White wrote: > LGTM, but IWBN to find out who is dereferencing that 0x0 result formerly > produced by SkBicubicImageFilter. Please don't submit this yet. I'm certainly in favor of each of the specific fixes to the image filters, but the fixes want unit tests. A fuzzer is excellent at finding these bugs, but I don't want to have to rely on it to be the only protection against regression. I'd like us to take at least one more holistic pass over the big plan here. I do think this CL has accomplished its goal in that it's got us thinking about what's best to do.
https://codereview.chromium.org/22799007/diff/40001/src/core/SkFlattenableSer... File src/core/SkFlattenableSerialization.cpp (right): https://codereview.chromium.org/22799007/diff/40001/src/core/SkFlattenableSer... src/core/SkFlattenableSerialization.cpp:33: if (!flattenable || !flattenable->isA(SkFlattenable::IMAGE_FILTER)) Please add braces around the bodies of if, for, while, etc.
Agree with #21 -- this review has been excellent for crystalizing some of our thinking around this issue, but it is not clear at all (yet) that we should continue with this particular approach.
On 2013/08/28 15:06:29, reed1 wrote: > Agree with #21 -- this review has been excellent for crystalizing some of our > thinking around this issue, but it is not clear at all (yet) that we should > continue with this particular approach. Is there a possibility we could land just the fuzzer part of this patch? The security folks have made this a prerequisite for using the skia serialization code in Chrome IPC.
On 2013/09/13 16:57:27, Stephen White wrote: > On 2013/08/28 15:06:29, reed1 wrote: > > Agree with #21 -- this review has been excellent for crystalizing some of our > > thinking around this issue, but it is not clear at all (yet) that we should > > continue with this particular approach. > > Is there a possibility we could land just the fuzzer part of this patch? The > security folks have made this a prerequisite for using the skia serialization > code in Chrome IPC. So, in case you guys change your mind, I updated this cl to only include the fuzzer. The bugfixes have already been committed with associated unit tests and the type checking will happen in a different cl and will possibly be different from what was proposed here.
patch #7 lgtm
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/22799007/72001
Message was sent while issue was closed.
Change committed as 11395
Message was sent while issue was closed.
Hi Robbie, Could you please take a look at this CL? It looks like the CQ submitted it without checking any of the verifiers (Tree status, Presubmit checks, etc): https://skia-tree-status.appspot.com/cq/sugoi%40chromium.org/22799007/72001
Message was sent while issue was closed.
On 2013/09/19 19:16:27, rmistry wrote: > Hi Robbie, > > Could you please take a look at this CL? It looks like the CQ submitted it > without checking any of the verifiers (Tree status, Presubmit checks, etc): > https://skia-tree-status.appspot.com/cq/sugoi%2540chromium.org/22799007/72001 Compare this with a CQ submitted CL that ran all verifiers: https://skia-tree-status.appspot.com/cq/djsollen%40google.com/23477067/13001 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
