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.

Sign up for GitHub

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

Jump to bottom

Use the VirusTotal API to scan submitted add-ons for malicious content #3246

Closed
seanbudd opened this issue Apr 15, 2024 · 18 comments · Fixed by #3765
Closed

Use the VirusTotal API to scan submitted add-ons for malicious content #3246

seanbudd opened this issue Apr 15, 2024 · 18 comments · Fixed by #3765
Labels
triaged

Comments

@seanbudd
Copy link
Member

The scanning implemented in #2660 generally covers cases where an add-on author may not be aware of security risks of an add-on. In general, CodeQL scanning is more designed around finding security issues caused by accident/ignorance, rather than maliciously designed code. A maliciously constructed add-on could be built and potentially pass these checks. Scanning with VirusTotal will further catch dangerous add-ons i.e. add-ons bundled with known malware.

https://docs.virustotal.com/reference/overview
@nvdaes
Copy link
Contributor

nvdaes commented Apr 15, 2024

I've found this GitHub Action to scan release assets with Virus Total.

@nvdaes
Copy link
Contributor

nvdaes commented Apr 15, 2024

Also, local files can be scanned, so that when the add-on is downloaded after crating the submission issue, it maybe used.
I don't have an API key, but if it's free I'll test in my fork of the store after finishing the PR about CodeQL.

@nvdaes
Copy link
Contributor

nvdaes commented Apr 16, 2024

I think that we should analize .exe files after unzipping the add-on, and, if Virus Total analizes them, we can post link to analysis in the corresponding issue.
Since we don't know how many files we will find in add-ons, I think that creating a script that checks for each json file produced in the analysis will be expensive and unpredictable, and perhaps for this reason the mentioned GitHub Action reports just filenames and links to Virus Total reports.
Luckily, we don't have many add-ons with .exe files.
If someone submitting such kind of add-ons is considered a trusted author, a list of trusted add-ons maybe created for updates.

@CyrilleB79
Copy link
Contributor

Is there only the possibility to analyse .exe files? Not other files such as .dll or .pyd?

Also, we can imagine that an add-on embeds a malicious executable code in a .exe renamed to .py, .pycle, .dat (or whatever extension). Then at runtime it may rename the file to .exe and execute it.

Is there any reason to restrict scanning to .exe?

@nvdaes
Copy link
Contributor

nvdaes commented Apr 16, 2024

Cyrille wrote:

Is there only the possibility to analyse .exe files? Not other files such as .dll or .pyd?

We can try to analize whatever file we went, even the .nvda-addon itself.

Also, we can imagine that an add-on embeds a malicious executable code in a .exe renamed to .py, .pycle, .dat (or whatever extension). Then at runtime it may rename the file to .exe and execute it.

Oh, it's true. I have also thought that non bundled files maybe downloaded.

Is there any reason to restrict scanning to .exe?

No. I've tried to analize emoticons add-on and of course it's not detected as malware. So I have thought that maybe better to analize unzipped files.
I've tested today for the first time. I have been able to upload the emoticons add-on from the web interface, not using my recently created API key.
I think that for testing I'll fork the used GitHub Action, who includes a workflow for testing the action itself, to discard problems when I pasted my API key in a repository secret.
Ideally we should be able to analize single add-ons. Another problem is that I don't know how to simulate a malicious add-on to test what happens in those cases.

@seanbudd
Copy link
Member Author

VirusTotal should be able to scan a whole add-on as a ZIP file if the add-on is uploaded with the extension changed to .zip

@XLTechie
Copy link
Contributor

XLTechie commented Apr 17, 2024 via email

@nvdaes
Copy link
Contributor

nvdaes commented Apr 17, 2024

Thanks @XLTechie. I"ll use this for testing in the next days.

@nvdaes
Copy link
Contributor

nvdaes commented Apr 18, 2024

For reference. I'll try to use the Virus Total command line:

https://github.com/VirusTotal/vt-cli

@nvdaes
Copy link
Contributor

nvdaes commented Apr 18, 2024

