Process to handle security reports #56
Conversation
|
In general, i'm okay |
There was a problem hiding this comment.
Looks great! I know you said "early", so I look forward to seeing what else we add. I know for myself it would be awesome to have some more of these steps have more details and links, similar to a runbook format.
Aside: the mermaid rendering is awesome. I need to use this more often.
100% if this mermaid flow seems "good" I will start working on the runbook part (I just wanted to avoid many logic/text changes combined). Sounds good if we include the runbook in this document and PR? Or do we prefer to do it in a separate one to simplify the review process? |
|
Ok, so based on the positive early feedback on the process I included the runbook too 🥳 |
Co-authored-by: Sebastian Beltran <[email protected]>
Co-authored-by: Sebastian Beltran <[email protected]>
Co-authored-by: Sebastian Beltran <[email protected]>
|
I solved the format issues in 054ab8a |
| F --> G[Handle Related PRs & Issues] | ||
| G --> H[Request GitHub to Remove Public PR/Issues] | ||
| H --> I[Create Public Placeholder Issue] | ||
| I --> J[Acknowledge within 5 days to the Reporter] |
There was a problem hiding this comment.
Ah, re-reading again and I see it is here: https://github.com/expressjs/security-wg/pull/56/files#diff-e6c76e85b62187db4232abf6ad2299fea45f03b96798e0a40b2510cf8854fbbdR80
That said, I would not be surprised if the "steps" get used more like a "checklist" for us, so I still think it would be good to include in the steps.
| - Acknowledge receipt of security reports within the required timeframe. | ||
| - Assign an Analyst to assess and validate the report. | ||
| - Ensure communication with the reporter throughout the process. | ||
| - Coordinate the remediation process if a vulnerability is confirmed. |
There was a problem hiding this comment.
You mention assigning an Analyst, are we specifically not "assigning a remediation developer" here for a reason?
There was a problem hiding this comment.
yes, of all the roles need to be assigned (though the act of "assigning" in reality doesn't need to be so formal)
each issue would need an analyst at minimum, and if it was determined to be a vuln, then remediation developer(s) and reviewer(s) will be needed
|
|
||
| 2.2 At this stage we are capable of determining the severity of the report based on the information provided and also if the report is still relevant. In case that the team has considered the report to be irrelevant or not valid, the Security Report Coordinator (SRC) will need to close the issue and inform the reporter that the report has been dismissed, ideally we can provide a reason for dismissal to prevent the report from being resubmitted within the project(s) in the future. | ||
|
|
||
| 2.3 If the report is considered relevant and valid, the Security Report Coordinator (SRC) will create an advisory and request a CVE number. The Security Report Coordinator (SRC) will also include the remediation developer(s), analyst(s) and potentially the reporter in the advisory, so they can start to work on private fork to fix the security issue (example: [GHSA-qw6h-vgh9-j6wx](https://github.com/expressjs/express/security/advisories/GHSA-qw6h-vgh9-j6wx)). |
There was a problem hiding this comment.
How to do this stuff in detail is exactly what I hope we can get training for the team on via the STF funding. @ctcpip has been incredibly helpful with this but having a more cross training and depth here will help keep it sustainable. I wonder if the GH training also intends to cover this, @sheplu have you gotten an update on that?
| ``` | ||
|
|
||
| ## Roles & Responsibilities | ||
|
|
There was a problem hiding this comment.
suggest adding a role for Finder, which is often, but not always, the same person as the reporter
| ### Security Report Coordinator (SRC) | ||
|
|
||
|
|
||
| This person acts the focal point for an specific security report and ensures the report follows all responsible disclosure guidelines, also coordinates the remediation process if a vulnerability is confirmed. The SRC ensures that the security report follows the process and coordinates necessary actions, but does not perform analysis, remediation, or patching themselves. |
There was a problem hiding this comment.
coordinators are often involved in analysis, remediation, and review. I don't see why we would need to codify a restriction on those activities
There was a problem hiding this comment.
The idea was to clarify that the coordinator is not necessarily responsible for performing the patch or conducting the analysis. However, this does not mean the coordinator can't also be the remediation developer or the analyst. In other words, one person can hold multiple roles 🤔
| - Assess the security report and determine its severity (assist in CVSS). | ||
| - Validate the reported vulnerability against best practices. | ||
| - Identify potential mitigation strategies. | ||
| - Prepare a report for the Security Report Coordinator. |
There was a problem hiding this comment.
well maybe report is not the best world, the idea is to ensure that is "sharing the findings" with the coordinator 🤔
docs/handle_security_reports.md
Outdated
|
|
||
| ### Step 1: Assign Security Report Coordinator (SRC) and consolidate the report | ||
|
|
||
| 1.1 One person from the Security Triage Team will volunteer and self-assign to oversee the case. It is expected that the person will remain assigned until the end of the process, so they effectively take the rol of [the Security Report Coordinator (SRC)](#security-report-coordinator-src). |
There was a problem hiding this comment.
This need not be limited to one person, but I'm fine with leaving this doc referring to that, just for simplicity. In practice, having two coordinators is acceptable, and often better than just one. Also, someone needs to be reviewing the actual advisory content before it goes out, and this role is best addressed by a second coordinator.
|
|
||
| * Move the issue to the private repository called [expressjs/security-triage](https://github.com/expressjs/security-triage). | ||
| * For any related pull requests, create an associated issue in [expressjs/security-triage](https://github.com/expressjs/security-triage) repository. Add a copy of the patch for the pull request to the issue. Add screenshots of discussion from the pull request to the issue. | ||
| * [Open a ticket with GitHub](https://support.github.com/contact) to delete the pull request using Expressjs (team) as the account organization. |
There was a problem hiding this comment.
we should add an item here to force-push overwriting the PR code. generally people leave the checkbox "allow edits by maintainers" checked by default, so this will usually be doable
in a PR comment, we can also note to the PR author that we did this and why
Co-authored-by: Chris de Almeida <[email protected]>
Co-authored-by: Chris de Almeida <[email protected]>
Co-authored-by: Chris de Almeida <[email protected]>
Co-authored-by: Chris de Almeida <[email protected]>
Co-authored-by: Chris de Almeida <[email protected]>
Co-authored-by: Chris de Almeida <[email protected]>
Co-authored-by: Chris de Almeida <[email protected]>
Co-authored-by: Chris de Almeida <[email protected]>
Co-authored-by: Chris de Almeida <[email protected]>
Co-authored-by: Chris de Almeida <[email protected]>
Co-authored-by: Chris de Almeida <[email protected]>
Co-authored-by: Chris de Almeida <[email protected]>
I am working on this process for the @expressjs/security-triage team.
I would love to get your feedback (@expressjs/express-tc @expressjs/security-wg @expressjs/security-triage ) on the current state before I begin defining this process in detail, as the changes will become more complex. Could you please share your first impressions of this process?
Note: This flow also covers unusual scenarios, such as when someone submits a security report as an issue or directly in a PR as a patch. I wanted to align this process with what Node.js has in place today. Reference