Skip to content
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.

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

The call to localtime_r may be unsound #499

Closed
quininer opened this issue Nov 10, 2020 · 42 comments · Fixed by #677
Closed

The call to localtime_r may be unsound #499

quininer opened this issue Nov 10, 2020 · 42 comments · Fixed by #677

Comments

@quininer
Copy link

I found that getenv and setenv in libc are not thread-safe [1], and most impl of localtime_r in libc directly call getenv [2]. This means that localtime_r may have data race with setenv.

In order to ensure soundness of setenv, libstd add a lock to it [1], but this means that using getenv without libstd will be unsound.

This problem is not easy to reproduce on glibc, because glibc's localtime_r caches timezone. but using musl can easily reproduce it.

  1. libstd: Add thread unsafety warnings around setenv() and unsetenv() rust-lang/rust#24741
  2. https://github.com/aosp-mirror/platform_bionic/blob/master/libc/tzcode/localtime.c#L1321 and https://git.musl-libc.org/cgit/musl/tree/src/time/__tz.c#n127

POC: https://gist.github.com/quininer/2063c31b0bc1753989122e782b182bea

@dekellum
Copy link
Contributor

Rust libstd has a similar, long open issue, for its own calls to getaddrinfo: rust-lang/rust#27970

@quodlibetor
Copy link
Contributor

Thank you for this!

From reading the associated issues, it seems like touching the environment is the thing that glibc actually considers to be non-thread-safe, is that right? In particular rust-lang/rust#27970 (comment) in particular, it seems like libc wants modifying the environment to be illegal in a multithreaded environment:

functions that modify the environment are all marked with const:env and regarded as unsafe. Being unsafe, the latter are not to be called when multiple threads are running or asynchronous signals are enabled, and so the environment can be considered effectively constant in these contexts, which makes the former safe.

I agree that the solution in the rust stdlib getaddrinfo issue (being able to take the env lock) would help.

I'm trying to think of a good solution here, and it's hard because none of the underlying libraries (libc or stdlib) provide us with the necessary tools. In particular, even if the stdlib provided us with access to the env lock, any other code that interacts with libc directly could inadvertently call setenv and leave us in the same situation. That would qualify as going outside of the safety properties rust is supposed to provide, though.

@Arnavion
Copy link

Arnavion commented Oct 11, 2021

For anyone landing here from CVE-2020-26235 that states:

In Rust time crate from version 0.2.7 and before version 0.2.23, unix-like operating systems may segfault due to dereferencing a dangling pointer in specific circumstances. [...] The issue was introduced in version 0.2.7 and fixed in version 0.2.23.

... the CVE text is misleading and the issue affects time 0.1 as used by chrono too. See time-rs/time#293 (comment) for details.

Edit: But also, note that the time 0.1 dependency doesn't actually matter because the call of locatime_r happens directly by chrono, not through time. See #602 (comment)

jonasbb added a commit to jonasbb/serde_with that referenced this issue Oct 18, 2021
RUSTSEC-2020-0159 is about chrono calling localtime_r.
Right now there is no safe version, see this issue:
chronotope/chrono#499
This was referenced Oct 19, 2021
@djc djc closed this as completed in #677 Jun 9, 2022
koenw added a commit to paritytech/ci-script that referenced this issue Jun 23, 2022
We don't actually need it and there seems to be a security issue that
has been open for years:
chronotope/chrono#499.

Chrono is still present in `Cargo.lock` though, so it seems one of our
dependencies still uses chrono, but I'll leave that for another day.
lopopolo added a commit to artichoke/artichoke that referenced this issue Aug 5, 2022
See the release announcement:

- https://github.com/chronotope/chrono/releases/tag/v0.4.20

It looks like the fix for RUSTSEC-2020-0159 vendors much of the relevant
code from `tz-rs` (which Artichoke already uses):

- chronotope/chrono#677

Previous `cargo deny` error (I think this started triggering today now
that there is a fixed version out):

```console
$ cargo deny check
error[A001]: Potential segfault in `localtime_r` invocations
   ┌─ /Users/lopopolo/dev/artichoke/artichoke/Cargo.lock:15:1
   │
15 │ chrono 0.4.19 registry+https://github.com/rust-lang/crates.io-index
   │ ------------------------------------------------------------------- security vulnerability detected
   │
   = ID: RUSTSEC-2020-0159
   = Advisory: https://rustsec.org/advisories/RUSTSEC-2020-0159
   = ### Impact

     Unix-like operating systems may segfault due to dereferencing a dangling pointer in specific circumstances. This requires an environment variable to be set in a different thread than the affected functions. This may occur without the user's knowledge, notably in a third-party library.

     ### Workarounds

     No workarounds are known.

     ### References

     - [time-rs/time#293](time-rs/time#293)
   = Announcement: chronotope/chrono#499
   = Solution: Upgrade to >=0.4.20
   = chrono v0.4.19
     ├── chrono-tz v0.6.1
     │   └── spinoso-time v0.5.0
     │       └── artichoke-backend v0.13.0
     │           └── artichoke v0.1.0-pre.0
     └── spinoso-time v0.5.0 (*)

advisories FAILED, bans ok, licenses ok, sources ok
```
jrguzman-ms pushed a commit to msft-mirror-aosp/platform.bionic that referenced this issue Jun 17, 2023
* Rationale

