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

Add support for VirusTotal #61

Merged
merged 6 commits into from
Jun 11, 2024
Merged

Conversation

nvdaes
Copy link

@nvdaes nvdaes commented May 27, 2024

Issue number

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.

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
Copy link
Author

nvdaes commented May 27, 2024

@nvdaes
Copy link
Author

nvdaes commented May 27, 2024

Test simulating that customNotifications is a malicious add-on, hard-coding the sha256 of a flagged add-on (here we see the VirusTotal failure):

https://github.com/nvdaes/addon-datastore/actions/runs/9251468377

@nvdaes
Copy link
Author

nvdaes commented May 27, 2024

Last test: after merging the manualApproval PR, simulating that NV Access accepted this add-on as a false positive:

https://github.com/nvdaes/addon-datastore/actions/runs/9251719545

@nvdaes
Copy link
Author

nvdaes commented May 27, 2024

@seanbudd , this is ready for review.

@nvdaes
Copy link
Author

nvdaes commented May 27, 2024

Sorry, I remembered that codeQl analysis workflow needs to be updated so that reviewedAddons.json can be used for codeQl and virusTotal, and the manualApproval pull request considers both analysis. I'll submit tests now.

@nvdaes
Copy link
Author

nvdaes commented May 27, 2024

Test: virusTotal success, codeQl excluding warnings fails:

https://github.com/nvdaes/addon-datastore/actions/runs/9259669099/job/25472099153

@nvdaes
Copy link
Author

nvdaes commented May 27, 2024

Test: maualApproval PR merged after analysis failure. The submission issue is closed as completed via its PR:

https://github.com/nvdaes/addon-datastore/actions/runs/9259808380

@nvdaes
Copy link
Author

nvdaes commented May 27, 2024

Test: making VirusTotal and codeQL analysis fail, to see how a unique pull request for manual approval is created, and just an issue comment requesting to keep the submission issue opened. readFeeeds 24.0.0 and the same issue testing just codeQl failure is ised:

https://github.com/nvdaes/addon-datastore/actions/runs/9260026346

@nvdaes
Copy link
Author

nvdaes commented May 27, 2024

I think that all changes are tested. The submission issue for readFeeds 24.0.0 can be found at

nvdaes#1387

@nvdaes
Copy link
Author

nvdaes commented May 27, 2024

Si I think this is ready for review, @seanbudd

README.md Outdated Show resolved Hide resolved
Comment on lines 84 to 87
git checkout -b ${{ github.event.sender.login }}${{ steps.get-data.outputs.issueNumber }}
git add .
git commit -m "Submit add-on"
git push origin ${{ github.event.issue.user.login }}${{ steps.get-data.outputs.issueNumber }}
git push origin ${{ github.event.sender.login }}${{ steps.get-data.outputs.issueNumber }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should remain as is. Please don't use the event sender

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

.github/workflows/sendJsonFile.yml Outdated Show resolved Hide resolved
@nvdaes
Copy link
Author

nvdaes commented Jun 11, 2024

@seanbudd , I think all your suggestions are applied.

.github/workflows/sendJsonFile.yml Outdated Show resolved Hide resolved
@seanbudd
Copy link
Member

note it seems I am unable to commit/push to your fork

@nvdaes
Copy link
Author

nvdaes commented Jun 11, 2024

I've applied your last suggestion about sender. I'll try to grant you access to my fork.

@nvdaes
Copy link
Author

nvdaes commented Jun 11, 2024

@seanbudd , I have send you an invitation as an admin of my fork. Please accept it.

@seanbudd
Copy link
Member

@coderabbitai review

@seanbudd seanbudd merged commit 8a76eb0 into nvaccess:master Jun 11, 2024
@seanbudd seanbudd deleted the virusTotal branch June 11, 2024 05:14
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

Successfully merging this pull request may close these issues.

2 participants