Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upfix: lock users in chroot jail #5
Conversation
|
Awesome work. Refactoring like that will really push this project forward. However I think we need good tests to make sweeping changes. We do have some tests in the specs directory but our test coverage is pretty low. We really should have feature-to-test parity and be committing tests along with code updates. I think what I'll do is accept this in master and create a stable branch. Then we (myself included) should work on some tests before merging master into stable. |
|
Thanks. Now that I have a sense of the project and expectations, I'll look into adding additional test coverage before pushing on with refactoring/extending. We could hold off on this pull request until it has corresponding testing and, instead, I can put together a localized fix to Your call, though. |
|
Good call with the localized fix to withCwd. Go for it. And thanks for finding this serious vulnerability. |
|
Yes, I second Simon's thanks. Agree with you guys on how to proceed. |
|
Alright, we should be good to go now. BTW, what's the philosophy here on versioning/tagging/etc? This seems to be a good point to update the npm package. Thoughts? |
fix: prevent users from traversing out of chroot directory
This commit properly resolves MDTM request filenames to locations within the CHROOT jail. Previously, requests were made relative to the filesystem root (/), instead of the server root (/srv/files/from/here). This allowed users to request MDTM on potentially sensitive files (/root, /home), while simultaneously denying legitimate requests within the shared directory. Note: all filesystem calls *must* be joined with the path to the server root (pathModule.join(self.root, filename)). fix #28 ref nodeftpd@57a9e5f ref nodeftpd#5 ref nodeftpd#9
asylumfunk commentedJan 28, 2014
Please reference comments on asylumfunk#5 and asylumfunk@6608b2d
This is a heftier commit than I've pushed before, so please ask away if anything is unclear.
I've tested this on both *NIX and WIN machines (as it deals with path separators), but please be diligent as this touches every(?) filesystem access method (implicitly or explicitly).