Fix path traversal in GET and PUT - #11
Open
HarshRajSinghania wants to merge 1 commit into
Open
Conversation
command_get() and command_put() passed the client supplied filename straight into fopen() with no check at all. Any slash in the name let a client read or write files anywhere on the filesystem, not just inside the directory the server was started in, and this required no login. Added is_safe_filename(), which rejects any name containing a slash or an empty string, and calls it before both fopen() calls. A blocked name now takes the exact same code path as a normal missing file, so the server does not reveal why a request failed. Tested with a small python script that speaks the packet protocol directly. Before the fix it could read a file outside the server directory and write one there too. After the fix both attempts get rejected and the server responds with the same error message it already used for a plain missing file. Also confirmed normal GET and PUT with an ordinary filename still work the same as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
command_get() and command_put() in server_ftp_functions.c take the client supplied filename and pass it straight into fopen(), with nothing checking it first. Since there is no login step in this protocol at all, anyone who can reach the server can send a path like an absolute path or something with ../ in it and read or write files anywhere the server process has permission to touch, not just inside the folder the server was started in.
I wrote a small script that speaks the packet protocol directly and confirmed this against the current code. It could pull a file from completely outside the server folder and also write a file there, without any authentication step at all.
The fix adds one small function, is_safe_filename(), which rejects any filename containing a slash or an empty string, and calls it right before both fopen() calls. If it rejects the name, the server takes the exact same path it already takes for a normal missing file, so a client trying this gets the same error message either way and learns nothing extra.
I tested this by rerunning the same script against the patched server. Both the read and the write attempts now get rejected and nothing gets touched outside the folder. I also checked that a normal GET and PUT with an ordinary filename still work exactly as before, so this should not break any existing usage.
Happy to adjust the approach if you would rather restrict this a different way, this seemed like the smallest change that closes the actual hole without touching anything else in the protocol.