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

chrono not maintaned - CVE-2020-26235 in time #650

Closed
acim opened this issue Oct 11, 2021 · 21 comments
Closed

chrono not maintaned - CVE-2020-26235 in time #650

acim opened this issue Oct 11, 2021 · 21 comments
Labels
dependencies upgrades to dependencies invalid rejected as a valid issue

Comments

@acim
Copy link

acim commented Oct 11, 2021

crono depends on vulnerable version of time and it seems not to be well maintained. Could you maybe replace it completely with some newer version of time as suggested here.

@clux clux added the dependencies upgrades to dependencies label Oct 11, 2021
@clux
Copy link
Member

clux commented Oct 11, 2021

Thanks for the report. Yeah, we will look into that. We don't have a lot of usage of chrono here, but it is also used by schemars for a feature so a fix would probably be needed in as well.

Usage of chrono:

  • depended on in kube-core without feature for the new Restart behaviour only - 1 file
  • optional feature in kube for refresh tokens + auth (Utc, DateTime, Duration, chrono::ParseError) - 3 files
  • transitively depended on by schemars via kube-derive - 0 files (but 1 test file)

@kazk
Copy link
Member

kazk commented Oct 11, 2021

The linked comment:

Everything below 0.2.7 is unaffected:
https://github.com/rustsec/advisory-db/blob/main/crates/time/RUSTSEC-2020-0071.md?plain=1#L36

However chrono does seem somewhat unmaintained, so maybe it makes sense to just switch to time 0.3 as your dependency entirely, which covers a lot of chrono's surface area, while also reducing the dependency count.

chronotope/chrono#602 (comment)

It's "somewhat" unmaintained. I'd ask chorno maintainers to merge chronotope/chrono#578 instead of trying to make all the crates drop chrono.

