723244 - RIL: implement parcel length guards

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

We have a guard in place for when parcel handling functions don't consume enough data from the buffer, but I'd also like to put guards in place for when they get confused and want to read more than the parcel's length provides.

This should be pretty simple: before we dispatch to the parcel handling function (one of the REQUEST_* or UNSOLICITED_* functions), inform Buf of the length. Then every time we read a value (it all goes through Buf.readUint8), decrement that number and raise when we hit negative values.

Comment 1

[Josh Matthews [:jdm]]

Could you provide an MXR link for where somebody interested should start looking?

Comment 2

[Philipp von Weitershausen [:philikon]]

Certainly. The best place to remember the length would be in Buf.processParcel() [1] just before we hand off to RIL.handleParcel(). Length checks + increments should happen in Buf.readUint8() since that's the basis to all other functions. Just after calling RIL.handleParcel() in [1], we want to reset all that state.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/system/b2g/ril_worker.js#465
[2] https://mxr.mozilla.org/mozilla-central/source/dom/system/b2g/ril_worker.js#193

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

I would like to take this bug if no one is currently working on it :)

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang from comment #3)
> I would like to take this bug if no one is currently working on it :)

Go for it!

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Attachment: quick implement, not yet verified

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 595351 [details] [diff] [review]
quick implement, not yet verified

Thanks for the patch! Unfortunately, this won't work. We can't use Buf.readIncoming to implement the guard that I was talking about because we could easily receive more than just one parcel at a time, and Buf.readIncoming doesn't know about parcel borders. We need to do this as a separate flag, like I pointed out in comment 2. 

