buffer: add Buffer.harden() method

[ChALkeR]

This is a strawperson currently, targeted at resolving Buffer controversies and providing an alternate safer Buffer API that could be opted in from the top level app code. This should both resolve potential concerns about Buffer unsafety (that are otherwise non-resolvable in general for performance reasons) and give ecosystem another nudge to migrate off unsafe deprecated Buffer(arg) API.

This adds Buffer.harden() method that:

1. Ensures all new buffers are zero-filled, even from Buffer.allocUnsafe() (similar to --zero-fill-buffers CLI flag).
2. Ensures that pooling is permanently disabled.
3. Enables runtime deprecation for deprecated Buffer(arg)
4. Freezes Buffer object.

That method is callable only from the top-level app, it will throw when called from dependencies (reuses already existing isInsideNodeModules() check for that).

Example:

> Buffer.allocUnsafe(10)
<Buffer 80 26 00 03 01 00 00 00 00 00>
> Buffer.from('sdfds').buffer
ArrayBuffer {
  [Uint8Contents]: <80 26 00 03 01 00 00 00 00 00 00 00 00 00 00 00 73 64 66 64 73 00 00 00 80 00 10 03 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ff ff ff ff 0a 00 00 00 ff ff ff ff 32 00 00 00 ff ff ff ff 38 00 00 00 80 26 00 03 01 00 00 00 00 00 00 00 00 00 00 00 38 24 81 03 01 00 00 00 00 00 00 00 ... 8092 more bytes>,
  byteLength: 8192
}
> Buffer.harden()
undefined
> Buffer.allocUnsafe(10)
<Buffer 00 00 00 00 00 00 00 00 00 00>
> Buffer.from('sdfds').buffer
ArrayBuffer { [Uint8Contents]: <73 64 66 64 73>, byteLength: 5 }
>

Unresolved:

1. Re-evaluating Buffer module could create non-hardened Buffers?
2. Child process Buffers are non hardened -- I thought about going into those by an env var, but let's keep that out of scope

/cc @nodejs/security-wg @nodejs/security @nodejs/buffer @mafintosh -- comments?

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[mscdex]

Something like this should only be activated by a command-line flag. Having it changeable during runtime via an API could cause confusion and potential breakage, especially if a third-party module calls it unexpectedly.

[ChALkeR]

@mscdex, this is deliberately not callable from a third-party module.

On the contrary – this API attempts to block modules from meddling with Buffer impl at runtime, which is something that they are currently perfectly able to do. I.e. this API could already be mostly emulated from userland with monkeypatching Buffer.

There were concerns voiced about CLI flags being less available in certain setups, and those are generally harder to use.

[vdeturckheim]

I am not fully sure about the behavior it could trigger inb a module using Buffer poorly. Do you have ideas of what could happen in such cases?

[mafintosh]

Great feature! I personally prefer a cli flag as well.
If a compiled bundle is executed I’m guessing the module detection wont work?
We use that a bunch for electron.

[ChALkeR]

@vdeturckheim Poorly in which way? It now ignores modules attempting to change Buffer.poolSize and treats that as 0 always. Also, this is expected to emit warnings on usage of deprecated Buffer() API. But other than that, I do not expect any issues in ecosystem modules.

