Add advisory for unicase

[sugar700]

I think this is a valid vulnerability, see CVE-2000-0979, CVE-2002-1374 and CVE-2014-6394 for similar vulnerabilities. In short, unicase used to consider "abc" and "a" to be equal.

[pinkforest]

Thanks for noticing it - It seems like the fixed one is latest 2.x release track

Affected versions have not been yanked - including non-semver for 1.x release track that has no fix if anyone is stuck on that.

Whilst I would personally agree that it is a valid vulnerability -

Controversial aspect here is that whilst non-controversially this was reported as a test failure - with the fact that the maintainer acted very soon after to fix it - did not acknowledge and announce / agree either implicitly or explicitly on any security related issue making it controversial to write advisory without maintainer providing any reaction(s) around the security / it's implication.

Could we get a simple informed response from the maintainer on this and outlay the CVE vector to @seanmonstar and perhaps allow the maintainer to yank the affected versions ?

@amousset would like to get your opinion as well on this

[amousset]

About the relevance of an advisory, I'm not sure it is clear with current policies but I think it makes sense. We could say that is not a vulnerability in itself, and that a lot of other bugs could be a sources of vulnerabilities is dependent crates without justifying an advisory for the upstream lib. But given the bug and the way this library could be used for ACLs/Headers matching/etc. (like in jsonrpc-http-server the Ascii API is not affected), it feels a bit like the netmask vulnerability that affected the npm ecosystem, and was a CVE on its own while the actual vulnerabilities were in dependent packages.

I agree that we should try to get the opinion of the maintainers about opening an advisory anyway.

[amousset]

After a closer look, I don't see cases in most downloaded dependent crates where this leads to actual vulnerabilities (especially as the Ascii comparison is unaffected), except maybe for interledger-rs (I opened https://github.com/interledger-rs/interledger-rs/pull/744 to update the dependency).
Therefore it's not that severe, but an advisory could still be relevant.

[pinkforest]

This crate has about 50k downloads a day, ~5k (10%) of them affected versions - Total downloads ~ 35 M

I propose the below actions:

If the maintainer is not going to address this in two weeks we merge as *informational / unsound -

*This will require a change from @xfix to change the advisory to informational / unsound

If the maintainer responds, yanks or otherwise addresses it we don't need an advisory for the older versions

[amousset]

"informational = unsound" has a specific meaning that does not match this case. It could maybe be a "notice" but it does not seem to have been used yet, so I'm not sure we want to use it (as it could break some tooling made around the db).

[pinkforest]

@tarcieri What is the guidance around this ? as informational necessary doesn't require concrete security issue but we have concerns but we are unsure about the terminology effect on this one.

Our unsound in the maintainers guideline is defined differently and broadly:
informational = "unsound" is used for issues that can only be triggered by a programmer (as opposed to e.g. a malicious input), and/or require very contrived code to trigger.

Also our contributing guideline links to - which has very broad definition of "Behaviour considered Undefined"
https://doc.rust-lang.org/reference/behavior-considered-undefined.html

unsafe-code-guidelines WG:

Unsound

Crate is not [sound](https://rust-lang.github.io/unsafe-code-guidelines/glossary.html#soundness-of-code--of-a-library), i.e., unsound.

A crate is unsound if, using its public API from safe code, it is possible to cause [Undefined Behavior](https://doc.rust-lang.org/reference/behavior-considered-undefined.html).

I think the undefined behaviour defined by the above only considers unsoundness in the limited context of unsafe

[seanmonstar]

I certainly could be wrong in this, so take with a grain of salt. But this seems like a slippery slope of marking all bugs as security vulnerabilities. I could see yanking perhaps being a better case here. Even then, though, I think simply fixing the bug in this case is sufficient. 🤷

Though, with regards to "unsound", this bug is not what Rust usually calls "unsound", which is behavior that can trigger undefined behavior in the compiler. The bug type is closer to just "incorrect".

[pinkforest]

@seanmonstar thank you for taking time and offering to yank the old versions - that would certainly alleviate my concerns 💜

it's tricky to get the balance on this right as some category of libraries can have critical downstream effect e.g. crypto but string comparison can be borderline and I would also certainly agree that this would be controversial advisory especially when there is no reported downstream that makes them vulnerable because of this bug.

[pinkforest]

Since there are no identified downstream usage which could drive any indication that there are security issues related this bug, I am going to close this one since additionally we do not have any recognised category together with the maintainer indicating that there are no security issues to action around this bug.

We'll have to have wider discussion around binary / library relationships in the Rust ecosystem at wide and how to deal with them.

I've recommended if the old versions could be yanked but that will be up to the maintainer if @seanmonstar wishes to do so.

Re-open if there is identified impacted downstream usage and we can have another discussion.

Thank you all for your valuable input 💜

_FWIW - There is https://github.com/interledger-rs/interledger-rs/pull/744 (thanks @amousset) which may be unmaintained as welll briefly looking at it and is potentially affected downstream - this can be handled separately.