itoa use of mem::uninitialized before 1.0.0

[pinkforest]

97,514,713 downloads all time, ~200k a day which ~80k old < 1 versions

@saethlin raised this earlier too - #1194 but more general discussion

I'm framing this issue on itoa crate issue -

I have some doubts that raising advisory would be too noisy as of right now as a transitive dep but just making the issue

plus @Nilstrieb @RalfJung @5225225 and @dtolnay as you've been across this as well ..

Should we raise informational = "notice" on itoa < 1.0.0 ... at some stage ?

To get the rest of the deps bump as there is actionable fix ?

Also maybe not now but e.g. after perhaps csv - it's largest user/s has/ve bumped up ?

csv (~50k downloads a day) is the most important user stuck on 0.4.8 issue open here:
BurntSushi/rust-csv#271

As itoa is not going to get backport and plenty people still using the old
dtolnay/itoa#36

Interestingly enough @RalfJung commented: BurntSushi/rust-csv#271 (comment)

Status update on that: latest rustc nightly mitigates that UB by making mem::uninitiailized actually initialize the memory, which makes it slower. That means users of itoa 0.4 will see a performance degradation.

Other users of old num-format, serde_prometheus, lopdf, yarte_helpers, itoap, varisat-dimacs, json_in_type, plenty others

It would be nice if someone does outreach to these other crates

Ralf was quick - bcmyers/num-format#29 - however num-format seems unmaintained

[dtolnay]

Can you clarify whether your interest in an advisory for this is primarily motivated by the use of a deprecated function, or by the potential performance change, or by something else?

[pinkforest]

To deter the continued use of now deemed unsound function despite it being assumed something else in the past.

[dtolnay]

Ralf's comment that you quoted above explains that this usage is going to continue working.

[pinkforest]

Yeah we have precedent that despite rustc bump making it sound we would usually still file advisory on these

It is perhaps regrettable that we cannot mark related rustc version in our advisories - I'll raise a separate tooling feature for that.

EDIT: Raised rustsec/rustsec#673

[saethlin]

rustc contains a mitigation for this use pattern, I do not think that mitigation means we shouldn't encourage people away from it. I think that informational = "notice" is probably appropriate here. At least for the way I use cargo-deny at work, that means that we would not get a CI failure, but at some point some human is going to see the output and may choose to take action on it at their leisure.

[dtolnay]

I would strongly encourage you to revisit that precedent if possible, although I appreciate this issue may not be the place to debate it. (It's my crate though, and I've already gotten pinged, so here I am…)

In my opinion you are striking the wrong balance, even considering that this would be a notice advisory. It makes advisories that much less beneficial to everyone, even the actually useful ones, when folks need to sift through noise to catch the ones which are meaningful.

In this case a notice would give people the satisfaction of choosing to upgrade from a version that works (and the Rust team has committed to continuing to work) to another version that works (and will likewise continue to work) but with a bigger version number. There are already tools which deliver satisfaction equivalent to this in automated better ways than the advisory process, such as https://crates.io/crates/cargo-outdated and Dependabot.

An advisory is about saying "this upgrade matters unusually much" and a notice is about saying "here is a piece of information that some of you may find actionable regarding this library" and I don't think those apply to itoa any more than any other library with a major version update.

[pinkforest]

Well nothing is decided yet - maintainer input is essential and very welcome whether we should do an advisory.

Often maintainers find it helpful for us to do these advisories and sometimes they don't -

We can't assume what the maintainer preference(s) are -

The balance depends on many factors - we are here to find and clarify these facts -

Facts also need to be clarified around indicating that rustc version itself does not stop us especially if the maintainer agrees that an advisory would be in order or other factor(s) that weigh in favor of raising an advisory -

We typically ask and clarify for the maintainer(s) opinion(s) as to whether we should and now it is great that we've been provided in depth informed opinion about this so we know it would be controversial advisory.

Security advisories are normal and they happen day to day -

