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

[Vulnerability] Path traversal results in an arbitrary file read/write #52

Closed
cha512 opened this issue Sep 16, 2023 · 2 comments
Closed

Comments

@cha512
Copy link

cha512 commented Sep 16, 2023

Suppose an attacker has a credential to access FTP server.

And suppose a user using fineFTP wants to share only a specific directory, as shown below.

#include <fineftp/server.h>

#include <iostream>
#include <thread>
#include <string>

int main()
{

	const std::string local_root =  "/home/kong2204/Desktop/";

	fineftp::FtpServer server(2121);

	server.addUserAnonymous(local_root, fineftp::Permission::All);
	server.addUser         ("MyUser",   "MyPassword", local_root, fineftp::Permission::ReadOnly);
	server.addUser         ("Uploader", "123456",     local_root, fineftp::Permission::DirList | fineftp::Permission::DirCreate | fineftp::Permission::FileWrite | fineftp::Permission::FileAppend);

	server.start(4);

	for (;;)
	{
		std::this_thread::sleep_for(std::chrono::milliseconds(100));
	}

	return 0;
}

In this case, an attacker can access unauthorized directories via path traversal.

Here's why.

The implementation of the toLocalPath function used by fineFTP to handle paths is shown below.

	std::string FtpSession::toLocalPath(const std::string& ftp_path) const
	{
		assert(logged_in_user_);

		// First make the ftp path absolute if it isn't already
		const std::string absolute_ftp_path = toAbsoluteFtpPath(ftp_path);

		// Now map it to the local filesystem
		return fineftp::Filesystem::cleanPathNative(logged_in_user_->local_root_path_ + "/" + absolute_ftp_path);
	}
	std::string FtpSession::toAbsoluteFtpPath(const std::string& rel_or_abs_ftp_path) const
	{
		std::string absolute_ftp_path;

		if (!rel_or_abs_ftp_path.empty() && (rel_or_abs_ftp_path[0] == '/'))
		{
			absolute_ftp_path = rel_or_abs_ftp_path;
		}
		else
		{
			absolute_ftp_path = fineftp::Filesystem::cleanPath(ftp_working_directory_ + "/" + rel_or_abs_ftp_path, false, '/');
		}

		return absolute_ftp_path;
	}

A logic is needed that checks whether the return string of toLocalPath is a subdirectory of the local root (or the local root itself). However, there isn't

Thus, if /../../../../ (or /..\..\..\..\..\ on Windows) is used, depending on the account's permissions, file reads or writes for unauthorized paths can become possible.

@FlorianReimold
Copy link
Member

@cha5126568: Thanks for finding this vulnerability! This is going to get fixed ASAP!

Please keep in mind that in any situation where this vulnerability matters you probably shouldn't use plain unencrypted FTP anyways, but also that's not an excuse for this vulneratbility.

FlorianReimold added a commit that referenced this issue Nov 2, 2023
…ore (#58)

- Performance improvements for FTP Downloads through memory-mapped file (Thanks to @bjuulp)
- Added googletest as submodule
- Added unit tests for fineftp-server. The tests require C++17 to compile and `curl` to be present in the `PATH` when executing them
- New CMake Options for Enabling / Disabling different components (See Readme.md)
- Binary downloads for Windows are now built with VS 2017 / v140 toolset
- Fixed a race condition on Windows that caused files not being fully flushed after fineftp-server reported the finished data upload (Thanks to @bjuulp)
- Reordered internal asio::strand implementation in order to prevent race conditions that haven't been detected, yet. (Thanks to @bjuulp)
- Fixed a vulnerability that enabled an attacker to access files above the root directory (reported by #52)
- Fixed many clang-tidy warnings
- The `APPE` (= append-to-file) command now creates a new file if it didn't exist already (this is the correct behavior according to RFC 959
- Updated the asio submodule to 1.28.2

---------

Co-authored-by: Bjarne Juul Pasgaard <24828375+bjuulp@users.noreply.github.com>
Co-authored-by: Bjarne Juul Pasgaard <bjp@skillson.dk>
@FlorianReimold
Copy link
Member

Solved by #58

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

2 participants