XS deep freeze conflicts with SES security constraints

[erights]

The XS Object.freeze takes a second optional boolean parameter that, if truthy, causes some form of transitive freezing. But unlike harden, it does more freezing than user code can do (and inspired the petrify notion we're still designing). As currently implemented, this enhanced Object.freeze can be used for attack.

---

previously:

Moddable-OpenSource/moddable#351
Moddable-OpenSource/moddable#353

are both memory unsafety problems in XS. Let's use this issue thread to accumulate XS issues that are a security concern for us, until we start on an engineering process of securing XS.

Memory unsafety bugs are of course fully fatal for us. But all sorts of other deviations from standard ES, including deviations allowed by the ES spec, are potential concerns:

[erights]

Attn @phoddie

[phoddie]

Thanks for the nudge.

Regarding Object.freeze we are open to options. The feature exists to support preloading. It isn't strictly necessary at runtime. We could make unavailable at runtime or move it off Object so it doesn't automatically propagate into Compartments.

[dckc]

A few more relevant issues:

• CESU-8 support in XS to address UTF-8/UTF-16 confusion risk #2118 UTF-8 strings compatibility / security
• console API on XS: compatibility, security impact? #2146 console compatibility / security (pretty orthogonal to the XS engine, but included here conservatively)
• XS reports TypeError differently than Node? #1780 XS reports TypeError differently than Node?
  • deterministic error messages as discussed in Centralize Zoe and ZCF Errors #1910

[dckc]

Let's focus this issue on XS deep freeze (Object.freeze(x, true)). I tested that it's outstanding:

function freezeBadArg() {
  'use strict';

  const x = { y: { size: 2 } };
  // @ts-ignore
  Object.freeze(x, true);
  try {
    x.y.color = 'blue';
    return x.y.color;
  } catch (e) {
    return e.message;
  }
}

test('extra Object.freeze arg on node', async t => {
  t.is(freezeBadArg(), 'blue');
});

test('extra Object.freeze arg on XS with SES', async t => {
  const bootScript = await ld.asset('../dist/bundle-ses-boot.umd.js');
  const opts = options(io);
  const vat = xsnap(opts);
  t.teardown(() => vat.terminate());
  await vat.evaluate(bootScript);

  await vat.evaluate(`
    const encoder = new TextEncoder();
    const send = msg => issueCommand(encoder.encode(JSON.stringify(msg)).buffer);
    send((${freezeBadArg})());
  `);
  t.deepEqual(opts.messages, ['"blue"']);
});

I was going to close this, since security-related issues in the moddable repo (Moddable-OpenSource/moddable#351 , Moddable-OpenSource/moddable#353 ) are closed, and other stuff mentioned above is tracked elsewhere, but deep freeze is still an issue.

[dckc]

@erights remind me which SES constraint this violates? i.e. how it can be used for attack?

You explained it to me once but it has since leaked out.

[phoddie]

I'm not certain that our extension to Object.freeze changes the security properties of the JavaScript language. The optional behavior implemented effectively performs Object.freeze recursively but does not change what is frozen. For example, private properties are not frozen.

Here's an example to check my understanding:

class Foo {
	#x = 12;
	y = {z: 12};
	
	set x(value) {
		this.#x = value;
	} 
	get x() {
		return this.#x;
	} 
}

let f = new Foo;
Object.freeze(f, true);
f.x = 11;
trace(f.x, "\n");
f.y.z = 12;

Setting f.x succeeds, with the following trace outputting 11 . Setting f.y.z throws because z is not writable.

Even if it is "safe", this extension to Object.freeze is not beneficial to Agoric's use of XS. Perhaps we should make it an optional feature that can be compiled in/out at build time, so that it doesn't need to be a consideration in the runtime?

Note: When building a ROM image, the XS linker does perform a deeper freeze that goes beyond what can be expressed by the JavaScript language today. But, that feature of XS is not used by the Agoric runtime.

[dckc]

...
Even if it is "safe", this extension to Object.freeze is not beneficial to Agoric's use of XS. Perhaps we should make it an optional feature that can be compiled in/out at build time, so that it doesn't need to be a consideration in the runtime?

That seems cost-effective. Yes, please, @phoddie

[erights]

I would love to understand its semantics. If it is approximately harden or purify, it may be great for us! Though we would package it differently.

[erights]

I removed the MN-1 label because this does not affect MN-1.

[erights]

Oh, I should have noticed @dckc that you added the MN-1 label recently. If I'm missing something, please add it back in and let me know. Thanks.

[dckc]

Right... It's more MN-3.

[Tartuffo]

@kriskowal This does not have an area label that is covered by our weekly tech / planning meetings. Can you assign the proper label? We cover: agd, agoric-cosmos, amm, core economy, cosmic-swingset, endo, getrun, governance, installation-bundling, metering, run-protocol, staking, swingset, swingset-runner, token economy, wallet, zoe contract.

[dckc]

Again, I suggest xsnap is covered in weekly endo / SES meetings.

[Tartuffo]

Agreed, I just added it to the ZH filter for the Zoe/ERTP meeting.