Skip to content

Prevent path traversal fix #3

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

Open
lowBudgetHypothermia wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Prevent path traversal fix #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1871f23
Added absolute path resolution to prevent path traversal exploit
Oct 21, 2025
70c22f8
Updated fsq.spec for rpm
Oct 21, 2025
0e07c4c
Changed path comparison to strncmp
Dec 8, 2025
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
12 changes: 12 additions & 0 deletions debian/changelog
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,15 @@
fsq (0.9.6) unstable; urgency=low

* Path comparison now using strncmp instead of for-loop.

-- Samuel Hasert <[email protected]> Mon, 08 Dec 2025 13:27:20 +0200

fsq (0.9.5) unstable; urgency=low

* Absolute file path resolution to prevent path traversal exploit.

-- Samuel Hasert <[email protected]> Tue, 21 Oct 2025 16:25:15 +0200

fsq (0.9.4) unstable; urgency=medium

* Support for IPv6.
Expand Down
6 changes: 6 additions & 0 deletions rpm/fsq.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ rm -rf %{buildroot}

%changelog

* Mon 08 Dec 2025 Samuel Hasert <[email protected]> 0.9.6
- Path comparison now using strncmp instead of for-loop.

* Tue 21 Oct 2025 Samuel Hasert <[email protected]> 0.9.5
- Absolute file path resolution to prevent path traversal exploit.

* Fri 2 Aug 2024 Thomas Stibor <[email protected]> 0.9.4-1
- Support for IPv6.

Expand Down
56 changes: 37 additions & 19 deletions src/fsqd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,21 +700,39 @@ static int write_access(struct fsq_session_t *fsq_session, char *lustre_dpath)
return rc;
}

/* File path name and Lustre directory name are matching
up to the end of the Lustre directory name. */
/* Resolve path to absolute path */
char resolved[PATH_MAX];
char *output = realpath(fpath, resolved);
Copy link
Member

@dennisklein dennisklein Dec 9, 2025

Choose a reason for hiding this comment

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

Shouldn't we check for an error here?

Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

