speculative buffer overrun in SpiderMonkey

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

speculative buffer overrun in SpiderMonkey

Luis Longeri
Hi,

Last week I posted this on firefox-dev and I was advised to re-post here.

When I stumbled into the Mealtdown and Spectre exploits news I jump into
reading the papers, didn't sleep much. So I decided to see what could be
done to get better Javascript protection against speculative buffer
overruns.

I have wanted to check this out for some time, so at last I downloaded
Firefox source code and I started to figure out if some protections could
be added. This is just dev poking around and I would like to ask if I am at
least in the right direction.

I briefly when over the code and I figure that NativeObject.h seems to
declare array indexing code used at least from the Javascript interpreter.
So I made the following changes that I understand should cut short any
speculative buffer overrun at least in these patched functions. I know this
has performance issues but it is just a test.

I compiled this and I am running Firefox in safe mode (from what I gather
this disables the jit compiler which I haven't even begin to look at).
From the limited testing I have done, this code is effectively called when
indexing arrays.
Right now I am using this patch in safe mode in a test computer, so far it
works okay but of course a little slow on sites heavy on javascript like
google drive.

I would like to ask if at least I am in the right direction or am I way off
course.

I just limit the indexing using a modulo operator, since the MOZ_ASSERT
check could be delayed by the CPU by a cache miss, I am enforcing a limit
on the index so if the indexing runs speculatively it will still be within
limits prior to being discarded. These indexing functions are called from
places where the index is checked with 'if' statements but those
evaluations can also be delayed by the CPU allowing for an speculative
execution of an overflow.

diff -r f78a83244fbe js/src/vm/NativeObject.h
--- a/js/src/vm/NativeObject.h    Thu Jan 04 11:44:30 2018 +0200
+++ b/js/src/vm/NativeObject.h    Thu Jan 04 16:05:11 2018 -0300
@@ -496,11 +496,13 @@
         return HeapSlotArray(elements_, true);
     }
     const Value& getDenseElement(uint32_t idx) const {
-        MOZ_ASSERT(idx < getDenseInitializedLength());
-        return elements_[idx];
+        uint32_t len = getDenseInitializedLength();
+        MOZ_ASSERT(idx < len);
+        return elements_[idx % len];
     }
     bool containsDenseElement(uint32_t idx) {
-        return idx < getDenseInitializedLength() &&
!elements_[idx].isMagic(JS_ELEMENTS_HOLE);
+        uint32_t len = getDenseInitializedLength();
+        return idx < len && !elements_[idx % len].isMagic(JS_ELEMENTS_HOLE)
;
     }
     uint32_t getDenseInitializedLength() const {
         return getElementsHeader()->initializedLength;
@@ -1196,9 +1198,11 @@
     // objects, but should only be called in a few places, and should be
     // audited carefully!
     void setDenseElementUnchecked(uint32_t index, const Value& val) {
-        MOZ_ASSERT(index < getDenseInitializedLength());
+        uint32_t len = getDenseInitializedLength();
+        MOZ_ASSERT(index < len);
         MOZ_ASSERT(!denseElementsAreCopyOnWrite());
         checkStoredValue(val);
+        index %= len;
         elements_[index].set(this, HeapSlot::Element,
unshiftedIndex(index), val);
     }

@@ -1217,10 +1221,12 @@
     }

     void initDenseElement(uint32_t index, const Value& val) {
-        MOZ_ASSERT(index < getDenseInitializedLength());
+        uint32_t len = getDenseInitializedLength();
+        MOZ_ASSERT(index < len);
         MOZ_ASSERT(!denseElementsAreCopyOnWrite());
         MOZ_ASSERT(!denseElementsAreFrozen());
         checkStoredValue(val);
+        index %= len;
         elements_[index].init(this, HeapSlot::Element,
unshiftedIndex(index), val);
     }

Regards,
llongeri
_______________________________________________
dev-tech-js-engine mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-tech-js-engine
Reply | Threaded
Open this post in threaded view
|

Re: speculative buffer overrun in SpiderMonkey

David Rajchenbach-Teller-2


On 16/01/2018 15:16, Luis Longeri wrote:
> I just limit the indexing using a modulo operator, since the MOZ_ASSERT
> check could be delayed by the CPU by a cache miss, I am enforcing a limit
> on the index so if the indexing runs speculatively it will still be within
> limits prior to being discarded. These indexing functions are called from
> places where the index is checked with 'if' statements but those
> evaluations can also be delayed by the CPU allowing for an speculative
> execution of an overflow.

I suspect that you're not testing what you intend to test. By design,
MOZ_ASSERT code is only executed in DEBUG builds, so it's not something
that can be exploited in a Meltdown/Specter scenario.

I *think* that the code you're looking for is actually in
`NativeObject::getSlot`.

Cheers,
 David
_______________________________________________
dev-tech-js-engine mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-tech-js-engine
Reply | Threaded
Open this post in threaded view
|

Re: speculative buffer overrun in SpiderMonkey

Luis Longeri
Thanks, I suspected that MOZ_ASSERT was only for DEBUG mode, but I didn't
check it. As an assert, I understand it should never assert in bug free JS
engine, so the value of the index is already checked by the time MOZ_ASSERT
is executed in DEBUG (or skipped in production build).
But the code I patched, such as the function getDenseElement, is called for
example from HasAndGetElement or GetArrayElement (in js/src/jsarray.cpp)
such as:

