Skip to content

Commit

Permalink
bp/FileHandler: check the path before opening a file
Browse files Browse the repository at this point in the history
This avoids getting ENAMETOOLONG or ENOTDIR which would result in
status 500.

(On Ceph, I have observed EINVAL responses from MDS instead of ENOTDIR
for opening a path ending with a slash.  That's a Ceph MDS bug, I
guess, but it hopefully becomes unreachable with this check.)
  • Loading branch information
MaxKellermann committed Aug 28, 2024
1 parent dc6b350 commit 91e23b0
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 0 deletions.
1 change: 1 addition & 0 deletions debian/changelog
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ cm4all-beng-proxy (18.0.90) unstable; urgency=low
* was: fail if the Multi-WAS socket buffer is full
* bp: fix several use-after-free bugs on translation responses
* bp: map ENAMETOOLONG to "414 Request-URI Too Long"
* bp: check the path before opening a file

--

Expand Down
67 changes: 67 additions & 0 deletions src/bp/FileHandler.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,70 @@

using std::string_view_literals::operator""sv;

inline bool
Request::CheckFilePath(const char *path, bool relative) noexcept
{
assert(path != nullptr);

const std::size_t path_length = strlen(path);

if (path_length >= PATH_MAX) [[unlikely]] {
DispatchError(HttpStatus::REQUEST_URI_TOO_LONG);
return false;
}

if (relative) {
if (path_length == 0 || *path == '/') [[unlikely]] {
DispatchError(HttpStatus::NOT_FOUND);
return false;
}
} else {
if (path_length < 2 || *path != '/') [[unlikely]] {
DispatchError(HttpStatus::NOT_FOUND);
return false;
}
}

if (path[path_length - 1] == '/') [[unlikely]] {
DispatchError(HttpStatus::NOT_FOUND);
return false;
}

return true;
}

inline bool
Request::CheckDirectoryPath(const char *path) noexcept
{
assert(path != nullptr);

const std::size_t path_length = strlen(path);

if (path_length >= PATH_MAX) [[unlikely]] {
DispatchError(HttpStatus::REQUEST_URI_TOO_LONG);
return false;
}

if (path_length == 0 || *path != '/') {
DispatchError(HttpStatus::NOT_FOUND);
return false;
}

return true;
}

bool
Request::CheckFileAddress(const FileAddress &address) noexcept
{
if (address.beneath != nullptr && !CheckDirectoryPath(address.beneath))
return false;

if (address.base != nullptr && !CheckDirectoryPath(address.base))
return false;

return CheckFilePath(address.path, address.base != nullptr);
}

inline void
Request::Handler::File::Close(UringGlue &uring) noexcept
{
Expand Down Expand Up @@ -345,6 +409,9 @@ Request::OnOpenStatError(int error) noexcept
void
Request::HandleFileAddress(const FileAddress &address) noexcept
{
if (!CheckFileAddress(address))
return;

handler.file.address = &address;

assert(address.path != nullptr);
Expand Down
17 changes: 17 additions & 0 deletions src/bp/Request.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,23 @@ private:

void HandleTokenAuth(UniquePoolPtr<TranslateResponse> response) noexcept;

/**
* Do formal checks on a #FileAddress and send an error
* response if the address is not acceptable.
*
* @return true if the address is acceptable, false if an
* error response has been submitted (this #Request instance
* may be destroyed)
*/
[[nodiscard]]
bool CheckFileAddress(const FileAddress &address) noexcept;

[[nodiscard]]
bool CheckFilePath(const char *path, bool relative) noexcept;

[[nodiscard]]
bool CheckDirectoryPath(const char *path) noexcept;

bool EvaluateFileRequest(FileDescriptor fd, const struct statx &st,
struct file_request &file_request) noexcept;

Expand Down

0 comments on commit 91e23b0

Please sign in to comment.