The question often comes up of how to use multiple time zones in C code.
If you're single-threaded, you can just use setenv() to manipulate $TZ.
toybox does this, for example. But that's not thread-safe in two
distinct ways: firstly, getenv() is not thread-safe with respect to
modifications to the environment (and between the way putenv() is
specified and the existence of environ, it's not obvious how to fully
fix that), and secondly the _caller_ needs to ensure that no other
threads are using tzset() or any function that behaves "as if" tzset()
was called (which is neither easy to determine nor easy to ensure).

This isn't a bigger problem because most of the time the right answer
is to stop pretending that libc is at all suitable for any i18n, and
switch to icu4c instead. (The NDK icu4c headers do not include ucal_*,
so this is not a realistic option for most applications.)

But what if you're somewhere in between? Like the rust chrono library,
for example? What then?

Currently their "least worst" option is to reinvent the entire wheel and
read our tzdata files. Which isn't a great solution for anyone, for
obvious maintainability reasons.

So it's probably time we broke the catch-22 here and joined NetBSD in
offering a less broken API than standard C has for the last 40 years.
Sure, any would-be caller will have to have a separate "is this
Android?" and even "is this API level >= 35?" path, but that will fix
itself sometime in the 2030s when developers can just assume "yes, it
is", whereas if we keep putting off exposing anything, this problem
never gets solved.

(No-one's bothered to try to implement the std::chrono::time_zone
functionality in libc++ yet, but they'll face a similar problem if/when
they do.)

* Implementation

The good news is that tzcode already implements these functions, so
there's relatively little here.

I've chosen not to expose `struct state` because `struct __timezone_t`
makes for clearer error messages, given that compiler diagnostics will
show the underlying type name (`struct __timezone_t*`) rather than the
typedef name (`timezone_t`) that's used in calling code.

I've moved us over to FreeBSD's wcsftime() rather than keep the OpenBSD
one building --- I've long wanted to only have one implementation here,
and FreeBSD is already doing the "convert back and forth, calling the
non-wide function in the middle" dance that I'd hoped to get round to
doing myself someday. This should mean that our strftime() and
wcsftime() behaviors can't easily diverge in future, plus macOS/iOS are
mostly FreeBSD, so any bugs will likely be interoperable with the other
major mobile operating system, so there's something nice for everyone
there!

The FreeBSD wcsftime() implementation includes a wcsftime_l()
implementation, so that's one stub we can remove. The flip side of that
is that it uses mbsrtowcs_l() and wcsrtombs_l() which we didn't
previously have. So expose those as aliases of mbsrtowcs() and
wcsrtombs().

Bug: chronotope/chrono#499
Test: treehugger
Change-Id: Iee1b9d763ead15eef3d2c33666b3403b68940c3c
jrguzman-ms pushed a commit to msft-mirror-aosp/platform.bionic that referenced this issue Jun 23, 2023
This works (by reading /etc/localtime) on NetBSD, but not on Android
since we have no such file. Fix that by using our equivalent system
property instead.

Also s/time zone/timezone/ in documentation and comments. We've always
been inconsistent about this (as is upstream in code comments and
documentation) but it seems especially odd now we expose a _type_ that
spells it "timezone" to talk of "time zone" even as we're describing
that type and its associated functions.

Bug: chronotope/chrono#499
Test: treehugger
Change-Id: I142995a3ab4deff1073a0aa9e63ce8eac850b93d
sepehrst pushed a commit to spsforks/android-bionic-libc that referenced this issue Apr 22, 2024
* Rationale

The question often comes up of how to use multiple time zones in C code.
If you're single-threaded, you can just use setenv() to manipulate $TZ.
toybox does this, for example. But that's not thread-safe in two
distinct ways: firstly, getenv() is not thread-safe with respect to
modifications to the environment (and between the way putenv() is
specified and the existence of environ, it's not obvious how to fully
fix that), and secondly the _caller_ needs to ensure that no other
threads are using tzset() or any function that behaves "as if" tzset()
was called (which is neither easy to determine nor easy to ensure).

This isn't a bigger problem because most of the time is to stop
pretending that libc is at all suitable for any i18n, and switch to
icu4c instead. (The NDK icu4c headers do include ucal_*, so this is a
somewhat realistic option for applications.)

But what if you're somewhere in between? Like the rust chrono library,
for example? What then?

Currently their "least worst" option is to reinvent the entire wheel and
read our tzdata files. Which isn't a great solution for anyone, for
obvious maintainability reasons.

So it's probably time we broke the catch-22 here and joined NetBSD in
offering a less broken API than standard C has for the last 40 years.
Sure, any would-be caller will have to have a separate "is this
Android?" and even "is this API level >= 35?" path, but that will fix
itself sometime in the 2030s when developers can just assume "yes, it
is", whereas if we keep putting off exposing anything, this problem
never gets solved.

