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.

Sign up for GitHub

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

Jump to bottom

Prevent malformed upload path causing arbitrary write #1170

Closed
wants to merge 4 commits into from
Closed

Prevent malformed upload path causing arbitrary write #1170

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9234eb8
prevent file upload write outside of the upload folder
marty1885 Jan 18, 2022
b82e53d
Merge branch 'master' of github.com:/drogonframework/drogon
marty1885 Jan 28, 2022
ab9a486
Merge branch 'master' of github.com:/drogonframework/drogon
marty1885 Jan 30, 2022
d0445ca
Prevent malformed upload path causing arbitrary write
marty1885 Jan 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions lib/src/HttpFileImpl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,12 @@ int HttpFileImpl::save(const std::string &path) const
{
filesystem::path fsUploadPath(utils::toNativePath(
HttpAppFrameworkImpl::instance().getUploadPath()));
fsPath = fsUploadPath / fsPath;
fsPath = (fsUploadPath / fsPath).lexically_normal();
}
filesystem::path fsFileName(utils::toNativePath(fileName_));
auto fsFileName =
filesystem::path(utils::toNativePath(fileName_)).lexically_normal();
if (fsFileName.is_relative())
return -1;
if (!filesystem::exists(fsPath))
{
LOG_TRACE << "create path:" << fsPath;
Expand All @@ -66,6 +69,14 @@ int HttpFileImpl::saveAs(const std::string &fileName) const
HttpAppFrameworkImpl::instance().getUploadPath()));
fsFileName = fsUploadPath / fsFileName;
}
fsFileName = fsFileName.lexically_normal();
if (fsFileName.is_relative())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it is not enough to check if path is not relative. In this case if filename_ will be /malicious-file user still will be able to upload outside of uploads folder.

Can I suggest to do something like:

int HttpFileImpl::save(const std::string &path) const
{
    assert(!path.empty());
    if (fileName_.empty())
        return -1;
    filesystem::path fsPath(utils::toNativePath(path));
    if (!fsPath.is_absolute() &&
        (!fsPath.has_parent_path() ||
         (fsPath.begin()->string() != "." && fsPath.begin()->string() != "..")))
    {
        filesystem::path fsUploadPath(utils::toNativePath(
            HttpAppFrameworkImpl::instance().getUploadPath()));
        fsPath = fsUploadPath / fsPath;
    }
fsPath += "/";
fsPath = fsPath.lexically_normal();

auto fsSaveToPath = filesystem::path(fsPath);
fsSaveToPath += filesystem::path(utils::toNativePath(fileName_));
fsSaveToPath = fsSaveToPath.lexically_normal();

if (fsSaveToPath.string().find(fsPath.string(), 0) != 0)
{
        LOG_ERROR
            << "Attempt writing outside of upload directory detected. Path: "
            << fileName;
        return -1;
}
// ... 
return saveTo(fsSaveToPath);
  1. You need to clean all multiple slashes and dot segments – lexically_normal.
  2. Concatenate path with std::filesystem::path::concat(+= operator) instead of std::filesystem::path::append(/ operator) because if the second argument of append is an absolute path it will resolve the whole path to the second argument.
  3. Call lexically_normal again to avoid // after concat.
  4. And then you need to check if final upload path starts with the uploads folder path.

Alternatively you can drop all .. and / symbols from the fileName_.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kirill89 I am very grateful for your advice. we'll fix this.

{
LOG_ERROR
<< "Attempt writing outside of upload directory detected. Path: "
<< fileName;
return -1;
}
if (fsFileName.has_parent_path() &&
!filesystem::exists(fsFileName.parent_path()))
{
Expand Down