Jenkins analysis#226
Conversation
|
Thanks! I didn't get a chance to look today, but I'll review this tomorrow. |
aarontp
left a comment
There was a problem hiding this comment.
Partial review for now, and I'll try to finish up tomorrow. Feel free to ignore these comments until I finish if that's easier for you.
|
|
||
| from __future__ import unicode_literals | ||
|
|
||
| from turbinia.evidence import RawDisk |
There was a problem hiding this comment.
We probably want the other cloud disk types here as well, right?
| return credentials | ||
|
|
||
| @staticmethod | ||
| def analyse_jenkins(version, credentials): |
There was a problem hiding this comment.
I think this should be s/analyse/analyze/
There was a problem hiding this comment.
Heh, english! :) fixed.
| """Bruteforce password hashes using John the Ripper. | ||
|
|
||
| Args: | ||
| password_hashes (list): List of password hashes. |
There was a problem hiding this comment.
Tiny nit, but maybe change it like s/hashes./hashes as strings./ just to disambiguate from the many other formats that hashes sometimes come in?
| return collected_file_paths | ||
|
|
||
|
|
||
| def bruteforce_password_hashes(password_hashes, timeout=90): |
There was a problem hiding this comment.
Just curious about the timeout here. That seems pretty quick for most password cracking, but I guess we just want to get the super easily cracked passwords? Given how long all the other disk processing tasks take to run, I would consider raising this number a bit.
There was a problem hiding this comment.
Yeah, this is taken from the cluedo stuff. It is meant to crack really easy passwords. I'll raise it a bit though. Good catch.
|
|
||
| findings = [] | ||
| credentials_registry = {hash: username for username, hash in credentials} | ||
| weak_passwords = bruteforce_password_hashes(credentials_registry.keys()) |
There was a problem hiding this comment.
Can you add a TODO here to set a timeout method parameter here that we can override with "dynamic task configuration" once it's available? That's part of the "Recipes" design that we talked about.
There was a problem hiding this comment.
I just filed #244 that you can reference here as well.
| raise RuntimeError('john the ripper failed.') | ||
|
|
||
| result = [] | ||
| pot_file = os.path.expanduser('~/.john/john.pot') |
There was a problem hiding this comment.
I guess we can't set the path to this as a command line flag? It seems really strange to me that this is the best way to retrieve the results. Maybe there's an alternate output format we can use?
There was a problem hiding this comment.
Yeah, I know this is not optimal but it is how John works. The only way to get the result is in the .pot file, and there is no way to control where that file is created. It always end up in ~/.john/ and there is no way to change that, i.e. no reason to add it as a flag.
|
Codefactor don't let me add TODOs. |
aarontp
left a comment
There was a problem hiding this comment.
LG, Just a couple more comments. Thanks!
|
|
||
| # Add the resulting evidence to the result object. | ||
| result.add_evidence(output_evidence, evidence.config) | ||
| result.close(self, success=True) |
There was a problem hiding this comment.
Can we add a 'status' here with a summary of what was discovered? Maybe we could use the first line of the analysis report (possibly with some small changes)?
| str: The version of Jenkins. | ||
| """ | ||
| version = None | ||
| version_re = re.compile('<version>(.*)</version>') |
There was a problem hiding this comment.
Just curious, what is the benefit to compiling this here?
| username_match = re.search(username_re, config) | ||
|
|
||
| if username_match and password_hash_match: | ||
| username = username_match.group(1) |
There was a problem hiding this comment.
Seems like as it is this method will only ever return one tuple of user/hash. Is it possible that there are multiple usernames/hashes in this file? If so, can we change this to extract all of them, and if not, should we change the method to just return a tuple?
| # Cancel the timer if the process is done before the timer. | ||
| if timer.is_alive(): | ||
| timer.cancel() | ||
| except OSError: |
There was a problem hiding this comment.
If there are potentially multiple users that can be extracted from the config (see my comments on _extract_jenkins_credentials()), then I guess we'd want to check for changes to the .pot file (or something similar) since john might have only cracked a subset of the users.
|
Ping. Can you take a look at the latest comments? I think the only thing I was hoping to address was the status, and I had a question about whether we could see more than one user in these files. Thanks! |
|
Actually, since I think you might be busy today, I'll ping you about just merging this, and then I can make a small change to update the status (and we can address the user issue later). |
aarontp
left a comment
There was a problem hiding this comment.
LGTM. A couple small things, but I'll fix those in a second pass.
* Scaffolding for Jenkins analysis * Add analysis and tests * Linter happy * Allow more input evidence * Add import of JenkinsAnalysisJob
Add analysis plugin for Jenkins. The plugin will:
Example output:
Jenkins analysis found potential issues.
Jenkins version: 2.121.2
1 weak password(s) found: