1325647 - Add IPDL mechanism to automatically generate bound checking code for IPC messages

Status: NEW

(Core :: Security: Process Sandboxing, defect)

Description

[Julian Hector [:tedd] [:jhector]]

Following the discussion from the sandbox meeting on Friday in Hawaii.

The idea is to be able to specify a valid range of possible integer values inside the IPDL files and generate code based on that information that performs the bound checking.

Having an automated range check when a message is received eliminates potential security vulnerabilities when a value is sent that is not within the expected range (for example by a compromised child).

Comment 1

[Julian Hector [:tedd] [:jhector]]

Attachment: [v1] PoC - automated bound checking using IDPL

This is my first attempt for introducing the above mentioned mechanism. It is very basic and hackish, but illustrates the approach.

This patch generates the following range checking code:
> if (((id) < (static_cast<int>(0))) || ((id) > (static_cast<int>(20)))) {
>    FatalError("Error 'id' value not within bounds");
>    return MsgValueError;
> }

inside the generated PTestBoundCheckParent.cpp file.

Comment 2

[Julian Hector [:tedd] [:jhector]]

Paul, should we track this kind of work under Bug 1287730 or rather Bug 925570. Or create a new meta bug for 'securing the sandbox' kind of work? Which all the improvements/issues :huseby found would fall under?

Comment 3

[Julian Hector [:tedd] [:jhector]]

Sorry, forgot to put you on ni?, see Comment 2

Comment 4

[Julian Hector [:tedd] [:jhector]]

From what I can tell we have the following 'integer' types to worry about:

> uint64_t, int64_t, uint32_t, int32_t, uint16_t, int16_t, uint8_t, int8_t, int, short, long

The first PoC version patch only works with integers defined in the parameter list of the messages, it does not include integers as part of a struct defined inside the IPDL file.

