Explore use of codechecking techniques in our CICD

[canturkisci]

Description

We should explore the use of DiffBlue https://techcrunch.com/2017/06/27/diffblue/ ; our assertion based model checking techniques like ExpliSAT (Ronen) which uses model checking to prove the correctness of software or alternatively find bugs / security vulnerabilities. To see if they can improve crawler security posture. We should also learn from this experience to understand if we can have such capabilities in VA.

[canturkisci]

@sahilsuneja1 @nadgowdas let us learn about diffblue and then Explisat by our next meeting and discuss. I will add to running agenda.

[sahilsuneja1]

Updates:

1. diffblue / explisat support c/c++/java, not python (yet)
2. codetrace rates user not codebase
3. bandit parses code to check for security issues. Ran bandit over agentless-system-crawler codebase. Findings below:

3.1 Run as: pip install bandit; bandit -r agentless-system-crawler > bandit_out

3.2 Output Summary:
Code scanned:
Total lines of code: 14983
Total lines skipped (#nosec): 0
Run metrics:
Total issues (by severity):
Undefined: 0
Low: 714 <------------646 issues due to 'assert', rest mix of following 14 categories
Medium: 44
High: 1 <--------------- high severity B605, one case only in a test
Total issues (by confidence):
Undefined: 0
Low: 3
Medium: 42
High: 714
Files skipped (0):

3.3 15 unique issues found: grep Issue bandit_out | sort | uniq | less. Examples below with some explanation as to what the issue means, or justification of benign existence.

Issue: [B101:assert_used] Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
- However, assert is removed with compiling to optimised byte code (python -o producing *.pyo files). This caused various protections to be removed. The use of assert is also considered as general bad practice.

Issue: [B104:hardcoded_bind_all_interfaces] Possible binding to all interfaces.
- NA

Issue: [B105:hardcoded_password_string] Possible hardcoded password: 'db2inst1'
Issue: [B105:hardcoded_password_string] Possible hardcoded password: 'db2inst1-pwd'
Issue: [B105:hardcoded_password_string] Possible hardcoded password: 'password'
Issue: [B107:hardcoded_password_default] Possible hardcoded password: 'db2inst1-pwd'
Issue: [B107:hardcoded_password_default] Possible hardcoded password: 'password'
- in TRL code; Just some random initialized value; possibly NA

Issue: [B108:hardcoded_tmp_directory] Probable insecure usage of temp file/directory.
- Although the functions guarantee that the filename is unique at the time it is selected, there is no mechanism to prevent another process or an attacker from creating a file with the same name after it is selected but before the application attempts to open the file.
- If the existing contents of the file are malicious in nature, an attacker may be able to inject dangerous data into the application when it reads data back from the temporary file.
- If an attacker pre-creates the file with relaxed access permissions, then data stored in the temporary file by the application may be accessed, modified or corrupted by an attacker.
- Finally, in the best case the file will be opened with the a call to open() using the O_CREAT and O_EXCL flags or to CreateFile() using the CREATE_NEW attribute, which will fail if the file already exists and therefore prevent the types of attacks described above. However, if an attacker is able to accurately predict a sequence of temporary file names, then the application may be prevented from opening necessary temporary storage causing a denial of service (DoS) attack

Issue: [B110:try_except_pass] Try, Except, Pass detected.
Issue: [B112:try_except_continue] Try, Except, Continue detected.
- in Stefan's code
- This pattern is considered bad practice in general, but also represents a potential security issue. A larger than normal volume of errors from a service can indicate an attempt is being made to disrupt or interfere with it. Thus errors should, at the very least, be logged.

Issue: [B310:blacklist] Audit url open for permitted schemes. Allowing use of file:/ or custom schemes is often unexpected.
- NA; we use http protocol with url
- The functions and objects urllib.urlopen, urllib.urlretrieve, urllib.URLopener, urllib.FancyURLopener, urllib2.urlopen, urllib2.Request allow opening http, ftp, and file urls. Often the ability to open file urls is unexpected and can lead to leakage of information about the local server or worse. This has led to bugs in the past. See OSSA-2014-041. Alert that these need to be audited.

Issue: [B311:blacklist] Standard pseudo-random generators are not suitable for security/cryptographic purposes.
- NA

Issue: [B404:blacklist] Consider possible security implications associated with subprocess module.
- This blacklist data checks for a number of Python calls known to have possible security implications. The following blacklist tests are run against any function calls encoutered in the scanned code base, triggered by encoutering ast.Call nodes

Issue: [B405:blacklist] Using ElementTree to parse untrusted XML data is known to be vulnerable to XML attacks. Replace ElementTree with the equivalent defusedxml package, or make sure defusedxml.defuse_stdlib() is called.
- TRL; parsing tomcat status page

Issue: [B504:ssl_with_no_version] ssl.wrap_socket call with no SSL/TLS protocol version specified, the default SSLv23 could be insecure, possible security issue.
- mtgraphite comm; should mention SSL ver; default ver is insecure

Issue: [B603:subprocess_without_shell_equals_true] subprocess call - check for execution of untrusted input.
- kind of good not using a shell inside subprocess; just 'audit these calls'
- may present a security issue if appropriate care is not taken to sanitize any user provided or variable input.
- Specifically, this test looks for the spawning of a subprocess without the use of a command shell. This type of subprocess invocation is not vulnerable to shell injection attacks, but care should still be taken to ensure validity of input.

Issue: [B604:any_other_function_with_shell_equals_true] Function call with shell=True parameter identified, possible security issue.
- NA: only in tests;
- shell=True allows running arbitrary input
- like ping 8.8.8.8; rm -rf '/'

                        # bad
			def ping(myserver):
				return subprocess.check_output('ping -c 1 %s' % myserver, shell=True)
		        ping('8.8.8.8; rm -rf /')
				64 bytes from 8.8.8.8: icmp_seq=1 ttl=58 time=6.32 ms
				rm: cannot remove /bin/dbus-daemon: Permission denied
                       
                        # better
		  	def ping(myserver):
			        args = ['ping', '-c', '1', myserver]
			        return subprocess.check_output(args, shell=False)		
			ping('8.8.8.8; rm -rf /')
				ping: unknown host 8.8.8.8; rm -rf /

Issue: [B605:start_process_with_a_shell] Starting a process with a shell, possible injection detected, security issue.
- NA; in tests; don't use "commands.getstatusoutput()"

Issue: [B607:start_process_with_partial_path] Starting a process with a partial executable path
- If the desired executable path is not fully qualified relative to the filesystem root then this may present a potential security risk.
- . If they are able to adjust the contents of the PATH variable, or manipulate the file system, then a bogus executable may be discovered in place of the desired one. This executable will be invoked with the user privileges of the Python process that spawned it, potentially a highly privileged user.
- use full paths starting from fs root '/'