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

Disallow upward tree traversal in Windows. #254

Merged
merged 1 commit into from Feb 28, 2018
Merged

Disallow upward tree traversal in Windows. #254

merged 1 commit into from Feb 28, 2018

Conversation

mbleigh
Copy link
Contributor

@mbleigh mbleigh commented Feb 28, 2018

Internal bug id: 74001072

@mbleigh mbleigh merged commit e396ff6 into master Feb 28, 2018
@mbleigh mbleigh deleted the mb-backslash branch February 28, 2018 22:54
@abeisgoat
Copy link
Contributor

This is still being reported as a vulnerability from this report. Anyone know how to mark this as fixed?

@ChALkeR
Copy link

ChALkeR commented Jun 25, 2018

@AbeHaskins It is not fixed, a patched version of superstatic was never released. See #255.
I tried pinging them (Google/Firebase) multiple times by side channels, that didn't help.

@ChALkeR
Copy link

ChALkeR commented Jun 25, 2018

@AbeHaskins On a personal opinion, I would suggest to switch to some maintained static file server instead of superstatic.

@ChALkeR
Copy link

ChALkeR commented Jun 26, 2018

@AbeHaskins Ah, sorry. I was answering from the phone and did not notice that you are a member of @firebase then — I assumed that you were a consumer of superstatic.

So, the answer is — release a fixed version of superstatic and wait for the datasets to be updated.

They do not include a fixed field atm because this was dislosed by @firebase and left for a long time without actual fixed version being released.
E.g. see the discussion at nodejs/security-wg#247 (comment). The normal process includes opening those PRs and landing those entries when an issue is fixed and disclosed or when there is no response from authors, but in this case @firebase disclosed this without releasing a fixed version for a long time, so it was added to the database by Node.js Security WG likewise, after a confirmation from Google that this can be treated as disclosed.

You can also try filing a PR to https://github.com/nodejs/security-wg after releasing the fixed version. That's not the direct location which npm sources their data from, but it's where this vuln entry originates from so other datasets (including npm's one) hopefully would notice the update shortly after.

@ChALkeR
Copy link

ChALkeR commented Jun 26, 2018

Also, my note from the original report (I am unsure if it was passed down here):

That fix seems to work, but the recommended solution to prevent path traversal is to resolve the path and then check that the resolved path is inside the base path.

...{some time after}…

Are there plans for implementing the recommended solution of additinally checking the actual resolved path?

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

4 participants