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

Prevent DoS attack #213

Closed
wants to merge 1 commit into from
Closed

Prevent DoS attack #213

wants to merge 1 commit into from

Conversation

Zarel
Copy link

@Zarel Zarel commented Jan 11, 2019

A pathname containing U+0000 NULL will crash fs.stat with the error message "TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes."

This commit prevents node-static from crashing.

A pathname containing `\u0000 NULL` will crash `fs.stat` with the error message "TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes."

This commit prevents node-static from crashing.
@tchakabam
Copy link
Contributor

shouldn't this be related to / fixed in the specific Node runtime in use?

if (pathname.indexOf(that.root) === 0) {
// file outside of the root or an invalid
// pathname.
if (pathname.indexOf(that.root) === 0 && pathname.indexOf('\u0000') === -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if you would do this like this, it should be somehow at least logging that this type of path has been ignored i think.

is there some documentation on this vulnerability and have there been other fixes to this exploit in other open-source codebases?

Copy link
Author

Choose a reason for hiding this comment

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

You don't log paths like ../ either; I just modeled my code after the existing code.

I'm not aware of documentation of this problem. I became aware of it because someone attacked my server with it.

@Zarel
Copy link
Author

Zarel commented Jan 14, 2019

It looks like this is new in Node.js v10:

https://alexatnet.com/node-js-10-important-changes/#fs-2

In previous Node versions, it would be passed as an error to the fs.stat callback, so it would respond with a 404:

        fs.stat(pathname, function (e, stat) {
            if (e) {
                finish(404, {});

But in Node 10 and later, fs.stat instead synchronously throws an error.

@Zarel
Copy link
Author

Zarel commented Jan 14, 2019

nodejs/node#18308

is the change in Node.js that led to this regression.

@lovasoa
Copy link

lovasoa commented Mar 21, 2020

This is a serious security issue. What is the state of this PR ?

frankdean added a commit to frankdean/node-static that referenced this pull request Jul 17, 2020
@kashif-m
Copy link

Why is this still not closed? 🤔

@brettz9
Copy link
Collaborator

brettz9 commented May 21, 2021

Closing as #227 avoids need for this PR.

@brettz9 brettz9 closed this May 21, 2021
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.

None yet

5 participants