Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Directory traversal #2

Open
cbiedl opened this issue Feb 6, 2024 · 4 comments
Open

Directory traversal #2

cbiedl opened this issue Feb 6, 2024 · 4 comments

Comments

@cbiedl
Copy link

cbiedl commented Feb 6, 2024

Hello,
thanks for that little program, it serves me well, and I'm considering packaging it for the Debian Linux distribution.
However, there's an issue: It seems ftp-proxy has no safeguard against requesting files from outside the given base directory, in other words, "get ../../../etc/passwd" will happily deliver that file - something that shouldn't happen from a security point of view.

avermeer-tc added a commit to funzoneq/tftp-proxy that referenced this issue Feb 7, 2024
@funzoneq
Copy link
Contributor

funzoneq commented Feb 7, 2024

Hi @cbiedl. I created a fix for it as you can see above, but I no longer have write access to this repository.

@hillu
Copy link

hillu commented Feb 7, 2024

@funzoneq Your patch does not work:

package main

import "fmt"
import "path"
import "path/filepath"

func main() {
	dir := "/var/lib/tftpboot"
	filename := "/../../../../../../../etc/passwd"
	fmt.Println(filepath.Clean(path.Join(dir, filename)))
}

gives me /etc/passwd which is exactly what I would want as an attacker.

What does work is

  • ensuring that filename starts with a slash
  • cleaning up the filename with path.Clean or filepath.Clean
  • joining that with dir

Like so:

path.Join(dir, path.Clean("/"+filename))

@funzoneq
Copy link
Contributor

funzoneq commented Feb 7, 2024

@hillu thanks for that. I updated the PR. #3

@cbiedl
Copy link
Author

cbiedl commented Feb 9, 2024

@funzoneq that is not sufficient I'm afraid. This one prepends a slash to user-supplied filename and should therefore do the trick:

file_path := path.Join(dir, filepath.Clean(path.Join("/", filename)))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants