Security issue - directory traversal and os.RemoveAll to delete home directory

[cstockton]

Vulnerability info:

Susceptible to removing the users entire home DIR due to unsafe handling of links mixed with the os.RemoveAll usage in main.go. This is particularly dangerous with this tool since it follows redirects. There are likely permutations of this exploit that could have much more serious consequences in a targeted attack (elevation).

Minimal example Warning: do not run this, it will remove your home dir:

hget http://domain.com/..

Suggestion:

Do not construct paths in this way, in code.. ever! Always use a deterministic method for building paths using paradigms known to mitigate directory traversal attempts. In your case changing all instances where you build paths, such as FolderOf to use a basic hash function derived from the URL. The key here is to not attempt to derive any file system operations from the file name directly.

i.e.:

func FolderOf(url string) string {
  return filepath.Join(os.Getenv("HOME"), dataFolder, sha1.Sum([]byte(url)))
}

[huydx]

@cstockton Thank for throughout comment, I solved the directory traversal attack by this commit https://github.com/huydx/hget/commit/587608b59638a4c2c179c48fd00e90c7bd89fcdd Please comment if my solution has any problem.

Thanks in advance

[cstockton]

Hey- you still have some issues here, just looking at that code I'm fairly certain that you would delete the .hget directory with ".".. I am not sure what kind of other caveats are possible here. I also think on windows this is just asking for it.. since the path separator changes. I don't have time to audit this right now, but if I did the file system interactions happen in too many places to follow safely, several code paths call RemoveAll, several call FolderOf, etc. It's hard to know what your inputs are because they come from so many places.

If I was you I might consider removing all filepath imports from the project and placing them in a method set on a single struct. Using a context.Context here might be nice because you could override the Url in it cleanly by deriving new context when you follow redirects (check with CheckRedirect) and set a new struct (and validate). If you have one struct that reads the url to generate file system paths it will make your project a bit more resilient and future proof.

Good work on such a quick turn around on your fix though man, take it easy and happy coding.

[huydx]

IMHO, until the only remove logic is rely on FolderOf logic, and I think if we maintain safety of this function to return only path that "inside" the safe path (~/.hget) is enough. If you find another false case can be happen, please comment below. Thanks.