Adjust SQL format-string rules to ignore inherently safe formats

This commit is contained in:
Jon McClintock 2017-10-05 16:24:29 +00:00
parent 6de76c9261
commit 7dfebaf91e
2 changed files with 52 additions and 5 deletions

View file

@ -23,7 +23,7 @@ import (
type SqlStatement struct { type SqlStatement struct {
gas.MetaData gas.MetaData
pattern *regexp.Regexp patterns []*regexp.Regexp
} }
type SqlStrConcat struct { type SqlStrConcat struct {
@ -42,7 +42,12 @@ func (s *SqlStrConcat) checkObject(n *ast.Ident) bool {
func (s *SqlStrConcat) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) { func (s *SqlStrConcat) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
if node, ok := n.(*ast.BinaryExpr); ok { if node, ok := n.(*ast.BinaryExpr); ok {
if start, ok := node.X.(*ast.BasicLit); ok { if start, ok := node.X.(*ast.BasicLit); ok {
if str, e := gas.GetString(start); s.pattern.MatchString(str) && e == nil { if str, e := gas.GetString(start); e == nil {
for _, pattern := range s.patterns {
if !pattern.MatchString(str) {
return nil, nil
}
}
if _, ok := node.Y.(*ast.BasicLit); ok { if _, ok := node.Y.(*ast.BasicLit); ok {
return nil, nil // string cat OK return nil, nil // string cat OK
} }
@ -59,7 +64,9 @@ func (s *SqlStrConcat) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
func NewSqlStrConcat(conf map[string]interface{}) (gas.Rule, []ast.Node) { func NewSqlStrConcat(conf map[string]interface{}) (gas.Rule, []ast.Node) {
return &SqlStrConcat{ return &SqlStrConcat{
SqlStatement: SqlStatement{ SqlStatement: SqlStatement{
pattern: regexp.MustCompile(`(?)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) `), patterns: []*regexp.Regexp{
regexp.MustCompile(`(?)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) `),
},
MetaData: gas.MetaData{ MetaData: gas.MetaData{
Severity: gas.Medium, Severity: gas.Medium,
Confidence: gas.High, Confidence: gas.High,
@ -77,7 +84,12 @@ type SqlStrFormat struct {
// Looks for "fmt.Sprintf("SELECT * FROM foo where '%s', userInput)" // Looks for "fmt.Sprintf("SELECT * FROM foo where '%s', userInput)"
func (s *SqlStrFormat) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) { func (s *SqlStrFormat) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) {
if node := gas.MatchCall(n, s.call); node != nil { if node := gas.MatchCall(n, s.call); node != nil {
if arg, e := gas.GetString(node.Args[0]); s.pattern.MatchString(arg) && e == nil { if arg, e := gas.GetString(node.Args[0]); e == nil {
for _, pattern := range s.patterns {
if !pattern.MatchString(arg) {
return nil, nil
}
}
return gas.NewIssue(c, n, s.What, s.Severity, s.Confidence), nil return gas.NewIssue(c, n, s.What, s.Severity, s.Confidence), nil
} }
} }
@ -88,7 +100,10 @@ func NewSqlStrFormat(conf map[string]interface{}) (gas.Rule, []ast.Node) {
return &SqlStrFormat{ return &SqlStrFormat{
call: regexp.MustCompile(`^fmt\.Sprintf$`), call: regexp.MustCompile(`^fmt\.Sprintf$`),
SqlStatement: SqlStatement{ SqlStatement: SqlStatement{
pattern: regexp.MustCompile("(?)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) "), patterns: []*regexp.Regexp{
regexp.MustCompile("(?)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) "),
regexp.MustCompile("%[^bdoxXfFp]"),
},
MetaData: gas.MetaData{ MetaData: gas.MetaData{
Severity: gas.Medium, Severity: gas.Medium,
Confidence: gas.High, Confidence: gas.High,

View file

@ -214,3 +214,35 @@ func TestSQLInjectionFalsePositiveD(t *testing.T) {
checkTestResults(t, issues, 0, "Not expected to match") checkTestResults(t, issues, 0, "Not expected to match")
} }
func TestSQLInjectionFalsePositiveE(t *testing.T) {
config := map[string]interface{}{"ignoreNosec": false}
analyzer := gas.NewAnalyzer(config, nil)
analyzer.AddRule(NewSqlStrFormat(config))
source := `
package main
import (
"database/sql"
"fmt"
"os"
"strconv"
//_ "github.com/mattn/go-sqlite3"
)
func main(){
db, err := sql.Open("sqlite3", ":memory:")
if err != nil {
panic(err)
}
id, _ := strconv.Atoi(os.Args[1])
q := fmt.Sprintf("SELECT * FROM foo where id = %d", id)
rows, err := db.Query(q)
if err != nil {
panic(err)
}
defer rows.Close()
}
`
issues := gasTestRunner(source, analyzer)
checkTestResults(t, issues, 0, "Not expected to match")
}