The point is to make potential malicious code in ecosystem modules more noticable upon review and limit what it can subtly do with .buffer property (that's also @mafintosh usecase, I believe).

[vdeturckheim]

@ChALkeR I think so to, I can't find any good reason not to have this :D

[ChALkeR]

@vdeturckheim Ah, and this is an explicit opt-in.

@mafintosh About bundles -- that is something that have to be looked at, but I don't expect problems here for two reasons:

1. This is unlikely to end up in node_modules because it would normally throw there, and for it to get bundled it has to end up in node_modules
2. We have a similarly guarded runtime Buffer() deprecation.

That would require testing to at least know what would happen, though.

---

@mafintosh @mscdex About CLI flag, there are several reasons why I decided against that atm:

1. Increase HTTP_MAX_HEADER_SIZE to 16kb #27645 indicates that CLI flags are not available on certian setups.
2. CLI flags are generally less visible and harder to use, I expect that most people don't use CLI flags.
3. CLI flags are defined by the deployment, while API usage is defined by code. Thus, it is easier to somehow miss CLI flags for these things without noticing that.
4. Adding a CLI flag would be a significant chunk of extra code/docs that is too early for this strawperson imo.
5. I have hopes to perhaps add similar .harden() API to other modules, which would accept configuration object for some modules, with js callbacks that could not be passed in a CLI flag.

What could resolve all of that (except for 4) is an additional CLI flag on top of this that would require call to Buffer.harden() in the first tick and would terminate the process if that won't be done.
E.g. --require-harden-buffer or something like that.

What would solve 4 is introducing that flag in a separate follow-up PR if that is something that everyone would want.

[ChALkeR]

One more way to enforce the "only top-level code can call this" rule even further would be to make sure that this can be called only before any requires of any third-party code from node_modules.

What does everyone think about that? I can implement that if that sounds like a good idea.

Also, perhaps, during only the first tick (that would make sense for enforcement cli flags).

[mcollina]

Frozen object are slower. How much slower in this case?

[RReverser]

I don't think that's true. It used to be a long time ago, but they should be the same as regular objects these days.

[mcollina]

Some indirect tests still shows regressions.

[ChALkeR]

@mcollina I will run benchmarks on this.

But I don't expect that frozen objects would be what would causes the most of performance hit here, Buffer.harden() also enables zero-filling for allocUnsafe and disables buffer pooling completely, I expect that to me more noticable.

This whole PR comes with an (opt-in) perf hit that would be documented once I get the numbers.

The primary targets for opt-in hardening are not performance-sensitive backend setups, but pretty much everything else.

[mscdex]

1. Increase HTTP_MAX_HEADER_SIZE to 16kb #27645 indicates that CLI flags are not available on certian setups.

I don't understand this argument. Users could easily opt-in/out of a specific configuration, even for *aaS.

[sam-github]

IMO, #27645 (comment) doesn't indicate that CLI flags are hard to set from a technical perspective, but that customers are unwilling to be personally responsible for relaxing a security constraint. In this case, the flag tightens security, so I don't see it as relevant. Both CLI/ENV and API are reasonable ways to (simultaneously) expose this capability.

[Trott]

I'd want to think/talk about the name (harden) to make sure we get it right. I like the general concept.

@ChALkeR Are you still working on this? What would help get this out of draft mode? You need to run some benchmarks and look at some other things? (Anything you need/want from other people? More reviews?)

[ChALkeR]

@Trott I will update this shortly this week, adding some docs.

I am also not very sure about freezing the Buffer prototype on the second thought, that will affect child objects and make e.g. toString() non-overridable.
E.g.:

$ node
> Object.freeze(Buffer.prototype);
> const x = Buffer.from('hello');
> x.toString = () => 'no';
> x.toString()
'hello'

That has a potential of breaking things, investigating how significant is that with Buffer.
Freezing Buffer.constructor could be an overkill, the main reason for Object.freeze here was to freeze Buffer object API.

[ChALkeR]

Buffer.prototype probably won't be frozen here because of this:

function tryBuffer() {
  function Foo(x) {
    const buf = Buffer.from(x)
    Object.setPrototypeOf(buf, Foo.prototype)
    return buf
  }
  Foo.prototype = Object.create(Buffer.prototype);
  Foo.prototype.toString = () => 'bar'

  const foo = new Foo('foo')
  return foo.toString()
}

console.log(tryBuffer()) // 'bar'
Object.freeze(Buffer.prototype)
console.log(tryBuffer()) // 'foo'

[ChALkeR]

Changes:

• undo freezing Buffer.prototype,
• now throws on unsafe Buffer() API by default, but I added an option to turn that into warnings (to speed up the adoption)
• calling Buffer.harden twice throws (could indicate a user, mistake, e.g. unexpected options)

[ChALkeR]

@mscdex @sam-github About CLI flag, I would prefer if it would be added in a way that would enforce using Buffer.harden programmatically during the first tick, for the following reasons:

• Better consistency, i.e. this adds additional gurantees
• Accidentally loosing a CLI flag during deployment setup wouldn't make things worse this way.
• We can pass options this way. Not only this API uses options now, I plan to add more similar methods to other modules that would also use options.

I plan to do that as a follow-up PR once this lands, e.g. in a way of --enforce-harden=buffer.

[ChALkeR]

Tests are still missing, but I will add them.

@mscdex I considered policy files. Once again -- it will hit significantly less adoption and have less effect on the ecosystem, also harder to add callbacks for future harden policies. Also harder to deploy apps. E.g. this would be a one-liner for command-line apps published npm, and adding policy files would be complex enough so approximately zero packages would do that. Policy files would have much less security impact on the ecosystem than I want to achieve.

We could support defining this in policy files, but I still think that this API is justified even if we do that, and that we could add policy files support later, if needed.

[Trott]

There was some discussion in the TSC today with some things said that probably need to be repeated here? @ChALkeR @jasnell @mhdawson

[ChALkeR]

I just pushed an update to label this as Experimental in the docs, per the TSC meeting discussion.

[addaleax]

This should not land without tests.

I still really don’t like the artificial restrictions on when this method is callable here, and I’d also still prefer being able to set this through a CLI flag (which has another advantage, namely that Workers inherit these options by default, as they should imo).

[jasnell]

Ping @ChALkeR... still want to do this?

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[github-actions]

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.