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 #293

Closed
quininer opened this issue Nov 10, 2020 · 48 comments
Closed

The call to localtime_r may be unsound #293

quininer opened this issue Nov 10, 2020 · 48 comments
Labels
A-core Area: anything not otherwise covered C-bug Category: bug in current code C-unsound 🚨 Category: unsound behavior

Comments

@quininer
Copy link

quininer commented Nov 10, 2020

because getenv and setenv are not thread-safe, localtime_r in time may data race with std::env::set_var in libstd.

I described this problem in chrono issue, and time is also affected.


Edit from @jhpratt:

Chrono users please see this comment.

@jhpratt
Copy link
Member

jhpratt commented Nov 10, 2020

First off: thanks! It's a nice find.

What would be an appropriate fix for this?

Mainly for myself: this is the sole location this function is called.

@quininer
Copy link
Author

I don't think there is any quick solution, we may have to reimplement localtime_r in Rust.

@jhpratt
Copy link
Member

jhpratt commented Nov 10, 2020

Ping @ymtvx, who created the original implementation. I'm not terribly familiar with these C APIs.

What output are you expecting from the snippet provided?

@jhpratt jhpratt added the A-core Area: anything not otherwise covered label Nov 10, 2020
@quininer
Copy link
Author

@jhpratt Oh, I realized that I paste wrong code and it has been updated. POC code will crash.

@quininer
Copy link
Author

Screenshot_20201111_111951

@quininer
Copy link
Author

In addition, I found that when using time crate, this crash is easy to reproduce in glibc, but it is difficult to use chrono.

@jhpratt jhpratt added the C-bug Category: bug in current code label Nov 11, 2020
@jhpratt
Copy link
Member

jhpratt commented Nov 11, 2020

So what is actually causing the segfault? Just trying to determine the true root cause so that I can figure out what action to take. I've never used lldb before, so all I see is the basic segfault message when running.

@quininer
Copy link
Author

setenv and getenv are not thread-safe, and localtime_r will call getenv, which cause to data races if setenv and localtime_r occur at same time.

To be more specific, getenv will return a pointer, and after setenv the pointer may point to an invalid address.

Rust's libstd avoids this problem by locking std::env::set_var and std::env::var, but call localtime_r directly will bypass the lock.

@dekellum
Copy link

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

Like localtime_r(3), getaddrinfo(3) is similarly marked as vulnerable to concurrent ENV mutation.

In the linked issue, it is contemplated using the libstd internal ENV lock around calls to getaddrinfo, but with concern over performance implications.

@hlavaatch
Copy link

hlavaatch commented Nov 14, 2020

So instead of worrying about concurrent env modification, you could read the env variable on startup and cache it, and avoid any calls to real getenv. If you want to be fancy you could intercept setenv and update the cached value in a thread safe way.

@jhpratt
Copy link
Member

jhpratt commented Nov 14, 2020

@hlavaatch That would add an up-front cost for everyone, which isn't desirable. It would also prevent the user from changing the TZ while the program is running.

With regard to intercepting setenv, what do you mean? This crate has no control over what users write.

@jhpratt
Copy link
Member

jhpratt commented Nov 17, 2020

I have created a draft security advisory on GitHub for this, and have requested a CVE. If granted, I will then file an advisory over at RustSec/advisory-db.

Time 0.2.23 was released earlier today, which avoids the issue entirely by not attempting to determine the local offset on Unix-like operating systems.

@jhpratt
Copy link
Member

jhpratt commented Nov 18, 2020

A security advisory has been published on GitHub. This issue has been assigned CVE-2020-26235.

@jhpratt
Copy link
Member

jhpratt commented Nov 21, 2020

Closing this issue as the unsoundness does not exist on the most recent release. In addition, a security advisory has been issued on this repository, a CVE has been issued, and a rustsec advisory has been issued. I don't think there's much more I can do at this point.

@osa1
Copy link

osa1 commented Nov 25, 2020

Couldn't we implement localtime_r in Rust, using thread-safe std::env functions? If someone mutates env in another thread using libc that's still a problem of course, but that already is a problem in Rust today and it seems like that's acceptable?

@jhpratt
Copy link
Member

jhpratt commented Nov 25, 2020

@osa1 There is presumably some way to work around this, yes. Feel free to put together a pull request if you have sufficient knowledge — the previous code was just commented out, not removed completely. The problem is more the call to localtime_r, which is a syscall by nature. The lock has to be held for a short while during the time the return value from that is used. So far as I know stdlib doesn't have a way to do this nicely; you'd presumably have to dig through how stdlib does it internally and copy it over.

@osa1
Copy link

osa1 commented Dec 16, 2020

The problem is more the call to localtime_r, which is a syscall by nature

What do you mean by this? localtime_r is a libc function, no?

@jcdyer
Copy link
Contributor

jcdyer commented Dec 18, 2020

Note to anyone else finding this. It is marked "closed", but as of 2020-12-18, all the local-offset methods are still unusable on the main branch on unix platforms, as they always return Err.

zevweiss added a commit to zevweiss/omnisensor that referenced this issue May 2, 2024
Version 0.4 had a transitive dependency on a version of the 'time' crate
that was triggering security alerts (see [1]).  It's not a terribly
critical problem and I'm pretty sure unlikely to cause us any
vulnerabilities or other problems in practice, but the new gpiocdev
trims its dependencies to avoid the problem entirely and is hence nicely
a bit lighter-weight, so updating seems like a win-win.

[1] time-rs/time#293
skygrango added a commit to skygrango/iced that referenced this issue May 3, 2024
There is a long-standing problem (time-rs/time#293) that has not yet been solved by time-rs
Switch to chrono as it seemed to solve the problem (chronotope/chrono#677)
skygrango added a commit to skygrango/iced that referenced this issue May 3, 2024
There is a long-standing problem (time-rs/time#293) that has not yet been solved by time-rs
Switch to chrono as it seemed to solve the problem (chronotope/chrono#677)
skygrango added a commit to skygrango/iced that referenced this issue May 3, 2024
…ystem

There is a long-standing problem (time-rs/time#293) that has not yet been solved by time-rs
Switch to chrono as it seemed to solve the problem (chronotope/chrono#677)
skygrango added a commit to skygrango/iced that referenced this issue May 3, 2024
There is a long-standing problem (time-rs/time#293) that has not yet been solved by time-rs
Switch to chrono as it seemed to solve the problem (chronotope/chrono#677)
zevweiss added a commit to zevweiss/omnisensor that referenced this issue May 7, 2024
Version 0.4 had a transitive dependency on a version of the 'time' crate
that was triggering security alerts (see [1]).  It's not a terribly
critical problem and I'm pretty sure unlikely to cause us any
vulnerabilities or other problems in practice, but the new gpiocdev
trims its dependencies to avoid the problem entirely and is hence nicely
a bit lighter-weight, so updating seems like a win-win.

[1] time-rs/time#293
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: anything not otherwise covered C-bug Category: bug in current code C-unsound 🚨 Category: unsound behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.