The call to `localtime_r` may be unsound
quininer opened this issue · 48 comments
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.
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.
I don't think there is any quick solution, we may have to reimplement localtime_r in Rust.
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 Oh, I realized that I paste wrong code and it has been updated. POC code will crash.
In addition, I found that when using time crate, this crash is easy to reproduce in glibc, but it is difficult to use chrono.
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.
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.
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.
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.
@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.
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.
A security advisory has been published on GitHub. This issue has been assigned CVE-2020-26235.
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.
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?
@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.
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?
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.
I second this - time also reports an Error on MacOS.
@jhpratt - I would be glad if you could get me started with a hint to facilitate a contribution to fix this 🙏. Thanks a lot!
@Byron Unfortunately I don't think it's feasible to do in the time crate alone. Something has to be exposed in Rust's standard library to allow locking the environment (the same way Rust does when accessing environment variables itself). The only other way to do this is to reimplement the libc method in Rust, which I've no idea how you'd even begin to do.
Note that there is a flag of sorts for the current 0.3 alpha release, but it has to be passed by the user compiling it (read: it can't be enabled by a library). It is still unsound, however.
Thanks for filling me in! I will be happy to do give it my best when the time comes as eventually I will need local time display to work for gitoxide. That said, it probably takes months until I get there 😅.
@jhpratt Having finally migrated to 0.3 I did enjoy the finesse behind the usage of unsound_local_offset. It's also my first encounter with a custom compile config and shows how much I don't know about rustc thanks to cargo doing its job so well.
That said, I dug into what it would mean to implement getting the timezone on unix system without a race, here is my findings:
- A sample implementation to port could be the MUSL libc version. The port would omit access to the
TZvariable or do so throughstd::env::var(). - Here is a parser for timezone files
The above could possibly be its own crate either just for unix or for all platforms when reusing tz offset code already present in time.
Could this be it or do you think I am missing something more fundamental? They advice against rolling your own crypto and I fear rolling your own tz query function is just as hard to get even half right 😅, but maybe even that would be better than nothing.
Edit: For reference, this topic follows me and I am more motivated than ever to attempt a permanent fix. The way I see it, Rust is currently unable to soundly get the tz offset on unix and that seems too strange.
I haven't read C code in quite a bit (though my new job will certainly get me up to speed quite quickly), let alone an implementation of libc.
I'm pretty sure I've said previously that it wouldn't be possible without reimplementing this part of libc, which is effectively what you're suggesting. If this is sound, I have no issue with doing so. I would prefer it to be in a separate crate (still under the time-rs GitHub org) just to allow others ease of access. I think we should be very careful when doing this, however, as it can easily go wrong.
I won't have a ton of time in the coming days as I have to get some paperwork complete, but if you want to put some code together I'll certainly look it over. A gist would be fine, as like I said I'd rather it be in a different repository. Just Unix is fine, as Windows has no issues along these lines that I know of.
Thanks for laying out a path ahead! Currently I planned to drop the code into my own git-features crate, and let it evolve from there, which might help for review and eventually placing it into its own crate.
However, after a short moment of excitement I now realize that for one, I can live without local time a little longer by avoiding it, and secondly that the parser I mentioned probably isn't quite where I would want to trust it with this task. So doing this would definitely need more time than anticipated.
On the bright side, now I am at a spot where it seems clear what has to be done and about how much effort it's going to be, so when the need for local-time on unix arises once again I will share my progress around that here.
Sounds good. Let's hope your need for the method comes back up 😄
Oh, it will, these glaring TODOs will have to be resolved one day 😅.
Could you also generate a security advisory for v0.1, or amend the existing one? The existing one says "Affected versions: >= 0.2.7, < 0.2.23" which I assume is based on the callsite of localtime_r added in 5f1c492 , but 0.1 also calls localtime_r from a public API, so it's also affected.
Not sure if I can amend the actual CVE, but amending it if possible is the right thing to do. I don't believe I was thinking of the 0.1 series when I wrote it up.
Well, my motivation for having the CVE mention 0.1 was that an active chrono release was affected by it. But that's actually not true, so it's less important now that the CVE should mention 0.1.
That does however mean chrono itself now needs a CVE though right, as it calls the problematic function itself?!
Yes, a CVE specifically about chrono should definitely happen, independent of whatever happens with time's CVE.
I've just opened a PR updating the RUSTSEC advisory to include 0.1: rustsec/advisory-db#1078. The advisory on GitHub has already been updated.
@jhpratt Hate to ask since the proper solution would be getting chrono to upgrade, but do you think it would be hard to backport the fix to 0.1.x? (not asking you to do it, just want to know whether it makes sense to work on a PR)
Yeah, this issue forcing the time 0.2 upgrade is... pretty disruptive to the ecosystem. Some way of fixing it in 0.1 is clearly preferable.
What would a backport even amount to? In time 0.2 and 0.3, the relevant methods return a Result, meaning we can just return the error value. This is not the case in time 0.1.
I second the backport. It makes more sense, especially with chrono not updating. That said, there is a PR upgrading it to the latest version of time, so, the place where this vulnerability is found the most would be solved shortly.
Backporting the fix to 0.1 for the sake of chrono will not solve the problem with chrono, as I explained in chronotope/chrono#602 (comment)
Oh right. That makes sense.
To be clear with regard to my previous comment, I'm open to backporting this to time 0.1. My concern is it's not clear what that would mean, as we can't just return Err(…) like is done in time 0.2 and 0.3.
For everyone landing here from the chrono security advisory RUSTSEC-2020-0159 (which linked to this time issue for some reason), you're probably in the wrong place.
The issue with chrono is not because of its dependency on time 0.1. It's because chrono itself calls localtime_r in a way that can have the issues described in the advisory. While time 0.1 also has an advisory (RUSTSEC-2020-0071) for calling localtime_r, and while chrono does default to enabling a dependency on time 0.1, chrono never calls the function in time 0.1 that would then call localtime_r. See chronotope/chrono#602 (comment)
That means that hoping for chrono to update to time 0.3 won't fix the issue. It also means that disabling the oldtime feature of chrono will not fix the issue.
Discussion of chrono should continue in chronotope/chrono#499 and chronotope/chrono#602
@jhpratt Do you mind editing #293 (comment) into the issue OP? It's getting collapsed into "### hidden items" by the botspam
Done.
because
getenvandsetenvare not thread-safe,localtime_rin time may data race withstd::env::set_varin libstd.
I strongly recommend reading the discussion in rust-lang/rust#27970. We seem to have converged on a consensus that reading the environment is safe because writing to the environment is unsafe and pretty much always wrong to do. IMO this was/is a non-issue and the real problem is in anything that calls env::set_var, and that env::set_var isn't marked unsafe. Again, that's explained in the 27970 issue.
I've been following that discussion. I think I've stated this before, but I will restore full functionality once set_var and remove_var are formally deprecated. I am aware of the fact that the bug ultimately is not here.
It looks like there now is a pure Rust zero dependency crate to obtain time offsets, https://github.com/x-hgg-x/tz-rs available for consumption. I don't mean to reopen any kind of discussion here - a new issue is probably a better choice for that, but wanted to leave a note to show that there are now options.
I was running into this issue and since I found a workaround that worked for me with tokio I thought I'd share. The solution I used is to not use the #[tokio::main] macro and to call UtcOffset::current_local_offset before starting the threaded portion of your program manually using the equivalent code according to the documentation.
fn main() {
// retrieve local time outside of the context of threading
let local_offset = match UtcOffset::current_local_offset() {
Ok(local_offset) => local_offset,
Err(_) => {
// optionally print a message and fallback to UTC
UtcOffset::UTC
}
};
tokio::runtime::Builder::new_multi_thread()
.enable_all()
.build()
.unwrap()
.block_on(threaded_main(local_offset));
}
async fn threaded_main(local_offset: UtcOffset) {
// place the rest of your main here and make use of local_offset
}
Just for completeness and as an update to this story, chrono now uses the recently released timezone parser apparently in an adapted form, which might be helpful in one day integrating it into time as well.
gitoxide is committed to using time for its time parsing, formatting and local-time needs, and I will be happy to contribute a solution similar to the one used in chrono when the current situation in time becomes a blocker. Can't wait, even though I hope somebody beats me to it 😁.