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

Process for advisories without a fix? #1092

Closed
saethlin opened this issue Oct 28, 2021 · 14 comments
Closed

Process for advisories without a fix? #1092

saethlin opened this issue Oct 28, 2021 · 14 comments

Comments

@saethlin
Copy link
Contributor

saethlin commented Oct 28, 2021

@Shnatsel wanted to discuss this #1079 (comment) and I feel like now is a pretty good time because arrow-rs has had this issue open about a soundness bug for a month now without a fix or an advisory: apache/arrow-rs#777 and I think I just found one or two more soundness defects.

So what should be done? Notify the crate author(s) and give them some number of days to patch before issuing an advisory? Do we track time from when an issue is opened identifying the problem as a soundness issue, or from when we tell them that this has prompted a RustSec advisory?

@Shnatsel
Copy link
Member

This is a difficult topic because issues that go unaddressed are a risk for the users, but on the other hand, publicizing them without prior notice puts a lot of pressure on the maintainers and contributes to maintainer burnout.

People also tend to react very negatively to real or perceived threats. Anything that reads even remotely like "fix this now or we'll publicly denounce you for not doing it" is going to be counter-productive on multiple levels.

So looks like we need to provide some sort of advance notice and draw the maintainers' attention to the issue, but avoid mentioning unilateral action from our side.

Possible solutions

My proposal is to post something like the following on the issue, tailored to the problem at hand:

This appears to be an instance of use-after-free, which is usually an exploitable security vulnerability. Please publish a fix as soon as reasonably possible.

I also encourage you to file a security advisory at https://github.com/RustSec/advisory-db so that the users of your crate can check if they're affected and upgrade!

Followed by a 2-week grace period, after which we would publish an advisory and post the usual notice on the bug:

Heads up: this issue has been included in the RustSec advisory database. It will be surfaced by tools such as cargo-audit or cargo-deny from now on.

Once a fix is released to crates.io, please open a pull request to update the advisory with the patched version, or file an issue on the advisory database repository.

Thoughts?

I'd also love to hear from @alamb who has been on the receiving end of this.

@tarcieri
Copy link
Member

That sounds fine to me @Shnatsel.

Though advisories for vulnerabilities without fixes can lead to alert fatigue, we've seen several positive outcomes from them too, most notably things like encouraging downstream users of a particular crate to open PRs with fixes for those vulnerabilities.

@saethlin
Copy link
Contributor Author

Perhaps we should also mention something right away like

If you do not file a security advisory within 2 weeks, we plan on filing one for you. This advisory will be surfaced by tools such as cargo-audit and cargo-deny. We are aware that this can be disruptive, please get in touch if this would be a problem for you.

@Shnatsel
Copy link
Member

I have considered that, but decided against it because it could be perceived a veiled threat along the lines of "fix this now or we'll publicly denounce you for not doing it". This is an especially touchy subject for projects that have had negative publicity due to soundness issues in the past - e.g actix-web or arrow-rs.

@saethlin
Copy link
Contributor Author

🤷 we are going to publicly announce the lack of a fix

But I'll defer to your experience here. I'm not surprised that the arrow-rs people are touchy.

@alamb
Copy link
Contributor

alamb commented Oct 29, 2021

Thank you for this discussion. @saethlin and @Shnatsel . I am an arrow-rs maintainer and Arrow PMC member.

TLDR is I think the process described in #1092 (comment) would have improved my experience with RUSTSEC - specifically the advanced warning, in the form of "hey we noticed this issue, would you like a chance to comment / clean it up before we publicize it more broadly?"would have helped.

Had we had advanced warning, occurred I think I probably would have spent time cleaning up the tickets to try and make things clearer so that by the time the RUSTSEC came out, a more nuanced picture of the risk would have been available.

In the arrow-rs case, for example, there is a certain API that does not do input checking but was not marked unsafe and thus if not used with extreme care can lead to undefined behavior. The fact that there are several tickets filed each with seemingly different symptoms but the same underlying cause I think is confusing and makes the external optics even worse. Even more confusing now is that there are RUSTSECs open for some of the security tickets, but not all of them,

