Skip to content

util: validate Duration/Timestamp in TimeUtil::FromString against spec limits#27443

Closed
jortles wants to merge 3 commits into
protocolbuffers:mainfrom
jortles:fix/timeutil-fromstring-validation
Closed

util: validate Duration/Timestamp in TimeUtil::FromString against spec limits#27443
jortles wants to merge 3 commits into
protocolbuffers:mainfrom
jortles:fix/timeutil-fromstring-validation

Conversation

@jortles

@jortles jortles commented May 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • TimeUtil::FromString for both Duration and Timestamp accepts malformed inputs that produce protos failing IsDurationValid/IsTimestampValid.
  • The companion JSON parser (JsonStringToMessage) correctly rejects all the same inputs, creating a cross-API divergence.
  • 8 sub-bug classes identified (4 Duration, 4 Timestamp):
Class Type Trigger Effect
A Duration Leading whitespace + negative (" -3.54s") Signs mismatch: seconds<0, nanos>0
B Duration Double minus ("--3.54s") Signs mismatch: seconds>0, nanos<0
C Duration >9 fractional digits ("0.1234567890s") nanos exceeds 999999999
D Duration seconds > 315576000000 ("315576000001s") Out-of-range seconds
E Timestamp Year 0000 ("0000-12-31T23:59:59Z") seconds < kTimestampMinSeconds
F Timestamp Year 10000 ("10000-01-01T00:00:00Z") seconds > kTimestampMaxSeconds
G Timestamp 5-digit year ("99999-01-01T00:00:00Z") Wildly past spec max
H Timestamp Negative year ("-0001-01-01T00:00:00Z") Not in RFC3339 spec

Fix: Add IsDurationValid() check after parsing in the Duration overload, and IsTimestampValid() check in the Timestamp overload. Return false and clear output on failure. Both validation functions already exist in TimeUtil and are used by ABSL_DCHECK elsewhere.

Impact: Systems using Duration/Timestamp for security-relevant time bounds (gRPC deadlines, Envoy timeouts, retry intervals, IAM token TTLs). Class A is most dangerous: seconds * 1e9 + nanos gives -2.46e9 instead of intended -3.54e9 — a 31% magnitude shift. 17 fuzzer crash inputs collected.

Found by AFL++ property-check fuzzing (pb_json_wkt_fuzzer mode 1).

Test plan

  • Verify TimeUtil::FromString(" -3.54s", &d) returns false (class A)
  • Verify TimeUtil::FromString("--3.54s", &d) returns false (class B)
  • Verify TimeUtil::FromString("0.1234567890s", &d) returns false (class C)
  • Verify TimeUtil::FromString("315576000001s", &d) returns false (class D)
  • Verify TimeUtil::FromString("0000-12-31T23:59:59Z", &t) returns false (class E)
  • Verify TimeUtil::FromString("10000-01-01T00:00:00Z", &t) returns false (class F)
  • Verify valid inputs still parse successfully
  • Run existing TimeUtil tests to confirm no regressions

@google-cla

google-cla Bot commented May 18, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jortles
jortles force-pushed the fix/timeutil-fromstring-validation branch from 0e70dd4 to e7f292a Compare May 18, 2026 14:36
@rgoldfinger6 rgoldfinger6 added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 18, 2026
@rgoldfinger6
rgoldfinger6 requested a review from tonyliaoss May 18, 2026 20:33
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 18, 2026
@tonyliaoss tonyliaoss added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 19, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 19, 2026
@rgoldfinger6 rgoldfinger6 added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 21, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 21, 2026
@rgoldfinger6
rgoldfinger6 force-pushed the fix/timeutil-fromstring-validation branch from 14ba6cb to b6c8f3f Compare May 21, 2026 17:28
@rgoldfinger6 rgoldfinger6 added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 21, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 21, 2026
@jortles

jortles commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

Added two test cases and a fix:

