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

Bandit static security code analysis: fix findings and add bandit to github actions #1820

Open
UlrichB22 opened this issue Dec 8, 2024 · 4 comments
Assignees
Labels

Comments

@UlrichB22
Copy link
Collaborator

This issue collects several fixes for 'bandit' findings.

I would also like to add Bandit to the Github actions as described in
https://github.com/marketplace/actions/bandit-scan or
https://github.com/marketplace/actions/bandit-by-pycqa.

@UlrichB22 UlrichB22 self-assigned this Dec 8, 2024
@UlrichB22
Copy link
Collaborator Author

UlrichB22 commented Dec 8, 2024

hashlib: Use of weak MD5 hash for security. Consider usedforsecurity=False
Test ID: B324
Severity: HIGH
Confidence: HIGH
CWE: [CWE-327](https://cwe.mitre.org/data/definitions/327.html)
File: [src/moin/utils/crypto.py](file:///mypath/src/moin/utils/crypto.py)
Line number: 120
More info: https://bandit.readthedocs.io/en/1.8.0/plugins/b324_hashlib.html

119	    """
120	    return hashlib.md5(repr(kw).encode()).hexdigest()
121
hashlib: Use of weak SHA1 hash for security. Consider usedforsecurity=False
Test ID: B324
Severity: HIGH
Confidence: HIGH
CWE: [CWE-327](https://cwe.mitre.org/data/definitions/327.html)
File: [src/moin/utils/plugins.py](file:///mypath/src/moin/utils/plugins.py)
Line number: 205
More info: https://bandit.readthedocs.io/en/1.8.0/plugins/b324_hashlib.html

204	        assert isinstance(pdir, str)
205	        modname = "moin_p_{}".format(hashlib.new("sha1", pdir.encode()).hexdigest())
206	        if modname not in sys.modules:

IMO these functions are used to generate unique names, so the addition 'usedforsecurity=False' is appropriate here.

@UlrichB22
Copy link
Collaborator Author

UlrichB22 commented Dec 9, 2024

try_except_pass: Try, Except, Pass detected.
Test ID: B110
Severity: LOW
Confidence: HIGH
CWE: [CWE-703](https://cwe.mitre.org/data/definitions/703.html)
File: [src/moin/utils/send_file.py](file:///mypath/src/moin/utils/send_file.py)
Line number: 141
More info: https://bandit.readthedocs.io/en/1.8.0/plugins/b110_try_except_pass.html

140	                fsize = file.tell()  # tell position
141	            except Exception:
142	                pass
143	            file.seek(0, 0)  # seek to start of file
try_except_pass: Try, Except, Pass detected.
Test ID: B110
Severity: LOW
Confidence: HIGH
CWE: [CWE-703](https://cwe.mitre.org/data/definitions/703.html)
File: [src/moin/utils/send_file.py](file:///mypath/src/moin/utils/send_file.py)
Line number: 144
More info: https://bandit.readthedocs.io/en/1.8.0/plugins/b110_try_except_pass.html

143	            file.seek(0, 0)  # seek to start of file
144	        except Exception:
145	            pass
146	    else:

Found following comment in send_file.py:

        # be extra careful as some file-like objects (like zip members) have a seek
        # and tell methods, but they just raise some exception (e.g. UnsupportedOperation)
        # instead of really doing what they are supposed to do (or just be missing).

I tested the download of different content-types including tgz and zip with binary files inside but did not get any exception.

@UlrichB22
Copy link
Collaborator Author

UlrichB22 commented Dec 9, 2024

any_other_function_with_shell_equals_true: Function call with shell=True parameter identified, possible security issue.
Test ID: B604
Severity: MEDIUM
Confidence: LOW
CWE: [CWE-78](https://cwe.mitre.org/data/definitions/78.html)
File: [src/moin/utils/SubProcess.py](file:///mymoinpath/src/moin/utils/SubProcess.py)
Line number: 196
More info: https://bandit.readthedocs.io/en/1.8.0/plugins/b604_any_other_function_with_shell_equals_true.html

195	        cmd,
196	        shell=True,
197	        close_fds=not subprocess.mswindows,
198	        bufsize=1024,
199	        stdin=subprocess.PIPE,
200	        stdout=subprocess.PIPE,
201	        stderr=subprocess.PIPE,
202	    )
203	    data, errors = p.communicate(input, timeout=timeout)
204	    return data, errors, p.returncode
205

The module 'utils/SubProcess.py' seems to be a leftover from Python2 times and it is not used anywhere. It has a built-in test function, but the test failed:

$ python ./SubProcess.py 
Traceback (most recent call last):
  File "/mymoinpath/src/moin/utils/./SubProcess.py", line 19, in <module>
    if not subprocess.mswindows:
           ^^^^^^^^^^^^^^^^^^^^
AttributeError: module 'subprocess' has no attribute 'mswindows'. Did you mean: '_mswindows'?

@UlrichB22
Copy link
Collaborator Author

hashlib: Use of weak MD5 hash for security. Consider usedforsecurity=False
Test ID: B324
Severity: HIGH
Confidence: HIGH
CWE: [CWE-327](https://cwe.mitre.org/data/definitions/327.html)
File: [src/moin/user.py](file:///mypath/src/moin/user.py)
Line number: 424
More info: https://bandit.readthedocs.io/en/1.8.0/plugins/b324_hashlib.html

423	        param = {}
424	        param["gravatar_id"] = hashlib.md5(email.lower()).hexdigest()
425

A test with 'user_use_gravatar = True' in wikiconfig.py leads to HTTP 500 and a traceback

[...]
  File "/mypath/src/moin/templates/utils.html", line 447, in template
    {%- set avatar = user.avatar(20) %}
    ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/mypath/src/moin/user.py", line 424, in avatar
    param["gravatar_id"] = hashlib.md5(email.lower()).hexdigest()
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: Strings must be encoded before hashing

gravatar.com recommends to use sha256 for hashing, see https://docs.gravatar.com/api/avatars/python/

Following email addresses can be used for testing: test1@example.com, test2@example.com, test3@example.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant