Informational advisory for SergioBenitez/Rocket#1312

[Qwaz]

The affected version of rocket contains a Clone trait implementation of LocalRequest that reuses the pointer to inner Request object. This causes data race in rare combinations of APIs if the original and the cloned objects are modified at the same time.

rwf2/Rocket#1312

[Qwaz]

I'm not sure how to represent LocalRequest's Clone implementation as path. I put rocket::local::LocalRequest::Clone::clone, but this looks awkward?

[8573]

I don't think ...::LocalRequest::Clone::... is right — Clone is not in LocalRequest's namespace. I think the proper way in Rust itself to be specific about selecting the clone method from the Clone trait rather than any other clone method of LocalRequest would be to write <rocket::local::LocalRequest as Clone>::clone (a qualified path), but, looking at the rustsec-crate source code, I think the most specific path supported here is rocket::local::LocalRequest::clone.

---

Although @SergioBenitez said "I am happy to have a RustSec advisory filed", I wonder, should this advisory be informational = "unsound" rather than a full security advisory, given your remarks on "Is this a security bug?" in rwf2/Rocket#1312?

[Qwaz]

I consider the severity of this bug is somewhere between low to medium according to npm's scale. There is an ongoing discussion in #313 to determine the boundary between informational advisories and security advisories, and we can decide which type of advisory to file based on where the discussion settle on. I'm also fine with filing an "unsound" informational advisory first and later considering an upgrade.

Thank you for your feedback on the path question. I'll wait and see if any maintainer can confirm that.

[8573]

I consider the severity of this bug is somewhere between low to medium according to npm's scale.

Okay, then I suppose I misunderstood the sentences—

I believe the security impact of this bug is minimal. I think It is still worthwhile to file a RustSec advisory after the bug confirmation considering the people who want to be notified about unsound APIs.

—as having meant "This doesn't seem important as a security bug, but it should have an advisory as a soundness bug"; I apologize for the misunderstanding. Personally I don't mind being notified about the bug regardless of which type it is, and, with your meaning clarified, I make no objection to (and, either way, my approval is not needed for) filing the advisory without the informational field.

[Qwaz]

The bug was reported before the existence of unsound informational advisory, so I was not particularly relating the report to the unsound advisory at that time. (I'm not against making the advisory informational, just clarification)

[SergioBenitez]

The APIs exposed by the local module, including LocalRequest, are documented and intended primarily for and nearly exclusive use during testing. As a result, the possibility of the unsound implementation leading to a vulnerability should be nil.

Nevertheless, these APIs are not concretely restricted to testing environments only, and so there exists a possibility, even if remote, that the APIs find themselves in use in a vulnerable path. However, in such a path, a LocalRequest request has no utility being cloned, and even less so prior to being populated, as required to effectuate this bug. Consequently, again, the possibility of a vulnerability arising should be nil.

Certainly, the implementation was unsound. However, based on the reasoning above, no inherent security vulnerability can arise from documented, expected, or probable use of the APIs. As such, I would advocate for publication as an informational advisory.

[Qwaz]

Thank you for the comment, @SergioBenitez. I've fixed the function path and made the advisory informational.

[Shnatsel]

Apologies for overlooking this PR. It's merged now.