@redshark1802 - Beat you to it my friend! Yes, a lot of different directions. Agreed that there's no reason for the Husky dependency. Using guard for inspiration, I'm planning on highlighting just two straightforward and simple ways to easily integrate into common development processes. I'm planning on documenting the below two use cases:

1. Client side pre-commit hook using the Git hooks subdirectory
2. CI (Travis) server side pre-commit hook resulting in a failed build. eg here

Obviously, more ways can be added and we can define a best approach but these two options should cover a good number of applicable use cases. Can always add more later!

from slither.

Sounds good. Go for it.

from slither.

@dguido

Overview

This task left room for creativity. My approach focuses on using tools consistent with the internal development stack (python over javascript) and outlines implementations for client and server side pre-commit hooks that prevent solidity code from being merged if it contains any vulnerabilities identified by the Slither default analysis. This could serve as the foundation for a more robust series of templates or framework for automating analysis and could wrap up into something like guard or pre-commit. This is just the first step.

Client side pre-commit hook

A ton of different ways to do this. For simplicity, using the /git/hooks Git subdirectory with a pre-commit hook that executes a script whenever the command git commit is ran locally.

Made the output a little more readable and it prevents commits from added or modified solidity files that return vulnerabilities. Could also be improved to take in arguments providing customization.

To get working, just overwrite the file contents in /git/hooks/pre-commit with the below snippet:

#!/bin/bash

# Find all solidity files ataged for commit (added or modified)
added_modified=$(git diff --cached --diff-filter=AM --name-only | grep .sol$)

# If no solidity files are affected, skip the analysis and exit successfully
[[ -z $added_modified ]] && exit 0

# Count of vulnerabilities
declare -i vulnerability_count=0

# Run analysis across all affected files
for f in $added_modified
do
	name=`echo "$f"`
	echo "running Slither on $name"

	$(python -m slither $f)
	vulnerabilities=$?

	if [ "${vulnerabilities}" -gt "0" ]; then ((vulnerability_count+=$vulnerabilities)); else echo "No vulnerabilities found in $name"; fi
	echo ""

done

# Formatting
echo ""

# Block commits containing vulnerabilities
if [ "${vulnerability_count}" -gt "0" ]; then
	echo "commit aborted: Please fix the $vulnerability_count vulnerabilities" && exit 1
else
	echo "ready to commit" && exit 0
fi

Output:

Server side pre-commit hook

Same as with client side, there are a lot of options here. I ended up creating a solution for TravisCI because it's fairly common and it supports really cool things like parallelization across builds/tests and the power of their virtualenvs.

To run with TravisCI, the below .travis.yml does the trick while allowing us to specify our exact environment. I'm using 3.6 here for Slither compatibility but this could be configured in a multitude of ways.

I would like to call out the before_install section since I was having difficulty including the python binding for solc. There are approaches where you can install via pip but they felt a little hacky and I wasn't getting stable builds with py-solc (and wanted to stay away from npm) so I just worked around it with an older approach to get this done but I don't think it's the best approach. Happy to change this if anyone has a better approach.

The script is the same one from above, we are just moving it the root directory of a given repo ExampleProject/preCommit.sh

language: python
python:
  - "3.6"
cache: pip
before_install:
  - sudo add-apt-repository -y ppa:ethereum/ethereum
  - sudo apt-get update
  - sudo apt-get install -y solc
install:
  - pip install slither-analyzer
script:
  - bash ./preCommit.sh

Output (From TravisCI):

This option could easily be integrated further into the build process since it's easy to tweak parameters as well as build triggers. Happy to discuss extending some of this functionality.

Wiki Update

Also, if you think the wiki should be updated with this information, I've created a gist for the update. Not super easy for external contributors to collaborate on a github wiki so I thought this was a good suitable to sharing my update:
https://gist.github.com/benstew/73bcc05cfbc3ef98f182005a0846dcd0

from slither.

@montyly - So I had some time to review. Setting up a commenting system is straightforward with Travis. I saw the Test-Slither repo but I haven't had time to update the actual PoC. But for this piece of work, we will have to use the Github Rest API. The steps should be:

1. Create a Github personal access token w/ repo/public repo scope
2. Save the token as an encrypted variable in the .travis.yml (pretty common) or just an env variable
3. Technically not a specific resource for comment posting but we can post as an issue. There will hopefully be future support of comments.

curl -H "Authorization: token ${GITHUB_TOKEN}" -X POST \
-d "{\"body\": \"Hello world\"}" \
"https://api.github.com/repos/${TRAVIS_REPO_SLUG}/issues/${TRAVIS_PULL_REQUEST}/comments"

4. Wire that up as part of the after success script if a changed file has detected vulnerabilities

from slither.

Hi @dguido - This issue caught my attention after checking out the recent Slither blog post. Really starting to see the power of detectors as I dig more through the codebase.

