install bandit and start "security" scannig our code #112

igalic · 2017-09-19T20:45:34Z

as discussed in #111, we would like to analyze our code for potential
security issues. a first step here is to install a code scanner and
start using it!

skarekrow · 2017-09-20T11:14:49Z

I don’t see the value in doing this. Most of the commit is adding a comment to skip the scanning in the first place. I can only imagine how many of those comments we’re going to have. Why are we using bandit?

araujobsd · 2017-09-20T11:18:09Z

I will give my 0.50 cents! Instead of do a make up, would be better let it exposed till somebody can real fix it, in case it is a real issue.

araujobsd

What is the value of doing it? Seems it is only polluting the code with comments instead of really fixing it.

igalic · 2017-09-20T12:38:21Z

the value is to be aware of what we're doing. # nosec: Yes, we want a shell-call here is vastly different from # nosec, for one.

For the other, we don't need to keep it:

Right now, libiocage is exeperimental, and i hope no one is using it for their production systems, we can still — openly — use an automated system — such as bandit, or maybe something else — to tease out the issues, or at least, become aware of them.

For instance, #113 is a consequence of this PR.

gronke · 2017-09-20T12:47:41Z

Bandit is not relevant for runtime execution, so it won't affect the enduser. It's better for us to have strict analysis tools in place to give people working on pull-requests a better chance to fit the project requirements. The tighter we define the code quality requirements on our build system, the easier it becomes to merge code contributions.

the value is to be aware of what we're doing. # nosec: Yes, we want a shell-call here is vastly different from # nosec, for one.

I absolutely want to be explicit with dangerous things like forks. 💛

araujobsd · 2017-09-20T12:49:07Z

Perhaps an additional section at README with suggestion of tools to be used before submit a PR makes more sense than enforce tools usage such like: mypy and/or bandit.

Or otherwise in a near future, if a submitter sends a patch and he is not aware of these tools he would feel frustrated and probably will go away.

I strongly suggest to create an additional section at README and expose these tools, before pick up a tool and enforce the usage. At least everybody will be aware of the tools the project is choosing.

As an additional example: travis will be always broke and soon or late, devs will start to ignore it, what is dangerous.

gronke · 2017-09-20T12:54:17Z

I strongly suggest to create an additional section at README and expose these tools, before pick up a tool and enforce the usage. At least everybody will be aware of the tools the project is choosing.

Yes. And in addition we should break the various checks into multiple Travis tasks. The total execution time will be longer, but on a PR it would show exactly which metric does not fit the requirements. We can always point a link to a description in our README. Good call, @araujobsd

skarekrow · 2017-09-20T12:54:42Z

+1

igalic · 2017-09-20T12:55:09Z

travis has been broken for weeks on every pull request that @gronke and myself have worked on, because we've been working on pull requests to improve the code-quality with the help of automated testing tools

we haven't merged anything that was broken, so master has been green (we need to reflect that as a badge in our README ;)

i absolutely agree with you on helping contributors submit PRs that will be travis-green from the get-go. To do that, we should add a make check (and reuse that in .travis.yml)

araujobsd · 2017-09-20T13:11:02Z

My suggestion would be, first start with the additional section listing all tools the project intend to use as best practices. After a while and after it gets mature among devs, it will be natural the adoption of it and then enforce it.

Overall all these tools have something good, just need apply in homeopathic doses.

igalic · 2017-09-20T13:45:40Z

switching gears, and focusing on the results of this PR so far:

most of our issues are currently B110: try_except_pass

that is, code of this form:

try:
  do_some_stuff()
except Exception:
  pass

we should consider refactoring this pattern to at least catch the "correct" exception.

araujobsd · 2017-09-20T13:50:41Z

I agree with the exception, it should catch the right exception and do something not only pass all exceptions! Although sometimes it is inevitable and it is a necessary evil.

igalic · 2017-09-20T13:52:51Z

Another class of issues currently marked with medium severity is possibly a bug in bandit: [B310:blacklist] Audit url open for permitted schemes. fails to notice that our mirror_url @setter already does the "audit" for schemes as bandit recommends!

I cannot seem to find anything related in bandit's issue tracker (laundpad!), so if someone knows any of its developers, it would be great to point them here.

as discussed in #111, we would like to analyze our code for potential security issues. a first step here is to install a code scanner and start using it!

* input: we're on python 3 * shell command: we actually want that in this case;) * exec() -> use fully qualified named

since this is now encapsulated in Makefile, we can switch .travis.yml to this ("simpler") method too. This addresses #115 in parts.

we don't need to browse through the svnweb view, we can simply click thru the repository

we have a in the @property.setter where the URLs are validated

any hash function that produces a string >= 12 characters would've done, so the choice of sha224 is almost arbitrary, however, sha1() is also deprecated already, and sha384 and sha512 are, as per their documentation, too slow.

In this case, it's an AttributeError when accessing an `object`'s attribute that does not exist.

It needs root permissions to run, anyway, so we have it very explicitly documented now.

where this is not possible, we should use pathlib to validate the path.

igalic

very light review
thank you very much for your hard work, @gronke

igalic · 2017-09-23T15:48:12Z

iocage/cli/__init__.py

@@ -25,7 +25,7 @@
 import os
 import re
 import signal
-import subprocess as su
+import subprocess  # nosec: B804


this should be B404, shouldn't it?
i disabled that entirely in the makefile, and tried disabling it in setup.cfg

aye! will update

Just figured out that despite of a defined exception, # nosec: <ANYTHING> does skip all bandit warnings for this line. We should be careful with that and fix upstream

igalic · 2017-09-23T15:50:43Z

iocage/lib/Config/Jail/Properties/Resolver.py

@@ -21,6 +21,7 @@
 # STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
 # IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 # POSSIBILITY OF SUCH DAMAGE.
