Avoid int overflow after year 2038

[bmwiedemann]

We interpret krb5 timestamps as uint32_t
as recommended in
https://web.mit.edu/kerberos/krb5-devel/doc/appdev/refs/types/krb5_timestamp.html to make the code work until year 2106.

note: I only tested that it compiles and passes tests now. Without this patch, I observed failures in test_sysdb_sudo and test_sudo_set_get_last_full_refresh with negative epoch values in the output.

This patch was done while reviewing potential year-2038 issues in openSUSE.

[gemini-code-assist]

Code Review

This pull request introduces explicit casts to uint32_t for various timestamps and expiration times across several files to handle time-related values. However, the review identifies critical issues with these changes: casting the result of ldb_msg_find_attr_as_int in sysdb_sudo.c does not prevent integer overflow or clamping for post-2038 timestamps, where ldb_msg_find_attr_as_uint64 should be used instead; the subtraction in krb5_child.c can lead to unsigned wrap-around on 32-bit platforms, bypassing expiration checks; and the subtraction in ldap_child.c risks unsigned underflow unless cast to time_t first.

[sssd-bot]

Review done using Claude Code / claude-opus-4-6

Functional Issues

Mixed signed/unsigned subtraction on 32-bit platforms in krb5_child.c

At src/providers/krb5/krb5_child.c:316, the expression (uint32_t)password_expiration - time(NULL) is dangerous on 32-bit platforms where time_t is int32_t. C's usual arithmetic conversions promote the int32_t operand to uint32_t, causing the subtraction to use unsigned arithmetic. If the password has already expired (password_expiration < time(NULL)), the result wraps to a large positive value instead of going negative, making the exp_time < 0 check on line 317 ineffective — expired passwords would be treated as valid.

Fix: perform the subtraction in a wide signed type:

exp_time = (long long)(uint32_t)password_expiration - (long long)time(NULL);

Format string mismatch in krb5_child.c

At src/providers/krb5/krb5_child.c:321, the type of exp_time was changed from long to long long, but the format specifier in the DEBUG call remains %ld. This is undefined behavior — it should be %lld (or, to be portable, cast to (long long) and use %lld, or use PRId64).

ldb_msg_find_attr_as_int cannot represent post-2038 values in sysdb_sudo.c

At src/db/sysdb_sudo.c:677, casting the return value of ldb_msg_find_attr_as_int to uint32_t does not fully address the Y2038 problem. ldb_msg_find_attr_as_int returns a signed int — on systems where int is 32-bit, values above INT32_MAX are already clamped or misinterpreted before the cast is applied. The cast works by accident on 64-bit Linux (where strtol returns a 64-bit long that gets truncated to 32-bit int, and the bit pattern happens to survive the uint32_t cast), but this is fragile and non-portable.

Additionally, this is not a krb5_timestamp — it is a time_t value representing the last sudo refresh time. The uint32_t reinterpretation recommended by MIT for krb5_timestamp does not apply here. A proper fix should use ldb_msg_find_attr_as_int64 or ldb_msg_find_attr_as_uint64 to correctly read potentially large timestamp values.

Unsigned underflow risk in ldap_child.c

At src/providers/ldap/ldap_child.c:778, (uint32_t)my_creds.times.endtime - kdc_time_offset casts only the left operand. kdc_time_offset is krb5_timestamp (signed 32-bit). If the compiler promotes both to uint32_t for the subtraction, and kdc_time_offset happens to exceed endtime (unlikely but possible with large clock skew), unsigned wraparound produces a nonsensical large value assigned to *expire_time_out (time_t).

Fix: widen to time_t before subtraction so it uses signed 64-bit arithmetic:

*expire_time_out = (time_t)(uint32_t)my_creds.times.endtime - kdc_time_offset;

Nits & Non-functional Issues

Incomplete coverage of krb5_timestamp usage

The PR addresses four call sites, but SSSD uses krb5_timestamp values in many other places (KCM credential storage, TGT renewal tracking, child process communication). A grep for krb5_timestamp and .times. across the codebase reveals additional sites that would need the same treatment for consistent Y2038 handling. This is noted for awareness — it may be acceptable to address incrementally.

No :relnote: tag in commit message

The fix changes how Kerberos timestamps are interpreted and directly fixes test failures (test_sysdb_sudo and test_sudo_set_get_last_full_refresh producing negative epoch values). If this is considered a user-visible bug fix, a :relnote: tag should be added to the commit message.

Missing test verification

The PR author notes "I only tested that it compiles and passes tests now." There is no new test coverage for the Y2038 boundary behavior. Existing tests that exercise values beyond 2038 (like the ones the author mentions were failing) serve as regression tests, but explicit boundary tests (e.g., timestamp 0x80000001) would strengthen confidence.

Review of Existing Review Comments

The Gemini Code Assist bot posted three comments, all of which raise valid concerns:

• sysdb_sudo.c:677 (Critical): Correctly identifies that ldb_msg_find_attr_as_int returns a signed int and internal strtol clamping makes the outer cast insufficient. The suggestion to use ldb_msg_find_attr_as_uint64 is sound, though ldb_msg_find_attr_as_int64 would also work and better match the semantic intent.

• krb5_child.c:316 (Critical): Correctly identifies the unsigned arithmetic hazard on 32-bit platforms. The suggested fix to cast both operands to long long before subtraction is appropriate.

• ldap_child.c:778 (High): Correctly identifies the unsigned underflow risk. The suggested fix to cast to time_t before subtraction is appropriate and simpler than widening both operands.

All three comments remain unaddressed in the current revision.

[bmwiedemann]

I added the 3 suggestions from Gemini and the %ld => %lld one from Claude. I'll leave reviewing for other incorrect usage of krb5_timestamp as signed int32 to you.