Agreed that adding a pre-commit hook seems like a logical/common implementation. I would suggest adding the documentation to the mentioned blog post as well as the Slither README.

Guard provides a solid and easy to digest setup but I'm curious how you envision the future of the static analysis framework? Would love to model the build process off a best/standardized approach that you use internally. Not coming from a devops background so any guidance here is greatly appreciated.

[Updated 10/23/2018] No dependencies and client side hook
If we don't care about providing flexibility of CI providers and/or don't a preference about dependencies for a client side hook, a quick setup could be with something like Husky for a pre-commit or pre-push hook.

I've outlined what that looks like with Travis + Solium here and included the linter as part of the build process, as demonstrated here with the failed build.

from slither.

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 150.0 DAI (150.0 USD @ $1.0/DAI) attached to it as part of the Ethereum Community Fund via ECF Web 3.0 Infrastructure Fund fund.

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $50,036.08 more funded OSS Work available on the Gitcoin Issue Explorer

from slither.

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Workers have applied to start work.

These users each claimed they can complete the work by 12 months from now.
Please review their action plans below:

1) benstew has applied to start work (Funders only: approve worker | reject worker).

Update README with steps for a client side hook using the hooks subdirectory of the Git directory (ie .git/hooks). Also, will provide a text to the recent Slither blog post so that we can feature there as well

Originally, I mentioned using a module but realizing there is no reason for that dependency since we can provide steps directly from .git/hooks.

Learn more on the Gitcoin Issue Details page.

from slither.

There can be a lot done, from a simple manual hook registration to husky.
Also there are also a lot of possibilities for build systems: travis, jenkins, gitlab devops to name the most popular ones.
imho it should be easy to setup and use and therefore I would be against 3rd party dependencies for a "simple" git pre-commit hook.

nvm, just saw that you applied :). @benstew would you like to work on this? I'd be happy to work on this if you aren't interested.

from slither.

@dguido - Let me know if you agree with the above approach and if we are good to go here.

from slither.

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 7 months, 2 weeks from now.
Please review their action plans below:

1) benstew has been approved to start work.

Update README with steps for a client side hook using the hooks subdirectory of the Git directory (ie .git/hooks). Also, will provide a text to the recent Slither blog post so that we can feature there as well

Originally, I mentioned using a module but realizing there is no reason for that dependency since we can provide steps directly from .git/hooks.

Learn more on the Gitcoin Issue Details page.

from slither.

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 150.0 DAI (150.0 USD @ $1.0/DAI) has been submitted by:

1. @benstew

@mkosowsk please take a look at the submitted work:

• (Link Not Provided) by @benstew

• Learn more on the Gitcoin Issue Details page
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $52,736.86 more funded OSS Work available on the Gitcoin Issue Explorer

from slither.

Hmmm, we're debating how to handle this case. Slither can emit false positive results, which may be problematic if we fail the build on any detected issues. As of today, there is no way to whitelist certain code as ok despite the detected failure.

from slither.

Ah, didn't realize those limitations. False positives would be problematic in the build process using the above approach. If the Slither analysis returns any number of identified vulnerabilities from the detectors, the build will fail.

The whitelisting approach of detected failures makes sense but I would need to think about it more. It's probable, in future iterations, pre-commit hooks will leverage analysis that runs deeper than static text so in addition to whitelisting certain pieces of code (eg functions or contracts), it's likely that more flexible, "intragration-y" coverage will need to be supported. I also stayed away from setting an arbitrary threshold even though that could be easily implemented in the above script.

There could be some more near term processes that would allow Slither to be included as a server side pre-commit hook and add value. One approach that I have documented below has primary three assumptions:

1. Impeding developer workflows will slow down development and not result in adoption
2. Continuous warnings or fails will result in lack of developer confidence about using Slither or other security tools
3. Identified vulnerabilities (even if false positives) are always better off being surfaced to the user

For this approach, I wanted the analysis to be noisy but not too disruptive to developer workflows. I thought that including identified Slither vulnerabilities as part of the standard PR process would do the trick. Granted, there are tons of other ways but this approach focuses on the three above assumptions and documents vulnerabilities while being easily integrated into existing dev workflows through TravisCI.

I've update the script the preCommit Script to not only be more concise/shorter but also to create a vulnerabilities.json file that is created or updated as part of the TravisCI build process and stores the detector response from the slither analysis of the changed solidity contracts of the affected commits. The script is not merge blocking and results in a successful exit code.

#!/bin/bash
CHANGED_FILES=($(git diff --name-only $TRAVIS_COMMIT_RANGE | grep '\.sol$'))

# If no solidity files are affected, skip the analysis and exit successfully
[[ -z $CHANGED_FILES ]] && exit 0

# Count of vulnerabilities
declare -i vulnerability_count=0

