Skip to content
New issue

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

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

Already on GitHub? # to your account

Prevent malformed upload path causing arbitrary write #1170

Closed
wants to merge 4 commits into from

Conversation

marty1885
Copy link
Member

@marty1885 marty1885 commented Jan 30, 2022

This PR prevents a malformed upload path causing the upload function to write outside of the destination directory by checking the upload path.

TBH I'm not 100% sure my patch fixes everything and didn't break unintentionally. Need some review.

@@ -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.

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

Successfully merging this pull request may close these issues.

3 participants