irc
irc copied to clipboard
Fix CVE-2020-26235 by removing chrono dependency
There are currently two rustsec advisories associated with the chrono
crate, because of the way it implements localtime.
In its current implementation, there is a risk of segmentation faults on Unix system if it is called in a multi-threaded context.
The irc
crate currently calls the offending code in three places and is therefore affected by this issue.
For the CTCP TIME handling, I have adjusted the code to use the time
crate instead, which avoids this issue by checking for the runtime context before retrieving the local time.
This only affects the one call where the local time was actually necessary, in the other two calls I've replaced it with the UTC time without any changes in functionality (timestamp associated with the PING command).
Note that there is a change in functionality for the CTCP TIME handling: If it is called on a Unix system within a multi-threaded context, it will fall back to retrieving the UTC time, and send that time as local time instead, with an offset +0000:
if tokens[0].eq_ignore_ascii_case("TIME") {
let local_time =
OffsetDateTime::now_local().unwrap_or_else(|_| OffsetDateTime::now_utc());
self.send_ctcp_internal(resp, &format!("TIME :{}", local_time.format(RFC_2822)?))
}
The thing I'm wondering is, isn't the main point of CTCP TIME getting the other person's timezone? Returning an UTC time probably isn't too useful, unless you're trying to see if their system clock is inaccurate.
Yeah, I wasn't entirely sure what to do in case the timezone couldn't be determined.
In the issue under https://github.com/time-rs/time/issues/293, the main conclusion seems to be that they'll have to wait until the standard library exposes the timezone. Until then, getting the timezone under a multi-threaded Unix setting is gated under the --cfg unsound_local_offset
flag.
So as far as the irc
crate is concerned, there seem to be two ways to resolve the situation:
- Use the UTC time as a fallback. This means that the function can silently return an incorrect result unless
RUSTFLAGS="--cfg unsound_local_offset"
is set during compilation. - Pass along the error and don't return a response at all. This seems more sensible to me, but I'm not knowledgeable enough with the
irc
crate to understand the side-effects that this could bring. One immediate problem is that the corresponding unit test won't pass under Linux unless you callcargo test -- --test-threads=1
.
My priority is for my program to pass the rustsec advisory test, and I didn't want to cause any new instabilities, so I went with solution (1).
I'm not familiar with the codebase either, but can you take the timezone as a field in the configuration? Then the consumer, before starting additional threads, can store the timezone to be used by this library. This library also could try to get the timezone earlier, when being single-threaded is more likely, such as during the construction of the configuration structure.
So, after continuing to work with my fork of this crate, it turns out that my changes break compatibility with servers that use SSL encryption. :disappointed:
I'll update you once I have found a solution that maintains full functionality.
edit: never mind! I was applying default-features=false
when I was importing the crate and then forgot about the implications.
The latest version of chrono has a fix for this, so perhaps you can just update to the latest version instead of removing chrono
https://rustsec.org/advisories/RUSTSEC-2020-0159.html
I merged in the reconfiguration in place of this. Thanks!