treat backslash as a path separator in UnsafePath by rajath201 · Pull Request #364 · google/cctz

rajath201 · 2026-07-30T12:10:24Z

Repro: on Windows, load_time_zone("file:America\..\America/Los_Angeles") walks out of TZDIR and back in and loads. The same name spelled with / is rejected.
Cause: UnsafePath splits the name on / only, so a ..\ component matches none of its four literal checks, though Windows takes \ as a separator too. Nothing downstream canonicalizes the name, so whatever clears the guard is what FOpen resolves.
Fix: look for a .. component delimited by either separator or a string boundary.

I diffed the shipped function against a component-based reference over every string up to length 7 drawn from {'.', '/', '', 'a'}, at pos 0 and 1. Under /-only semantics it is exact: 0 missed, 0 over-rejected. Once \ also separates, 2878 of those 43689 names are admitted, including ..\, \.., a\.. and ..\etc\passwd. The replacement is 0/0 under both, and its verdict is unchanged on non-Windows for every case. All 485 zones in testdata/zoneinfo still load.

Both file-backed loaders funnel through this helper, so one fix covers them. AndroidZoneInfoSource matches the name against a tzdata index instead of building a path, so it needs nothing.

devbww · 2026-07-30T16:54:49Z

+// 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)


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.

Done, dropped the _WIN64 test.

Right, _WIN64 implies _WIN32. Dropped the second test.

devbww · 2026-07-30T17:02:52Z

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


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.

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

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

devbww · 2026-07-30T17:05:20Z

+
+// Returns true if the zone name starting at pos contains an unsafe path,
+// that is, a ".." component that would escape the zoneinfo directory.
 inline bool UnsafePath(const std::string& name, std::size_t pos) {


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.

devbww · 2026-07-30T17:07:29Z

-  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) {


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

Right, done as part of the reformulation below.

Done, folded into your formulation below.

devbww · 2026-07-30T17:15:59Z

+  for (std::size_t i = pos; (i = name.find("..", i)) != std::string::npos;
+       ++i) {
+    if (i != pos && !IsPathSeparator(name[i - 1])) continue;  // e.g., "a.."
+    if (i + 2 != name.size() && !IsPathSeparator(name[i + 2])) continue;


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.

Done, same.

Agreed, gone with the rewrite below.

devbww · 2026-07-30T17:22:22Z

+  for (std::size_t i = pos; (i = name.find("..", i)) != std::string::npos;
+       ++i) {
+    if (i != pos && !IsPathSeparator(name[i - 1])) continue;  // e.g., "a.."
+    if (i + 2 != name.size() && !IsPathSeparator(name[i + 2])) continue;
    return true;
  }


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.

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.

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.

devbww · 2026-07-30T17:34:58Z

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


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?

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.

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.

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.

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.

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.

treat backslash as a path separator in UnsafePath

5412235

devbww suggested changes Jul 30, 2026

View reviewed changes

rajath201 added 2 commits August 1, 2026 14:51

rework UnsafePath loop per review, scope backslash tests to Windows

4e2bb4b

scope non-regular-file expectations to non-MSVC builds

226a2b0

devbww approved these changes Aug 5, 2026

View reviewed changes

Uh oh!

Conversation

rajath201 commented Jul 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants