Add support for Virus Total

[nvdaes]

Issue number

Fixes issue #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.

Testing performed

• Use the sha256 of a malicious add-on, simulating that this sha correspond to a non malicious add-on.
• Use the real sha256 calculated for the same add-on.

nvdaes#1299

[nvdaes]

@seanbudd,I think this is readyfor review.
Also, we may include the URL with Virus Totalresults in the NVDA store. If this is accepted,itmay be addressed in this pull request, adding a step to include the URL in add-on metadata.
NV Access may want to test sending to a private datastore a malicious add-on, after creating a secret with a Virus Total API key.

[seanbudd]

I've created an API key now by the way

[XLTechie]

@nvdaes how long is the retention period? What is the workflow going to show that will be helpful if the artifact is missing?

…

Note that we are posting a link to the workflow, not the artifact URL: artifacts maybe deleted after the retention period, so linking to the workflows ensures that we have a permanent link, though this maybe controversial and I'm open to receive further comments.

[nvdaes]

Luke wrote:

how long is the retention period?

By default, 90 days:

Configuring the retention period for GitHub Actions artifacts and logs in your organization .

What is the workflow going to show that will be helpful if the artifact is missing?

Just that it failed.

[XLTechie]

Personally I think the artifact is what is important, and 90 days is more than enough time for NV Access to decide whether to override the decision of the automated system. If 90 days is the limit, then think we should have a cron-timed cleanup workflow that posts a comment and closes any open PRs/issues that failed their initial check. Github already has a sample workflow for this, all we need to do is use it with a label for failed security checks. It will even post an appropriate comment. That is nearly a whole NVDA release cycle's worth of time, and I think the author should be able to submit a replacement issue by then if NV Access hasn't responded. I don't believe there is value in keeping these open indefinitely if the artifact expires, nor are authors likely to be patient that long in any case.

[nvdaes]

Luke Luke wrote:

If 90 days is the limit, then think we should have a cron-timed cleanup workflow that posts a comment and closes any open PRs/issues that failed their initial check.
Github already has a sample workflow for this, all we need to do is use it with a label for failed security checks. It will even post an appropriate comment.

If you can, please share the mentioned GitHub workflow.
Let's wait to see if NV Access accepts your proposal. For me it's reasonable.

[nvdaes]

Also, note that, in this case, the artifact is generated even if VirusTotal doesn't flag an add-on as malicious, to see other results like Non detected, suspicious, etc.

[XLTechie]

It is the stale issue/PR closer workflow. In this case we would need to modify it so it only looks for issues that have the "failed-virusTotal-check" or similar label, instead of just generic stale issues/PRs. Tutorial (basic, not how we would use it exactly, but it gets there): https://docs.github.com/en/actions/managing-issues-and-pull-requests/closing-inactive-issues Full docs: https://github.com/marketplace/actions/close-stale-issues

[XLTechie]

See these input keywords: only-labels days-before-stale days-before-close stale-issue-message stale-pr-message close-issue-message close-pr-message stale-issue-label close-issue-label delete-branch Oh, also debug-only (for testing).

[seanbudd]

Hi @nvdaes - I'm considering this blocked by #3397
Once that's resolved I think we can look at this again

[seanbudd]

@nvdaes - unrelated, but would you consider adding this for NVDA releases? right now we manually upload release builds (beta, rc, stable) to virus total. I think it would be worth also scanning alpha builds if we can do it automatically. Right now it will require some work to create an NVDA installer artifact to scan in GitHub Actions.

[XLTechie]

@seanbudd Are you thinking to have AppVeyor notify a webhook when the merge compile is complete, including the same artifact uploaded to NV Access server, or the link to same?

[seanbudd]

@XLTechie - I think ideally compiling NVDA on GitHub actions, it would be a big step to help us migrate from appVeyor. But yes, we can also use a webhook to trigger the action

[nvdaes]

About NVDA and VirusTotal, I used Appveyor years ago to manage add-ons, but I prefer GitHub Actions since it was simple for me, and I felt that the process to add projects to Appveyor was not very accessible and required to emulate a mouse click. So I haven't investigated about Appveyor from that time.
I have used GitHub Actions preparing NVDA source code to test add-ons. I can try to work trying to add VirusTotal scanning for NVDA artifacts with GitHub Actions.
If work should be done with Appveyor, I think that @perhaps @XLTechie can do it better than me.

[XLTechie]

@XLTechie - I think ideally compiling NVDA on GitHub actions, it would be a big step to help us migrate from appVeyor.

@seanbudd I wasn't aware that was a goal of NV Access. @LeonarddeR seems to think it's impractical (nvaccess/nvda#10516 (comment)), and the discussion is still closed on that subject.

[LeonarddeR]

Interesting subject, I will update nvaccess/nvda#10516 with most recent findings.

[seanbudd]

I've triaged nvaccess/nvda#10516 and added more information there . This PR should also be unblocked now.

[nvdaes]

Note: The sha of the malicious add-on has been commented without removing it from the js file used for the VirusTotal analysis. Should we remove it?

[nvdaes]

@seanbudd I've addressed your review and checked that:

1. If virusTotal and analysis fails, a PR with two files, falsePositiveAddons.json and reviewedAddons.json can be created:

nvdaes#1346

• If just virusTotal fails (to test that this will work as expected if just one of the jobs fails, virusTotal or codeQL analysis), a pull request with just a file is created.
• A separate comment mentioning the pull request number is created. See tests at
  [Submit add-on]: test VirusTotal and analysis failure nvdaes/addon-datastore#1339

[seanbudd]

If virusTotal and analysis fails, a PR with two files, falsePositiveAddons.json and reviewedAddons.json can be created:

Can you please make this one file? There's no need to maintain two separate whitelists for add-ons

[nvdaes]

If virusTotal and analysis fails, a PR with two files, falsePositiveAddons.json and reviewedAddons.json can be created:

Can you please make this one file? There's no need to maintain two separate whitelists for add-ons
OK.

[nvdaes]

@seanbudd, now we use a file named reviewedAddons.json for CodeQL analysis and VirusTotal. After testing that the PR includes a unique file, I've set the overwrite for the manualApproval artifact in VirusTotal and in the CodeQL analysis. The sha256 will be the same, so in the real life this can be done.
See tests in nvdaes#1352

[seanbudd]

This PR now contains a large number of files in addons/readFeeds

[nvdaes]

Sorry. I test this creating or relabelling issues and making changes on the master branch of my fork, and sometimes changes are committed accidentally to this branch.
I think that now all maybe fixed. I've tested again the creation of the PR for manual approval.
See nvdaes#1361

[seanbudd]

will this will happen if getAddonId fails?

[nvdaes]

No, since this needs codeQL-analysis and virusTotal-analysis, which need createPullRequest, which needs verifySubmitter, which won't be run if getAddonId fails.

[seanbudd]

thanks @nvdaes - this almost ready just one more question

[seanbudd]

Thanks @nvdaes for all your efforts here!

[nvdaes]

Thank you @seanbudd for your reviews. Testing has been hard, so thanks for your patience 😀