No issues reported for secDevLabs (vulnerable apps)

[aleisalem]

Summary

I am new to using gosec. So, I was trying it against a set of vulnerable go apps in the secDevLabs. The tools reports absolutely no issues although the apps are designed to be vulnerable with some of the vulnerabilities being really obvious (e.g., concatenated SQL statements for SQLi). Am I doing something wrong? Thanks a lot in advance :)

Steps to reproduce the behavior

1. Clone, for example, the copy-n-paste app which is vulnerable to SQL injection attacks.
2. Go to the app/ directory (not sure if necessary but it avoids "failed to import" errors).
3. Run gosec ./... --verbose (just to check the vulnerable files are scanned).

gosec version

v2.14.0

Go version (output of 'go version')

go1.18.7 linux/amd64

Operating system / Environment

Ubuntu Linux 22.04

Expected behavior

I expect to see some security issues like in line 49 according to the rule G202: SQL query construction using string concatenation.

Actual behavior

The tools reports zero issues:
Summary:
Gosec : v2.14.0
Files : 6
Lines : 391
Nosec : 0
Issues : 0

[ccojocar]

I think on a first look that we should detect this issue, but it seems that somehow the SQL rule doesn't. This looks like a bug to me.

[audunmo]

Might be that some of these apps from secDevLabs could be good to benchmark the performance of gosec?

[audunmo]

I have a hypothesis of why this is happening, will try to implement a fix. Specifically, I think the issue is that gosec looks for uses of string formatting calls, and for concat calls, but never both on the same statement

[audunmo]

So I've been looking a bit at why gosec doesn't flag the SQL injections in secDevLabs. The hypothesis I've been working from is that the failure arises because of how gosec handles SQLi via format calls seperately from how it handles SQLi via string concatenations. I still haven't properly confirmed this hypothesis, nor have I found a good fix to the problem.

However, I've been doing some digging. I've added a test case for the case of combined formatting and concatenation, and gosec does not catch it. What I've observed is that when handling this case, the formatting checker only gets the last string in the concatenation. So fmt.Sprintf("SELECT * FROM x WHERE '" + os.Args[1] + "' = 'lala'") only produces "' = 'lala'" for inspection by gosec. You can see this behaviour in my fork, running dlv test --build-flags='github.com/securego/gosec/v2/rules' and inspecting the locals. Screenshot attatched:

And here's the test code that produces the above values. You can see how it's just the last section of the string arg passed to the fmt call that gets analyzed

// Format string with unsafe concatenation
package main

import (
	"database/sql"
	"fmt"
	"os"
)

func main(){
	db, err := sql.Open("sqlite3", ":memory:")
	if err != nil {
		panic(err)
	}
	q := fmt.Sprintf("SELECT * FROM foo where name = '" + os.Args[1] + "' WHERE 1=1")
	rows, err := db.Query(q)
	if err != nil {
		panic(err)
	}
	defer rows.Close()
}

[audunmo]

Just saw that the SQL in test is borked.. Disregard that 😅. Bug is still there though

[ccojocar]

SQL injections rule needs a revamp to catch more advanced issue. Closing this for now.