Tests (FromStringRejectsMalformedDuration, FromStringRejectsMalformedTimestamp):

  • Duration class A: " -3.54s" (leading whitespace + negative)
  • Duration class B: "--3.54s" (double minus)
  • Duration class C: "0.1234567890s" (>9 fractional digits)
  • Duration class D: "315576000001s" (out of range)
  • Timestamp class E: "0000-12-31T23:59:59Z" (year 0000)
  • Timestamp class F: "10000-01-01T00:00:00Z" (year 10000)
  • Plus positive tests confirming valid inputs still parse

Fix: Moved seconds range check before CreateNormalizedTimestamp() in the Timestamp overload. The original placement called CreateNormalizedTimestamp() first (which has an ABSL_DCHECK on range), then checked IsTimestampValid() — but the DCHECK fires before the validation can return false. Now we reject out-of-range seconds before calling CreateNormalizedTimestamp().

All 15 tests in time_util_test pass locally.

@jortles
jortles force-pushed the fix/timeutil-fromstring-validation branch from 46427d8 to b53f717 Compare May 26, 2026 18:32
jortles added 2 commits June 5, 2026 15:16
@jortles @tonyliaoss
util: validate Duration/Timestamp in TimeUtil::FromString against spe…
19db95e 
…c limits

TimeUtil::FromString accepted malformed inputs that produce protos
failing IsDurationValid/IsTimestampValid.  This includes:
- Duration with mismatched signs (seconds<0, nanos>0)
- Duration with >9 fractional digits (nanos > 999999999)
- Duration with seconds > 315576000000
- Timestamp with year 0000, 10000+, 5-digit years, negative years

The companion JSON parser (JsonStringToMessage) correctly rejects
all these inputs, creating a cross-API divergence.

Add IsDurationValid/IsTimestampValid checks after parsing in each
FromString overload.  Return false and clear output on failure.

Found by: AFL++ property-check fuzzing

Signed-off-by: Anthony Hurtado <[email protected]>
@jortles @tonyliaoss
Add validation tests and fix DCHECK ordering for FromString
0a44a86 
Adds tests for all 8 malformed input classes:
- Duration: leading whitespace + negative, double minus, >9 fractional
  digits, seconds out of range
- Timestamp: year 0000, year 10000

Also moves the seconds range check before CreateNormalizedTimestamp()
to avoid hitting its DCHECK in debug builds before the IsTimestampValid
check can run.
@tonyliaoss
tonyliaoss force-pushed the fix/timeutil-fromstring-validation branch from b53f717 to 0a44a86 Compare June 5, 2026 19:16
@tonyliaoss tonyliaoss added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 5, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 5, 2026
@karenwuz

karenwuz commented Jun 9, 2026

Copy link
Copy Markdown
Member

@jortles looks like cmake test is failing, can you fix?

@tonyliaoss

tonyliaoss commented Jun 10, 2026

Copy link
Copy Markdown
Member

There seems to be a CLA check error. Could we resolve this before continuing? Thanks!

https://github.com/protocolbuffers/protobuf/pull/27443/checks?check_run_id=80377388216

@tonyliaoss
tonyliaoss force-pushed the fix/timeutil-fromstring-validation branch from df20a69 to 20fca1a Compare June 10, 2026 14:38
@jortles
fix: use absl::ToUnixSeconds to avoid 32-bit time_t truncation
b57f445 
On 32-bit platforms, absl::ToTimespec clamps to INT32_MAX/MIN when
seconds overflow time_t. This silently converts out-of-range
timestamps into in-range values, causing validation to pass for
years like 0000 and 10000.

Replace ToTimespec with ToUnixSeconds (returns int64_t) so the full
64-bit seconds value reaches the range checks.
@jortles
jortles force-pushed the fix/timeutil-fromstring-validation branch from 20fca1a to b57f445 Compare June 24, 2026 21:18
@haberman haberman added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 25, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonyliaoss tonyliaoss tonyliaoss approved these changes

Assignees

@jortles jortles

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jortles @karenwuz @tonyliaoss @haberman @rgoldfinger6