Skip to content

Attempt to fix v2 file overwrite vulnerability #213

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 15, 2019
Merged

Attempt to fix v2 file overwrite vulnerability #213

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f85851
Ignore potentially unsafe files
BRKurek Apr 27, 2019
15e59f1
Only track previously seen hardlinks
BRKurek Apr 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions lib/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ function Parse () {
me._stream = new BlockStream(512)
me.position = 0
me._ended = false
me._hardLinks = []
Copy link
Owner

@isaacs isaacs May 15, 2019

Choose a reason for hiding this comment

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

A link -> target map object would be a more efficient and useful data structure than an array.


me._stream.on("error", function (e) {
me.emit("error", e)
Expand Down Expand Up @@ -250,7 +251,20 @@ Parse.prototype._startEntry = function (c) {

if (onend) entry.on("end", onend)

if (entry.type === "File") {
Copy link

@yoshiedm yoshiedm Apr 29, 2019

Choose a reason for hiding this comment

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

I'm not so sure if it's customary in this project, but I'd love to see a few comments explaining how this mitigates the security issue. May be valuable for someone maintaining this code in the future. Thanks!

Copy link

@yoshiedm yoshiedm May 1, 2019

Choose a reason for hiding this comment

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

Another request would be to cover the edge cases that exercise this logic in unit tests.

this._hardLinks.forEach(function(link) {
if (link.path === entry.path) {
Copy link
Owner

@isaacs isaacs May 14, 2019

Choose a reason for hiding this comment

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

While this addresses the vulnerability, it's not an ideal fix, because it also changes the behavior of the export.

A better solution would be to remove the file (or even just rimraf.sync() before creating any File type, which will slow things down, but it's a very old version that is already being migrated away from anyway.)

ev = "ignoredEntry"
}
})
}

this._entry = entry

if (entry.type === "Link") {
this._hardLinks.push(entry)
}

var me = this

entry.on("pause", function () {
Expand Down