My proposed approach here was to get the biggest dependency bumped up and then notice the rest.

The second biggest dependency that will be affected by this is probably going to get advisory anyways around it being unmaintained so the signal would be there already -

So I am not sure what we are referring with the noise here.

[dtolnay]

I agree with you that security advisories are normal and are about helping users, not about finger-pointing; far be it from me to conceal security issues with my code. It's clear to me that concealing a legitimate security issue would not be in my interests because it damages people's ability to have trust in using my crates; since all code is at risk of flaws, the best someone can ask is that they're handled responsibly every time. The reason I'm in this thread and not any of the other ones, where I could be making the same argument, is because this is the one I got pinged on to share my opinion.

The problem is I'm not seeing the connection to security in the itoa case, or the connection to helping users. Concretely: since there isn't a security issue in current or any future Rust version, a notice would be a net negative to the userbase of the rustsec db, in the form of noise.

Maybe the core of the issue comes down to the Rust team's efforts on language stability being disregarded/counteracted. I can only speak from my experience with the standard library team but I'd guess the lang team would see it similarly: stability is an enormously important factor in language evolution decisions. Our decision-making on how to treat uninitialized in future versions of Rust is informed by the tradeoffs around stability and churn and real-world code. When we reach a team decision that uninitialized, as used by itoa, is going to continue to work, that decision is consciously about churn reduction and carries an opportunity cost. It becomes disheartening when the reception from outside the team is "eh, let's go cause the churn anyway". What you're seeing (when dozens of issues in dozens of projects, and collectively hundreds of people getting alerted already at this point, or thousands with the addition of a security notice) is the churn that the Rust teams decided not to make necessary.

[pinkforest]

Informational advisories are not necessarily concrete security issues -

Both the intended usage and effect of informational is very different to the regular ones -

Maybe this is where the misaligned expectations come to play -

The scope of the informational advisories - as we try to document them - is to supply factual information about any matters concerning the crates to help any maintainer(s) to make informed choices about the dependencies they use - whether they contain notices or unmaintained advisories there of that may or may not raise concrete security related issues depending on the individual user's risk appetite and / or other factors unique to them - this is why we call them "advisories" and not security "issues".

It is up to the downstream to decide to choose to use a dependency based on factual information that is not perhaps being kept maintained - what I would expect on the ones who don't bump the dependency - alongside the current reality on what we know now based on what happened in the past.

I totally agree that the itoa crate does not "deserve" a security "issue" but this is the only way we could potentially notify via "advisory" the downstream that somewhere alongside their upstream - but downstream from itoa - has not kept their dependencies up to date and unmaintained crates we all know can raise security concerns alone beyond the issue described here.

It comes down to communicating expectations appropriately and ensuring people understand the context through and if there has been any advisories that have communicated the issue(s) wrong way then that would be another issue to fix.

[dtolnay]

Like in my previous comment, I still struggle to see the connection to security in the itoa situation; (the edits to) your last comment makes it clear to me that what you have in mind is grossly outside the scope of what I would want from a security advisory database.

From your edit — "I totally agree that the itoa crate does not deserve a security issue" — it sounds like you agree that none of this really has anything to do with mem::uninitialized, ultimately, despite the issue title. "This is the only way we could potentially notify [people that something downstream of itoa] has not kept their dependencies up to date"— false, go ahead and file unmaintained advisories on anything downstream of itoa that is not updating its dependencies. As we all know, unmaintained crates can raise security concerns beyond anything to do with their dependency on itoa. Itoa itself is maintained, including the 0.4 series if there were anything legitimately in need of fixing there for security reasons.

