Exchanged chrono Dependency for Equal Implementation Using The Still-Maintained time Crate

[noahcoetsee]

The reason for this pull request is the recent security advisory, RUSTSEC-2020-0159. This issue detected a potential segfault in localtime_r invocations, and given the chrono crate's lack of maintenance, the time crate, which fixed the issue, is now the preferred crate for datetime offsets and formatting.

The format used is still identical to the one achieved via chrono, but this implementation also prevents the issues caused by RUSTSEC-2020-0159, which were failing CI/CL actions in many repositories dependent upon rust-simple_logger.

The changes should still work in a no_alloc, no_std situation as expected.

(note: yes I see that I did a typo and put RUSTSEC-2021 instead of RUSTSEC-2020 in the commit. oops)

[borntyping]

Merged, though having some trouble getting tests to pass with this change. I also had to remove use winapi::um::wincon::ENABLE_VIRTUAL_TERMINAL_PROCESSING; so I could compile it on linux - if this is a useful change, can you put in in a separate MR with a commit explaining what it does?

[borntyping]

Tests pass with RUSTFLAGS="--cfg unsound_local_offset" cargo test, but without that time panics when trying to get the local offset:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: IndeterminateOffset', src/lib.rs:317:26
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[borntyping]

@noahcoetsee Reverted these changes for now. As much as I'd like to avoid RUSTSEC-2020-0159 maybe causing a segfault, time will cause a segfault panic for all Unix users in its current state if I merge this MR as is (time-rs/time#293, time-rs/time#380) - which I think is most users of this library.

[noahcoetsee]

Yeah, that totally makes sense. I was unaware of that issue.

I think first of all I'll add this new check which should avoid that issue for the most part, until the primary issue is resolved.