I've got the following artifact analyzing clipContentsDesigner 31.'.0.0 add-on. I think that we can show the workflow like done in codeQl analysis, if we find failures, malicious or suspicious positive numbers in stats.
Artifact (with the default retention days
https://github.com/nvdaes/addon-datastore/actions/runs/8739900542/artifacts/1426240194

@nvdaes
Copy link
Contributor

nvdaes commented Apr 18, 2024

Aother artifact including analysis of a test provided by @XLTechie , for some reason empty.
Note: I used *.json in the path artifact, so discussions and submitters.json are included.

https://github.com/nvdaes/addon-datastore/actions/runs/8743424504/artifacts/1427157401

In this case, the results key in the vt.json file is not empty. I still think that we should use the stats key

@nvdaes
Copy link
Contributor

nvdaes commented Apr 20, 2024

I'm seeing that analysis for clipContentsDesigner 31.0.0 add-on, which doesn't include malware, is not consistent between checks. For example,in one of mytest the status is quewed (maybe that VirusTotaldidn't finish the analysis). Also once I saw 3 failures (I suppose that this means that 3antivirus failed to analize the add-on). And reports are updated from time to time.
So I suppose that we should block add-ons whose analysis result includes a report of at least one malicious result, and also we may add the virusTotalAnalysisURL, similar to the review URL, so that people can easily find the last report:

https://docs.virustotal.com/docs/most-recent-report

This was referenced Apr 21, 2024
Merged
Closed
seanbudd pushed a commit that referenced this issue May 24, 2024
@nvdaes
Add support for Virus Total (#3294)
313f6d0 
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.
@seanbudd
Copy link
Member Author

before closing this, we'd like to run a scan of all submitted add-ons with virus total

@nvdaes
Copy link
Contributor

nvdaes commented May 24, 2024

Sean wrote:

before closing this, we'd like to run a scan of all submitted add-ons with virus total

I'm working on this. I've created a workflow to download each add-on and submit it to virusTotal with my API key. Now they should be analyzed.
At least one add-on hasn't been submitted:
https://github.com/nvdaes/addon-datastore/actions/runs/9223105164/job/25375629531

@josephsl
Copy link
Contributor

Hi,

Looks like there is an issue with the modified YML file with the workflow output for Windows App Essentials 24.06 submission saying:

Invalid workflow file: .github/workflows/sendJsonFile.yml#L95
error parsing called workflow
".github/workflows/sendJsonFile.yml"
-> "./.github/workflows/checkAndSubmitAddonMetadata.yml" (source branch with sha:313f6d05ae992ecb7fcbe8fbf0373266098a02ca)
: You have an error in your yaml syntax on line 315

This could be the reason why add-on submissions from yesterday are not being processed.

Thanks.

@nvdaes
Copy link
Contributor

nvdaes commented May 26, 2024

This has been fixed, but seems to be problems with the VirusTotal CLI or API at this moment. This has worked well, but seems that VirusTotal CLI is not generating well formed json files.
Also, I tried to submit all add-ons available on the store to VirusTotal, to be analyzed after they have been scanned.
Most of them have been submitted, but some seems to have invalid URLs. Here's the log file of GitHub Actions. Searching the word "Error" we can see not submitted add-ons (the json filename and the add-on download URL).
logs_24152049556.zip

@seanbudd
Copy link
Member Author

@nvdaes - can you please open a new PR to nvaccess/addon-datastore-staging? I've reverted previous PRs

@seanbudd seanbudd mentioned this issue Jun 11, 2024
Merged
seanbudd pushed a commit to nvaccess/addon-datastore-staging that referenced this issue Jun 11, 2024
@nvdaes
Add support for VirusTotal (#61)
8a76eb0 
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.
@seanbudd seanbudd mentioned this issue Jun 11, 2024
Merged
@nvdaes
Copy link
Contributor

nvdaes commented Jun 11, 2024

Thanks @seanbudd . An add-on has been submitted afterVirusTotal support has beeen added. When the add-on submission is merged, I'll inform about the VirusTotal support on the add-ons mailing list.
For reference, the submitted add-on belongs to @jpavonabian

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
triaged
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Merge virus total support nvaccess/addon-datastore-staging
5 participants
@seanbudd @josephsl @XLTechie @nvdaes @CyrilleB79