Wordpress & Docker by tomchop · Pull Request #240 · google/turbinia

tomchop · 2018-08-23T11:16:07Z

This PR adds a couple Workers and analysis modules:

Docker logs extraction job: Extract logs from Docker containers
Wordpress analysis worker: Analyze Wordpress web access logs and assess the likelihood of compromise.

aarontp · 2018-08-24T06:24:52Z

Thanks for the PR! I didn't get a chance to look at this today, but I'll take a look tomorrow.

aarontp

Thanks! Just a couple minor comments.

aarontp · 2018-08-28T04:43:34Z

+
+    # Add the resulting evidence to the result object.
+    result.add_evidence(output_evidence, evidence.config)
+    result.close(self, success=True)


Can we add a 'status' parameter here with a summary of what was found? I think the first line of the 'analysis' variable is close to what would be good. See more on this below.

aarontp · 2018-08-28T04:46:48Z

+                self._get_timestamp(log_line), edited_file))
+
+    if findings:
+      findings.insert(0, 'Potential Wordpress compromise found:')


If we slightly change this, then the first line of what is returned from this function can be easily passed in as the 'status' parameter to result.close(). Maybe something along the lines of 'Potential Wordpress compromise found (N findings)' or 'N findings related to potential Wordpress compromise'?

Sounds good. I'll change it to "Wordpress access logs found (install, theme editing)". It's less misleading than "compromise" in the case we only have one finding.

aarontp · 2018-08-28T04:59:32Z

+      findings.insert(0, 'Potential Wordpress compromise found:')
+      return '\n'.join(findings)
+
+    return 'No theme editing found in Wordpress access logs'


Isn't this no theme editing or installations? Maybe either enumerate or just keep it generic?

Changed to 'No Wordpress install or theme editing found in access logs'

aarontp · 2018-08-28T04:59:50Z

@@ -0,0 +1,52 @@
+# -*- coding: utf-8 -*-
+# Copyright 2016 Google Inc.


update the date maybe?

aarontp · 2018-08-28T05:31:03Z

+             in evidence]
+    return tasks
+
+class DockerLogAnalysisJob(TurbiniaJob):


Actually, I missed one big thing here. Why would these Tasks only be specific to Docker containers[1]. Can we at minimum add separate Tasks for the local system logs? I'm assuming that shouldn't require any changes to the Tasks themselves, and is maybe just another Artifact that needs to be referenced?

We don't have to do it in this PR, but I wonder if there is a better way to handle this architecturally as well, for example possibly adding a Docker enumeration Task that creates a new DockerContainer evidence type. That might not work without some extra effort though, because the two evidence types may be too fundamentally different to be handled generically in a Task. I wonder if there is a reliable way to do a mount of the container image to make it more generic for the Tasks? cc: @berggren and @rgayon for thoughts on this part.

[1] This brings up another big point that we should make our other analysis Tasks should also be looking to 'recurse' into Docker containers as well

Yes, this came up during a chat with @rgayon earlier this week.

