Skip to content

treat backslash as a path separator in UnsafePath #364

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
rajath201 wants to merge 3 commits into
base: master
Choose a base branch
from
+31 −12
Open

treat backslash as a path separator in UnsafePath #364

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5412235
treat backslash as a path separator in UnsafePath
rajath201 Jul 30, 2026
4e2bb4b
rework UnsafePath loop per review, scope backslash tests to Windows
rajath201 Aug 1, 2026
226a2b0
scope non-regular-file expectations to non-MSVC builds
rajath201 Aug 4, 2026
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
26 changes: 16 additions & 10 deletions src/time_zone_info.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,17 +418,23 @@ inline FilePtr FOpen(const char* path) {
#endif
}

// Returns true if the zone name starting at pos contains an unsafe path.
// Returns true if c separates path components. Windows accepts either
// form, so a "..\" component walks up a directory just like a "../" one.
inline bool IsPathSeparator(char c) {
#if defined(_WIN32) || defined(_WIN64)

@devbww devbww Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As I understand things, _WIN32 => _WIN64, so there is no need to test the latter symbol.

It looks like we have used the double test 11 out of 14 times, but that doesn't mean we should propagate it further.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, dropped the _WIN64 test.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, _WIN64 implies _WIN32. Dropped the second test.

return c == '/' || c == '\\';
#else
return c == '/';
#endif
}

// Returns true if the zone name starting at pos contains an unsafe path,
// that is, a ".." component that would escape the zoneinfo directory.

@devbww devbww Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When we recently added this function, I suggested framing it as "unsafe" rather than "contains .. component" so that it might admit additional "unsafe" reasons in the future. Therefore, I wouldn't add a function-level comment suggesting ".." is the one and only reason.

If you think a ".." comment is still necessary, I'd move it to the code implementing that criterion within the function.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Restored the original function comment and moved the ".." note onto the check inside the loop.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Restored the plain "unsafe path" function comment and moved the ".." specifics onto the loop inside.