Also, it says versions below 0.2.7 is unaffected and chrono is using 0.1 (https://github.com/chronotope/chrono/blob/3467172c31188006147585f6ed3727629d642fed/Cargo.toml#L39).

@clux clux added the invalid rejected as a valid issue label Oct 11, 2021
@clux
Copy link
Member

clux commented Oct 11, 2021

Yeah, that's true. If we were affected by the CVE cargo audit (which picks up RusSec/advisory-db) would've showed it and given us an advisory issue in CI.

@clux clux closed this as completed Oct 11, 2021
@acim
Copy link
Author

acim commented Oct 11, 2021

It's "somewhat" unmaintained. I'd ask chorno maintainers to merge chronotope/chrono#578 instead of trying to make all the crates drop chrono.

That would be the best, let's hope you have success with this.

@acim
Copy link
Author

acim commented Oct 11, 2021

Yeah, that's true. If we were affected by the CVE cargo audit (which picks up RusSec/advisory-db) would've showed it and given us an advisory issue in CI.

cargo audit doesn't fail, but cargo pants does:

Vulnerable Dependencies

[1/1] pkg:cargo/time@0.1.44
1 known vulnerability found

Vulnerability Title: [CVE-2020-26235] In Rust time crate from version 0.2.7 and before version 0.2.23, unix-like opera...
╭─────────────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ ID          │ 090a08dc-6a51-4073-b874-d71193524758                                                                                  │
├─────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Description │ In Rust time crate from version 0.2.7 and before version 0.2.23, unix-like operating systems may segfault due to dere │
│             │ ferencing a dangling pointer in specific circumstances. This requires the user to set any environment variable in a d │
│             │ ifferent thread than the affected functions. The affected functions are time::UtcOffset::local_offset_at, time::UtcOf │
│             │ fset::try_local_offset_at, time::UtcOffset::current_local_offset, time::UtcOffset::try_current_local_offset, time::Of │
│             │ fsetDateTime::now_local and time::OffsetDateTime::try_now_local. Non-Unix targets are unaffected. This includes Windo │
│             │ ws and wasm. The issue was introduced in version 0.2.7 and fixed in version 0.2.23.                                   │
├─────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ CVSS Score  │ 5.3                                                                                                                   │
├─────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ CVSS Vector │ CVSS:3.0/AV:N/AC:H/PR:L/UI:N/S:U/C:N/I:N/A:H                                                                          │
├─────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Reference   │ https://ossindex.sonatype.org/vulnerability/090a08dc-6a51-4073-b874-d71193524758?component-type=cargo&component-name= │
│             │ time&utm_source=cargo-pants&utm_medium=integration&utm_content=0.4.2                                                  │
╰─────────────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯


Inverse Dependency graph
time 0.1.44 (registry+https://github.com/rust-lang/crates.io-index)
└── chrono 0.4.19 (registry+https://github.com/rust-lang/crates.io-index)
    ├── k8s-openapi 0.13.1 (registry+https://github.com/rust-lang/crates.io-index)
    │   └── tests 0.1.0 (path+file:///home/acim/Projects/public/kube-rs/tests)
    ├── kube 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube)
    │   ├── kube-runtime 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube-runtime)
    │   └── tests 0.1.0 (path+file:///home/acim/Projects/public/kube-rs/tests)
    ├── kube 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube)
    │   ├── kube-runtime 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube-runtime)
    │   └── tests 0.1.0 (path+file:///home/acim/Projects/public/kube-rs/tests)
    ├── kube-core 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube-core)
    │   ├── kube 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube)
    │   │   ├── kube-runtime 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube-runtime)
    │   │   └── tests 0.1.0 (path+file:///home/acim/Projects/public/kube-rs/tests)
    │   └── kube 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube)
    │       ├── kube-runtime 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube-runtime)
    │       └── tests 0.1.0 (path+file:///home/acim/Projects/public/kube-rs/tests)
    ├── kube-core 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube-core)
    │   ├── kube 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube)
    │   │   ├── kube-runtime 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube-runtime)
    │   │   └── tests 0.1.0 (path+file:///home/acim/Projects/public/kube-rs/tests)
    │   └── kube 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube)
    │       ├── kube-runtime 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube-runtime)
    │       └── tests 0.1.0 (path+file:///home/acim/Projects/public/kube-rs/tests)
    └── tame-oauth 0.4.7 (registry+https://github.com/rust-lang/crates.io-index)
        ├── kube 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube)
        │   ├── kube-runtime 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube-runtime)
        │   └── tests 0.1.0 (path+file:///home/acim/Projects/public/kube-rs/tests)
        └── kube 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube)
            ├── kube-runtime 0.61.0 (path+file:///home/acim/Projects/public/kube-rs/kube-runtime)
            └── tests 0.1.0 (path+file:///home/acim/Projects/public/kube-rs/tests)


Audited Dependencies: 194
Vulnerable Dependencies: 1

@kazk
Copy link
Member

kazk commented Oct 11, 2021

Note that k8s-openapi uses chrono for Time in apimachinery, so kube removing chrono doesn't mean much anyway. I don't think chrono is unmaintained, either (it's very commonly used). They're probably just busy, and the version of time they're using is actually not vulnerable according to the advisory-db too.

@clux
Copy link
Member

clux commented Oct 11, 2021

cargo audit doesn't fail, but cargo pants does:

That might be a bug in cargo-pants by itself. Quick search reveals it's not reading the unaffected key. (edit; it's not reading other stuff either, so i'm probably searching in the wrong place).

@Arnavion
Copy link

Arnavion commented Oct 11, 2021

Also, it says versions below 0.2.7 is unaffected and chrono is using 0.1 (https://github.com/chronotope/chrono/blob/3467172c31188006147585f6ed3727629d642fed/Cargo.toml#L39).

FYI: chronotope/chrono#499 (comment) + time-rs/time#293 (comment)

Edit: And also chronotope/chrono#602 (comment)

@clux clux reopened this Oct 11, 2021
@Arnavion
Copy link

Here's my thoughts from k8s-openapi's POV:

  1. The time dependency is irrelevant, as I said in CVE-2020-26235 advisory for time 0.1 dependency chronotope/chrono#602 (comment)

  2. As a result, the fact that chrono would be not vulnerable if it had merged the time 0.3 PR is also irrelevant, and does not by itself indicate that chrono is unmaintained.

  3. While chrono has not had any new commits for ~9 months, the chrono maintainer is active in other chronotope repos as of last month. So I wouldn't assume that chrono is unmaintained as of this moment.

  4. chrono is vastly more popular than time, both in terms of number of downloads and number of reverse deps. In particular, chrono 0.4 has ~100k downloads per day while time 0.3 has ~2k. (time 0.1 has ~100k downloads, of which ~80k are presumably because of chrono.) So k8s-openapi users likely have to use chrono to interop with other libraries, so imposing a conversion to-and-from time would be annoying.

  5. Modifying the environ dynamically is not common, and even less common to do it at the same time as calling anything that would invoke localtime_r. libstd has the same problem with getaddrinfo, and isn't in a rush to fix it for the same reason.

So I'm not too worried about switching unless other parts of the ecosystem are planning to.

@clux
Copy link
Member

clux commented Oct 11, 2021

Yeah, it would not be an easy thing to get the ecosystem to change. Some comments:

It might still be "right" to update the CVE to match old versions of time (because it's just a coincidence that chrono doesn't use the vulnerable api), but that would cause a huge amount of disruptive CVE advisory issues on everything using cargo audit + chrono. But if the CVE is indeed invalid for chrono, well at least people can ignore the incoming advisory issues.

That said, migrating can be done for us. Even the prevalent to_rfc3339() formatting does have a Formattable alternative in time, but it looks uglier than chrono's nice api though.

Anyway, I left a comment on the chrono PR in the meantime to ask if people are watching it (even if it may or may not be as urgent as I originally thought).

@Arnavion
Copy link

The time::format_description::well_known::Rfc3339 you linked isn't actually sufficient, since it does not allow specifying the decimal precision of seconds. One has to drop down to impl Formattable for [FormatItem<'_>] for that which does allow you to specify that (FormatItem -> Component -> Subseconds -> SubsecondDigits). But yes, it's possible.

Lack of features is not a reason to stick with chrono, only ecosystem-pervasiveness is.

@Arnavion
Copy link

Arnavion commented Oct 11, 2021

Lack of features is not a reason to stick with chrono, only ecosystem-pervasiveness is.

Actually, I take that back. Every struct in time::format_description has public fields but is marked #[non_exhaustive], so they cannot be constructed from k8s-openapi.

Edit: Okay, looks like I'm expected to use time::macros::format_description instead of constructing the FormatItems myself.

@jhpratt
Copy link

jhpratt commented Oct 12, 2021

Maintainer of time here. Happy to answer any questions you may have.

Regarding usage: time 0.3 has minimal download count because it was released recently. Time 0.2 had significant uptake. Time (especially 0.3) is far more lightweight. The 0.3 series has zero mandatory dependencies.

Regarding formatting: Is the format known at compile-time? If so, use the macro. If not, you can still use format_description::parse to get a value that implements Formattable and Parsable at runtime (the syntax is the same). If, for whatever reason, you need control beyond that, you can still construct a modifier; it just requires that you use do T::default() before assigning fields (due to the non-exhaustiveness).

I can't speak as to chrono, as I only know what others can see as well. However, there hasn't been a commit since January. That's a pretty significant amount of time.

@Arnavion
Copy link

Regarding formatting: [...]

No worries. I've already implemented it. I don't have any questions.

If, for whatever reason, you need control beyond that, you can still construct a modifier; it just requires that you use do T::default() before assigning fields (due to the non-exhaustiveness).

Yes, I did consider this, since I'm not a fan of the macro DSL. But non-exhaustiveness also prevents S { field: value, ..Default::default() } and thus requires writing { let mut s = S::default(); s.field = value; s }, and I'm not sure which one I dislike more :)

It's not a big deal either way. I had to write it once and be done with it.

@jhpratt
Copy link

jhpratt commented Oct 12, 2021

For what it's worth I use a macro internally (for testing purposes) that expands struct literal syntax into the latter of what you said. It's absolutely a hack but 100% worth it. I wish Rust had a way to permit ..Default::default() in non-exhaustive, but there are legitimate concerns there. I'd absolutely opt in to it if I could.

@acim acim changed the title crono not maintaned - CVE-2020-26235 in time chrono not maintaned - CVE-2020-26235 in time Oct 12, 2021
@dimbleby
Copy link
Contributor

So far as getting rid of time 0.1 is concerned, isn't the simplest thing to disable the oldtime feature from chrono?

eg in k8s-openapi, this seems to be sufficient:

diff --git a/Cargo.toml b/Cargo.toml
index dfe517b1a..4663aa37d 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -30,7 +30,7 @@ links = "k8s-openapi-0.13.1"
 [dependencies]
 base64 = "0.13"
 bytes = "1"
-chrono = { version = "0.4.1", features = ["serde"] }
+chrono = { version = "0.4.1", default-features = false, features = ["alloc", "serde"] }
 http = { version = "0.2", optional = true }
 percent-encoding = { version = "2", optional = true }
 schemars = { version = "0.8", optional = true }

I realise that a security advisory for chrono is going to follow which may be harder to deal with, especially if chrono maintainers continue to be inactive, but this is surely a step in the right direction.

@acim
Copy link
Author

acim commented Oct 18, 2021

Now cargo audit also shows the error, so this will break so many builds.

@Arnavion
Copy link

So far as getting rid of time 0.1 is concerned, isn't the simplest thing to disable the oldtime feature from chrono?

For the sake of shutting up any auditing tools, yes. For the sake of actually solving the problem that triggered the CVE in the first place, no.

@dimbleby
Copy link
Contributor

Yes; as as I say, I understand that fixing the upcoming duplicate CVE in chrono is likely to be a tougher problem. Still, baby steps...

@Arnavion
Copy link

Arnavion commented Oct 18, 2021

I can do that (and default-features = false is something I've been doing in my personal projects anyway). Note that https://rustsec.org/advisories/RUSTSEC-2020-0159 is live now, so your auditing tools will complain for that reason as well.

Edit: Done in Arnavion/k8s-openapi@1a14517

Arnavion added a commit to Arnavion/k8s-openapi that referenced this issue Oct 26, 2021
This also disables the extra-traits feature of syn, since it was being used for
one `#[derive(Debug)]` that can be done without, and
one `!=` involving `AttrStyle` that can be done with `if let` instead.

base64, chrono and serde_json have "alloc" and "std" features where one of them
is required. In all three cases, it's possible to have both features enabled
and that's essentially treated the same as if only the std feature is enabled.
So this commit enables the "alloc" feature, and if anything else in
the end user's dep tree enables the "std" feature, that's fine.

The one noticeable breaking change is that enabling the schemars feature of
k8s-openapi no longer enables the derive feature of schemars, so users who want
to use `#[derive(schemars::JsonSchema)]` in their own code will need to add
an explicit dependency on schemars to enable that feature.

Ref: kube-rs/kube#650 (comment)
Arnavion added a commit to Arnavion/k8s-openapi that referenced this issue Oct 26, 2021
This also disables the extra-traits feature of syn, since it was being used for
one `#[derive(Debug)]` that can be done without, and
one `!=` involving `AttrStyle` that can be done with `if let` instead.

base64, chrono and serde_json have "alloc" and "std" features where one of them
is required. In all three cases, it's possible to have both features enabled
and that's essentially treated the same as if only the std feature is enabled.
So this commit enables the "alloc" feature, and if anything else in
the end user's dep tree enables the "std" feature, that's fine.

The one noticeable breaking change is that enabling the schemars feature of
k8s-openapi no longer enables the derive feature of schemars, so users who want
to use `#[derive(schemars::JsonSchema)]` in their own code will need to add
an explicit dependency on schemars to enable that feature.

Ref: kube-rs/kube#650 (comment)
@clux
Copy link
Member

clux commented Aug 8, 2022

Closing this as chrono 0.4.20 is out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies upgrades to dependencies invalid rejected as a valid issue
Projects
None yet
Development

No branches or pull requests

6 participants