-
Notifications
You must be signed in to change notification settings - Fork 933
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
Document how to add Slither to a git pre-commit hook #48
Comments
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
|
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.
|
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. 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. |
There can be a lot done, from a simple manual hook registration to husky. nvm, just saw that you applied :). |
@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:
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! |
@dguido - Let me know if you agree with the above approach and if we are good to go here. |
Sounds good. Go for it. |
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. 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. |
OverviewThis 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 Client side pre-commit hookA ton of different ways to do this. For simplicity, using the 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 #!/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 Server side pre-commit hookSame 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 I would like to call out the The script is the same one from above, we are just moving it the root directory of a given repo
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 UpdateAlso, 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: |
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: @mkosowsk please take a look at the submitted work:
|
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. |
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:
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 #!/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 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. |
@dguido - Let me know if there is anything you would still like to discuss on this |
The PR looks good! Some comments:
|
Thanks for the feedback @montyly.
|
Hey @ceresstation, the issue is close to being finalized. |
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 |
@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:
|
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. |
@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. |
@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? |
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. |
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.
|
For those who came here searching for a working - repo: local
hooks:
# Static code analyzer for solidity (Currently fails to resolve the dependency properly)
- id: slither
name: Slither analysis for smart
entry: bash -c 'for file in "$@"; do slither "$file"; done'
language: system
always_run: true
files: ^(src/|test/) And an accompanying question and answer. Note this configuration hides warnings nested in node module dependencies. And this is a more chaotic configuration with different syntax usages: # Static code analyzer for solidity.
- id: Slither on src
language: system
name: Slither analysis for smart contracts in src/
# Run on all checks on all files: Temporarily skip: naming-convention,solc-version,reentrancy-benign,reentrancy-eth,reentrancy-no-eth,reentrancy-events,reentrancy-unlimited-gas
# entry: bash -c 'for file in "$@"; do slither --exclude naming-convention,solc-version,pragma,timestamp,unused-import --filter-paths="(node_modules|test)" "$file"; done'
# Run all checks on all src files.
entry: bash -c 'for file in "$@"; do if [[ $file == src/* ]]; then slither --filter-paths="(node_modules)" "$file"; fi; done'
- id: Slither on test
language: system
name: Slither analysis for smart contracts in test/
# Run on all checks on all files: Temporarily skip: naming-convention,solc-version,reentrancy-benign,reentrancy-eth,reentrancy-no-eth,reentrancy-events,reentrancy-unlimited-gas
# entry: bash -c 'for file in "$@"; do slither --exclude naming-convention,solc-version,pragma,timestamp,unused-import --filter-paths="(node_modules|src)" "$file"; done'
# Run all checks on all test files.
entry: bash -c 'for file in "$@"; do if [[ $file == test/* ]]; then slither --exclude naming-convention,solc-version,pragma,timestamp,unused-import --filter-paths="(node_modules)" "$file"; fi; done'
# Run on all checks on all files:
# entry: bash -c 'for file in "$@"; do slither --filter-paths="(node_modules)" "$file"; done'
# Run only one detection: calls-loop, similar-names
# entry: bash -c 'for file in "$@"; do slither --filter-paths="(node_modules)" --detect shadowing-local "$file"; done' |
... or via something like (but not exactly) guard.
The text was updated successfully, but these errors were encountered: