Skip to content

Commit

Permalink
Fixed again a critical bug with path travel...
Browse files Browse the repository at this point in the history
  • Loading branch information
@petersirka
petersirka committed Feb 14, 2019
1 parent f1e94c1 commit de16238
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 4 deletions.
10 changes: 7 additions & 3 deletions index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7387,9 +7387,13 @@ F.$requestcontinue = function(req, res, headers) {

// Stops path travelsation outside of "public" directory
// A potential security issue
if (req.uri.pathname.indexOf('./') !== -1) {
req.$total_status(404);
return;
for (var i = 0; i < req.uri.pathname.length; i++) {
var c = req.uri.pathname[i];
var n = req.uri.pathname[i + 1];
if ((c === '.' && n === '/') || (c === '%' && n === '2' && req.uri.pathname[i + 2] === 'e')) {
req.$total_status(404);
return;
}
}

F.stats.request.file++;
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@
"name": "Sarp Aykent",
"email": "[email protected]"
}],
"version": "3.2.2",
"version": "3.2.3",
"homepage": "http://www.totaljs.com",
"bugs": {
"url": "https://github.com/totaljs/framework/issues",
Expand Down

15 comments on commit de16238

@truedat101
Copy link

Choose a reason for hiding this comment

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

@petersirka should this be clarified if this affects all versions, and back patch fixes for users who are on older packages? Happy to help on this if you need someone to review older versions.

@truedat101
Copy link

Choose a reason for hiding this comment

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

Also @petersirka do you have details on steps to reproduce this issue. Was this introduced in 3.x and fixed in 3.2.3? If it is a new issue NOT affecting all version before 3.2.3, we should request clarity in the CVE alert. The current one claims that all versions are affected prior to this patch.

@petersirka
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi, 3.2.3 fixes everything what I know about the bug. 3.2.0 + 3.2.1 + 3.2.2 didn't contain the best solution and in 3.2.3 is a new fix for escaped space.

But if you use reverse proxy like Nginx or Apache in front of Total.js then you're safe, only all Total.js apps without reverse proxy are wrong.

@truedat101
Copy link

@truedat101 truedat101 commented on de16238 Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for clarifying. I guess my question is, did bug exist before 2.0? I still have sites on 1.7.2, 1.9.8, etc. It's not simply enough for me to throw a reverse proxy in front of my web app framework. My team reviews the security alerts from github. Github will send out an alert for repositories with an npm package lock for any cve alerts. I can't sweep it under the rug. If the problem only affects 3.2.0, 3.2.1, and 3.2.2, then the CVE alert is worded incorrectly, and should only state the affected versions === 3.2.0, 3.2.1, 3.2.0, not all versions < 3.2.3.

@petersirka
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For older Total.js apps you can use this definition file: security.js.zip more info in this blog post.

I can't sweep it under the rug.

Yes I too. So what do you suggest?

@truedat101
Copy link

@truedat101 truedat101 commented on de16238 Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good suggestion to grab the patch. But it doesn't solve the security warning emails we will get from github on a weekly basis, and it also shows up in the dashboard. Two approaches I can think of:

  • Approach 1: Manual patch with security.js, and users who don't want to see the github warnings manually remove total.js from their package.json, and regenerate package-lock.json, and just checkin the total.js module into the project somewhere.
  • Approach 2: Patch existing tagged "releases" with security.js, and publish a patch release. So 1.7.x releases would get a 1.7.3 release with the patch. 1.8.x would get a 1.8.1 with the patch. 1.9.x would get a 1.9.9 patch. And continue this pattern for all official releases.

For approach 1, this gets my team off my back, and gets github off my back, but it puts me at further risk because I have no further notifications about CVEs from github. I'd have to actively monitor the problem. I think for a temporary fix this is ok, but generally long term not acceptable. I don't have the budget to upgrade to 3.2.3 or greater, and don't want to do it at this time.

For approach 2, this is a burden on you a bit, handling patching and merging, but I think it probably takes up to two hours tops, and at the end, you can contact https://www.certimetergroup.com/ folks if they were the ones who notified you of the security bug, and explain to them you've released legacy patches, and so the CVE should get modified to only included affected releases, not all releases < to 3.2.3. By submitting the patch list of releases, the CVE gets updated, and github stops bothering us with warnings if we move to a patched release.

I can help, just let me know if you need help on any of this. I realize you probably want to get everyone onto the newest release support wise. I'm not clear if you've pushed migration documention between releases, assuming there are code or api changes that need to be made. So sometimes it is not practical to upgrade if there is more work to make upgrades. Moving to a minor patch release seems like the best approach for legacy users.

@truedat101
Copy link

@truedat101 truedat101 commented on de16238 Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So actually, for approach 2, that's just a patch release per Major.Minor.X version: 3.2.1, 3.1.1, 3.0.1, 2.9.5, 2.8.1, 2.7.1, 2.6.3, 2.5.1, 2.4.1, 2.3.1, 2.2.1, 2.1.1, 2.0.2, 1.9.9, 1.8.1, 1.7.3, and forget about legacy stuff before 1.7.x. That's still a lot, but it's a quick patch if its just a drop in of the security.js + update package.json file to make a release tag.

@petersirka
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll think about it these days and I'll perform NPM update for older versions. It will be better for older projects.

@truedat101
Copy link

Choose a reason for hiding this comment

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

Ok, please let me know what you decide, and if you need any testing or suggestions for the CVE. I'm assuming the current CVE is too broad, that all versions are not infected, or at least, with the security patch in new NPM module patch releases, this is enough to get them to change the scope of the CVE.

@petersirka
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yesterday I updated these versions only in NPM:

  • 2.0.0 + 2.0.1 + 2.0.2 to 2.0.3
  • 2.1.0 to 2.1.1
  • 2.2.0 to 2.2.1
  • 2.3.0 to 2.3.1
  • 2.4.0 to 2.4.1
  • 2.5.0 to 2.5.1
  • 2.6.0 + 2.6.1 + 2.6.2 to 2.6.3
  • 2.7.0 to 2.7.1
  • 2.8.0 to 2.8.1
  • 2.9.0 ... 2.9.4 to 2.9.5
  • 3.0.0 to 3.0.1
  • 3.1.0 to 3.1.1
  • 3.2.0 + 3.2.1 + 3.2.2 + 3.2.3 to 3.2.4

Open Versions tab in https://www.npmjs.com/package/total.js ... I don't know but https://docs.npmjs.com/cli/deprecate doesn't work for some versions...

@truedat101
Copy link

Choose a reason for hiding this comment

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

en fuego 🔥 . Nice work!

Thanks. Any chance you can get 1.9.8->1.9.9?

@petersirka
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry for delay. In very older version you need to use this definition file: security.js.zip ... It's modified a bit.

@truedat101
Copy link

Choose a reason for hiding this comment

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

Ok, but you realize that doesn't calm down github from complaining about security vulnerabilities?

@petersirka
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't notified about security vulnerabilities from GitHub. I solved problem with help of people from CG and Snyk. Very old versions will be without fix in NPM, but I gave a solution for fixing (that's important).

@truedat101
Copy link

Choose a reason for hiding this comment

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

Anyone who uses package-lock.json and checks in code that includes total.js before 3.2.4 (your most recent patch mentioned in the CV) will see this :

We found potential security vulnerabilities in your dependencies.

You can see this message because you have been granted access to vulnerability alerts for this repository.
Manage your notification settings or learn more about vulnerability alerts.

in the top of the repository as a banner, and there will also be a weekly email. That's fine for a one person shop to ignore, but actually enterprises that rely on the software may have their dependencies audited and will be obligated to report security vulnerabilities found as part of regulatory process. So I'm suggesting, you you do want to support legacy versions by pushing the patch to NPM and then resubmit the new patches to the security researchers that reported this issue. They can reduce the scope of the CVE to fixed on the specific versions mentioned in de16238#commitcomment-32506669

Please sign in to comment.