>diff --git a/dom/system/b2g/ril_worker.js b/dom/system/b2g/ril_worker.js
>--- a/dom/system/b2g/ril_worker.js
>+++ b/dom/system/b2g/ril_worker.js
>@@ -188,19 +188,27 @@ let Buf = {
> 
>   /**
>    * Functions for reading data from the incoming buffer.
>    *
>    * These are all little endian, apart from readParcelSize();
>    */
> 
>   readUint8: function readUint8() {

We're going to add some pretty drastic behaviour change to readUint8 (it can now throw) and the relationship with the other read*() functions. So we should add a sentence to that comment, pointing out that readUint8() needs to be the base implementation for all read*() functions. E.g.

  readUint8() implements parcel length guards, preventing parcel handlers from reading
  beyond the parcel end. All other read functions should therefore use readUint8() internally.

>-    let value = this.incomingBytes[this.incomingReadIndex];
>-    this.incomingReadIndex = (this.incomingReadIndex + 1) %
>-                             this.INCOMING_BUFFER_LENGTH;
>+    let value;
>+
>+    if (this.readIncoming) {
>+        value = this.incomingBytes[this.incomingReadIndex];
>+        this.incomingReadIndex = (this.incomingReadIndex + 1) %
>+                                 this.INCOMING_BUFFER_LENGTH;
>+        this.readIncoming--;

Mozilla coding style nit: Please use two spaces for indention.

>+    } else {
>+        throw "no data available";
>+    }

Please use a more descriptive error message (and also, please capitalize and use punctuation), e.g. "Trying to read data beyond the parcel end!" Also, please throw an Error object, it gives us access to the stacktrace:

  throw new Error("Trying to read data beyond the parcel end!");

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> Comment on attachment 595351 [details] [diff] [review]
> quick implement, not yet verified
> 
> Thanks for the patch! Unfortunately, this won't work. We can't use
> Buf.readIncoming to implement the guard that I was talking about because we
> could easily receive more than just one parcel at a time, and
> Buf.readIncoming doesn't know about parcel borders. We need to do this as a
> separate flag, like I pointed out in comment 2. 
That's what lines 420 ~ 424 in patched file were meant to fix. The patch was first implemented with one additional flag and reduced modified lines to what it looks now. To check whether there is still data available for reading in readUint8(), we might simply check whether incomingReadIndex equals to incomingWriteIndex. But in order to check parcel boundary, some flag carrying that information must be added. Besides, readParcelSize() calls readUint8(), too. There can't be parcel size information before it is read from readUint8(). That means readUint8() should not refer to some flag that has only meaning of parcel size.

It's the time that Buf.readIncoming should show up. It originally carries the information of how much data is available in the buffer. This fits readParcelSize() with the reason mentioned above. It also decrease by the size of data read in processIncoming(). This is also one of the steps to be implemented into readUint8(). It looks like readIncoming is the best candidate for the final implementation, only except that "it originally carries the information of how much data is available in the buffer". The lines 420 ~ 424 assigns Buf.currentParcelSize to readIncoming right before diving into processParcel() and restore it back after. During the life time of processParcel(), the meaning of readIncoming in readUint8() is "the size of available data in the parcel". This solves the point in comment #2.

> >diff --git a/dom/system/b2g/ril_worker.js b/dom/system/b2g/ril_worker.js
> >--- a/dom/system/b2g/ril_worker.js
> >+++ b/dom/system/b2g/ril_worker.js
> >@@ -188,19 +188,27 @@ let Buf = {
> > 
> >   /**
> >    * Functions for reading data from the incoming buffer.
> >    *
> >    * These are all little endian, apart from readParcelSize();
> >    */
> > 
> >   readUint8: function readUint8() {
> 
> We're going to add some pretty drastic behaviour change to readUint8 (it can
> now throw) and the relationship with the other read*() functions. So we
> should add a sentence to that comment, pointing out that readUint8() needs
> to be the base implementation for all read*() functions. E.g.
> 
>   readUint8() implements parcel length guards, preventing parcel handlers
> from reading
>   beyond the parcel end. All other read functions should therefore use
> readUint8() internally.
> 
> >-    let value = this.incomingBytes[this.incomingReadIndex];
> >-    this.incomingReadIndex = (this.incomingReadIndex + 1) %
> >-                             this.INCOMING_BUFFER_LENGTH;
> >+    let value;
> >+
> >+    if (this.readIncoming) {
> >+        value = this.incomingBytes[this.incomingReadIndex];
> >+        this.incomingReadIndex = (this.incomingReadIndex + 1) %
> >+                                 this.INCOMING_BUFFER_LENGTH;
> >+        this.readIncoming--;
> 
> Mozilla coding style nit: Please use two spaces for indention.
Sorry, my fault.

> >+    } else {
> >+        throw "no data available";
> >+    }
> 
> Please use a more descriptive error message (and also, please capitalize and
> use punctuation), e.g. "Trying to read data beyond the parcel end!" Also,
> please throw an Error object, it gives us access to the stacktrace:
> 
>   throw new Error("Trying to read data beyond the parcel end!");
Ok, I'll fix that.

BTW, I don't have try-server account yet and will apply for one and learn to follow mozilla reviewing rules. Thanks for your quick reply :)

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang from comment #7)
> During the life time
> of processParcel(), the meaning of readIncoming in readUint8() is "the size
> of available data in the parcel". This solves the point in comment #2.

This is too clever for my taste. I don't like changing the meaning of readIncoming like that. Please use a separate variable to track just the amount of bytes read from the current parcel. It will be *much* clearer.

> BTW, I don't have try-server account yet and will apply for one and learn to
> follow mozilla reviewing rules. Thanks for your quick reply :)

That's good, but you should note that the RIL worker stuff doesn't have any unit tests yet (bug 710092), so the try server won't help you much. Please verify your patches on the phone.

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Philipp von Weitershausen [:philikon] from comment #8)
> (In reply to Vicamo Yang from comment #7)
> > During the life time
> > of processParcel(), the meaning of readIncoming in readUint8() is "the size
> > of available data in the parcel". This solves the point in comment #2.
> 
> This is too clever for my taste. I don't like changing the meaning of
> readIncoming like that. Please use a separate variable to track just the
> amount of bytes read from the current parcel. It will be *much* clearer.
Ok. Then what if I create a new flag Buf.readAvailable, which is assigned to non-zero whenever parcel size is available. And fork readUint8() as:

  readUint8Unchecked: function readUint8Unchecked() {
    // original readUint8()
  }

  readUint8: function() {
    if (!this.readAvailable) {
      // throw ...
    }
    return readUint8Unchecked();
  }

The readUint8Unchecked() will be referenced in readParcelSize().

> > BTW, I don't have try-server account yet and will apply for one and learn to
> > follow mozilla reviewing rules. Thanks for your quick reply :)
> 
> That's good, but you should note that the RIL worker stuff doesn't have any
> unit tests yet (bug 710092), so the try server won't help you much. Please
> verify your patches on the phone.
We have internal training for debug tools today. I'll see if I can find some way to add some test scripts.

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang from comment #9)
> Ok. Then what if I create a new flag Buf.readAvailable, which is assigned to
> non-zero whenever parcel size is available. And fork readUint8() as:
> 
>   readUint8Unchecked: function readUint8Unchecked() {
>     // original readUint8()
>   }
> 
>   readUint8: function() {
>     if (!this.readAvailable) {
>       // throw ...
>     }
>     return readUint8Unchecked();
>   }
> 
> The readUint8Unchecked() will be referenced in readParcelSize().

Excellent idea! Make it so. :)

Comment 11

[Philipp von Weitershausen [:philikon]]

I just discovered this during a casual read of the ril_worker.js code:

  processParcel: function processParcel() {
    let response_type = this.readUint32();
    let length = this.readIncoming - UINT32_SIZE;

That last line there is totally wrong. It should be

    let length = this.currentParcelSize - UINT32_SIZE;

Please be sure to include this fix in your patch, Vicamo. Thanks!

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

I've removed all this lines. The additional variable 'length' does exactly the same thing with 'this.readAvailable'.

Comment 13

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang from comment #12)
> I've removed all this lines. The additional variable 'length' does exactly
> the same thing with 'this.readAvailable'.

Ok. I'm curious to see your patch :) Can you upload it?

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

With great help from Thinker, I have some xpcshell based tests by now. But I found I should have hooks for handleParcel() instead of testing equality of some internal vars after processIncoming(). I'll upload a more complete version after b2g meeting today. And, it will still not come with try server reports for I don't have the permission yet. Thanks.

Comment 15

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang from comment #14)
> With great help from Thinker, I have some xpcshell based tests by now.

Nice. We should integrate that with bug 710092 once get that landed. Let's focus on the implementationf or now.

> But I
> found I should have hooks for handleParcel() instead of testing equality of
> some internal vars after processIncoming(). I'll upload a more complete
> version after b2g meeting today. And, it will still not come with try server
> reports for I don't have the permission yet. Thanks.

Like I said earlier, the try server results are useless since it doesn't even build the RIL code.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part1: Implement Parcel Read Guard

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part2: Add testscripts

You may verify this patch with:

  $ cd $(MOZ_OBJDIR); make -C dom/system/b2g/test xpcshell-tests

Comment 18

[Philipp von Weitershausen [:philikon]]

Comment on attachment 597291 [details] [diff] [review]
Part2: Add testscripts

Yay for writing tests! But I would much rather prefer the approach that was started in bug 710092. Perhaps you could integrate these tests with that? Thanks!

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Do you mean waiting for bug 710092 being landed and re-upload a new one?

Comment 20

[Philipp von Weitershausen [:philikon]]

Comment on attachment 597290 [details] [diff] [review]
Part1: Implement Parcel Read Guard

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

很好的! Very nice patch. r=me

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part2: Add testscripts, V2

Integrate patch template from attachment 592188 [details] [diff] [review] of bug 710092. Note that changes made to dom/system/b2g/tests/ril_worker_wrapper.js, dom/system/b2g/tests/test_ril_worker_messages.js and dom/telephony/Makefile.in in that patch are not included for they are not required to this patch.

Comment 22

[Philipp von Weitershausen [:philikon]]

Comment on attachment 597694 [details] [diff] [review]
Part2: Add testscripts, V2

Very nice patch!

>+/**
>+ * Create a parcel suitable for postRILMessage().
>+ *
>+ * @return an Uint8Array carrying all parcel data.
>+ *
>+ * @note fakeParcelSize will be written to parcel size field to test
>+ * incorrect parcel reading. With negative value, it will be replaced
>+ * with the correct one determined by num of the arguments.
>+ */

Please document the parameters.

>+function newIncomingParcel(fakeParcelSize, response, request) {
>+  let realParcelSize = arguments.length - 3 + 2 * /*UINT32_SIZE*/4;
>+  let buffer = new ArrayBuffer(realParcelSize + /*PARCEL_SIZE_SIZE*/4);

Please const those values and use them here instead of these ugly comments.

>+  // write parcel data
>+  for (let ii = 3; ii < arguments.length; ++ii) {
>+    writeUint8(arguments[ii]);
>+  }

I would prefer to use an explicit `data` parameter instead of using the rest of `arguments`. It can be a simple JS array.

>+    if (!parcel) {
>+      parcel = newIncomingParcel(-1,
>+          worker.RESPONSE_TYPE_UNSOLICITED,
>+          worker.REQUEST_REGISTRATION_STATE,
>+          0, 0, 0, 0

Nit: indent all parameters the same way, e.g.:

      parcel = newIncomingParcel(-1,
                                 worker.RESPONSE_TYPE_UNSOLICITED,
                                 worker.REQUEST_REGISTRATION_STATE,
                                 [0, 0, 0, 0]);

>+add_test_incoming_parcel(null, function (worker) {

Please name your test function, e.g.:

  add_test_incoming_parcel(null, function test_the thing_with_the_other_thing(worker) {

>+    do_print("Test normal buffer read ...");
>+
>+    do_check_throws(function (worker) {
>+        // reads exactly the same size, should not throw anything.
>+        worker.Buf.readUint32();

Nit: please indent by 2 spaces always. Please fix this here and everywhere else.

r=me with these nits addressed. Please upload a new patch, I will check it in.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part2: Add testscripts, V3

Comment 24

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d17d2dc9754
https://hg.mozilla.org/integration/mozilla-inbound/rev/d25397e84645

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9d17d2dc9754
https://hg.mozilla.org/mozilla-central/rev/d25397e84645