if self.timestamps {
    let datetime = match OffsetDateTime::now_local() {
        Ok(x) => x,
        Err(_) => OffsetDateTime::now_utc(),
    };
    #[cfg(not(feature = "stderr"))]
    println!(
        "{} {:<5} [{}] {}",
        datetime.format(&FORMAT).unwrap(),
        level_string,
        target,
        record.args()
    );
    #[cfg(feature = "stderr")]
    eprintln!(
        "{} {:<5} [{}] {}",
        datetime.format(&FORMAT).unwrap(),
        level_string,
        target,
        record.args()

winapi::um::wincon::ENABLE_VIRTUAL_TERMINAL_PROCESSING; so I could compile it on linux - if this is a useful change, can you put in in a separate MR with a commit explaining what it does?

I'm honestly not sure how that got there, that wasn't a change that I made intentionally.

[borntyping]

I wondered about making a change like that, though I can't think of a way to make this change that isn't a breaking change that users might not notice - e.g. I really don't want users to update and not notice their timestamp format has changed. I think for now the best I can do is stick with chrono until time-rs/time#380 is resolved.

[jhpratt]

@borntyping No, the time crate will not segfault unless you're using an old version (which is not the case here). time-rs/time#293 is closed for a reason.

Also, the time crate doesn't panic when you try to get the local offset. The panic is you unwrapping a Result. The method is fallible for a reason.

By sticking with chrono, you are knowingly using code that has an active, exploitable security vulnerability. It is impossible to guarantee soundness on your end.

[borntyping]

I mixed up segfault and panic when typing out an earlier comment.

I'm aware the method is intentionally fallible, but this library was built on the assumption that it could safely get dates in the local time and so doesn't have an alternative it could fall back to in the case where it doesn't. Unexpectedly switching to UTC dates instead (at either runtime or build time) carries far more risk to users than a potential segfault.

I do not make any guarantees about the soundness of this library (see LICENSE). The code in this MR isn't in a state that I can release it in, and I've not had time to work on exploring writing something that uses time instead of chrono, doesn't break backwards compatibility, and safely explains the failure cases to users. If someone is able to provide a MR I can merge that at least covers the first two, I'll endeavour to merge and release it as quickly as possible.

[borntyping]

(...also, I'd love to see even a example of this issue getting exploited to any interesting result, even theoretically.)

[jhpratt]

I mixed up segfault and panic when typing out an earlier comment.

As I said, it doesn't panic in this method either. The panic is the .unwrap() that's in this library's code.

this library was built on the assumption that it could safely get dates in the local time

That's a fundamental flaw, to be perfectly honest. It's just simply not the case in any operating system to my knowledge.

Unexpectedly switching to UTC dates instead (at either runtime or build time) carries far more risk to users than a potential segfault.

Are you sure about that? A segfault can crash a program outright with no possibility of recovering. This crate has 1.57 million downloads; you've no idea the type of sensitive contexts it's being used in.

I do not make any guarantees about the soundness of this library (see LICENSE).

Yes, I understand that the license gives you legal cover, but it's still generally not a good idea to have unsoundness. It's a social thing, not legal.

If someone is able to provide a MR I can merge that at least covers the first two, I'll endeavour to merge and release it as quickly as possible.

If you're dead-set on using the local time and not UTC, that is impossible with the current state of the Rust ecosystem. The most reasonable thing to do is to try and get the local time and fall back to UTC if that fails.

(...also, I'd love to see even a example of this issue getting exploited to any interesting result, even theoretically.)

Someone calling std::env::set_var (a safe method) in another thread at the same time one of the effected methods is called leads to a segmentation fault, crashing the program. This is something that a library inherently cannot guarantee, as someone else could call std::env::set_var.

[borntyping]

That's a fundamental flaw, to be perfectly honest. It's just simply not the case in any operating system to my knowledge.

Yes, that the design is flawed is my point. The result of that flaw is that the time crate can't be dropped in as a replacement without additional work.

Yes, I understand that the license gives you legal cover, but it's still generally not a good idea to have unsoundness. It's a social thing, not legal.

I'm not talking about legality here. A license is more than just a legal statement. I don't provide any guarantees at all for this library - I don't even guarantee it works.

If you're dead-set on using the local time and not UTC, that is impossible with the current state of the Rust ecosystem.
The most reasonable thing to do is to try and get the local time and fall back to UTC if that fails.

This isn't a reasonable thing to do silently in this case as the output doesn't include timezone information. This could easily result in logfiles where the timestamps are from two different timezones and some entries appear to be many hours away from the time they created. In an extreme case, that can be a security issue.

The core feature of this library is that it logs in a specific format, so not using local time would be a severe breaking change that I would want to communicate very clearly to users. It's not a requirement that it can be used without ever failing though, so It'd be fine to change this to use time if the library raised an error during the init method (which it already does in other cases).

Alternatively, there could be an option to use or fall back to UTC time. As I understand it, it's not currently possible for users on Unix to use local time without adding a compile time flag; so an option like this may be useful anyway. If it does fail due to the time crate failing to get the local timezone, it should direct users to the documentation on the compile time flag.

Again, I've no opposition to replacing chrono with time in this library. If you want to fix up this MR or create a new MR to do this I'll be happy to try and get it merged and released as quickly as possible. If not, I'll get round to it some other time.

[jhpratt]

It'd be fine to change this to use time if the library raised an error during the init method

If this is the case, why not just do OffsetDateTime::now_local.expect("whatever message you want") in initialization?

As I understand it, it's not currently possible for users on Unix to use local time without adding a compile time flag

That is correct.

Personally I don't use this library as my requirements for logging are quite a bit different. To be honest the only reason I chimed in is because I saw it was reverted for reasons I didn't quite understand.

[borntyping]

Dude, your comments have been awfully aggressive for someone who isn't sure what's going on.

I merged the MR as I assumed the test failures were simple and could be easily fixed by removing a single line that wasn't needed; and reverted the changes when I realized a lot more work was required for the code to build and pass tests so it could be released. I reverted so that the main branch would still contain working code, as I didn't have time to work on fixing it and the new version was never actually published.

If this is the case, why not just do OffsetDateTime::now_local.expect("whatever message you want") in initialization?

The code changes are might be pretty simple here, documenting it properly so it explains why it's failing and how to fix it is a little less simple. As I've said, I'll either merge in a working fix for this, or fix it myself starting from the changes in this MR when I feel I have some time to work on it.

[jhpratt]

Dude, your comments have been awfully aggressive for someone who isn't sure what's going on.

I'm the maintainer of the time crate and you have a serious, active security vulnerability. That is difficult to understate. I'm not particularly familiar with the API of this crate, but the time crate I couldn't possibly be more knowledgeable about. If you believe that's "someone who isn't sure what's going on", then yeah, I suppose I am. I don't think I've been particularly "aggressive" — the only thing I said that is, in my opinion, borderline is that there was a flawed design decision by assuming a fallible API could never fail — something you agreed with.

If you think what I'm saying is rude, I can drop out and leave you to your own devices. I won't mind if that's what you want. I was just throwing out some possibilities for when you find the time to work on this. I get it that you haven't had time; all I'm doing is trying to help by suggesting possible fixes within your constraints. I'm trying to give you simple solutions based on what you've told me.

[borntyping]

...calling the library flawed isn't even close to my frustration here. This library was written years ago when I was still learning the language (before Rust version 1 was released!). I only maintain it at all because people still use it.

My frustration is the arguments over the semantics of comments, assuming I don't understand the issue, and phrases like "why not just do [...]?". You've spent most of this thread arguing and very little of it on suggesting fixes. Between us we've probably spent more time on this thread than it would have taken to fix the issue. I should have responded better - usually when I see or get comments like these it's from people who are completely uninvolved looking to cause trouble, and so I didn't make the connection that you had any involvement with the time crate.

[borntyping]

Released an implementation of this in v1.15.0.