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

Update virustotal command #1918

Merged
merged 2 commits into from
Dec 28, 2017
Merged

Conversation

rasa
Copy link
Member

@rasa rasa commented Dec 21, 2017

Add * option to check all installed apps
Add --global option to check globally installed apps
Add 60+ sec. pause after 4 queries/min to avoid 429/Too Many Requests
Add immediate abort if query limit is exceeded
Add INFO/WARN/ERROR prefixes to messages
Add DarkGray/DarkCyan/DarkRed colors to INFO/WARN/ERROR messages
Add using apikey if stored via: scoop config virustotal_api_key apikey
Change colors to DarkGreen/DarkYellow/Yellow/Red for 0/1/2/3+ hit count
Remove fragment from URL when submitting to virustotal.com
Standardized INFO/WARN/ERROR messages

Add --global option to check globally installed apps
Add 60+ sec. pause after 4 queries/min to avoid 429/Too Many Requests
Add immediate abort if query limit is exceeded
Add INFO/WARN/ERROR prefixes to messages
Add DarkGray/DarkCyan/DarkRed colors to INFO/WARN/ERROR messages
Add using apikey if stored via: scoop config virustotal_api_key apikey
Change colors to DarkGreen/DarkYellow/Yellow/Red for 0/1/2/3+ hit count
Remove fragment from URL when submitting to virustotal.com
Standardized INFO/WARN/ERROR messages
Copy link
Member

@r15ch13 r15ch13 left a comment

Choose a reason for hiding this comment

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

API Key, Colors and throttling are nice!
@pcrama what do you think?

}
}

if(!$apps) {
my_usage; exit 1
}

if($global -and !(is_admin)) {
Copy link
Member

Choose a reason for hiding this comment

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

--global option isn't necessary because it only uses the manifest files. It doesn't check the installed program itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha. I'll remove.

}

if(is_scoop_outdated) {
update_scoop
Copy link
Member

Choose a reason for hiding this comment

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

Should be scoop update instead

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha. I'll fix.


$_ERR_UNSAFE = 2
$_ERR_EXCEPTION = 4
$_ERR_NO_INFO = 8

$exit_code = 0

Function Info($msg) {
Copy link
Member

Choose a reason for hiding this comment

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

INFO/WARN/ERROR functions should be moved to core.ps1 (we should add a better logging/output system anyway 😁)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.

@r15ch13 r15ch13 merged commit af41fc6 into ScoopInstaller:master Dec 28, 2017
@rasa rasa deleted the rasa-virustotal branch December 29, 2017 14:46
@pcrama
Copy link
Contributor

pcrama commented Dec 30, 2017

@rasa & @r15ch13, thank you for carrying this work further!

I didn't see this update and will work on it a bit more.

One thing that already stands out as strange to me is that only installed applications may be checked. My idea (or use case) is to check, preferrably even before downloading, that the software isn't infected.

If I can do it, I would even try to check dependencies that would be installed, too [e.g. checking youtube-dl should automatically check ffmpeg], again, before they are installed.

I like the idea of using '*' to test all installed apps, but I am not too sure that this is entirely clear for the user: AFAIU, the hashes of the most up to date versions will be checked in VirusTotal, independently of the exact version actually currently installed. I struggle with how to make this clear (in the documentation or even better in the command output).

I will open a PR with changes/proposals soon (WIP is in https://github.com/pcrama/scoop/tree/virustotal-apikey).

@rasa
Copy link
Member Author

rasa commented Dec 30, 2017

One additional thought. What happens if a user installs a particular version with scoop install 7zip@16.04 Do we support that now, and if not, can we?

@pcrama
Copy link
Contributor

pcrama commented Dec 31, 2017

As far as I know, this is not supported. I have currently no idea if it can be.

@pcrama
Copy link
Contributor

pcrama commented Dec 31, 2017

I have submitted #1934 (https://github.com/pcrama/scoop/tree/virustotal-apikey-fixes-20171231), the other branch will be deleted.

r15ch13 pushed a commit that referenced this pull request Mar 14, 2018
* Fix interpretation of response's status code to detect redirections

* Improve documentation of virustotal subcommand

- usage & configuration of virustotal_api_key
- special parameter '*' to test all installed apps
- make necessity of having a virustotal_api_key for --scan explicit
- show that it's possible to check several packages at once

* Never use virustotal_api_key to query if a package is safe

The URL in the code wasn't an API end-point anyway.

* Refactor logic to warn user about apps unknown to VirusTotal

* Warn once when virustotal_api_key's absence prevents VirusTotal submission

This is preparation for changes to come in the package submission logic.

* Use API to submit download link to VirusTotal, rate limited in EAFP fashion

This is a roundabout way to get the file to be scanned without having
to download & upload it ourselves.

Rate limiting is implemented using EAFP: if submission fails, we wait
at least 60s before retrying at most once.

* Color undecided VirusTotal information the same way as `dangerous' files

If the scanning is still in progress, VirusTotal returns 0 malicious,
0 suspicious and 0 undetected.  Err on the safe side and color this
the same way as `dangerous' files.

* Remove requirement to only verify installed apps

The initial use case for this feature was to scan packages to avoid
installing dangerous apps.  Assuming they are infected, we want if
possible to avoid downloading them at all.

* Check dependencies with VirusTotal, too (by default)

* Manually apply `Lint: PSAvoidUsingCmdletAliases' (see e1bb1e9, #2075)

This is to avoid conflicts when merging lukesampson:master

* Explain applist's return value transformation: drop `global' flag for each app

* Move variable declarations and apps list generation to the top

* Reformat code and comply to linted function names

* Reduce nesting, remove hacky hash/url retrieval

* Remove $global variables

* Fix regression bug in Search-VirusTotal()

* Remove applist() because it's irrelevant if app is installed globally
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.

3 participants