Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid SharedArrayBuffer until required #59

Merged
merged 3 commits into from
May 3, 2021
Merged

fix: avoid SharedArrayBuffer until required #59

merged 3 commits into from
May 3, 2021

Conversation

snyamathi
Copy link
Contributor

@snyamathi snyamathi commented Apr 21, 2021

@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!

support/types.js Outdated
);
// Avoid invoking SharedArrayBuffer constructor until required, then memoize
Object.defineProperty(isSharedArrayBufferToString, 'working', {
get: (function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

support/types.js Outdated
);
// Avoid invoking SharedArrayBuffer constructor until required, then memoize
Object.defineProperty(isSharedArrayBufferToString, 'working', {
get: (function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

support/types.js Outdated
Comment on lines 248 to 249
typeof SharedArrayBuffer !== 'undefined' &&
isSharedArrayBufferToString(new SharedArrayBuffer())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@snyamathi snyamathi Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

function isSharedArrayBuffer(value) {
if (typeof SharedArrayBuffer === 'undefined') {
if (typeof SharedArrayBufferCopy === 'undefined') {
Copy link
Contributor Author

@snyamathi snyamathi Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😛

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a semver-patch to me.

@snyamathi snyamathi changed the title feat: avoid SharedArrayBuffer until required fix: avoid SharedArrayBuffer until required Apr 22, 2021
@snyamathi
Copy link
Contributor Author

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

@ljharb ljharb requested a review from lukechilds April 27, 2021 03:46
@goto-bus-stop goto-bus-stop self-assigned this Apr 29, 2021
@nikashitsa
Copy link

Looking forward for this fix 😊 (@lukechilds)

@goto-bus-stop
Copy link
Member

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

@goto-bus-stop goto-bus-stop merged commit 7d2a968 into browserify:master May 3, 2021
@snyamathi snyamathi mentioned this pull request May 6, 2021
@snyamathi snyamathi deleted the SharedArrayBuffer branch May 6, 2021 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants