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.

Sign up for GitHub

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

Jump to bottom

feat: add urls (http+https) to json report #681

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add urls (http+https) to json report #681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*.dll
*.so
*.dylib

.idea
# Test binary, built with `go test -c`
*.test

Expand Down
1 change: 1 addition & 0 deletions cmd/analyze/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ RUN apt-get update && apt-get upgrade -y && \
iptables \
iproute2 \
podman \
sslsplit \
software-properties-common && \
update-alternatives --set iptables /usr/sbin/iptables-legacy && \
update-alternatives --set ip6tables /usr/sbin/ip6tables-legacy
Expand Down
34 changes: 34 additions & 0 deletions cmd/analyze/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"net/url"
"os"
"os/exec"
"strings"

"github.com/ossf/package-analysis/internal/analysis"
Expand Down Expand Up @@ -195,6 +196,11 @@ func main() {
"error", err)
}

err = generateSSLCerts()
if err != nil {
log.Panic("Error creating certificates")
}

if runMode[analysis.Static] {
log.Info("Starting static analysis")
staticAnalysis(pkg)
Expand All @@ -206,3 +212,31 @@ func main() {
dynamicAnalysis(pkg)
}
}

func generateSSLCerts() error {
Alik-Kold marked this conversation as resolved.
Show resolved Hide resolved
log.Debug("Generating SSLSplit CA certificates")
createSslCerts := exec.Command("openssl", "genrsa", "-out", "/proxy/certs/ca.pem", "2048")
createSslCerts.Dir = "/proxy/certs"
Alik-Kold marked this conversation as resolved.
Show resolved Hide resolved
err := createSslCerts.Start()

if err != nil {
Alik-Kold marked this conversation as resolved.
Show resolved Hide resolved
return err
}
err = createSslCerts.Wait()
if err != nil {
return err
}

createSslCerts = exec.Command("openssl", "req", "-new", "-nodes", "-x509", "-sha256", "-out", "ca.crt", "-key", "ca.pem", "-addext", "authorityKeyIdentifier=keyid:always,issuer:always", "-subj", "/O=Package Analysis CA/CN=Package Analysis CA/", "-set_serial", "0", "-days", "3650")
createSslCerts.Dir = "/proxy/certs"
err = createSslCerts.Start()

if err != nil {
return err
}
err = createSslCerts.Wait()
if err != nil {
return err
}
return nil
}
76 changes: 73 additions & 3 deletions internal/dynamicanalysis/analysis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,17 @@ package dynamicanalysis

import (
"fmt"

"github.com/ossf/package-analysis/internal/analysis"
"github.com/ossf/package-analysis/internal/dnsanalyzer"
"github.com/ossf/package-analysis/internal/log"
"github.com/ossf/package-analysis/internal/packetcapture"
"github.com/ossf/package-analysis/internal/sandbox"
"github.com/ossf/package-analysis/internal/strace"
"github.com/ossf/package-analysis/pkg/api/analysisrun"
"io/ioutil"
"os/exec"
"strings"
"syscall"
)

