{"action":"created","issue":{"url":"https://api.github.com/repos/huydx/hget/issues/7","repository_url":"https://api.github.com/repos/huydx/hget","labels_url":"https://api.github.com/repos/huydx/hget/issues/7/labels{/name}","comments_url":"https://api.github.com/repos/huydx/hget/issues/7/comments","events_url":"https://api.github.com/repos/huydx/hget/issues/7/events","html_url":"https://github.com/huydx/hget/issues/7","id":170717780,"number":7,"title":"Security issue - directory traversal and os.RemoveAll to delete home directory","user":{"login":"cstockton","id":180184,"avatar_url":"https://avatars.githubusercontent.com/u/180184?v=3","gravatar_id":"","url":"https://api.github.com/users/cstockton","html_url":"https://github.com/cstockton","followers_url":"https://api.github.com/users/cstockton/followers","following_url":"https://api.github.com/users/cstockton/following{/other_user}","gists_url":"https://api.github.com/users/cstockton/gists{/gist_id}","starred_url":"https://api.github.com/users/cstockton/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cstockton/subscriptions","organizations_url":"https://api.github.com/users/cstockton/orgs","repos_url":"https://api.github.com/users/cstockton/repos","events_url":"https://api.github.com/users/cstockton/events{/privacy}","received_events_url":"https://api.github.com/users/cstockton/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2016-08-11T18:48:40Z","updated_at":"2016-08-13T06:56:16Z","closed_at":"2016-08-12T09:29:32Z","body":"Vulnerability info:\r\n\r\n> 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).\r\n\r\nMinimal example **Warning: do not run this, it will remove your home dir:**\r\n```bash\r\nhget http://domain.com/..\r\n```\r\n\r\nSuggestion:\r\n> 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.\r\n\r\ni.e.:\r\n```go\r\nfunc FolderOf(url string) string {\r\n  return filepath.Join(os.Getenv(\"HOME\"), dataFolder, sha1.Sum([]byte(url)))\r\n}\r\n```"},"comment":{"url":"https://api.github.com/repos/huydx/hget/issues/comments/239606397","html_url":"https://github.com/huydx/hget/issues/7#issuecomment-239606397","issue_url":"https://api.github.com/repos/huydx/hget/issues/7","id":239606397,"user":{"login":"cstockton","id":180184,"avatar_url":"https://avatars.githubusercontent.com/u/180184?v=3","gravatar_id":"","url":"https://api.github.com/users/cstockton","html_url":"https://github.com/cstockton","followers_url":"https://api.github.com/users/cstockton/followers","following_url":"https://api.github.com/users/cstockton/following{/other_user}","gists_url":"https://api.github.com/users/cstockton/gists{/gist_id}","starred_url":"https://api.github.com/users/cstockton/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cstockton/subscriptions","organizations_url":"https://api.github.com/users/cstockton/orgs","repos_url":"https://api.github.com/users/cstockton/repos","events_url":"https://api.github.com/users/cstockton/events{/privacy}","received_events_url":"https://api.github.com/users/cstockton/received_events","type":"User","site_admin":false},"created_at":"2016-08-13T06:56:16Z","updated_at":"2016-08-13T06:56:16Z","body":"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.\r\n\r\nIf 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.\r\n\r\nGood work on such a quick turn around on your fix though man, take it easy and happy coding."}}
{"actor":{"display_login":"cstockton"}}