In theory we could use the exact same plugin with a minor change in the artifact. The "docker log" format is slightly different than a normal nginx or apache access log (it's, essentially, access logs wrapped in JSON). The analysis logic ignores the JSON and just greps through the output, which will work on both JSON-wrapped logs and regular access logs.

I'm happy to rename DockerLogExtractionJob to AccessLogExtractionJob to reflect regular log grabbing using different artifacts (nginx, apache, and also docker) and passing that on to the existing Task. Can we just add an extra FileArtifactExtractionTask in L51.

Finally, there's a bit of a nuance in Docker logs - the cases I was thinking to tackle with this Job was when the container logs to stdout and Docker logs that in its log. In theses cases, if you mount the docker container as a regular FS, you won't find logs anywhere in the mountpoint since they are all sent outside of the container.

Yeah, that would be great if we can rename it and just add an extra Artifact extraction. As you suggest, just adding it to L51 should work if there is already a suitable artifact type.

aarontp · 2018-08-30T05:51:40Z

Let me know when to take another look. It looks like some tests are failing as of now.

tomchop · 2018-09-05T11:55:07Z

Weird, tests seem to be passing on my VM and on Travis. Codefactor is blocking on a cyclic import but I think it's not WAI (all imports are done in get_jobs). Thoughts?

aarontp · 2018-09-05T17:36:53Z

Yeah, not sure what is up with the codefactor cyclic import issue. I don't see anything that should be causing that. Other than that, is this ready for review? Once it's ready I can do a quick check to make sure that the tests pass in my environment, and if they do I think we can ignore the codefactor issue.

Onager · 2018-09-05T17:53:48Z

@tomchop Does local pylint also complain about a cyclic import?

tomchop · 2018-09-06T10:50:41Z

@aarontp Yeah I fixed the comments, should be ready for review
@Onager No cyclic import error on local pylint

aarontp · 2018-09-07T06:11:48Z

Sorry, I wasn't able to get this done today. I'll take a look in the morning.

aarontp · 2018-09-07T18:05:04Z

@@ -0,0 +1,19 @@
+{"log":"10.128.0.2 - - [27/Jun/2018:19:29:54 +0000] \"POST /wp-admin/install.php?step=2 HTTP/1.1\" 200 872 \"-\" \"Mozilla/5.0 (Windows; U; Windows NT 10.0) http/2.8.11 Tcl/8.6.7\"\n","strea


Is this file formatted correctly? There are newlines mid-line in a lot of places.

Also, wow, that's an ugly log format, :).

Yup, this is how Kubernetes logs things when it logs from stdout. I'll change this to "normal" access log fiels

aarontp

Cool, I think this looks much better handling Docker this way, thanks for changing that up. Mostly only some minor comments/nits (feel free to ignore them), but the couple things I wanted to cover is the test data format, and how gzip'd files are handled. Thanks!

rgayon · 2018-09-14T09:31:37Z

FYI, I'm renaming the "docker log" artifacts in ForensicArtifacts/artifacts#291 as those are actually very specific to GKE environments.

aarontp

LGTM, thanks!

I'm not sure why github thinks this review is outstanding, the issues raised in the review have already been addressed.

* Docker & Wordpress * Cleanup * Fix reviewer comments * Rename classes & address reviewer comments. * Fix reviewer comments + add support for gzipped files * Fix ugly logs * Fix broken text

tomchop added 2 commits August 23, 2018 13:00

Docker & Wordpress

23bcf48

Cleanup

058c817

tomchop requested a review from Onager August 23, 2018 11:16

tomchop assigned Onager and aarontp Aug 23, 2018

tomchop requested a review from aarontp August 23, 2018 11:18

berggren added the New analysis label Aug 27, 2018

Onager previously requested changes Aug 27, 2018

View reviewed changes

Comment thread turbinia/jobs/analysis/docker_logs.py Outdated

Comment thread turbinia/jobs/analysis/docker_logs.py Outdated

Comment thread turbinia/jobs/analysis/docker_logs.py Outdated

Comment thread turbinia/workers/analysis/wordpress_test.py Outdated

Fix reviewer comments

6eb809e

aarontp reviewed Aug 28, 2018

View reviewed changes

Rename classes & address reviewer comments.

dc2dc31

Merge branch 'master' into wordpress

b3dbae0

tomchop requested review from Onager and aarontp September 5, 2018 13:14

aarontp reviewed Sep 7, 2018

View reviewed changes

tomchop added 2 commits September 10, 2018 13:48

Fix reviewer comments + add support for gzipped files

64572d2

Fix ugly logs

618a0e4

Onager mentioned this pull request Sep 10, 2018

Added job manager class #248

Closed

Fix broken text

65cefb7

tomchop requested a review from aarontp September 13, 2018 08:52

aarontp approved these changes Sep 17, 2018

View reviewed changes

aarontp merged commit 55e9a6b into google:master Sep 17, 2018

tomchop deleted the wordpress branch August 6, 2019 16:00

		@@ -0,0 +1,52 @@
		# -- coding: utf-8 --
		# Copyright 2016 Google Inc.

		@@ -0,0 +1,19 @@
		{"log":"10.128.0.2 - - [27/Jun/2018:19:29:54 +0000] \"POST /wp-admin/install.php?step=2 HTTP/1.1\" 200 872 \"-\" \"Mozilla/5.0 (Windows; U; Windows NT 10.0) http/2.8.11 Tcl/8.6.7\"\n","strea

Conversation

tomchop commented Aug 23, 2018

Uh oh!

aarontp commented Aug 24, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aarontp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomchop Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarontp commented Aug 30, 2018

Uh oh!

tomchop commented Sep 5, 2018

Uh oh!

aarontp commented Sep 5, 2018

Uh oh!

Onager commented Sep 5, 2018

Uh oh!

tomchop commented Sep 6, 2018

Uh oh!

aarontp commented Sep 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarontp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgayon commented Sep 14, 2018

Uh oh!

aarontp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tomchop Aug 29, 2018 •

edited

Loading