Threagile / threagile

Agile Threat Modeling Toolkit
https://threagile.io
MIT License
577 stars 126 forks source link

Segmentation fault on incomplete threat model #66

Open CMon opened 2 months ago

CMon commented 2 months ago

I create a threat model and am in the middle of creation, now I need to clean out all stuff i forget. During this journey I encounter some crashes (see #65). Now I have one I can not easily fix myself:

[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x6d818a]

goroutine 1 [running]:
github.com/threagile/threagile/pkg/security/risks/builtin.(*ServerSideRequestForgeryRule).createRisk(0xcbd480?, 0xc0001a4d88, 0xc0004c1760, 0xc000304ea0)
        /app/pkg/security/risks/builtin/server-side-request-forgery-rule.go:92 +0x70a
github.com/threagile/threagile/pkg/security/risks/builtin.(*ServerSideRequestForgeryRule).GenerateRisks(0x160b560, 0xc0001a4d88)
        /app/pkg/security/risks/builtin/server-side-request-forgery-rule.go:56 +0x305
github.com/threagile/threagile/pkg/model.applyRiskGeneration(0xc0001a4d88, 0xc00026a030, {0x160b560, 0x0, 0x8?}, {0xf45468, 0xc0000129fe})
        /app/pkg/model/read.go:97 +0x2a2
github.com/threagile/threagile/pkg/model.ReadAndAnalyzeModel(0xc0004e1188, {0xf45468, 0xc0000129fe})
        /app/pkg/model/read.go:56 +0x539
github.com/threagile/threagile/internal/threagile.(*Threagile).initAnalyze.func1(0xc0004bcc00?, {0xda0c41?, 0x4?, 0xda0c45?})
        /app/internal/threagile/analyze.go:21 +0xec
github.com/spf13/cobra.(*Command).execute(0xc0004ec608, {0xc000467240, 0x4, 0x4})
        /go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:983 +0xaca
github.com/spf13/cobra.(*Command).ExecuteC(0xc0004ec308)
        /go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:1115 +0x3ff
github.com/spf13/cobra.(*Command).Execute(...)
        /go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:1039
github.com/threagile/threagile/internal/threagile.(*Threagile).Execute(0xc0006bef00)
        /app/internal/threagile/threagile.go:16 +0x25
main.main()
        /app/cmd/threagile/main.go:12 +0x32

I added (in pkg/security/risks/builtin/server-side-request-forgery-rule.go

    // report error if no trustboundary found, this does not fix the crash but at least I know where to fix stuff
    if input.TrustBoundaries[technicalAsset.GetTrustBoundaryId(input)] == nil {
        _, _ = fmt.Fprintf(os.Stderr, "missing trust boundary for technical asset: %q\n", technicalAsset.Id)
    }

before:

    // adjust for cloud-based special risks
    if impact == types.LowImpact && input.TrustBoundaries[technicalAsset.GetTrustBoundaryId(input)].Type.IsWithinCloud() {
        impact = types.MediumImpact
    }

This does not fix the crash but at least I got a hint what I need to fix.

I think the bug is somewhere else, there should be some kind of sanatize method after the parse that checks for the existance of technical assets inside of trust boundaries, and even more if there are more dependencies. or the createRisk methods need a way to report an error.

joreiche commented 2 months ago

technical assets may or may not be inside trust boundaries so the code should handle it gracefully. a recent change I made changed internal structures to pointers which is why you likely now see crashes where before it just gave you an empty struct. this change was made so that other items can cross-reference items without having to copy them.

I am happy to help fixing the issue you are having. if you need help resolving the issue, please provide a threat model I can use to reproduce the issue

CMon commented 2 months ago

I fixed it inside my threat model by adding the print line, then seeing whats missing and then fixing it in the yaml. But at least in my opinion the threagile tool should already include such error handling. It would be more helpful if such error appear for example in a pipeline than some strange segmentation fault. I am not very fluent in go, so "seeing" which variables are pointers is not in my skill set. Otherwise I would have suggested to do it the C++ way, check each pointer and report an error if its not valid.