forked from nvaccess/addon-datastore
-
Notifications
You must be signed in to change notification settings - Fork 1
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fixes issue nvaccess#3246 Summary of the issue VirusTotal may catch malware bundled with add-ons. Also, knowing the sha256 of scanned add-ons, the URL to see results at different datetimes maybe built, allowing users to see this information even before installing an add-on if this was included in the NVDA store in the future. Development strategy Virus Total CLI is installed when needed. Add-ons are scanned when the submission issue is created. Info about the add-on file is requested to Virus Total later, when the pull request is created, to give time to Virus Total to show results, trying to avoid getting empty analysis. NV Access needs to create an API key in Virus Total. The addonMetadata.json artifact is used to get the add-on id and sha256. A falsePositiveAddons.json file has been added. If VirusTotal analysis fails, a pull request will be created adding the sha256 of the addon to a list associated with the add-on ID, in the falsePositiveAddons.json file. If VirusTotal should be skipped for this add-on, NV Access will merge the created pull request, delete the branch created for the submission (in the form submitterIssueNumber), and relabel the issue to trigger a new workflow.
- Loading branch information
Showing
5 changed files
with
154 additions
and
16 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,9 @@ on: | |
issueTitle: | ||
required: true | ||
type: string | ||
secrets: | ||
virusTotalApiKey: | ||
required: true | ||
|
||
jobs: | ||
getAddonId: | ||
|
@@ -225,11 +228,103 @@ jobs: | |
uses: peter-evans/close-issue@v3 | ||
with: | ||
issue-number: ${{ inputs.issueNumber }} | ||
codeQL-analysis: | ||
|
||
virusTotal-analysis: | ||
needs: createPullRequest | ||
runs-on: windows-latest | ||
strategy: | ||
matrix: | ||
python-version: [ 3.11 ] | ||
permissions: | ||
contents: read | ||
issues: write | ||
env: | ||
API_KEY: ${{ secrets.virusTotalApiKey }} | ||
steps: | ||
- name: Checkout repository | ||
uses: actions/checkout@v4 | ||
- name: Download add-on metadata | ||
uses: actions/download-artifact@v4 | ||
with: | ||
name: addonMetadata | ||
- name: Install virusTotal | ||
run: choco install vt-cli | ||
- name: Set Virus Total analysis status | ||
id: setVirusTotalAnalysisStatus | ||
uses: actions/github-script@v7 | ||
with: | ||
script: | | ||
const setVirusTotalAnalysisStatus = require('./.github/workflows/virusTotalAnalysis.js') | ||
setVirusTotalAnalysisStatus({core}) | ||
- name: Upload results | ||
id: uploadResults | ||
if: failure() | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: VirusTotal | ||
path: vt.json | ||
overwrite: true | ||
- name: Upload manual approval | ||
id: uploadManualApproval | ||
if: failure() | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: manualApproval | ||
path: reviewedAddons.json | ||
overwrite: true | ||
- name: Warn if analysis fails | ||
if: failure() | ||
uses: peter-evans/create-or-update-comment@v4 | ||
with: | ||
issue-number: ${{ inputs.issueNumber }} | ||
body: | | ||
VirusTotal has flagged this add-on as malicious. | ||
You can open this link and [see the results of the analysis](${{ steps.setVirusTotalAnalysisStatus.outputs.analysisUrl }}). | ||
Please contact the flagged security vendors to get them to review and unflag the false positive. | ||
Please ask here or email [email protected] if you need assistance with this process. | ||
codeQL-analysis: | ||
needs: [createPullRequest] | ||
uses: ./.github/workflows/codeql-analysis.yml | ||
createManualApproval: | ||
needs: [getAddonId, virusTotal-analysis, codeQL-analysis] | ||
if: ${{ always() && contains(join(needs.*.result, ','), 'failure') }} | ||
runs-on: windows-latest | ||
strategy: | ||
matrix: | ||
python-version: [ 3.11 ] | ||
permissions: | ||
contents: write | ||
issues: write | ||
pull-requests: write | ||
steps: | ||
- name: Checkout repository | ||
uses: actions/checkout@v4 | ||
- name: Download artifacts | ||
uses: actions/download-artifact@v4 | ||
with: | ||
merge-multiple: true | ||
- name: Create pull request | ||
id: cpr | ||
uses: peter-evans/create-pull-request@v6 | ||
with: | ||
add-paths: reviewedAddons.json | ||
title: Add reviewed add-on (${{ needs.getAddonId.outputs.addonId }}) | ||
branch: reviewedAddon${{ github.event.issue.number }} | ||
commit-message: Add reviewed add-on (${{ needs.getAddonId.outputs.addonId }}) | ||
body: | | ||
This add-on needs to be reviewed by NV Access due to analysis failure. | ||
Review ${{ inputs.issueNumber }} for more information. | ||
author: github-actions <[email protected]> | ||
delete-branch: true | ||
- name: Request to keep issue opened | ||
uses: peter-evans/create-or-update-comment@v4 | ||
with: | ||
issue-number: ${{ inputs.issueNumber }} | ||
body: | | ||
Please, don't close this issue. | ||
Wait until #${{ steps.cpr.outputs.pull-request-number }} is merged. | ||
mergeToMaster: | ||
needs: [getAddonId, createPullRequest, codeQL-analysis] | ||
needs: [getAddonId, createPullRequest, codeQL-analysis, virusTotal-analysis] | ||
permissions: | ||
contents: write | ||
pull-requests: write | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,10 +15,9 @@ jobs: | |
runs-on: windows-latest | ||
permissions: | ||
actions: read | ||
contents: write | ||
contents: read | ||
security-events: write | ||
issues: write | ||
pull-requests: write | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
|
@@ -69,18 +68,14 @@ jobs: | |
name: results-${{ matrix.language }} | ||
path: results/*.sarif | ||
overwrite: true | ||
- name: Create pull request | ||
id: cpr | ||
- name: Upload manual approval | ||
id: uploadManualApproval | ||
if: failure() | ||
uses: peter-evans/create-pull-request@v6 | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
add-paths: reviewedAddons.json | ||
title: Add reviewed add-on (${{ steps.setSecurityAnalysisStatus.outputs.addonId }}) | ||
branch: reviewedAddon${{ github.event.issue.number }} | ||
commit-message: Add reviewed add-on (${{ steps.setSecurityAnalysisStatus.outputs.addonId }}) | ||
body: "This add-on needs to be reviewed by NV Access due to security analysis failure" | ||
author: github-actions <[email protected]> | ||
delete-branch: true | ||
name: manualApproval | ||
path: reviewedAddons.json | ||
overwrite: true | ||
- name: Warn if analysis fails | ||
if: failure() | ||
uses: peter-evans/create-or-update-comment@v4 | ||
|
@@ -95,8 +90,6 @@ jobs: | |
Please review the warnings and consider fixing this in the add-on. | ||
If you can provide more context on the failure in the submission, please do. | ||
See the [submission guide](https://github.com/nvaccess/addon-datastore/blob/master/docs/submitters/submissionGuide.md) for more details. | ||
Please, don't close this issue. | ||
Wait until #${{ steps.cpr.outputs.pull-request-number }} is merged. | ||
analyze: | ||
name: Analyze add-on | ||
needs: analyzeExcludingWarnings | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
module.exports = ({core}) => { | ||
const fs = require('fs'); | ||
const { exec } = require('child_process'); | ||
const addonMetadataContents = fs.readFileSync('addonMetadata.json'); | ||
const addonMetadata = JSON.parse(addonMetadataContents); | ||
const addonId = addonMetadata.addonId; | ||
core.setOutput('addonId', addonId); | ||
const sha256 = addonMetadata.sha256; | ||
const analysisUrl = `https://www.virustotal.com/gui/file/${sha256}`; | ||
console.log(analysisUrl); | ||
core.setOutput('analysisUrl', analysisUrl); | ||
const reviewedAddonsContents = fs.readFileSync('reviewedAddons.json'); | ||
const reviewedAddonsData = JSON.parse(reviewedAddonsContents); | ||
if (reviewedAddonsData[addonId] !== undefined && reviewedAddonsData[addonId].includes(sha256)) { | ||
core.info('VirusTotal analysis skipped'); | ||
return; | ||
} | ||
exec(`vt file ${sha256} -k ${process.env.API_KEY} --format json`, (err, stdout, stderr) => { | ||
console.log(`err: ${err}`); | ||
console.log(`stdout: ${stdout}`); | ||
console.log(`stderr: ${stderr}`); | ||
const vtData = JSON.parse(stdout); | ||
fs.writeFileSync('vt.json', stdout); | ||
const stats = vtData[0]["last_analysis_stats"]; | ||
const malicious = stats.malicious; | ||
if (malicious === 0) { | ||
core.info('VirusTotal analysis succeeded'); | ||
return; | ||
} | ||
if (reviewedAddonsData[addonId] === undefined) { | ||
reviewedAddonsData[addonId] = []; | ||
} | ||
reviewedAddonsData[addonId].push(sha256); | ||
stringified = JSON.stringify(reviewedAddonsData, null, 2); | ||
fs.writeFileSync('reviewedAddons.json', stringified); | ||
core.setFailed('VirusTotal analysis failed'); | ||
}); | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,10 @@ The add-on store includes the following security measures: | |
- The checksum allows NVDA to ensure that add-on releases are immutable. | ||
- [Code scanning with CodeQL](https://docs.github.com/en/code-security/code-scanning/introduction-to-code-scanning/about-code-scanning-with-codeql) can detect vulnerabilities in Python and JavaScript code included in submitted add-ons. | ||
- NV Access can manage [code scanning alerts](https://docs.github.com/en/code-security/code-scanning/managing-code-scanning-alerts/about-code-scanning-alerts), available from the Code scanning link from the [Security page](https://github.com/nvaccess/addon-datastore/security). | ||
- [Virus Total](https://www.virustotal.com/) is used to scan submitted add-ons. | ||
If malicious content is detected, the add-on will not be automatically included in the store. | ||
Please contact the flagged security vendors to get them to review and unflag the false positive. | ||
Please email [email protected] if you need assistance with this process. | ||
|
||
|
||
### Human review process / code audit | ||
|