(No-one's bothered to try to implement the std::chrono::time_zone
functionality in libc++ yet, but they'll face a similar problem if/when
they do.)

* Implementation

The good news is that tzcode already implements these functions, so
there's relatively little here.

I've chosen not to expose `struct state` because `struct __timezone_t`
makes for clearer error messages, given that compiler diagnostics will
show the underlying type name (`struct __timezone_t*`) rather than the
typedef name (`timezone_t`) that's used in calling code.

I've moved us over to FreeBSD's wcsftime() rather than keep the OpenBSD
one building --- I've long wanted to only have one implementation here,
and FreeBSD is already doing the "convert back and forth, calling the
non-wide function in the middle" dance that I'd hoped to get round to
doing myself someday. This should mean that our strftime() and
wcsftime() behaviors can't easily diverge in future, plus macOS/iOS are
mostly FreeBSD, so any bugs will likely be interoperable with the other
major mobile operating system, so there's something nice for everyone
there!

The FreeBSD wcsftime() implementation includes a wcsftime_l()
implementation, so that's one stub we can remove. The flip side of that
is that it uses mbsrtowcs_l() and wcsrtombs_l() which we didn't
previously have. So expose those as aliases of mbsrtowcs() and
wcsrtombs().

Bug: chronotope/chrono#499
Test: treehugger
Change-Id: Iee1b9d763ead15eef3d2c33666b3403b68940c3c
sepehrst pushed a commit to spsforks/android-bionic-libc that referenced this issue Apr 22, 2024
* Rationale

The question often comes up of how to use multiple time zones in C code.
If you're single-threaded, you can just use setenv() to manipulate $TZ.
toybox does this, for example. But that's not thread-safe in two
distinct ways: firstly, getenv() is not thread-safe with respect to
modifications to the environment (and between the way putenv() is
specified and the existence of environ, it's not obvious how to fully
fix that), and secondly the _caller_ needs to ensure that no other
threads are using tzset() or any function that behaves "as if" tzset()
was called (which is neither easy to determine nor easy to ensure).

This isn't a bigger problem because most of the time the right answer
is to stop pretending that libc is at all suitable for any i18n, and
switch to icu4c instead. (The NDK icu4c headers do include ucal_*,
so this is a somewhat realistic option for applications.)

But what if you're somewhere in between? Like the rust chrono library,
for example? What then?

Currently their "least worst" option is to reinvent the entire wheel and
read our tzdata files. Which isn't a great solution for anyone, for
obvious maintainability reasons.

So it's probably time we broke the catch-22 here and joined NetBSD in
offering a less broken API than standard C has for the last 40 years.
Sure, any would-be caller will have to have a separate "is this
Android?" and even "is this API level >= 35?" path, but that will fix
itself sometime in the 2030s when developers can just assume "yes, it
is", whereas if we keep putting off exposing anything, this problem
never gets solved.

(No-one's bothered to try to implement the std::chrono::time_zone
functionality in libc++ yet, but they'll face a similar problem if/when
they do.)

* Implementation

The good news is that tzcode already implements these functions, so
there's relatively little here.

I've chosen not to expose `struct state` because `struct __timezone_t`
makes for clearer error messages, given that compiler diagnostics will
show the underlying type name (`struct __timezone_t*`) rather than the
typedef name (`timezone_t`) that's used in calling code.

I've moved us over to FreeBSD's wcsftime() rather than keep the OpenBSD
one building --- I've long wanted to only have one implementation here,
and FreeBSD is already doing the "convert back and forth, calling the
non-wide function in the middle" dance that I'd hoped to get round to
doing myself someday. This should mean that our strftime() and
wcsftime() behaviors can't easily diverge in future, plus macOS/iOS are
mostly FreeBSD, so any bugs will likely be interoperable with the other
major mobile operating system, so there's something nice for everyone
there!

The FreeBSD wcsftime() implementation includes a wcsftime_l()
implementation, so that's one stub we can remove. The flip side of that
is that it uses mbsrtowcs_l() and wcsrtombs_l() which we didn't
previously have. So expose those as aliases of mbsrtowcs() and
wcsrtombs().

Bug: chronotope/chrono#499
Test: treehugger
Change-Id: Iee1b9d763ead15eef3d2c33666b3403b68940c3c
sepehrst pushed a commit to spsforks/android-bionic-libc that referenced this issue Apr 22, 2024
This works (by reading /etc/localtime) on NetBSD, but not on Android
since we have no such file. Fix that by using our equivalent system
property instead.

Also s/time zone/timezone/ in documentation and comments. We've always
been inconsistent about this (as is upstream in code comments and
documentation) but it seems especially odd now we expose a _type_ that
spells it "timezone" to talk of "time zone" even as we're describing
that type and its associated functions.

Bug: chronotope/chrono#499
Test: treehugger
Change-Id: I142995a3ab4deff1073a0aa9e63ce8eac850b93d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.