# get year-month-day hour:minute:second from date
date=`date '+%Y-%m-%d %H:%M:%S'`

# Standardize writes to file
generate_post_data()
{
  cat <<EOF
{
  "account": {
    "user": "$USER",
  },
  "vulnerability": "$1",
	"date": "$date"
}
EOF
}

# Run analysis across all affected files
for f in $CHANGED_FILES
do
	name=$f
	slither_analysis="$(slither $f 2>&1)"
	while IFS= read -r line
		do
			if echo "$line" | grep -q "INFO:Detectors:*�"
				then
					(( vulnerability_count++ ))
					generate_post_data "$line" >> vulnerabilities.json
			fi
	done <<< "$slither_analysis"
done
echo ""

exit 0

The above file results in a vulnerabilities JSON file that is a record of the identified vulnerabilities and can be configured to preference.

{
  "account": {
    "user": "benstew",
  },
  "vulnerability": "INFO:Detectors: Backdoor function found in C.i_am_a_backdoor ",
	"date": "2018-10-30 18:26:10"
}

I have then updated the .travis.yml file to run an executable file that makes use of the github API to update a comment on the PR outlining the the details of the vulnerabilities.json file. This will be generated and commented on every commit affecting a solidity file with a Slither identified vulnerability.

after_success:
- bash ./githubComment.sh

To update a github comment or issue, it's just a simple curl to the Github API and environment variables are made available and secured through TravisCI.

from slither.

@dguido - Let me know if there is anything you would still like to discuss on this

from slither.

Hi @dguido have you had a chance to look at @benstew's thoughts? Thanks!

from slither.

The PR looks good!

Some comments:

• The output for a vulnerability uses multiple lines, so we should keep everything which is in between two INFO:Detectors: (or between the last INFO:Detectors: and the end of the output)
• running slither only on modified files will make slither missing vulnerabilities. For example, you can do a modification in Base.sol that does not add a bug in Base.sol , but that will add a bug in Child.sol (asuming that Child.sol uses Base.sol). Is there a way to run slither on the whole project (slither . ?) and only report new vulnerabilities?

from slither.

Thanks for the feedback @montyly.

• Yes, that makes sense. I wasn't sure what was the best output but it's highly customizable. Could even look to include timestamps, commit hashes, etc if multiple contributors and you want to treat it more as a log.
• It's somewhat of a double edged sword. It's definitely possible to run slither on the whole project but then you will face a longer wait and potential for more false positives/noise. This approach focuses on adding slither to a normal dev workflow in a way that is efficient (not overly noisy or time consuming) but could easily be adjusted to run slither on the entire project. Would just change CHANGED_FILES to be all .sol files whether touched or not. Currently, there are definitely edge cases where vulnerabilities could be missed but I don't think this is the perfect solution. Ideally, there would be a way to whitelist certain parts of the code to prevent false positives and provide a quicker analysis but that is a much more lofty goal.

from slither.

Hey @montyly is this one good to pay out or do you still need a few more changes from @benstew?

from slither.

Hey @ceresstation, the issue is close to being finalized.
We are discussing with @benstew over slack about some improvements for it

from slither.

Hey @benstew, any luck with the recommendations I sent you on slack?

If there is a blocking point, let me know how I could help

from slither.

@dguido @montyly @benstew - Hey all, Gitcoin Ambassador here to ask how the submitted improvements have been going?

from slither.

@CPSTL - Hoping that @montyly's 👍 is a sign that work is completed?

from slither.

@CPSTL, @mkosowsk - Any update here?

from slither.

Hey @benstew, I am still trying to make a solution working and a documentation from your proposal and this tutorial https://damien.pobel.fr/post/github-api-from-travisci/ on slither-test.

I simplified the solution here: https://github.com/trailofbits/slither-tests/blob/master/githubComment.sh

I have some concerns about the use of the GITHUB_TOKEN, and I did not have time to investigate them. For example, I am wondering if anyone with access to the travis configuration could steal the GITHUB_TOKEN to interact on github on the behalf of the original user. If so, it's important to highlight it in the doc and to favor the use of a 'fake' account to post the comment.

from slither.

@montyly - Thanks for the update. Will take a look. There are secure ways of passing the github token as an environment variable. I will try to take a look this weekend.

from slither.

@montyly - You can encrypt private environment variables in the Travis config file. https://docs.travis-ci.com/user/environment-variables/

Wouldn't this solve your issue?

from slither.

So I think that the configuration I had in mind is not really possible yet. I am closing this issue and will think about alternative solutions.

@mkosowsk can you validate the bounty for @benstew? He proposed a solution that what closed to what I am looking for and helped me to determine better what I need.

from slither.

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 150.0 DAI (150.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @benstew.

• Learn more on the Gitcoin Issue Details page
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $37,096.57 more funded OSS Work available on the Gitcoin Issue Explorer

from slither.