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

Security improvement: don't reveal powered-by #2813

Closed
wants to merge 1 commit into from
Closed

Security improvement: don't reveal powered-by #2813

wants to merge 1 commit into from

Conversation

madarche
Copy link

The first thing recommended when setting up an Express instance is to secure it by removing its "X-Powered-By" header.

https://www.npmjs.com/package/helmet
https://blog.risingstack.com/node-js-security-checklist/
https://strongloop.com/strongblog/best-practices-for-express-in-production-part-one-security/

So let's save cycles by not setting this header in the first place.

This header is also totally useless. If some people are needing it somewhere for a very specific usecase they can add a little custom middleware that returns the "X-Powered-By" header again.

The first thing recommended when setting up an Express instance is to
secure it by removing its "X-Powered-By" header.

https://www.npmjs.com/package/helmet
https://blog.risingstack.com/node-js-security-checklist/
https://strongloop.com/strongblog/best-practices-for-express-in-production-part-one-security/

So let's save cycles by not setting this header in the first place.

This header is also totally useless. If some people are needing it
somewhere for a very specific usecase they can add a little custom
middleware that returns the "X-Powered-By" header again.
@madarche
Copy link
Author

As this is a breaking change, this is targeted for 5.x.

@dougwilson
Copy link
Contributor

Hi! Can you elaborate on what exactly the security risk is that we would be resolving with this change? As long as things like IIS, ASP.Net, PHP, and more are sending this header by default, I don't see any good reason for Express to stop doing it unless you can provide very concrete information (and not speculation) on how this header is a security risk.

@dougwilson dougwilson self-assigned this Nov 23, 2015
@dougwilson dougwilson added the pr label Nov 23, 2015
@dougwilson
Copy link
Contributor

After talking around, as long as PHP, ASP.Net and others are sending X-Powered-By by default, we will as well. If you are planning an industry-wide change to get this header removed from everything by default, please keep us in the loop :)

@dougwilson dougwilson closed this Nov 23, 2015
@madarche
Copy link
Author

When a web server/application is to be secured, be it PHP, Apache, Nginx, (whatever the software) the first thing to do is removing the web server/framework/etc. fingerprints if possible, i.e. the Server and X-Powered-By headers. Agreed it's not the most important point, but it's efficient (evidences provided in the literature links below).

The previously cited links all agree on it:

Here is literature from SANS, one of the most trusted source for computer security training (ask your network):
common services that are particularly prone to banner grabbing are web servers

Here is literature from OWASP, which provides recommendations widely followed by the industry (ask your network): Web framework fingerprinting is an important subtask of the information gathering process. Knowing the type of framework can automatically give a great advantage if such a framework has already been tested by the penetration tester. It is not only the known vulnerabilities in unpatched versions but specific misconfigurations in the framework and known file structure that makes the fingerprinting process so important. etc. :

The "X-Powered-By" header is not listed as a useful HTTP header:
https://www.owasp.org/index.php/List_of_useful_HTTP_headers

Now why would Helmet gather more than 1500 stars? Who the hell are those freaks who are not doing as PHP and as ASP.Net?

Node.js and Express are about aesthetic, stripping the useless, removing the cruft and going fast! All what you are ceaselessly doing in all your contributions.

So now let's reverse the burden of proof: where are the proofs that using X-Powered-By by default is a good practice?

@dougwilson
Copy link
Contributor

I would like to hear why Microsoft still ships it on by default with ASP.Net and why PHP ships it on by default, etc. If you can provide that information, perhaps you can sway us, but for now, we won't be changing this feature.

Helmet does WAY MORE than turn off this header. If you showed me a module that literally did nothing but disable this header and had 1500 stars, that's a different story.

Basically, get ASP.Net and PHP to commit to removing it and we will as well. Until then, you can either turn it off like you have to do with every platform or create a module and publish it to npm that has it off by default for people to use.

@madarche
Copy link
Author

Of course Helmet does way more than just removing the X-Powered-By header. But developers and project managers deploying Helmet buy into this choice.

@EvanHahn, @evilpacket, @gergelyke @hacksparrow, @crandmck, as authors of the aforementioned software and articles, would you care to convince @dougwilson to remove the X-Powered-By header in Express 5?

@gergelyke
Copy link

+1 for removal - I think @madarche summarized all the reasons pretty well
also, "because Microsoft does it, we do it as well" is not a valid reason to keep it

@dougwilson
Copy link
Contributor

Look, removing the header provides no security benefits. Removing the "frivolous" header only does that--removes a non-useful header. What is the point of the header you say? Well, it's to give credit to the hard-working people who have enabled you to write your code so fast, who you never even gave money to, as this is free, open source. This library is even MIT licensed, so please, feel free to fork it and provide your own copy that does not include the header if you wish.

A lot of people incorrectly assume that "if that header is not there, attackers won't know I'm running Express!". I was walked-though a commercial security product that does server identification by security engineering. This software includes specific detection for Express.js, even (being popular), and these are some of the rules it uses:

  1. Are there response headers all in lower-case? The more there are, the more points assigned as likely being Node.js server, which in turn counts towards Express.js.
  2. Some requests to random URLs are made looking for a 404. If the response body is in the format "Cannot GET {url}" then it gives a massive number of points towards it being Express.js. This is actually the main give away it uses to know you are an Express.js server.
  3. What does the ETag header look like if there is one? There are definitions for the different versions of the ETag header format for Express.js, so this header matching certain formats not only gives points towards being Express.js, but even hints at the version of Express.js you are using, since the format has changed over time (even the X-Powered-By header doesn't provide that level of detail, since it does not contain a version).

Even then, when using the server vulnerability attacking part of that software, it will simply try Express.js attacks against a server, regardless of if it even thinks the server is Express.js. This is because, since everything is configurable, there is no good way to truly know.

The X-Powered-By and Server headers are frivolous headers, I agree. Does removing them help with security? No, only increases obscurity, which is not a valid form of security by any means.

Just think of it this way: we give you Express.js for free and in return "trick" you into giving us credit for our hard work or you have to put 1 line of code in your application, which is already code and your developers should understand how to code when using Express.js.

There is no further discussion to be had on this topic.

@expressjs expressjs locked and limited conversation to collaborators Nov 24, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants