fix: avoid SharedArrayBuffer until required

[snyamathi]

@goto-bus-stop

We are auditing uses of SharedArrayBuffer per https://developers.google.com/search/blog/2021/03/sharedarraybuffer-notes which is a vector for some timing attacks https://blog.mozilla.org/security/2018/01/03/mitigations-landing-new-class-timing-attack/

One of the uses of SharedArrayBuffer on our site is from this module.

The type check for isSharedArrayBufferToString has an eager calculation for if SharedArrayBuffer exists which invokes the constructor, triggering the warning. Simply calling require('util'); is enough to trigger the warning.

This PR moves that eager calculation to a lazy, but memoized, calculation which would prevent the warning unless there is some code explicitly calling isSharedArrayBuffer. If there is such code, I would certainly like to know that specifically, rather than just knowing every time someone has included the util module.

This does add some overhead / lines of code with the memoized function so I can understand any reservations, but this would make auditing sites a heck of a lot easier.

Thanks!

[snyamathi]

I wrote this as a getter to keep the same API of <function_name>.working - however this could be re-written any number of ways.

[ljharb]

a getter limits backwards compatibility, and also makes things slow. it would be ideal to avoid them.

[ljharb]

Note that there's nothing inherently insecure about SAB being accessed, it's whether the feature exists that's a problem. Meaning, a security audit that tries to identify code that's using SAB is failing to identify timing attacks, because there's all sorts of ways attackers could evaluate their own code to take advantage of SAB.

The safest thing you could do is, at the top of your program, run delete window.SharedArrayBuffer, and then no amount of JS code can run these sort of attacks (without reaching into an iframe).

[ljharb]

a getter limits backwards compatibility, and also makes things slow. it would be ideal to avoid them.

[ljharb]

the problem with this approach is that later-run code can delete SharedArrayBuffer and cause this check to fail, incorrectly.

in other words, the check must be done eagerly, on first load. It's totally fine, however, to cache SharedArrayBuffer somehow at module level, and only do the constructor check when the function is invoked, if that would help fix the false positive.

[snyamathi]

@ljharb I see what you're saying with regard to deleting the global SharedArrayBuffer. Any buffers created between the time we initialize util and when we delete SharedArrayBuffer would be reported incorrectly.

Would that same later-run code deleting SharedArrayBuffer not also affect the current implementation because of these lines here though?

node-util/support/types.js

Lines 245 to 248 in 4b1c0c7

	function isSharedArrayBuffer(value) {
	if (typeof SharedArrayBuffer === 'undefined') {
	return false;
	}

var util = require('./node_modules/util');  // This is the currently published util module
var buffer = new SharedArrayBuffer();
delete SharedArrayBuffer;
util.types.isSharedArrayBuffer(buffer)
// false

We can fix both that issue and the one that I was going to introduce by doing as you suggested and storing a copy of SharedArrayBuffer (as the oh-so-creatively-named SharedArrayBufferCopy) which fixes the case mentioned above.

var util = require('.') // this is the PR's version of the util module
var buffer = new SharedArrayBuffer();
delete SharedArrayBuffer;
util.types.isSharedArrayBuffer(buffer);
// true

---

And I agree that there is nothing inherently dangerous about SharedArrayBuffer, this PR is purely to help auditing and is no way suggesting that how it's used in this repo is dangerous.

[snyamathi]

So after this change, we would check calls at some later time against the copy of SharedArrayBuffer which existed initially. Even if someone deleted SharedArrayBuffer we would still return correctly for any buffers created before SharedArrayBuffer was deleted.

.. it does look ugly though 😛

[ljharb]

Seems like a semver-patch to me.

[snyamathi]

Thank you for the review - any next steps to get this published? Please let me know how I can help and thanks.

[nikashitsa]

Looking forward for this fix 😊 (@lukechilds)

[goto-bus-stop]

Thanks for the PR and thanks @ljharb for the review :) I'll get this released as soon as I can