inline bool UnsafePath(const std::string& name, std::size_t pos) {

@devbww devbww Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When this function was first added it was only called from one place, which, I assume, prompted the inline.

But now that #355 added another call site, we should remove the inline.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed.

// Path traversal: exact match ".."
if (name.compare(pos, std::string::npos, "..") == 0) return true;
// Path traversal: leading component "../"
if (name.compare(pos, 3, "../") == 0) return true;
// Path traversal: interior component "/../"
if (name.find("/../", pos) != std::string::npos) return true;
// Path traversal: trailing component "/.."
if (name.size() - pos >= 3 &&
name.compare(name.size() - 3, 3, "/..") == 0) {
for (std::size_t i = pos; (i = name.find("..", i)) != std::string::npos;
++i) {

@devbww devbww Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the ".." we found at position i was safe, then a ".." at position i + 1 will also be safe (because it is preceded by a '.').

Therefore, s/++i/i += 2/.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, done as part of the reformulation below.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, folded into your formulation below.

if (i != pos && !IsPathSeparator(name[i - 1])) continue; // e.g., "a.."
if (i + 2 != name.size() && !IsPathSeparator(name[i + 2])) continue;

@devbww devbww Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While, at this point, we know name.size() >= 2 (because we found a ".."), we don't know (theoretically) that i <= SIZE_MAX - 2. So, I would formulate the first conditional as i != name.size() - 2 to avoid any odor of overflow.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, same.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, gone with the rewrite below.

return true;
}

@devbww devbww Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Even though it computes the same function, I find this much clearer as ...

  for (auto i = pos; (i = name.find("..", i)) != std::string::npos; i += 2) {
    if ((i == pos || IsPathSeparator(name[i - 1])) &&
        (i == name.size() - 2 || IsPathSeparator(name[i + 2]))) {
      return true;
    }
  }

That is, return true if the ".." is: at the beginning or preceded by a separator AND at the end or followed by a separator.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, took this as written. Reran the brute force against the component-based reference to be safe: still 0 missed and 0 over-rejected across all 43689 names under either separator set, and no verdict changes on non-Windows.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is clearer, took it verbatim. To be safe I re-ran the differential check against a component-based reference over every string up to length 7 from {'.', '/', '', 'a'} at pos 0 and 1: still 0 missed and 0 over-rejected under both separator sets, and no verdict change from the old code under /-only semantics.

return false;
Expand Down
4 changes: 4 additions & 0 deletions src/time_zone_lookup_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,10 @@ TEST(TimeZone, Failures) {
EXPECT_FALSE(load_time_zone("file:/../etc/passwd", &tz));
EXPECT_FALSE(load_time_zone("file:America/../America/Los_Angeles", &tz));

// Windows accepts '\' as a path separator, so those escape as well.
EXPECT_FALSE(load_time_zone("file:..\\etc\\passwd", &tz));
EXPECT_FALSE(load_time_zone("file:America\\..\\America/Los_Angeles", &tz));

@devbww devbww Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In #353 I expressed concern that these load_time_zone() tests can/do fail for reasons other than path and file-type restrictions. Here, for example, in non-Windows environments we fail because the files do not exist, not because the paths are unsafe.

Is there someway to formulate all of these such that they are actually testing what they look like they're testing? Should we introduce #ifs?

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wrapped the two new ones in #if defined(_WIN32) with a note. On Windows they do check the guard: if it admitted America..\America/Los_Angeles, the name resolves back into TZDIR and loads, which would fail the EXPECT_FALSE. Elsewhere backslash is an ordinary filename character, so they could only ever fail as nonexistent names, as you say. The existing America/../America/Los_Angeles line has that same self-checking property on every platform since TZDIR points at real data here; the absolute ones like /../etc/passwd do lean on the target not parsing as TZif. Making those hermetic would mean exposing UnsafePath to the test, which seems like more surface than it is worth.

@rajath201 rajath201 Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The formulation that actually proves a rejection is a name that would otherwise load: escape TZDIR and come back in, ending at a real zone. file:America/../America/Los_Angeles already has that property here (with TZDIR set, removing the check makes it load, which fails the test), and file:America..\America/Los_Angeles is the Windows analogue. So I scoped the backslash lines to #if defined(_WIN32), since elsewhere '' is an ordinary filename character and they could only pass as nonexistent names, and the comment now notes which line proves the rejection. The ../etc/passwd style cannot distinguish "blocked" from "not found" on any platform, so those are smoke tests at best. I can rework the earlier block along the same escape-and-return lines in a follow-up if you want.

@devbww devbww Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I scoped the backslash lines to #if defined(_WIN32), since elsewhere '' is an ordinary filename character and they could only pass as nonexistent names, and the comment now notes which line proves the rejection. The ../etc/passwd style cannot distinguish "blocked" from "not found" on any platform, so those are smoke tests at best. I can rework the earlier block along the same escape-and-return lines in a follow-up if you want.

I think what you have done is fine for now. Thanks.

(Although, the "non-regular files and directories" expectations should probably be conditionalized on #if !defined(_MSC_VER) to also match their implementation.)

In the big picture, though, perhaps we should start a conversation with @derekmauro about what this whole UnsafePath() thing is trying to achieve. The description of #353 said, 'An attacker-controlled zone name like "../../../../../../tmp/evil" escapes TZDIR,' but I didn't ask enough questions before so I'm not sure what is wrong with that. An attacker-controlled absolute name would also escape TZDIR. Should absolute names be excluded from the ".." check, or should absolute names be rejected altogether? (The latter would currently break local_time_zone(), which uses load_time_zone("/etc/localtime", ...)). But instead of all that, I'm thinking it is rather just up to the application to eliminate the possibility of "an attacker-controlled zone name" without any "help" from the library.

@rajath201 rajath201 Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, wrapped the non-regular-file expectations in #if !defined(_MSC_VER) to match FOpen.

On the bigger question, I don't have a strong opinion on where the line belongs. Rejecting absolute names outright would break local_time_zone() as you note, and an application taking untrusted zone names has to vet them anyway. The value I see in the '..' check is narrower: for a relative name it keeps the resolved path under the configured prefix, so a TZ-style value can't reach outside the zoneinfo tree, and it costs one scan of the name at load. Whether that belongs in the library or the application seems worth the conversation with Derek; this PR just makes the existing check mean the same thing on Windows.

@devbww devbww Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds good to me. Thanks again.

I'll leave it to @derekmauro as to whether he wants to discuss the question here, or perhaps some time in the future.


// Reject non-regular files and directories.
EXPECT_FALSE(load_time_zone("file:/dev/null", &tz));
EXPECT_FALSE(load_time_zone("file:/dev/stdin", &tz));
Expand Down