Prevent path traversal fix #3

lowBudgetHypothermia · 2025-10-21T15:32:18Z

For review by C.Tacke

dennisklein · 2025-12-09T10:50:11Z

src/fsqd.c

-	   up to the end of the Lustre directory name. */
+	/* Resolve path to absolute path */
+	char resolved[PATH_MAX];
+	char *output = realpath(fpath, resolved);


Shouldn't we check for an error here?

Copilot

Pull request overview

This PR addresses a critical path traversal security vulnerability in the file storage queue daemon (fsqd) by implementing absolute path resolution and improving path validation logic.

Key Changes:

Introduces realpath() to resolve file paths to their canonical absolute form, preventing path traversal attacks via .. or symbolic links
Replaces character-by-character path validation loop with strncmp() for cleaner comparison logic
Reorders validation sequence to check write access before initializing storage, improving security and error handling flow

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
src/fsqd.c	Core security fix: adds path canonicalization via `realpath()`, refactors path prefix validation, and reorders write access validation to occur before storage initialization
rpm/fsq.spec	RPM changelog entries for versions 0.9.6 (strncmp refactor) and 0.9.5 (path traversal fix)
debian/changelog	Debian changelog entries corresponding to the RPM spec updates

Critical Issues Identified:

Missing NULL check for realpath() return value could cause crashes and undefined behavior
The lustre_dpath should also be canonicalized to prevent symlink-based bypass
Missing automated tests for this critical security fix
Inconsistent indentation using spaces instead of tabs

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-09T10:57:16Z

src/fsqd.c

-	   up to the end of the Lustre directory name. */
+	/* Resolve path to absolute path */
+	char resolved[PATH_MAX];
+	char *output = realpath(fpath, resolved);


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 · 2025-12-09T10:57:16Z

src/fsqd.c

+    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);
-	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;
+
+    }


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 · 2025-12-09T10:57:16Z

src/fsqd.c

+              "lustre_dpath '%s' is not a strict prefix of fpath '%s'",
+              lustre_dpath, fpath);
+        return rc;
+


[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 · 2025-12-09T10:57:16Z

src/fsqd.c

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


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 · 2025-12-09T10:57:17Z

src/fsqd.c

+	/* Resolve path to absolute path */
+	char resolved[PATH_MAX];
+	char *output = realpath(fpath, resolved);
+    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);
-	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;
+
+    }
+
+    /* 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;
+    }
+
+    return 0;


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:

Paths with .. components are properly rejected

Symlinks pointing outside the allowed directory are rejected

Valid paths within the allowed directory are accepted

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 · 2025-12-09T10:57:17Z

src/fsqd.c

+    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;
+    }


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 · 2025-12-09T10:57:17Z

src/fsqd.c

-	   up to the end of the Lustre directory name. */
+	/* Resolve path to absolute path */
+	char resolved[PATH_MAX];
+	char *output = realpath(fpath, resolved);


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;

}

Implemented absolute path resolution using realpath function to prevent path traversal using client provided file paths. This is done when checking for write access.

ChristianTackeGSI · 2025-12-17T09:42:28Z

@dennisklein I haven't yet looked at the whole stub thing.

And as talked yesterday: I really like this, because it means, that we can run CI (even on Github Actions), etc.

Could we have the stub thing in a dedicated PR, please? Either it's easy enough, then we merge it before this one.

What do you think?

dennisklein reviewed Dec 9, 2025

View reviewed changes

dennisklein requested a review from Copilot December 9, 2025 10:51

Copilot started reviewing on behalf of dennisklein December 9, 2025 10:51 View session

Copilot AI reviewed Dec 9, 2025

View reviewed changes

lowBudgetHypothermia requested review from ChristianTackeGSI and dennisklein and removed request for dennisklein December 9, 2025 17:01

s.hasert added 3 commits December 16, 2025 19:35

Added absolute path resolution to prevent path traversal exploit

1871f23

Implemented absolute path resolution using realpath function to prevent path traversal using client provided file paths. This is done when checking for write access.

Updated fsq.spec for rpm

70c22f8

Changed path comparison to strncmp

0e07c4c

dennisklein force-pushed the prevent_path_traversal_fix branch from ec4983b to 0e07c4c Compare December 16, 2025 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Prevent path traversal fix #3

Prevent path traversal fix #3

Uh oh!

lowBudgetHypothermia commented Oct 21, 2025

Uh oh!

dennisklein Dec 9, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

ChristianTackeGSI commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

Prevent path traversal fix #3

Are you sure you want to change the base?

Prevent path traversal fix #3

Uh oh!

Conversation

lowBudgetHypothermia commented Oct 21, 2025

Uh oh!

dennisklein Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

ChristianTackeGSI commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants