Skip to content

Commit

Permalink
Remove support for serde with pre-built binaries
Browse files Browse the repository at this point in the history 
This is a potential security vulnerability, particularly as the binaries
have not yet been reproduced.

cc serde-rs/serde#2538
  • Loading branch information
@jhpratt
jhpratt committed Aug 18, 2023
1 parent a98fe2d commit 500f8e4
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 4 deletions.
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,15 @@ The format is based on [Keep a Changelog]. This project adheres to [Semantic Ver

---

## 0.3.26 [2023-08-18]

This release contains only a single change. `serde` is required to be a version prior to 1.0.171.
This is due to the decision by the maintainer of `serde` to include pre-built binaries that are
executed without the end user's knowledge. As of the time of publishing, the included binary has not
even been reproduced. This is a security risk, and the `time` project strongly opposes this
decision. While this may break some users' builds due to conflicting versions, it is a necessary
step to ensure the security.

## 0.3.25 [2023-08-02]

### Fixed
Expand Down
5 changes: 3 additions & 2 deletions Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ resolver = "2"

[workspace.dependencies]
time-core = { path = "time-core", version = "=0.1.1" }
time-macros = { path = "time-macros", version = "=0.2.11" }
time-macros = { path = "time-macros", version = "=0.2.12" }

criterion = { version = "0.5.1", default-features = false }
deranged = { version = "0.3.7", default-features = false }
Expand All @@ -16,7 +16,8 @@ num_threads = "0.1.2"
quickcheck = { version = "1.0.3", default-features = false }
quickcheck_macros = "1.0.0"
rand = { version = "0.8.4", default-features = false }
serde = { version = "1.0.126", default-features = false }
# <= 1.0.171 due to serde-rs/serde#2538
serde = { version = ">= 1.0.126, <= 1.0.171", default-features = false }

This comment has been minimized.

Sign in to view

Copy link
@epage

epage Aug 19, 2023

Please don't as this can create unbuildable combinations of dependencies

See https://doc.rust-lang.org/nightly/cargo/reference/specifying-dependencies.html#multiple-requirements

This comment has been minimized.

Sign in to view

Copy link
@jhpratt

jhpratt Aug 19, 2023

Author Member

I am aware. It is directly addressed in the changelog. Security is the top priority for me.

This comment has been minimized.

Sign in to view

Copy link
@epage

epage Aug 19, 2023

Please leave security to cargo audit and don't make decisions on behalf of your dependents.

This comment has been minimized.

Sign in to view

Copy link
@jhpratt

jhpratt Aug 19, 2023

Author Member

I don't have stats, but I imagine a majority of people do not use cargo audit. This is the most blatant disregard for safety I've seen from a crate this popular. I am doing everything in my ability to prevent end users from relying on an unknown, unverified executable.

Hopefully this is short term. I have faith that David will reverse course given the severe backlash, at which point I'll undo this change.

This comment has been minimized.

Sign in to view

Copy link
@adamreichold

adamreichold Aug 19, 2023 •
edited
Loading

This also speaks to a certain disregard for the agency of downstream users due to breaking builds and tools like cargo-outdated. For me personally, this helps as much as it interferes with security as I cannot use cargo-outdated after this change to improve security by easily following the ecosystem.

Personally, I think it is preferable to let the leaf projects handle things like pinning a version of serde and work on the larger issue by communication, not by changing dependency trees.

This comment has been minimized.

Sign in to view

Copy link
@dtolnay

dtolnay Aug 19, 2023

Contributor

Regarding the disruption of cargo outdated: I found it's not possible to work around by pinning an old time version, but it is possible to work around by requiring >= a high serde_derive version, which will force an old time version while unbreaking cargo outdated (dtolnay/cargo-expand@ee163f0). This at least lets you use cargo outdated on the rest of the dependency graph even if you ultimately publish without the high serde_derive req.

(Hopefully this does not backfire on @jhpratt's intentions with this commit.)

This comment has been minimized.

Sign in to view

Copy link
@jhpratt

jhpratt Aug 19, 2023

Author Member

No worries @dtolnay. I have no issues with instructions on workarounds for users who are undoubtedly aware of the security implications, given that they're viewing a commit dedicated solely to that issue.

As to whether I am disregarding CI breakage for people? As I said, yes, I am. I view security as the top priority, even if that means breaking people's CI.

As indicated, I'm hopeful that David will reverse course here. Making the change opt-in is sufficient to me. If/when that happens, I'll promptly create a new release relaxing the versioning requirement.

This comment has been minimized.

Sign in to view

Copy link
@adamreichold

adamreichold Aug 19, 2023

Sadly, the workaround does only seem to work for direct dependencies. I have both a direct and indirect dependencies on time and I still run into

> cargo outdated
error: failed to select a version for `serde`.
    ... required by package `time v0.3.26`
    ... which satisfies dependency `time = "^0.3"` of package `dbase v0.4.0`
    ... which satisfies dependency `dbase = "^0.4.0"` of package `metadaten v0.1.0 (/tmp/cargo-outdatedHcJdzO)`
versions that meet the requirements `>=1.0.126, <=1.0.171` are: 1.0.171, 1.0.170, 1.0.169, 1.0.168, 1.0.167, 1.0.166, 1.0.165, 1.0.164, 1.0.163, 1.0.162, 1.0.161, 1.0.160, 1.0.159, 1.0.158, 1.0.157, 1.0.156, 1.0.155, 1.0.154, 1.0.153, 1.0.152, 1.0.151, 1.0.150, 1.0.149, 1.0.148, 1.0.147, 1.0.146, 1.0.145, 1.0.144, 1.0.143, 1.0.142, 1.0.141, 1.0.140, 1.0.139, 1.0.138, 1.0.137, 1.0.136, 1.0.135, 1.0.134, 1.0.133, 1.0.132, 1.0.131, 1.0.130, 1.0.129, 1.0.128, 1.0.127, 1.0.126

all possible versions conflict with previously selected packages.

  previously selected package `serde v1.0.183`
    ... which satisfies dependency `serde = "^1.0.183"` of package `metadaten v0.1.0 (/tmp/cargo-outdatedHcJdzO)`

failed to select a version for `serde` which could resolve this conflict

This comment has been minimized.

Sign in to view

Copy link
@AlexTMjugador

AlexTMjugador Aug 19, 2023 •
edited
Loading

I don't have stats, but I imagine a majority of people do not use cargo audit.

For one, I run cargo deny on my CI workflows, which causes them to fail when new security advisories and other dependency-related concerns arise. I already care about security and do due diligence, as it is my responsibility.

I really think the time crate needs to stop the ecosystem policing it has been trying to do lately. First with its MSRV bumps, then with this. Frankly, the projects affected by security issues are those responsible for taking action, not time. As a transitive dependency for many projects, time simply lacks the full context needed to properly assess this potential security threat, which is a fundamental requirement to any well-posed security policy. Also, as the saying goes, "security at the expense of usability comes at the expense of security", especially when the change is rooted in weak reasons deriving from statements along the lines of "I think not many people care about this, so let's break CI to get my voice heard".

A better solution would be to voice your concerns in a changelog entry (which I do read, by the way), and contact @dtolnay or another serde project maintainer directly to establish a plan to fix the underlying issue. Holding your users hostage of your opinion by breaking their CI workflows if they disagree with your context-unaware security assessment is just silly.

I really respect your work on the time crate, but please, can you stop making an habit of breaking CI workflows in minor version bumps? I don't want to come back to this repository every few months for these kind of things. time has been one of the most tedious cargo dependencies I've had to deal with.

Edit: for what it's worth, I also think that serde needs a way to opt-out of such precompiled binaries, which have been causing problems for me and other people. Is it that hard to not break reasonable workflows others have built around crates, I ask?

Edit 2:

I have faith that David will reverse course given the severe backlash, at which point I'll undo this change.

This is just food for thought, but given that you didn't reverse course with somewhat aggressive MSRV bumps despite the feedback against that you received, what should make us think that David will agree with you here?

This comment has been minimized.

Sign in to view

Copy link
@jhpratt

jhpratt Aug 19, 2023

Author Member

which causes them to fail when new security advisories and other dependency-related concerns arise

Keep in mind that there is not a security advisory issued yet. And from the discussions I've seen, they're reluctant to even issue one because the binary hasn't been proven to be harmful. I vehemently disagree with that reasoning. An arbitrary binary should be presumed to be dangerous unless proven otherwise (which has not yet been the case).

First with its MSRV bumps

you didn't reverse course with somewhat aggressive MSRV bumps despite the feedback against that you received

Please keep anything relating to MSRV to the open discussion. That discussion is open-ended and seeks feedback on places "where the policy has caused demonstrably extraneous effort or a concrete inability to do something desired". I will note, however, that there has been zero impact from the policy change to date.

time simply lacks the full context needed to properly assess this potential security threat

I disagree. In no situation is there reason to trust an unverified binary that is being executed without knowledge.

especially when the change is rooted in weak reasons deriving from statements along the lines of "I think not many people care about this, so let's break CI to get my voice heard".

I have never said anything remotely close to this. My decision to not support the most recent versions of serde is rooted solely on the basis of security. It has nothing to do with people "not caring" or deliberately breaking CI. On the contrary, I believe that it has been significantly publicized now to the community (though it was not just a few days ago).

Holding your users hostage of your opinion by breaking their CI workflows if they disagree with your context-unaware security assessment is just silly.

I do not appreciate your tone. Regardless, I have provided the context for my decision, and the fact that it is a vulnerability simply is not an opinion — it is a fact at this point in time given that the binary has not been reproduced.

making an habit of breaking CI workflows in minor version bumps

CI breaking is a side effect that I fundamentally cannot control; I don't control what people do in CI. Even adding a new method in time can break someone's CI.

what should make us think that David will agree with you here?

David is an experienced and highly knowledgeable person who I know can accept feedback. This isn't merely my impression of him from online; I have met him in person. Should he not reverse course, the restriction in time will remain. Upon the next breaking release of time, I will likely consider removing support altogether. Don't read much into that, though — there is no breaking release planned for a significant period of time.

This comment has been minimized.

Sign in to view

Copy link
@adamreichold

adamreichold Aug 19, 2023 •
edited
Loading

the fact that it is a vulnerability simply is not an opinion — it is a fact at this point in time given that the binary has not been reproduced.

If it were that simple, the established channels to handle vulnerabilities, e.g. RustSec, would suffice to handle it instead of creating this special-purpose release.

CI breaking is a side effect that I fundamentally cannot control; I don't control what people do in CI. Even adding a new method in time can break someone's CI.

That appears dishonest to me as the changelog explicitly states that there was control, i.e. it was deliberate decision which took the increased probability of broken builds due to version conflicts into account and weighed it against the assessed threat.

I think reasonable people can have different perspectives on this issue and the chosen approach does feel like overreach to me personally.

This comment has been minimized.

Sign in to view

Copy link
@jhpratt

jhpratt Aug 19, 2023

Author Member

The changelog merely acknowledges that I am aware that it may break CI for some. It is not and was not the intent. I would be more than happy if no one's build broke, but I knew that was almost certainly not going to be the case.

This comment was marked as abuse.

Sign in to view

Copy link
@uklotzde

uklotzde Aug 19, 2023

Imposing such a decision on the users of this library is a bad move, regardless of the discussions around serde. Hotheaded and unprofessional.

Time to remove all time dependencies :(((

This comment has been minimized.

Sign in to view

Copy link
@AlexTMjugador

AlexTMjugador Aug 19, 2023 •
edited
Loading

Keep in mind that there is not a security advisory issued yet. And from the discussions I've seen, they're reluctant to even issue one because the binary hasn't been proven to be harmful. I vehemently disagree with that reasoning. An arbitrary binary should be presumed to be dangerous unless proven otherwise (which has not yet been the case).

A crate can already execute arbitrary code at build time via build scripts and procedural macros. If your supply chain threat model is strict enough, you should already treat any dependency as a black box that may execute malicious code unless proven safe. (In such a strict threat model, you may also find good reasons to treat updates to the Rust compiler as potentially unsafe unless approved, which also relates to how strict MSRV requirements may render crates unaligned with the necessary security audits in such controlled environments. But I agree that MSRV is another topic in itself, so I won't mention it any further.)

You may argue that downloading source code from crates.io is safer than downloading a random binary from somewhere else, but nothing prevents source code from being obfuscated, attackers may take control of a crate published on crates.io, and build servers can get infected with malware that redirects requests to crates.io to another website trusted by the HTTPS stack. It's reasonable to establish a threat model where executing a random binary is not significantly different from just pulling a dependency from crates.io.

Edit: moreover, I don't think that the "any binary is unsafe unless proven safe" model for RustSEC would scale well. Verifying safety for binaries in a timely fashion sounds like too much work for a few people, and introduces political concerns I'd rather steer away from.

In no situation is there reason to trust an unverified binary that is being executed without knowledge.

I know that serde now executes binaries, and under my threat model that's not a significant enough difference with respect to running arbitrary code on build via other means (in the previous section I addressed this topic).

I do not appreciate your tone. Regardless, I have provided the context for my decision, and the fact that it is a vulnerability simply is not an opinion — it is a fact at this point in time given that the binary has not been reproduced.

I apologize for my admittedly harsh tone, but as I stated in previous occasions, I also do not appreciate being caught on the crossfire of things I don't even have a saying. It'd be nice to at least gather user feedback before taking these measures, via GitHub issues or discussions, for example.

Edit: or, if you really want to keep doing this sort of things, it'd be really thoughtful to not put additional busywork on maintainers of likely less resourceful open source projects by keeping an eye on reverse dependencies, and submitting PRs to them to fix issues with time that you directly cause for supposedly non-breaking version bumps. A few days ago a maintainer from a dependency I use in a npm project was kind enough to do this to a repository of mine due to a mistake they made, and I really appreciated their responsibility. As a side bonus, that might give you some insight on how your projects are used in the wild.

Edit 2: and yes, I'm willing to submit pull requests as soon as I think some library can be improved, or if I directly cause undue inconvenience to my users. It's having good netiquette for fellow developers, in my opinion.

This comment has been minimized.

Sign in to view

Copy link
@jhpratt

jhpratt Aug 20, 2023

Author Member

@uklotzde Please be respectful and follow the code of conduct.

A crate can already execute arbitrary code at build time via build scripts and procedural macros.

That code can be reviewed (and frequently is by organizations). The binary cannot.

It'd be nice to at least gather user feedback before taking these measures, via GitHub issues or discussions, for example.

I will be opening a vote on GitHub discussions in a few minutes specifically for this issue.

supposedly non-breaking version bumps

Again, I don't appreciate the undertone here. What I have done in time is non-breaking, just as what was done in serde is non-breaking.

As a side bonus, that might give you some insight on how your projects are used in the wild.

I am already quite aware of how people use time in the reality, and welcome people to open issues for use cases they do not believe I am supporting.

This comment has been minimized.

Sign in to view

Copy link
@jhpratt

jhpratt Aug 20, 2023

Author Member

A poll has been posted: #611

This comment has been minimized.

Sign in to view

Copy link
@adamreichold

adamreichold Aug 20, 2023 •
edited
Loading

A poll has been posted: #611

If you are genuinely seeking input on this matter, then as @epage pointed out over there, a balanced description would have been a necessary prerequisite. I have not read a single comment here which discusses the change in build times, so framing the matter solely around that seems disingenuous.
serde_json = "1.0.68"
serde_test = "1.0.126"
trybuild = "1.0.68"
Expand Down
2 changes: 1 addition & 1 deletion time-macros/Cargo.toml
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "time-macros"
version = "0.2.11"
version = "0.2.12"
authors = ["Jacob Pratt <[email protected]>", "Time contributors"]
edition = "2021"
rust-version = "1.67.0"
Expand Down
2 changes: 1 addition & 1 deletion time/Cargo.toml
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "time"
version = "0.3.25"
version = "0.3.26"
authors = ["Jacob Pratt <[email protected]>", "Time contributors"]
edition = "2021"
rust-version = "1.67.0"
Expand Down

0 comments on commit 500f8e4

Please sign in to comment.