if (index < nobj->getDenseInitializedLength()) {
            vp.set(nobj->getDenseElement(size_t(index)));

That IF statement is the branching that could trigger a speculative
execution of the getDenseElement function if index is greater or equal to
the initialized length.

I haven't properly debugged my changes, I only tested by adding logging to
the functions I modified to checked that some (don't recall which now) are
actually called when I access array elements from javascript in Firefox.
I don't understand yet the structure of the code and I still need to learn
testing setups, I hope to advance on this in my spare time.
I'll take a look at the core reference you mention.

Thanks,
llongeri


On Tue, Jan 16, 2018 at 12:36 PM, David Teller <[hidden email]> wrote:

>
>
> On 16/01/2018 15:16, Luis Longeri wrote:
> > I just limit the indexing using a modulo operator, since the MOZ_ASSERT
> > check could be delayed by the CPU by a cache miss, I am enforcing a limit
> > on the index so if the indexing runs speculatively it will still be
> within
> > limits prior to being discarded. These indexing functions are called from
> > places where the index is checked with 'if' statements but those
> > evaluations can also be delayed by the CPU allowing for an speculative
> > execution of an overflow.
>
> I suspect that you're not testing what you intend to test. By design,
> MOZ_ASSERT code is only executed in DEBUG builds, so it's not something
> that can be exploited in a Meltdown/Specter scenario.
>
> I *think* that the code you're looking for is actually in
> `NativeObject::getSlot`.
>
> Cheers,
>  David
>
_______________________________________________
dev-tech-js-engine mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-tech-js-engine
Reply | Threaded
Open this post in threaded view
|

Re: speculative buffer overrun in SpiderMonkey

Steve Fink-4
In reply to this post by David Rajchenbach-Teller-2
On 1/21/18 7:30 AM, Luis Longeri wrote:

> Thanks, I suspected that MOZ_ASSERT was only for DEBUG mode, but I didn't
> check it. As an assert, I understand it should never assert in bug free JS
> engine, so the value of the index is already checked by the time MOZ_ASSERT
> is executed in DEBUG (or skipped in production build).
> But the code I patched, such as the function getDenseElement, is called for
> example from HasAndGetElement or GetArrayElement (in js/src/jsarray.cpp)
> such as:
>
> if (index < nobj->getDenseInitializedLength()) {
>              vp.set(nobj->getDenseElement(size_t(index)));
>
> That IF statement is the branching that could trigger a speculative
> execution of the getDenseElement function if index is greater or equal to
> the initialized length.


Yes, that looks like an example of a spectre-vulnerable computation.

I don't think modulus is a good fix, though; you have to do a division,
which I think can take a number of cycles and tie up an ALU or division
unit. It would be better to mask, though that means calculating or
maintaining a mask value. (And it isn't precise; the attacker could
snoop nearby data.)

It looks like the bug for this is
https://bugzilla.mozilla.org/show_bug.cgi?id=1430051 which has other ideas.

_______________________________________________
dev-tech-js-engine mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-tech-js-engine
Reply | Threaded
Open this post in threaded view
|

Re: speculative buffer overrun in SpiderMonkey

Luis Longeri
Yes, modulus is a terrible waste of CPU resources, if there was a machine
code instruction that could do a>b?a:0 without speculative execution would
be great.
The solution in the link is nice and very clever, but it also requires 5
opcodes.
It would be nice to have an machine opcode such as in AMD's GPU like the
bitfield insert (BFI_INT) which is a 3 operand instruction that returns dst
= (src1 & src0) | (src2 & ~src0).

On Tue, Jan 30, 2018 at 5:05 AM, Steve Fink <[hidden email]> wrote:

> On 1/21/18 7:30 AM, Luis Longeri wrote:
>
>> Thanks, I suspected that MOZ_ASSERT was only for DEBUG mode, but I didn't
>> check it. As an assert, I understand it should never assert in bug free JS
>> engine, so the value of the index is already checked by the time
>> MOZ_ASSERT
>> is executed in DEBUG (or skipped in production build).
>> But the code I patched, such as the function getDenseElement, is called
>> for
>> example from HasAndGetElement or GetArrayElement (in js/src/jsarray.cpp)
>> such as:
>>
>> if (index < nobj->getDenseInitializedLength()) {
>>              vp.set(nobj->getDenseElement(size_t(index)));
>>
>> That IF statement is the branching that could trigger a speculative
>> execution of the getDenseElement function if index is greater or equal to
>> the initialized length.
>>
>
>
> Yes, that looks like an example of a spectre-vulnerable computation.
>
> I don't think modulus is a good fix, though; you have to do a division,
> which I think can take a number of cycles and tie up an ALU or division
> unit. It would be better to mask, though that means calculating or
> maintaining a mask value. (And it isn't precise; the attacker could snoop
> nearby data.)
>
> It looks like the bug for this is https://bugzilla.mozilla.org/s
> how_bug.cgi?id=1430051 which has other ideas.
>
>
_______________________________________________
dev-tech-js-engine mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-tech-js-engine