irc icon indicating copy to clipboard operation
irc copied to clipboard

Fix CVE-2020-26235 by removing chrono dependency

Open snowpoke opened this issue 2 years ago • 5 comments

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)?))
}

snowpoke avatar Jan 18 '22 21:01 snowpoke

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.

FreeFull avatar Jan 18 '22 23:01 FreeFull

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:

  1. 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.
  2. 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 call cargo 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).

snowpoke avatar Jan 19 '22 02:01 snowpoke

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.

8573 avatar Jan 19 '22 05:01 8573

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.

snowpoke avatar Jan 23 '22 01:01 snowpoke

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

eminence avatar Aug 12 '22 17:08 eminence

I merged in the reconfiguration in place of this. Thanks!

aatxe avatar Mar 02 '23 18:03 aatxe