You might ask "why did you not clean those tickets up prior to a RUSTSEC intervention"? And the answer is the politics of the project -- specifically one of the community members started a fork of the project, because among other reasons, the opinion that the existing code could not be made sufficiently safe. Any discussion about "we can make the existing code safer vs rewriting everything" generated many back and forth wall-of-text emails and strong emotions (you can see the mailing list discussion about this here

So in summary:

  1. I think it is a fair to include a RUSTSEC advisory for this issue and thank you for doing so
  2. I would have liked to spend time clarifying the issues / making the risk clearer before the issue being publicized more broadly

In closing, thank you for the work you do on RUSTSEC -- I think it is valuable and is spurring positive improvements to the arrow-rs crate.

@alamb
Copy link
Contributor

alamb commented Oct 29, 2021

Given this discussion, I am going to go and clean up the arrow-rs security tickets to consolidate them. 👍 Apologies if this messes up the RUSTSECtrackingg

@jorgecarleitao
Copy link
Contributor

jorgecarleitao commented Oct 30, 2021

I agree with incentivizing maintainers to fill their own advisories on every publicly known vulnerability.

I think Tokyo offered us a good example:

(All actions were executed by the maintainer)

It is unfortunate that both developers and maintainers act differently if/when the vulnerability is presented in advisory-db because it incentivizes to not being transparent. IMO we should work towards changing this: imo advisories should a "chore" that maintainers perform as part of a disclosure process; it should not have a negative connotation associated to it - bugs are a fact of life, we are just increasing transparency about them given their implications to infosec.

IMO the decision tree goes a bit like this:

  1. maintainers do have the bandwidth to patch:
    • they release a patch and document it in an advisory
    • users update
  2. maintainers do not have the bandwidth to patch:
    • they document the issue in an advisory
    • users audit their own code, fix uses of unsound APIs, and add exception to .cargo/audit.toml (e.g, like here or here)
  3. maintainers do not have the bandwidth to document the vulnerability:
    • the community steps in and documents it instead

In this direction, I would consider re-wording the first notification to something like:

This appears to be an instance of use-after-free, which is usually an exploitable security vulnerability.

If a fix and release is feasible within two weeks, I encourage you to release a patch and file a security advisory at https://github.com/RustSec/advisory-db so that users are notified by cargo audit that they should update to a newer version.

If a fix and release is not feasible within two weeks, I encourage you to file a security advisory at https://github.com/RustSec/advisory-db describing the vulnerability and how users can avoid it, so that they are notified by cargo audit that they should audit their code and protect themselves from this vulnerability.

And the second to something like

Heads up: I have included this issue in the RustSec advisory database. I encourage you to take a look and PR more details on how users can avoid being exposed to it on their own applications.

Once a fix is released to crates.io, I encourage you to update the advisory with the patched version so that users know that they can update the dependency. 👍

this may better represent the advisory db as a tool to document vulnerabilities (and how users can protect themselves).

@alamb
Copy link
Contributor

alamb commented Nov 1, 2021

FWIW I don't think it is a problem to see the Rustsec database as a public shaming mechanism that helps to raise the priority of security issues of rust projects. I think it is serving that purpose for arrow-rs and that it is a good thing.

However given that perspective, I would not be surprised if most maintainers of projects will not be the ones filing their rustsec entries about their own projects.

@Qwaz
Copy link
Contributor

Qwaz commented Nov 1, 2021

I'm not sure if you were trying to make a joke, but I think it is very problematic if people start to perceive RustSec as a public shaming mechanism especially for open source projects that the maintainers voluntarily work on. Such a view will make everyone (RustSec people, library maintainers, and bug reporters) involved unhappy and unnecessarily hostile to each other. A bug reporting should be a cooperative process, and I believe an advisory database should be viewed as a communication tool to share knowledge about bugs not as a public database of shameful events.

P.S. I didn't realize that you are an arrow-rs maintainer and thought that you are offending them at first 😅

@alamb
Copy link
Contributor

alamb commented Nov 1, 2021

I'm not sure if you were trying to make a joke, but I think it is very problematic if people start to perceive RustSec as a public shaming mechanism especially for open source projects that the maintainers voluntarily work on.

I was not trying to make a joke.

A bug reporting should be a cooperative process, and I believe an advisory database should be viewed as a communication tool to share knowledge about bugs not as a public database of shameful events.

I agree this would be an ideal outcome

@alamb
Copy link
Contributor

alamb commented Dec 28, 2021

BTW I wanted to say that #1131 (where @saethlin proactively updated the rustsec db when the arrow fix was release) was a really nice experience for me.

I actually had it on my list of things to do to update the db, but having it done by maintainers of rustsec meant that I came away from the while rustsec experience feeling much more like this is intended to be a communications exercise rather than just additional work foisted from an outside party.

So thank you 🎩

@pinkforest
Copy link
Contributor

pinkforest commented Aug 14, 2022

We have now a pretty fair established policy around this so closing this as completed.

We do need to do more to educate people about that it helps them and we are not against them but that is wider goal.

It is a difficult thing to do to balance the interests between all the stakeholders - we can't really have any formulaic method of communication or templates as we have to have a personalised way of communication to motivate the maintainer a.k.a the human touch(tm)

If we need to further discuss this we can do in the discussions - thanks all! :)

If any documentation / guidance / policy needs to be fixed we can file a PR as result of the discussion if any.

@alamb
Copy link
Contributor

alamb commented Aug 15, 2022

It is a difficult thing to do to balance the interests between all the stakeholders - we can't really have any formulaic method of communication or templates as we have to have a personalised way of communication to motivate the maintainer a.k.a the human touch(tm)

These are wise words indeed 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants