buffer: runtime-deprecate Buffer constructor everywhere by default

[seishun]

Creating a new PR per the vote in #15346 (comment).

Some backstory:

• In Node.js 6.0.0, new Buffer APIs (Buffer.from(), Buffer.allocUnsafe() and Buffer.alloc()) were introduced to address security concerns of the Buffer constructor, which was deprecated in the docs in the same version. (see buffer: add Buffer.from(), Buffer.alloc() and Buffer.allocUnsafe(), soft-deprecate Buffer(num) #4682, Buffer(number) is unsafe #4660)
• In Node.js 8.0.0, a "pending deprecation" for the Buffer constructor was introduced. When Node.js is launched with the --pending-deprecation flag, the use of Buffer() and new Buffer() causes a DeprecationWarning to be emitted. (see src, buffer: add --pending-deprecation flag #11968)
• It was proposed to revisit this and possibly move the warning from behind the flag in Node.js 9.0.0. (see buffer: discuss future direction of Buffer constructor API #9531 (comment), buffer: discuss future direction of Buffer constructor API #9531 (comment)).
• In Node.js 10.0.0, unconditional runtime deprecation for the Buffer constructor was introduced for code outside of node_modules, with a plan to enable it for all code in a "later release". (see buffer: do deprecation warning outside node_modules #19524 (comment)).

Several reasons for enabling runtime deprecation by default, in no particular order:

• Potentially prevents possible DoS vulnerabilities caused by accidentally calling Buffer(num) instead of Buffer(string) due to insufficient input validation.
• Buffer(num) zero-fills by default since 8.0.0, preventing accidental data disclosure. (see buffer: zero fill Buffer(num) by default #12141) However, it was not backported to earlier Node.js versions, which could result in security issues if new code is written with the assumption of zero-filling and then run on Node.js versions that don't zero-fill. (this and above is described in more detail here)
• Some developers might be unaware that Buffer(num) zero-fills and still be using it in performance-critical code, where Buffer.allocUnsafe() might be more appropriate.
• Calling Buffer() without new would be runtime-deprecated as well, so this functionality could potentially be removed in a later Node.js version. This would make it possible to refactor Buffer into a proper ES6 class that can be subclassed.
• Following "pending deprecation" with actual runtime deprecation will create a precedent that will cause more people to use the --pending-deprecation flag to test their code in the future. This will in turn allow us to introduce new runtime deprecations more smoothly by going through the "behind-the-flag" stage first.
• Eventually, most code will use the new Buffer API, which will make it less likely that people new to Node.js will ever need to learn about the deprecated API.

Reasons why runtime deprecation only outside of node_modules is insufficient:

• Many modules are not actively tested on the latest Node.js versions, which means the problematic code in them will never be fixed, since no users will see and report the warning.
• Many applications and modules depend on old module versions, which means they'll continue running with problematic code even if all dependencies have been fixed long ago.

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

[nodejs-github-bot]

@seishun build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/79/pipeline

[apapirovski]

This has to happen at some point, IMO. 👍

[addaleax]

I’m sorry if you saw this coming, but I’m -1 on this.

• The changes in v10.x make it a lot less likely that new code relying on the Buffer constructor will be introduced
• v6.x is in maintenance mode; by the time we release this, it will have only half a year left until EoL, so I don’t think we should consider the lack of zero-filling on that release line as an issue (and we could probably introduce it at this point anyway)
• For those who do care about the DoS vector, --pending-deprecation is a sensible way of detecting potentially problematic usage

[seishun]

Then I guess this will go back to the TSC agenda.

[Trott]

@seishun If you meant to put it on the @nodejs/tsc agenda, please add the tsc-agenda label. You added tsc-review which is just for "Hey, TSC, weigh in on this in the issue tracker and pay attention to this!" If that's what you meant to do, then perfect. :-D

I have mixed feelings about putting this on the agenda at this time, but I (and anyone else) can't stop it, so go for it if you think that's the right thing to do. One thing I expect will be wanted before a decision is made is some idea (from gzemnid or elsewhere) as to whether usage is declining or not now that we have the warning for code outside of node_modules. Of course, we still run into the "no matter which side you're on, the data will argue for your point of view" problem:

• If someone thinks we should do this, then declining usage means this won't be too disruptive to do!

• If someone thinks we should not do this, then declining usage means we don't have to do it because what we've done is enough for now at least. Usage is declining!

• If someone thinks we should do this and usage is steady or increasing, that means we must do this because OMG usage is increasing! Ack!

• If someone does not think we should do this and usage is steady or increasing, that means we should not do this because the burden of bug reports for maintainers is too much. It's especially unfair if they are using Buffer constructor safely/correctly.

Not sure what to do about that conundrum but I suppose that's not exactly your problem to solve. :-D

Maybe the rule should be: If someone asks for data, they need to state beforehand how the different possible trends will affect their decision.

[seishun]

@Trott It was just a prediction. I was hoping that this could get resolved without actually putting it on the agenda, but it seems unavoidable now given the lack of activity.

[mcollina]

I am -1 for the time being. I suspect the new npm audit command is helping a lot in fixing those issues. The warning is reported doing npm install, so the user is warned. @Trott <https://github.com/Trott> It was just a prediction. I was hoping

…

that this could get resolved without actually putting it on the agenda, but it seems unavoidable now given the lack of activity. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#21351 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL418ImkaM7D2PWZmVfpr5QF3YLAmhks5t_T2QgaJpZM4UphOs> .

[seishun]

I suspect the new npm audit command is helping a lot in fixing those issues. The warning is reported doing npm install, so the user is warned.

npm doesn't warn about Buffer usage.

[mcollina]

Most of the old packages had a vulnerability reported and a CVE for this. At least those with the highest download numbers. Il giorno sab 23 giu 2018 alle 11:30 Nikolai Vavilov < notifications@github.com> ha scritto:

…

I suspect the new npm audit command is helping a lot in fixing those issues. The warning is reported doing npm install, so the user is warned. npm doesn't warn about Buffer usage. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#21351 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL46i1ofFTGVP8s2uS7PcsX48piaGqks5t_ibfgaJpZM4UphOs> .

[ChALkeR]

@seishun, @nodejs/tsc
I think that it's not correct to make a decision now.

This is targeting Node.js 11.0 at least, as it's a semver-major.

I think that it's too early for a decision at this point, as 4.x reached EOL just recently, and people are currently actively dropping support for it. But last time I checked, Buffer( usage was still pretty high, e.g. yarn is having a fun time porting off it (see TritonDataCenter/node-sshpk#46 for context).

I.e. I don't think that we should decide now will it go into 11.0 or not.

I would prefer to wait some more time closer to 11.0 release to see how the ecosystem looks like.
Somewhere around the first half of August, probably.

---

@addaleax

The changes in v10.x make it a lot less likely that new code relying on the Buffer constructor will be introduced

That doen't remove the benefits here. There is a large number of efficiently unmaintained modules at npm that would not update and that people would not migrate off unless we land this (nobody cares about unmaintained code in deps), and that would take too long for someone to review if their usage of Buffer is safe or not.

[ChALkeR]

@mcollina

I suspect the new npm audit command is helping a lot in fixing those issues.

npm audit warns only of those modules that someone (mostly I) reviewed and specifically found unsafe, and reported to the Node.js SWG.

I am doing this entirely in my free time, and I target high-impact modules (i.e. sort by popularity).
No one ever checked the dangereous code in less popular modules afaik, and npm does not magically do it by itself.

And what we have currently won't force the less maintained modules to update, it's just preventing the growth of the problem somewhat.

[mhdawson]

This was discussed in the TSC meeting today. The consensus was that we should follow @ChALkeR suggestion and wait until ~August to review the state of the ecosystem at that point and make a decision.

Will remove from TSC agenda, if still necessary, please re-add when the time comes.

[ChALkeR]

I updated the data. Everyone involved should have access, buffer-ecosystem-usage repo, ping me if you are not included.

Analysis

Depecated Buffer() constructor usage seems to have only grown from February when measured by packages count, but downloads/month fell down by 30%, though — so the increase in packages count is just caused by a large number of new (yet) unpopular modules.

Also, that growth in modules count using Buffer() constructor is ~30% less than the same growth over the same period last year.

Usage in downloads/month has decreased, but that decrease is not big enough and there are large and widely used modules that are effectively unmaintained and do not even land PRs to fix Buffer usage for a long time. E.g., Joyent-owned, e.g: TritonDataCenter/node-asn1#30 and TritonDataCenter/node-http-signature#67. http-signature seems to be the most-downloaded module that is still hitting the old API. Perhaps someone could ping Joyent about those? I guess I would be able to take a look at other cases…

Personal opinion

As it looks now, it seems unlikely to me that it would be possible to completely runtime-deprecate Buffer constructor in 11.0 without a major fallout. My opinion re: 11 will change if the usage significantly decreases over the next month, but I find that unlikely.

Even if we don't land this in time for v11, I hope that we make at least some change to speed up the migration, something that will include code from node_modules. I also hope that it we will be possible to land full deprecation on v12, but I think that we should take some action in the meantime in v11.

The thing is, the current deprecation is ineffective for not-very-supported modules to update their code, as users don't see the warnings from node_modules.
It's effective only for actively supported modules and new modules that are developed/tested on Node.js v10.

On the other hand, at this point displaying the warning for code inside node_modules would just print them to everyone and make everyone rant. Also, in most cases it would be not something that users would be able to directly control. But if we somehow display that warning to e.g. ~1% of code in node_modules, it might be effective enough for users to notify module authors about that, while not annoying everyone to the extreme.

So, some ideas

The trick would be to somehow print warnings for code in dependencies, so dependencies would be notified of this, but at the same time not print out those warnings by default to just every unsuspecting Node.js user out there.

That has to be reproducable, preferrably without users having to add flags just to reproduce the warning they observed without the flag. Because of that, just randomizing and displaying for 1% runs would be a bad idea.

1. Time-based approach

Print warnings for both code outside of node_modules (as now) and code inside node_modules that was called shortly after application startup (i.e. first few ticks). I.e. warn for Buffer() constructor usage that happened either in the first few ticks or outside node_modules.

That needs testing, but I expect it include only a small additional portion of code from node_modules, while being reproducable. Imo printing only if that code was called in the first few ticks might be a good solution that could cover a significant portion of the cases and speed up popular libs porting off unsafe Buffer construcor usage.

2. Depth-based approach

Perhaps, we can measure the require chain length? I.e. print warning for the code in node_modules that has been directly required from outside of node_modules.

That could be more fragile though, I guess, and reach out less modules out there. E.g. if user->A->B->C, and B is an unmaintained module, no one would notice warnings from C.

[seishun]

Also, in most cases it would be not something that users would be able to directly control.

Who do you mean by users? If you mean developers who are using the module, then they have the option of migrating off the module or forking it, if all else fails.

[seishun]

It's now August everywhere on Earth, so re-adding to the agenda.

[BridgeAR]

Should this stay open?

[seishun]

Should this stay open?

Is there a reason not to?

[Trott]

Since we're coming up on the 12.x release in less than two months, I guess it's a good time to check in on the TSC folks that have reviewed this already.

@apapirovski Are you still +1 on this landing on master at this time?

@addaleax Still a -1 for the reasons provided?

@mcollina @fhinkel Still -1 from both of you for the reasons you provided too?

@Fishrock123 had asked to be a permanent abstention on Buffer constructor stuff, but I'm not sure if it was tongue-in-cheek or not.

Anyone else on @nodejs/tsc have a definite opinion at this time? This isn't a vote (yet, at least), but I think it's useful to know where people stand. (Since a vote is likely, having the conversation before it comes to that will help the vote come to a conclusion faster.)

[mcollina]

I’m still -1.

[addaleax]

Yes, I’m still -1.

[apapirovski]

Still +1 but I don't feel strongly about this.

[fhinkel]

Should this stay open?

Is there a reason not to?

@seishun Should we try to land this for 14?

[seishun]

@fhinkel Sure, I'll be happy to rebase it if there's support for it.

[fhinkel]

Hi @nodejs/tsc , we have some plus and minus ones on this. Can we revisit whether this should land in the next release?

[Trott]

Summarizing the current status in case it's helpful for anyone else:

Ultimately, this is about estimating the value of the security improvement, estimating the cost of user/maintainer pain, and deciding if the former is worth the latter.

new Buffer() zero-fills on all supported versions of Node.js, leaving only the "oops, you thought you were getting a string but it's really a number and now you've been DoS'ed" issue as something relevant in supported releases.

Users already get a runtime deprecation warning right now if new Buffer() is in their own code. The change here is that users would start seeing warnings for new Buffer() use in their dependencies.

Lastly, there are three competing interests here, I think:

• Maintainers of (often multiple) dependencies who have often used new Buffer() in a safe way and don't want to be bothered by a flood of issues from end users telling them to update their "broken" stuff, especially things they haven't had to otherwise maintain for years.

• End users who will see all these warnings about code they don't control.

• People interested in maximizing the security of the Node.js ecosystem, even though it might mean some near-term end user pain and maintainer pain.

Those three populations are not distinct. A single person can belong to all three, two, one, or none of the above.

[jasnell]

@nodejs/tsc ... should we keep this PR open? Do we anticipate that we will ever do a full runtime deprecation on the Buffer constructor?

[gireeshpunathil]

My take is that we don't runtime deprecate this API.

[bnb]

@nodejs/tsc given that there's roughly a year and a half of people asking the TSC if this should land without a definitive "yes" but several definitive "no", we should consider this a "no" and close it out?

[cjihrig]

Is there any new/updated data available on Buffer constructor usage in the ecosystem?

[Trott]

Is there any new/updated data available on Buffer constructor usage in the ecosystem?

You might be able to use @ChALkeR's gzemnid to get some idea.

[Trott]

@nodejs/tsc given that there's roughly a year and a half of people asking the TSC if this should land without a definitive "yes" but several definitive "no", we should consider this a "no" and close it out?

+1 to closing. If we ever do decide to do this, we can open a new PR or re-open this one.

[bnb]

I'm going to close this given the above +1 plus two 👍🏻 and the context that this has been stale for an excessively long time without progress. If folks do want to approach this again, feel free to reopen this or create a totally new PR.