Critical security issue: realpath() can return NULL on failure (e.g., if the path doesn't exist or permission is denied), but the code doesn't check for this. The subsequent strlen(resolved) on line 706 would then operate on an uninitialized buffer, leading to undefined behavior.

Add a null check and proper error handling:

char *output = realpath(fpath, resolved);
if (output == NULL) {
    int rc = -errno;
    FSQ_ERROR(*fsq_session, rc,
              "realpath failed for fpath '%s'", fpath);
    return rc;
}
const size_t L_resolved = strlen(resolved);
Suggested change
char *output = realpath(fpath, resolved);
char *output = realpath(fpath, resolved);
if (output == NULL) {
int rc = -errno;
FSQ_ERROR(*fsq_session, rc,
"realpath failed for fpath '%s'", fpath);
return rc;
}

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

Potential bug: If realpath() returns NULL (failure case), the output pointer will be NULL, and the LOG_DEBUG statement on line 707-708 will attempt to format it with %s, which could cause undefined behavior or a crash.

This reinforces the need for a NULL check immediately after the realpath() call before any use of output or resolved.

Suggested change
char *output = realpath(fpath, resolved);
char *output = realpath(fpath, resolved);
if (output == NULL) {
int rc = -errno;
FSQ_ERROR(*fsq_session, rc,
"realpath() failed for fpath '%s': %s",
fpath, strerror(errno));
return rc;
}

Copilot uses AI. Check for mistakes.
const size_t L_resolved = strlen(resolved);
LOG_DEBUG("fpath: '%s', realpath output: '%s', resolved: '%s'",
fpath, output, resolved);

/* Resolved file path name and Lustre directory name are
matching up to the end of the Lustre directory name. */
LOG_DEBUG("verify lustre_dpath '%s' is a strict prefix of fpath '%s'",
lustre_dpath, fpath);
Comment on lines 712 to 713
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

Misleading debug message: The LOG_DEBUG statement on line 712 still says it's verifying against fpath, but the actual verification below now uses the resolved path. Update the message to reflect this:

LOG_DEBUG("verify lustre_dpath '%s' is a strict prefix of resolved path '%s'",
          lustre_dpath, resolved);
Suggested change
LOG_DEBUG("verify lustre_dpath '%s' is a strict prefix of fpath '%s'",
lustre_dpath, fpath);
LOG_DEBUG("verify lustre_dpath '%s' is a strict prefix of resolved path '%s'",
lustre_dpath, resolved);

Copilot uses AI. Check for mistakes.
for (size_t l = 0; l < L_lustre_dpath; l++) {
if (fpath[l] != lustre_dpath[l]) {
int rc = -EACCES;
FSQ_ERROR(*fsq_session, rc,
"lustre_dpath '%s' is not a strict prefix of fpath '%s'",
lustre_dpath, fpath);
return rc;
}
}

return 0;
/* Make sure lustre_dpath is shorter */
if (L_resolved <= L_lustre_dpath) {
int rc = -EACCES;
FSQ_ERROR(*fsq_session, rc,
"lustre_dpath '%s' is not a strict prefix of fpath '%s'",
lustre_dpath, fpath);
return rc;

Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

[nitpick] Unnecessary blank line before closing brace. This is inconsistent with the rest of the codebase style where error return blocks don't have extra blank lines (see lines 695-701 as an example).

Suggested change

Copilot uses AI. Check for mistakes.
}
Comment on lines +706 to +723
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

Inconsistent indentation: This code block uses spaces for indentation, but the rest of the file uses tabs (see lines 686-692, 695-701, etc.). This should be changed to use tabs to maintain consistency with the existing codebase style.

Copilot uses AI. Check for mistakes.

/* Compare paths to make sure resolved path (resolved) includes
lustre_dpath. */
if (strncmp(resolved, lustre_dpath, L_lustre_dpath) != 0) {
int rc = -EACCES;
FSQ_ERROR(*fsq_session, rc,
"lustre_dpath '%s' is not a strict prefix of fpath '%s'",
lustre_dpath, fpath);
return rc;
}
Comment on lines +727 to +733
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

Potential security vulnerability: The code resolves fpath to an absolute path but compares it against lustre_dpath which is NOT resolved. If lustre_dpath contains symbolic links, an attacker could potentially bypass the path traversal protection.

For example, if lustre_dpath = "/data/storage" but /data is actually a symlink to /other, the resolved fpath would be /other/storage/file.txt while lustre_dpath would still be /data/storage/, causing the comparison to fail even for legitimate paths, or worse, allowing paths under a different /data directory.

Consider resolving lustre_dpath to its canonical form as well before comparison:

char resolved_lustre[PATH_MAX];
char *lustre_output = realpath(lustre_dpath, resolved_lustre);
if (lustre_output == NULL) {
    int rc = -errno;
    FSQ_ERROR(*fsq_session, rc,
              "realpath failed for lustre_dpath '%s'", lustre_dpath);
    return rc;
}

Then compare resolved against resolved_lustre instead.

Copilot uses AI. Check for mistakes.

return 0;
Comment on lines +703 to +735
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The path traversal fix in write_access() function lacks test coverage. Given the critical security nature of this change (preventing path traversal exploits), this should have automated tests to verify:

  1. Paths with .. components are properly rejected
  2. Symlinks pointing outside the allowed directory are rejected
  3. Valid paths within the allowed directory are accepted
  4. Edge cases like paths exactly matching the lustre_dpath length

Consider adding security tests to src/test/ directory to ensure this vulnerability cannot be reintroduced.

Copilot uses AI. Check for mistakes.
}

static int init_fsq_dev_null(char *fpath_local, int *fd_local,
Expand Down Expand Up @@ -1008,19 +1026,19 @@ static void *thread_sock_client(void *arg)
if (fsq_session.fsq_packet.state & FSQ_DISCONNECT)
goto out;

rc = init_fsq_storage(fpath_local, &fd_local, &fsq_session);
rc = write_access(&fsq_session, lustre_dpath);
if (rc) {
FSQ_ERROR(fsq_session, rc,
"init_fsq_storage failed '%s'",
FSQ_STORAGE_DEST_STR(
fsq_session.fsq_packet.fsq_info.fsq_storage_dest));
/* FSQ error field is filled inside write_access function. */
rc = fsq_send(&fsq_session, FSQ_ERROR | FSQ_REPLY);
goto out;
}

rc = write_access(&fsq_session, lustre_dpath);
rc = init_fsq_storage(fpath_local, &fd_local, &fsq_session);
if (rc) {
/* FSQ error field is filled inside write_access function. */
FSQ_ERROR(fsq_session, rc,
"init_fsq_storage failed '%s'",
FSQ_STORAGE_DEST_STR(
fsq_session.fsq_packet.fsq_info.fsq_storage_dest));
rc = fsq_send(&fsq_session, FSQ_ERROR | FSQ_REPLY);
goto out;
}
Expand Down