To make it more user-friendly, we could add a check that boundaries are only given for the above mentioned types (the same idea could be applied to other types, but I don't know of any that could benefit from this).

Also, a less than 0 check is not required for any unsigned types and only leads to compiler warnings, so in theory we could only do the upper bound check.

Another feature could be to have an inclusive and an exclusive boundary, i.e. [0, 20] means everything between 0 and 20 also including 0 and 20, whereas (10, 15] means everything from 10 to 15 including 15 but excluding 10. But I don't know if that would lead to too much confusion.

Paul what do you think?

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Julian Hector [:tedd] [:jhector] from comment #2)
> Paul, should we track this kind of work under Bug 1287730 or rather Bug
> 925570. Or create a new meta bug for 'securing the sandbox' kind of work?
> Which all the improvements/issues :huseby found would fall under?

1287730 works.

Comment 6

[Julian Hector [:tedd] [:jhector]]

Attachment: [v2] PoC - automated bound checking using custom types

After getting back some feedback from :billm (via email) here a new approach, this patch introduces a new helper serializer for signed/unsigned integers.

The goal is to achieve the same bound checking as proposed in the previous patch, with the difference that it is entirely inside the type system and the IDPL file doesn't need to be touched.

:billm, is this what you were talking about in the email?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8826112 [details] [diff] [review]
[v2] PoC - automated bound checking using custom types

Review of attachment 8826112 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this is what I was thinking of. Seems nice.

Comment 8

[Julian Hector [:tedd] [:jhector]]

Attachment: Add signed/unsigned integer serializer helpers

Proposing your idea as a patch and if we can land this, I will try to get developer to migrate to these types if is possible.

Comment 9

[Julian Hector [:tedd] [:jhector]]

Attachment: DO NOT COMMIT - example code on how to use the new serialize helpers

This patch illustrates how to use added helper serializer.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8830726 [details] [diff] [review]
Add signed/unsigned integer serializer helpers

Review of attachment 8830726 [details] [diff] [review]:
-----------------------------------------------------------------

I'm sorry, but I was thinking this over more and I don't think it works. The problem is that you're really just providing a ParamTraits specialization for a type that's typedefed to uint32_t (or some other integer type). The compiler doesn't recognize the difference between TestArrayIdx and uint32_t, so ParamTraits<uint32_t> is the same as ParamTraits<TestArrayIdx>. I think you'll just get multiple definition errors. Did you try to compile this with a couple of examples?

Probably a better way to go is to create a wrapped integer type, maybe BoundedInteger<A, B>, that is guaranteed to hold an integer between A and B. Then you could define ParamTraits for BoundedInteger. And you could typedef TestArrayIdx to BoundedInteger<10> or whatever. You can add conversion operations between integers and BoundedInteger so that the class is easy to use.

Sorry again for not noticing this the first time around.

Comment 11

[Julian Hector [:tedd] [:jhector]]

Thank you for the reply (sorry for my late response).

I see what you mean, I will work out a new PoC and also do some testing with mixing the typedef type and regular integers.

Comment 12

[Julian Hector [:tedd] [:jhector]]

Attachment: Add signed/unsigned integer serializer helpers

Had to add a Log function, otherwise it threw an error while compiling.

Comment 13

[Julian Hector [:tedd] [:jhector]]

Attachment: DO NOT COMMIT - example code on how to use the new serialize helpers

:billm, so I added some code to the example usage, also including a uint32_t parameter in the message declaration. This code compiled fine for me, and it didn't complain about multiple definitions.

However, thinking about your suggestion of an extra class, which is guaranteed to hold an integer between A and B, this could also be beneficial beyond just receiving that type.

Let's assume the child passes an index for an array to the parent, having the helper templates like in the patch from Comment 12 would guarantee that the received integer is within its bounds, but any further modifications of that type could lead to an out of bounds.

So something like this:
> TestArrayIdx iterator = idx; // idx being the received from the child
> iterator += 20;

since TestArrayIdx is typedef'ed to uint32_t, the value of |iterator| can be out of bounds.

So having a class for bounded integer would be useful to guarantee a value within its bounds even when the variable is further modified inside the code.

I will work on a PoC and report back in this bug.

Comment 14

[Julian Hector [:tedd] [:jhector]]

I had a longer discussion about this with :ckerschb, implementing a class for a bounded integer would also mean to overload all possible operators, which are about 40 in order to guarantee that the integer value never exceeds its boundaries.

Which is totally doable, but a lot of effort and I want to make sure if we want something and something like this can also land, otherwise I would just be wasting my time. We would probably also need to implement nsISupport, so we can have reference counted objects and the likes.

As far as the use cases go, I clearly see a benefit for integers used as an array index to a fixed size array, or integers giving information of width or height. It would also make the work for auditing more easy, because an auditor can skip checking whether or not bound checking is done correctly for an integer when the newly implemented type is used.

I would like to work on this, but don't want to waste my time if at the end it won't land because there is no interest.

The reviewer must also pay special attention to the implementation of the overloaded operators etc.

:billm, what do you think? What is your opinion?

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Julian Hector [:tedd] [:jhector] from comment #13)
> Created attachment 8833862 [details] [diff] [review]
> DO NOT COMMIT - example code on how to use the new serialize helpers
> 
> :billm, so I added some code to the example usage, also including a uint32_t
> parameter in the message declaration. This code compiled fine for me, and it
> didn't complain about multiple definitions.

The case where I think you'll have trouble is where you want to have two different bounds-checked integer types. For example:

typedef uint32_t TestArrayIdx1;
typedef uint32_t TestArrayIdx2;

template<>
struct ParamTraits<TestArrayIdx1>
  : public UnsignedIntegerSerializer<TestArrayIdx1, 19>
{};

template<>
struct ParamTraits<TestArrayIdx2>
  : public UnsignedIntegerSerializer<TestArrayIdx2, 23>
{};

I think you will get a compile error here because TestArrayIdx1 and TestArrayIdx2 are the same type, so you can't define two different ParamTraits specializations for them.

Regarding the operator overloading, I was assuming that you would only define coercion operators for these types. Something like this:

class TestIdx {
public:
  MOZ_IMPLICIT TestIdx(uint32_t value) : mValue(value) {}
  MOZ_IMPLICIT operator uint32_t() const { return mValue; }

private:
  uint32_t mValue;
};

You wouldn't be able to do things like |x += 12;|. But this class would only be used for arguments to IPDL messages, so you wouldn't need to do that anyway.