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

buffer: runtime-deprecate Buffer constructor everywhere by default #21351

Closed
wants to merge 1 commit into from
Closed

buffer: runtime-deprecate Buffer constructor everywhere by default #21351

wants to merge 1 commit into from

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Jun 15, 2018

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

Some backstory:

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 nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module. labels Jun 15, 2018
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

This has to happen at some point, IMO. 👍

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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

@addaleax addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. notable-change PRs with changes that should be highlighted in changelogs. labels Jun 15, 2018
@Trott Trott modified the milestone: 11.0.0 Jun 15, 2018
@seishun
Copy link
Contributor Author

seishun commented Jun 15, 2018

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

@Trott
Copy link
Member

Trott commented Jun 15, 2018

@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
Copy link
Contributor Author

seishun commented Jun 22, 2018

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

@seishun seishun added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jun 22, 2018
@mcollina
Copy link
Member

mcollina commented Jun 23, 2018 via email

@seishun
Copy link
Contributor Author

seishun commented Jun 23, 2018

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
Copy link
Member

mcollina commented Jun 23, 2018 via email

@ChALkeR
Copy link
Member

ChALkeR commented Jun 23, 2018

@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
Copy link
Member

ChALkeR commented Jun 23, 2018

@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
Copy link
Member

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.

@mhdawson mhdawson added tsc-review and removed tsc-review tsc-agenda Issues and PRs to discuss during the meetings of the TSC. labels Jun 27, 2018
@ChALkeR
Copy link
Member

ChALkeR commented Jul 28, 2018

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
Copy link
Contributor Author

seishun commented Jul 28, 2018

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
Copy link
Contributor Author

seishun commented Aug 1, 2018

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

@seishun seishun added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 1, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Mar 9, 2019

Should this stay open?

@Trott Trott removed the tsc-review label Mar 10, 2019
@seishun
Copy link
Contributor Author

seishun commented Mar 10, 2019

Should this stay open?

Is there a reason not to?

@Trott
Copy link
Member

Trott commented Mar 10, 2019

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
Copy link
Member

I’m still -1.

@addaleax
Copy link
Member

Yes, I’m still -1.

@apapirovski
Copy link
Member

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

@fhinkel
Copy link
Member

fhinkel commented Oct 26, 2019

Should this stay open?

Is there a reason not to?

@seishun Should we try to land this for 14?

@seishun
Copy link
Contributor Author

seishun commented Oct 27, 2019

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

@fhinkel
Copy link
Member

fhinkel commented Oct 28, 2019

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

@Trott
Copy link
Member

Trott commented Oct 28, 2019

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.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:20
@jasnell jasnell added the blocked PRs that are blocked by other issues or PRs. label Jul 7, 2020
@jasnell
Copy link
Member

jasnell commented Jul 7, 2020

@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
Copy link
Member

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

@bnb
Copy link
Contributor

bnb commented Jan 7, 2022

@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
Copy link
Contributor

cjihrig commented Jan 7, 2022

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

@Trott
Copy link
Member

Trott commented Jan 8, 2022

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
Copy link
Member

Trott commented Jan 8, 2022

@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
Copy link
Contributor

bnb commented Jan 11, 2022

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.

@bnb bnb closed this Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. buffer Issues and PRs related to the buffer subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.