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

Reconsider of chrono crate due to https://rustsec.org/advisories/RUSTSEC-2020-0071.html #134

Closed
DanielJoyce opened this issue Oct 25, 2021 · 6 comments
Labels
low priority Should be handled as time permits task Task or chore that is not a bug or enhancement

Comments

@DanielJoyce
Copy link
Contributor

I don't we hit the vulnerable code paths, and if we do, we don't use set_env, though we can't make transitive guarantees about crates we consume.

I think we can probably use the time crate ( v > 0.3 ) for our needs. Maybe?

@louislang louislang added low priority Should be handled as time permits enhancement New feature or request labels Dec 3, 2021
@kylewillmon
Copy link
Contributor

As long as we don't enable the oldtime feature in chrono, it won't pull in the time crate and this won't be an issue.

But right now, xtask depends on simplelog which uses chrono. And cargo resolves dependencies at the workspace level, so we can't avoid the time crate without removing the xtask -> simplelog dependency as well.

@maxrake maxrake added task Task or chore that is not a bug or enhancement and removed enhancement New feature or request labels Mar 25, 2022
@kylewillmon
Copy link
Contributor

Update: simplelog no longer relies on chrono (as of this commit which has now been included in a release). However, phylum-types has added a dependency on chrono (in this commit) and explicitly sets default-features which includes oldtime.

@cd-work
Copy link
Contributor

cd-work commented Jun 23, 2022

I don't think we have much control over hitting the vulnerable code paths, but this should be fixed (chronotope/chrono#677) in the next release of chrono.

@kylewillmon
Copy link
Contributor

kylewillmon commented Jun 23, 2022

It looks like I was fooled by the RUSTSEC advisory into thinking this was about the oldtime feature of chrono... So most of my work on this has been in removing the usage of time 0.1. This comment clears up my misunderstanding...

Luckily, removing duplicate dependencies is a good thing, so my time has not been wasted.

@cd-work
Copy link
Contributor

cd-work commented Jun 23, 2022

Yeah and as I've mentioned the next release should fix this. I've went down this rabbit hole when I saw the advisory for the first time and it appears that you're basically screwed since any dependency might cause a conflict by just using the env STD methods. So we could certainly be affected at any time if a dependency introduces a change, considering that we're using threading heavily.

But since chrono is currently in the process of releasing a beta, I'd hope we can expect the next proper release within a couple days/weeks.

@cd-work
Copy link
Contributor

cd-work commented Aug 12, 2022

The CVE has been revoked (since technically chrono only reads without a lock and doesn't write). And additionally chrono now relies on STD's env support to ensure they're covered by STD's lock.

So that should solve all the issues with chrono.

@cd-work cd-work closed this as completed Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low priority Should be handled as time permits task Task or chore that is not a bug or enhancement
Projects
None yet
Development

No branches or pull requests

5 participants