const (
Expand All @@ -20,6 +23,7 @@ const (
type Result struct {
StraceSummary analysisrun.StraceSummary
FileWrites analysisrun.FileWrites
URLs []string
}

var resultError = &Result{
Expand All @@ -28,6 +32,24 @@ var resultError = &Result{
},
}

func ParseURLsFromSSlStripOutput(content []byte) []string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it is worth de-duping URLs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could do this further downstream? (i.e. during reporting) - having all the original data could be useful for some cases

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more efficient if this method took an io.Writer (pass in the opened file), rather than reading all the bytes in.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

var result []string
ret := strings.Split(string(content), "\n")

for _, value := range ret {
lineParts := strings.Split(value, " ")
if len(lineParts) < 11 {
continue
}

schema := lineParts[3]
host := lineParts[8]
path := lineParts[10]
result = append(result, schema+"://"+host+path)
Comment on lines +41 to +48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be useful having an example line from the log here so readers can understand what is being parsed.

}
return result
}

func Run(sb sandbox.Sandbox, args []string) (*Result, error) {
log.Info("Running dynamic analysis",
"args", args)
Expand All @@ -45,11 +67,58 @@ func Run(sb sandbox.Sandbox, args []string) (*Result, error) {
// Run the command
log.Debug("Running dynamic analysis command",
"args", args)

log.Debug("Reroute all http traffic through sslsplit")
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")
Comment on lines +72 to +84
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

err = iptables.Start()

if err != nil {
log.Fatal(err.Error())
}
err = iptables.Wait()
if err != nil {
log.Fatal(err.Error())
}

log.Debug("starting sslsplit")
sslsplit := exec.Command("sslsplit", "-d", "-L", "/tmp/ssl.flow", "-l", "/tmp/sslLinks.flow", "-k", "/proxy/certs/ca.pem", "-c", "/proxy/certs/ca.crt", "http", "0.0.0.0", "8081", "https", "0.0.0.0", "8080")
Alik-Kold marked this conversation as resolved.
Show resolved Hide resolved
err = sslsplit.Start()

if err != nil {
log.Fatal(err.Error())
}

r, err := sb.Run(args...)
if err != nil {
return resultError, fmt.Errorf("sandbox failed (%w)", err)
}

log.Debug("stopping sslsplit")
err = sslsplit.Process.Signal(syscall.SIGINT)
if err != nil {
return nil, err
}

log.Debug("reading sslsplit results")
body, err1 := ioutil.ReadFile("/tmp/sslLinks.flow")
if err1 != nil {
log.Fatal("unable to read file: %v", err1)
}

urls := ParseURLsFromSSlStripOutput(body)

log.Debug("Stop the packet capture")
pcap.Close()

Expand All @@ -73,11 +142,11 @@ func Run(sb sandbox.Sandbox, args []string) (*Result, error) {
Stderr: lastLines(r.Stderr(), maxOutputLines, maxOutputBytes),
},
}
analysisResult.setData(straceResult, dns)
analysisResult.setData(straceResult, dns, urls)
return &analysisResult, nil
}

func (d *Result) setData(straceResult *strace.Result, dns *dnsanalyzer.DNSAnalyzer) {
func (d *Result) setData(straceResult *strace.Result, dns *dnsanalyzer.DNSAnalyzer, sslSplitResult []string) {
for _, f := range straceResult.Files() {
d.StraceSummary.Files = append(d.StraceSummary.Files, analysisrun.FileResult{
Path: f.Path,
Expand Down Expand Up @@ -121,4 +190,5 @@ func (d *Result) setData(straceResult *strace.Result, dns *dnsanalyzer.DNSAnalyz
}
d.StraceSummary.DNS = append(d.StraceSummary.DNS, c)
}
d.URLs = sslSplitResult
}
20 changes: 19 additions & 1 deletion internal/sandbox/sandbox.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ type Sandbox interface {
// The result of the supplied command will be returned in an instance of
// RunResult.
Run(...string) (*RunResult, error)

UploadFileToContainer(srcFile string, destFile string) *exec.Cmd
Alik-Kold marked this conversation as resolved.
Show resolved Hide resolved
// Clean cleans up a Sandbox.
//
// Once called the Sandbox cannot be used again.
Expand Down Expand Up @@ -127,6 +127,16 @@ type podmanSandbox struct {
volumes []volume
}

func (s *podmanSandbox) UploadFileToContainer(srcFile string, destFile string) *exec.Cmd {
destParam := fmt.Sprintf("%s:%s", s.container, destFile)
args := []string{
"cp",
srcFile,
destParam,
}
return podman(args...)
}

type (
Option interface{ set(*podmanSandbox) }
option func(*podmanSandbox) // option implements Option.
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than hardcoding the input cert location, I suggest creating an Option for passing "/proxy/certs/ca.cert" in to the sandbox.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at the new Copy option that has been added.

uploadCmd.Stdout = logOut
uploadCmd.Stderr = logErr
if err := uploadCmd.Run(); err != nil {
return result, fmt.Errorf("error uploading file to container: %w", err)
}

// Start the container
startCmd := s.startContainerCmd(logDir)
startCmd.Stdout = logOut
Expand Down
2 changes: 1 addition & 1 deletion internal/worker/rundynamic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func RunDynamicAnalysis(pkg *pkgmanager.Pkg, sbOpts []sandbox.Option) (analysisr
results.StraceSummary[phase] = &phaseResult.StraceSummary
results.FileWrites[phase] = &phaseResult.FileWrites
lastStatus = phaseResult.StraceSummary.Status

results.URLs = phaseResult.URLs
if lastStatus != analysis.StatusCompleted {
// Error caused by an issue with the package (probably).
// Don't continue with phases if this one did not complete successfully.
Expand Down
1 change: 1 addition & 0 deletions pkg/api/analysisrun/result.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ type (
type DynamicAnalysisResults struct {
StraceSummary DynamicAnalysisStraceSummary
FileWrites DynamicAnalysisFileWrites
URLs []string
Alik-Kold marked this conversation as resolved.
Show resolved Hide resolved
}

type StaticAnalysisResults = json.RawMessage
Expand Down
3 changes: 3 additions & 0 deletions sandboxes/dynamicanalysis/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,9 @@ ENV NODE_PATH="/app/node_modules"
# Test stuff
RUN ruby --version && php --version && python3 --version && pip --version && node --version && npm --version && rustc --version && cargo --version

ENV REQUESTS_CA_BUNDLE=/usr/local/share/ca-certificates/ca.crt
ENV SSL_CERT_FILE=/usr/local/share/ca-certificates/ca.crt
Alik-Kold marked this conversation as resolved.
Show resolved Hide resolved

ENTRYPOINT [ "sleep" ]

CMD [ "30m" ]
11 changes: 11 additions & 0 deletions tools/network/iptables.rules
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@
*filter
:INPUT ACCEPT [0:0]
:CNI-ADMIN - [0:0]

# SSLSplit Routing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll need to check that these don't accidentally allow the sandbox to bypass the rules below for the given ports.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

-A INPUT -p tcp --dport 80 -j ACCEPT
-A OUTPUT -p tcp --dport 80 -j ACCEPT
-A INPUT -p tcp --dport 443 -j ACCEPT
-A OUTPUT -p tcp --dport 443 -j ACCEPT
-A INPUT -p tcp --dport 8081 -j ACCEPT
-A OUTPUT -p tcp --dport 8081 -j ACCEPT
-A INPUT -p tcp --dport 8080 -j ACCEPT
-A OUTPUT -p tcp --dport 8080 -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.
Expand Down
Loading