+import typing


this should be a separate commit

igalic · 2017-09-23T15:56:25Z

setup.cfg

@@ -1,6 +1,9 @@
 [metadata]
 description-file = README.md

+[bandit]
+skip = B404


does this actually work

It did complain about B404, so I don't think so.

gronke · 2017-09-23T19:17:55Z

I don’t see the value in doing this. Most of the commit is adding a comment to skip the scanning in the first place.

@skarekrow the errors reported by bandit have been valuable to know. It found for example swallowed exceptions where we didn't precisely defined the expected errors. It's a nice addition to MyPy and should be required in the build process.

No further changes requested.

igalic mentioned this pull request Sep 20, 2017

Refactor: replace our ad-hoc path handling with pathlib #113

Open

araujobsd self-requested a review September 20, 2017 11:22

araujobsd previously requested changes Sep 20, 2017

View reviewed changes

gronke mentioned this pull request Sep 20, 2017

Documentation of Code Analysis stages #115

Closed

igalic force-pushed the chore/sec-scan branch 2 times, most recently from d33a622 to 27f2421 Compare September 20, 2017 13:39

igalic and others added 12 commits September 23, 2017 09:45

install bandit and start "security" scannig our code

97d1a52

as discussed in #111, we would like to analyze our code for potential security issues. a first step here is to install a code scanner and start using it!

"solve" high severity issues

1decfbe

* input: we're on python 3 * shell command: we actually want that in this case;) * exec() -> use fully qualified named

document how to run our tests

3679a7f

since this is now encapsulated in Makefile, we can switch .travis.yml to this ("simpler") method too. This addresses #115 in parts.

security: replace http FreeBSD URL with HTTPS

ac61658

replace svnweb url with base-svn repo

be879d1

we don't need to browse through the svnweb view, we can simply click thru the repository

document (#nosec) that our url is validated

9812d67

we have a in the @property.setter where the URLs are validated

md5() is deprecated, replace it with sha224()

2def3d8

any hash function that produces a string >= 12 characters would've done, so the choice of sha224 is almost arbitrary, however, sha1() is also deprecated already, and sha384 and sha512 are, as per their documentation, too slow.

Be explicit about the checked Exception

7549f65

In this case, it's an AttributeError when accessing an `object`'s attribute that does not exist.

Update script only needs be executed by root

6895054

It needs root permissions to run, anyway, so we have it very explicitly documented now.

use fully qualified paths for exec

1802a66

where this is not possible, we should use pathlib to validate the path.

catch correct exception

743b358

import libzfs in ZFSShareStorage

0f5e7d8

gronke force-pushed the chore/sec-scan branch 2 times, most recently from fd145f4 to d06a6c2 Compare September 23, 2017 14:18

igalic commented Sep 23, 2017

View reviewed changes

gronke added 2 commits September 23, 2017 17:20

address issues reported by bandit

3e4d811

remove unused bandit config from setup.cfg

05e2cfd

gronke force-pushed the chore/sec-scan branch from d06a6c2 to 05e2cfd Compare September 23, 2017 17:27

correct typo in FreeBSD download url

f31745f

gronke approved these changes Sep 23, 2017

View reviewed changes

gronke merged commit 74fde47 into master Sep 23, 2017

gronke deleted the chore/sec-scan branch September 23, 2017 19:35

This was referenced Sep 23, 2017

analyse libiocage code for security sensitive issues #111

Closed

Automatically check for bad excepts #119

Closed

Download release checksums via trusted connection #43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

install bandit and start "security" scannig our code #112

install bandit and start "security" scannig our code #112

igalic commented Sep 19, 2017

skarekrow commented Sep 20, 2017

araujobsd commented Sep 20, 2017 •

edited

Loading

araujobsd left a comment •

edited

Loading

igalic commented Sep 20, 2017

gronke commented Sep 20, 2017

araujobsd commented Sep 20, 2017

gronke commented Sep 20, 2017

skarekrow commented Sep 20, 2017

igalic commented Sep 20, 2017

araujobsd commented Sep 20, 2017

igalic commented Sep 20, 2017

araujobsd commented Sep 20, 2017

igalic commented Sep 20, 2017

igalic left a comment

igalic Sep 23, 2017

gronke Sep 23, 2017

gronke Sep 23, 2017

igalic Sep 23, 2017

igalic Sep 23, 2017

gronke Sep 23, 2017

gronke commented Sep 23, 2017

install bandit and start "security" scannig our code #112

install bandit and start "security" scannig our code #112

Conversation

igalic commented Sep 19, 2017

skarekrow commented Sep 20, 2017

araujobsd commented Sep 20, 2017 • edited Loading

araujobsd left a comment • edited Loading

Choose a reason for hiding this comment

igalic commented Sep 20, 2017

gronke commented Sep 20, 2017

araujobsd commented Sep 20, 2017

gronke commented Sep 20, 2017

skarekrow commented Sep 20, 2017

igalic commented Sep 20, 2017

araujobsd commented Sep 20, 2017

igalic commented Sep 20, 2017

araujobsd commented Sep 20, 2017

igalic commented Sep 20, 2017

igalic left a comment

Choose a reason for hiding this comment

igalic Sep 23, 2017

Choose a reason for hiding this comment

gronke Sep 23, 2017

Choose a reason for hiding this comment

gronke Sep 23, 2017

Choose a reason for hiding this comment

igalic Sep 23, 2017

Choose a reason for hiding this comment

igalic Sep 23, 2017

Choose a reason for hiding this comment

gronke Sep 23, 2017

Choose a reason for hiding this comment

gronke commented Sep 23, 2017

araujobsd commented Sep 20, 2017 •

edited

Loading

araujobsd left a comment •

edited

Loading