To go into detail on itoa, the salient difference between 0.4 and 1.0 is that itoa::fmt and itoa::write functions are removed. Those functions used to be the main entrypoint of the crate in 0.4.0, but superseded by itoa::Buffer in 0.4.3, and the old API deleted in 1.0.0. Given the only "security-related consideration" (whether we're calling it issue or not) is in crates downstream of itoa who might be unmaintained, hopefully it's abundantly clear that itoa::fmt and itoa::write removal has no bearing on their security; this is just API evolution/polish which is inconsequential to security.

You can use a dependency on itoa 0.4 as a proxy for a crate being unmaintained, which is totally fair as a way to identify crates that should get an unmaintained advisory. If a crate uses itoa 0.4 but is maintained (I guess they liked the older API better, or they want to support older compilers, or whatever other reason) but you still feel the security advisory database should advise about the situation (absent any relation to security), you're just turning the advisory database into a worse version of cargo outdated. I am skeptical that the advisory database (with manual PRs per crate, and humans getting pinged like in this thread) is the way to meet this use case. It's up to you but I would urge you to at least use a different advisory kind, because again it's just noise as far as my own use of the database is concerned. informational = "has-outdated-dependency" for the downstream crate which is maintained but chose not to update a dependency? informational = "has-newer-major-version" for past semver versions of the dependency? Like I said, this is just a worse version of cargo outdated at this point.

[pinkforest]

Why did those functions were removed in first place then ?

Just because the behaviour changed in 1.0 shouldn't stop us flagging 0.4 -

Whether 0.4 got backported fix or not - yes fix to an issue we all know exists -

Just because you removed the functions - for the good reasons - doesn't in isolation mean anything.

This is about itoa 0.4 now creating issues in the ecosystem that has security considerations involved beyond the unmaintained.

These considerations don't get raised if people use 1.0 version either directly or via their transitive dependencies where the issue is fixed.

We can't assume the maintainer intent why they did not bump to 1.0 but we know that 0.4 should not be used due to the issue.

[Noratrieb]

This is about itoa 0.4 now creating issues in the ecosystem that has security considerations involved beyond the unmaintained.

The point is that itoa 0.4 will not cause security issues anywhere using a recent compiler. I don't think it's worth adding an advisory for what is, in the end, a non-issue.

[pinkforest]

Yeah well people use the "old" compiler that is a fact of life and we can't assume that everyone is using the latest nightly.

This mitigation (not a fix?) is not in stable yet is it ?

I already raised issue that we need to include rustc field here:
rustsec/rustsec#673

Nothing here should perhaps stop us filing advisory about known issue and being transparent about it.

And this advisory naturally would contain informational about compiler versions and how these changes mitigate things.

[5225225]

The point is that itoa 0.4 will not cause security issues anywhere using a recent compiler.

To be honest, using any compiler. We don't emit noundef for integers, so LLVM has to assume that we're treating it as a MaybeUninit<u8> already, as I understand it. As in, I'm not aware that there's any chance of miscompilation here.

The mitigation makes itoa 0.4 fully defined behaviour. Not according to the docs (and not when running under miri or memory sanitizer), but the typical run of itoa 0.4 in a normal environment after the mitigation would not be LLVM UB, even if we did emit noundef.

Or, put another way, itoa 0.4 is clearly sound by our rules if we use mem::zeroed();. mem::uninitialized() is Basically just mem::zeroed, but with a different value (when it does decide to do the filling, which is "always unless you're looking for UB")

If we didn't have the 0x01 filling and we emit noundef on ints. I would be a bit more worried about it. But we don't.

[pinkforest]

As a precedent I will also cite unicase: #1176

We don't generally file informationals where the maintainer does not believe there is any security issues we should care about.

Let me be clear on the above so let's not go guessing about it 🤷‍♀️

This shouldn't stop us communicating though and getting these clarified which we are now doing.

This is why I directly asked whether we should file an advisory or not and to me it sounds like we should not.

[Noratrieb]

There is and will be no compiler version where itoa 0.4 has any vulnerabilities.

[pinkforest]

Great, so then we can close this then and we don't need to file advisories given the maintainer does not want us to either.

Thank you all for clarifying this situation with itoa.