feat: add urls (http+https) to json report #681
Conversation
There was a problem hiding this comment.
Hi Alik!
Thanks for working on this feature! I look forward to giving it a run.
I've added some initial comments and plan to have a play with your implementation soon.
One other suggestion - it might be nice to have a flag/env var that can be used to enable/disable this feature.
This allows us to eventually merge the CL and iron out any bugs, etc before having it running in production.
Caleb
| @@ -5,6 +5,17 @@ | |||
| *filter | |||
| :INPUT ACCEPT [0:0] | |||
| :CNI-ADMIN - [0:0] | |||
|
|
|||
| # SSLSplit Routing | |||
There was a problem hiding this comment.
I'll need to check that these don't accidentally allow the sandbox to bypass the rules below for the given ports.
There was a problem hiding this comment.
These rules do allow traffic heading to ports 80,443,8080,8081 to hit the blocked addresses below.
Please update these rules so that they don't allow the rules at the bottom to be bypassed.
There was a problem hiding this comment.
I spent some time today playing with this and have found a way to ensure undesired traffic is not inadvertently able to bypass the filter.
Basically, we add PREROUTING rules to RETURN before the REDIRECT rules (added by the sandbox command) are added.
The iptables.rules file I have that works is:
# Create the chain used by podman networking for user-defined rules
#
# Note: the subnet "172.16.16.0/24" used here must match the subnet
# used in podman-analysis.conflist.
*nat
# Never allow the traffic below to be redirected by NAT.
-A PREROUTING -i cni-analysis -p tcp -s 172.16.16.0/24 --match multiport --dports 80,443 -d 169.254.169.254/32 -j RETURN
-A PREROUTING -i cni-analysis -p tcp -s 172.16.16.0/24 --match multiport --dports 80,443 -d 10.0.0.0/8 -j RETURN
-A PREROUTING -i cni-analysis -p tcp -s 172.16.16.0/24 --match multiport --dports 80,443 -d 172.16.0.0/12 -j RETURN
-A PREROUTING -i cni-analysis -p tcp -s 172.16.16.0/24 --match multiport --dports 80,443 -d 192.168.0.0/16 -j RETURN
COMMIT
*filter
:INPUT ACCEPT [0:0]
:CNI-ADMIN - [0:0]
# SSLSplit Routing
-A INPUT -p tcp -s 172.16.16.0/24 --match multiport --dports 8080,8081 -j ACCEPT
# Block access to this host from the container network.
-A INPUT -s 172.16.16.0/24 -j DROP
# Block access to metadata.google.internal/AWS metadata.
-A CNI-ADMIN -d 169.254.169.254/32 -j DROP
# Block access to Private address spaces.
-A CNI-ADMIN -s 172.16.16.0/24 -d 10.0.0.0/8 -j DROP
-A CNI-ADMIN -s 172.16.16.0/24 -d 172.16.0.0/12 -j DROP
-A CNI-ADMIN -s 172.16.16.0/24 -d 192.168.0.0/16 -j DROP
COMMIT
| @@ -428,6 +438,14 @@ func (s *podmanSandbox) Run(args ...string) (*RunResult, error) { | |||
| } | |||
| errWriter := io.MultiWriter(errWriters...) | |||
|
|
|||
| log.Debug("upload certs to container") | |||
| uploadCmd := s.UploadFileToContainer("/proxy/certs/ca.crt", "/usr/local/share/ca-certificates/ca.crt") //upload certs to container | |||
There was a problem hiding this comment.
Rather than hardcoding the input cert location, I suggest creating an Option for passing "/proxy/certs/ca.cert" in to the sandbox.
There was a problem hiding this comment.
Please take a look at the new Copy option that has been added.
| @@ -28,6 +32,24 @@ var resultError = &Result{ | |||
| }, | |||
| } | |||
|
|
|||
| func ParseURLsFromSSlStripOutput(content []byte) []string { | |||
There was a problem hiding this comment.
Do you think it is worth de-duping URLs?
There was a problem hiding this comment.
Maybe we could do this further downstream? (i.e. during reporting) - having all the original data could be useful for some cases
There was a problem hiding this comment.
it's very useful to have the raw data as it will allow us to analyze it and search for data such as exfiltrated secrets for example, nevertheless, we ought to have the URLS in the report as some addresses are proven IOC's.
| @@ -28,6 +32,24 @@ var resultError = &Result{ | |||
| }, | |||
| } | |||
|
|
|||
| func ParseURLsFromSSlStripOutput(content []byte) []string { | |||
There was a problem hiding this comment.
It would be more efficient if this method took an io.Writer (pass in the opened file), rather than reading all the bytes in.
There was a problem hiding this comment.
you still have to parse it so you ought to read the file.
Or would you rather I'll read the file inside the function?
| if len(lineParts) < 11 { | ||
| continue | ||
| } | ||
|
|
||
| schema := lineParts[3] | ||
| host := lineParts[8] | ||
| path := lineParts[10] | ||
| result = append(result, schema+"://"+host+path) |
There was a problem hiding this comment.
It could be useful having an example line from the log here so readers can understand what is being parsed.
| iptables := exec.Command("iptables", "-t", "nat", "-A", "PREROUTING", "-i", "cni-analysis", "-p", "tcp", "--dport", "80", "-j", "REDIRECT", "--to-port", "8081") | ||
| err := iptables.Start() | ||
|
|
||
| if err != nil { | ||
| log.Fatal(err.Error()) | ||
| } | ||
| err = iptables.Wait() | ||
| if err != nil { | ||
| log.Fatal(err.Error()) | ||
| } | ||
|
|
||
| log.Debug("Reroute all https traffic through sslsplit") | ||
| iptables = exec.Command("iptables", "-t", "nat", "-A", "PREROUTING", "-i", "cni-analysis", "-p", "tcp", "--dport", "443", "-j", "REDIRECT", "--to-port", "8080") |
There was a problem hiding this comment.
So I think that either these two rules can be added to a *nat section in iptables.rules.
However, it may be desirable to only optionally enable SSLSplit, in which case it would be ideal if these two rules were removed after the sandbox exits (even better if it had a -s option for the containers IP address)
|
This fork is now available within this repo under the |
closes #585