add gosec scanning to ci (#1349)

gosec severity=medium passes, all severity=low errors are from unhandled
errors, we have 107 of them. tbh it doesn't look worth it to me, but maybe
there are a few assholes even itchier than mine out there. medium has some
good stuff in it, and of course high makes sense if we're gonna do this at
all.

this adds some nosec annotations for some things like sql sprintfs where we
know it's clean (we're constructing the strings with variables in them). fixed
up other spots where we were sprinting without need.

some stuff like filepath.Clean when opening a file from a variable, and file
permissions, easy stuff...

I can't get the CI build to shut up, but I can locally get it to be pretty
quiet about imports and it just outputs the gosec output. fortunately, it
still works as expected even when it's noisy. I got it to shut up by unsetting
some of the go mod flags locally, but that doesn't seem to quite do it in
circle, printed the env out and don't see them, so idk... i give up, this
works

closes #1303

.circleci/config.yml

@@ -6,7 +6,7 @@ jobs:
     working_directory: ~/go/src/github.com/fnproject/fn
     environment: # apparently expansion doesn't work here yet: https://discuss.circleci.com/t/environment-variable-expansion-in-working-directory/11322
       - GOPATH=/home/circleci/go
-      - GOVERSION=1.11
+      - GOVERSION=1.11.2
       - OS=linux
       - ARCH=amd64
       - FN_LOG_LEVEL=debug
@@ -32,6 +32,11 @@ jobs:
       - run: nproc
       # fixes git-diff warning: "warning: inexact rename detection was skipped due to too many files.""
       - run: git config diff.renamelimit 65535
+      # NOTE: if GOFLAGS and GOMODULE are set, gosec will be noisy. unset them (run this before any 'make' command)
+      - run: |
+          wget -O - -q https://raw.githubusercontent.com/securego/gosec/master/install.sh | sh -s 1.2.0
+          ./bin/gosec -quiet -severity medium ./...
+
       - run: make clear-images
 
       # Work out what needs rebuilding and what has changed

Makefile

@@ -1,9 +1,7 @@
-export GO111MODULE=on
-export GOFLAGS=-mod=vendor
 # Just builds
 .PHONY: mod
 dep:
-	go mod vendor -v
+	GO111MODULE=on GOFLAGS=-mod=vendor go mod vendor -v
 
 .PHONY: mod-up
 dep-up:

api/agent/agent.go

@@ -119,6 +119,7 @@ type agent struct {
 type Option func(*agent) error
 
 // RegistryToken is a reserved call extensions key to pass registry token
+/* #nosec */
 const RegistryToken = "FN_REGISTRY_TOKEN"
 
 // New creates an Agent that executes functions locally as Docker containers.

api/agent/drivers/docker/docker_client.go

@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
 	"strings"
 	"time"
 
@@ -297,7 +298,7 @@ func (d *dockerWrap) LoadImages(ctx context.Context, filePath string) error {
 	ctx, closer := makeTracker(ctx, "docker_load_images")
 	defer closer()
 
-	file, err := os.Open(filePath)
+	file, err := os.Open(filepath.Clean(filePath))
 	if err != nil {
 		return err
 	}

api/agent/resource.go

@@ -8,6 +8,7 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"runtime"
 	"strconv"
 	"strings"
@@ -493,7 +494,7 @@ func getMemoryHeadRoom(usableMemory uint64, cfg *Config) (uint64, error) {
 }
 
 func readString(fileName string) (string, error) {
-	b, err := ioutil.ReadFile(fileName)
+	b, err := ioutil.ReadFile(filepath.Clean(fileName))
 	if err != nil {
 		return "", err
 	}

api/common/logging.go

@@ -83,7 +83,7 @@ func SetLogDest(to, prefix string) {
 			return
 		}
 	case "file":
-		f, err := os.OpenFile(parsed.Path, os.O_RDWR|os.O_APPEND|os.O_CREATE, 0666)
+		f, err := os.OpenFile(parsed.Path, os.O_RDWR|os.O_APPEND|os.O_CREATE, 0600)
 		if err != nil {
 			logrus.WithError(err).WithFields(logrus.Fields{"to": to, "path": parsed.Path}).Error("cannot open file, defaulting to stderr")
 			return

api/common/tls_utils.go

@@ -10,7 +10,7 @@ import (
 	"path/filepath"
 )
 
-// A simple TLS Config generator using cert/key
+// NewTLSSimple creates a new tls config with the given cert and key file paths
 func NewTLSSimple(certPath, keyPath string) (*tls.Config, error) {
 
 	err := checkFile(certPath)
@@ -34,7 +34,7 @@ func NewTLSSimple(certPath, keyPath string) (*tls.Config, error) {
 	}, nil
 }
 
-// Add a Client CA
+// AddClientCA adds a client cert to the given tls config
 func AddClientCA(tlsConf *tls.Config, clientCAPath string) error {
 
 	err := checkFile(clientCAPath)
@@ -42,7 +42,7 @@ func AddClientCA(tlsConf *tls.Config, clientCAPath string) error {
 		return err
 	}
 	// Create a certificate pool from the certificate authority
-	authority, err := ioutil.ReadFile(clientCAPath)
+	authority, err := ioutil.ReadFile(filepath.Clean(clientCAPath))
 	if err != nil {
 		return fmt.Errorf("Could not read client CA (%s) certificate: %s", clientCAPath, err)
 	}
@@ -58,7 +58,7 @@ func AddClientCA(tlsConf *tls.Config, clientCAPath string) error {
 	return nil
 }
 
-// Add CA
+// AddCA adds a ca cert to the given tls config
 func AddCA(tlsConf *tls.Config, caPath string) error {
 
 	err := checkFile(caPath)
@@ -66,7 +66,7 @@ func AddCA(tlsConf *tls.Config, caPath string) error {
 		return err
 	}
 
-	ca, err := ioutil.ReadFile(caPath)
+	ca, err := ioutil.ReadFile(filepath.Clean(caPath))
 	if err != nil {
 		return fmt.Errorf("could not read ca (%s) certificate: %s", caPath, err)
 	}

api/datastore/sql/migratex/migrate.go

@@ -283,6 +283,7 @@ func SetVersion(ctx context.Context, tx *sqlx.Tx, version int64, dirty bool) err
 	// TODO need to handle down migration better
 	// ideally, we have a record of each up/down migration with a timestamp for auditing,
 	// this just nukes the whole table which is kinda lame.
+	/* #nosec */
 	query := tx.Rebind("DELETE FROM " + MigrationsTable)
 	if _, err := tx.Exec(query); err != nil {
 		logrus.WithError(err).Error("error deleting version table")
@@ -290,6 +291,7 @@ func SetVersion(ctx context.Context, tx *sqlx.Tx, version int64, dirty bool) err
 	}
 
 	if version >= 0 {
+		/* #nosec */
 		query = tx.Rebind(`INSERT INTO ` + MigrationsTable + ` (version, dirty) VALUES (?, ?)`)
 		if _, err := tx.ExecContext(ctx, query, version, dirty); err != nil {
 			logrus.WithError(err).Error("error updating version table")
@@ -316,6 +318,7 @@ func Version(ctx context.Context, tx *sqlx.Tx) (version int64, dirty bool, err e
 		return NilVersion, false, nil
 	}
 
+	/* #nosec */
 	query := tx.Rebind(`SELECT version, dirty FROM ` + MigrationsTable + ` LIMIT 1`)
 
 	err = tx.QueryRowContext(ctx, query).Scan(&version, &dirty)

api/datastore/sql/migrations/10_add_app_id.go

@@ -113,6 +113,7 @@ func up10(ctx context.Context, tx *sqlx.Tx) error {
 		}
 
 		for _, t := range []string{"routes", "calls", "logs"} {
+			/* #nosec */
 			q := fmt.Sprintf(`UPDATE %s SET app_id=:id WHERE app_name=:name;`, t)
 			_, err = tx.NamedExecContext(ctx, tx.Rebind(q), app)
 			if err != nil {
@@ -138,6 +139,7 @@ func up10(ctx context.Context, tx *sqlx.Tx) error {
 			statement := t["statement"]
 			tableName := t["table"]
 			columns := t["columns"]
+			/* #nosec */
 			_, err = tx.ExecContext(ctx, tx.Rebind(fmt.Sprintf("ALTER TABLE %s RENAME TO old_%s;", tableName, tableName)))
 			if err != nil {
 				return err
@@ -146,11 +148,13 @@ func up10(ctx context.Context, tx *sqlx.Tx) error {
 			if err != nil {
 				return err
 			}
+			/* #nosec */
 			_, err = tx.ExecContext(ctx, tx.Rebind(fmt.Sprintf("INSERT INTO %s (%s) SELECT %s FROM old_%s;", tableName, columns, columns, tableName)))
 			if err != nil {
 				return err
 			}
 
+			/* #nosec */
 			_, err = tx.ExecContext(ctx, tx.Rebind(fmt.Sprintf("DROP TABLE old_%s;", tableName)))
 			if err != nil {
 				return err
@@ -204,7 +208,8 @@ func down10(ctx context.Context, tx *sqlx.Tx) error {
 	// it is required for some reason, can't do this within the rows iteration.
 	for _, app := range res {
 		for _, t := range []string{"routes", "calls", "logs"} {
-			q := "UPDATE " + t + " SET app_name=:name WHERE app_id=:id;"
+			/* #nosec */
+			q := fmt.Sprintf("UPDATE %s SET app_name=:name WHERE app_id=:id;", t)
 			_, err = tx.NamedExecContext(ctx, tx.Rebind(q), app)
 			if err != nil {
 				return err

api/datastore/sql/mysql/mysql.go

@@ -1,13 +1,13 @@
 package mysql
 
 import (
-	"fmt"
+	"net/url"
+	"strings"
+
 	"github.com/fnproject/fn/api/datastore/sql/dbhelper"
 	"github.com/go-sql-driver/mysql"
 	_ "github.com/go-sql-driver/mysql"
 	"github.com/jmoiron/sqlx"
-	"net/url"
-	"strings"
 )
 
 type mysqlHelper int
@@ -25,12 +25,11 @@ func (mysqlHelper) PostCreate(db *sqlx.DB) (*sqlx.DB, error) {
 
 }
 func (mysqlHelper) CheckTableExists(tx *sqlx.Tx, table string) (bool, error) {
-	query := tx.Rebind(fmt.Sprintf(`SELECT count(*)
+	query := tx.Rebind(`SELECT count(*)
 	FROM information_schema.TABLES
-	WHERE TABLE_NAME = '%s'
-`, table))
+	WHERE TABLE_NAME = ?`)
 
-	row := tx.QueryRow(query)
+	row := tx.QueryRow(query, table)
 
 	var count int
 	err := row.Scan(&count)

api/datastore/sql/sql.go

@@ -14,15 +14,14 @@ import (
 	"time"
 
 	"github.com/fnproject/fn/api/common"
-	"github.com/fnproject/fn/api/datastore/sql/migratex"
-	"github.com/fnproject/fn/api/datastore/sql/migrations"
-	"github.com/fnproject/fn/api/models"
-	"github.com/jmoiron/sqlx"
-
 	"github.com/fnproject/fn/api/datastore"
 	"github.com/fnproject/fn/api/datastore/sql/dbhelper"
+	"github.com/fnproject/fn/api/datastore/sql/migratex"
+	"github.com/fnproject/fn/api/datastore/sql/migrations"
 	"github.com/fnproject/fn/api/id"
 	"github.com/fnproject/fn/api/logs"
+	"github.com/fnproject/fn/api/models"
+	"github.com/jmoiron/sqlx"
 	"github.com/sirupsen/logrus"
 )
 
@@ -492,6 +491,7 @@ func (ds *SQLStore) GetApps(ctx context.Context, filter *models.AppFilter) (*mod
 	if err != nil {
 		return nil, err
 	}
+	/* #nosec */
 	query = ds.db.Rebind(fmt.Sprintf("SELECT DISTINCT id, name, config, annotations, syslog_url, created_at, updated_at FROM apps %s", query))
 	rows, err := ds.db.QueryxContext(ctx, query, args...)
 	if err != nil {
@@ -636,6 +636,7 @@ func (ds *SQLStore) GetFns(ctx context.Context, filter *models.FnFilter) (*model
 		return res, err
 	}
 
+	/* #nosec */
 	query := fmt.Sprintf("%s %s", fnSelector, filterQuery)
 	query = ds.db.Rebind(query)
 	rows, err := ds.db.QueryxContext(ctx, query, args...)
@@ -670,6 +671,7 @@ func (ds *SQLStore) GetFns(ctx context.Context, filter *models.FnFilter) (*model
 }
 
 func (ds *SQLStore) GetFnByID(ctx context.Context, fnID string) (*models.Fn, error) {
+	/* #nosec */
 	query := ds.db.Rebind(fmt.Sprintf("%s WHERE id=?", fnSelector))
 	row := ds.db.QueryRowxContext(ctx, query, fnID)
 
@@ -684,9 +686,8 @@ func (ds *SQLStore) GetFnByID(ctx context.Context, fnID string) (*models.Fn, err
 }
 
 func (ds *SQLStore) RemoveFn(ctx context.Context, fnID string) error {
-
 	return ds.Tx(func(tx *sqlx.Tx) error {
-
+		/* #nosec */
 		query := tx.Rebind(fmt.Sprintf("%s WHERE id=?", fnSelector))
 		row := tx.QueryRowxContext(ctx, query, fnID)
 
@@ -753,6 +754,7 @@ func (ds *SQLStore) InsertCall(ctx context.Context, call *models.Call) error {
 }
 
 func (ds *SQLStore) GetCall1(ctx context.Context, appID, callID string) (*models.Call, error) {
+	/* #nosec */
 	query := fmt.Sprintf(`%s WHERE id=? AND app_id=?`, callSelector)
 	query = ds.db.Rebind(query)
 	row := ds.db.QueryRowxContext(ctx, query, callID, appID)
@@ -769,6 +771,7 @@ func (ds *SQLStore) GetCall1(ctx context.Context, appID, callID string) (*models
 }
 
 func (ds *SQLStore) GetCall(ctx context.Context, fnID, callID string) (*models.Call, error) {
+	/* #nosec */
 	query := fmt.Sprintf(`%s WHERE id=? AND fn_id=?`, callSelector)
 	query = ds.db.Rebind(query)
 	row := ds.db.QueryRowxContext(ctx, query, callID, fnID)
@@ -811,6 +814,7 @@ func (ds *SQLStore) GetCalls(ctx context.Context, filter *models.CallFilter) (*m
 func (ds *SQLStore) GetCalls1(ctx context.Context, filter *models.CallFilter) ([]*models.Call, error) {
 	res := []*models.Call{}
 	query, args := buildFilterCallQuery(filter)
+	/* #nosec */
 	query = fmt.Sprintf("%s %s", callSelector, query)
 	query = ds.db.Rebind(query)
 	rows, err := ds.db.QueryxContext(ctx, query, args...)
@@ -962,8 +966,10 @@ func where(b *bytes.Buffer, args []interface{}, colOp string, val interface{}) [
 	}
 	args = append(args, val)
 	if len(args) == 1 {
+		/* #nosec */
 		fmt.Fprintf(b, `WHERE %s`, colOp)
 	} else {
+		/* #nosec */
 		fmt.Fprintf(b, ` AND %s`, colOp)
 	}
 	return args
@@ -1093,6 +1099,7 @@ func (ds *SQLStore) UpdateTrigger(ctx context.Context, trigger *models.Trigger)
 
 func (ds *SQLStore) GetTrigger(ctx context.Context, appId, fnId, triggerName string) (*models.Trigger, error) {
 	var trigger models.Trigger
+	/* #nosec */
 	query := ds.db.Rebind(fmt.Sprintf("%s WHERE name=? AND app_id=? AND fn_id=?", fnSelector))
 	row := ds.db.QueryRowxContext(ctx, query, triggerName, appId, fnId)
 
@@ -1189,6 +1196,7 @@ func (ds *SQLStore) GetTriggers(ctx context.Context, filter *models.TriggerFilte
 		return res, err
 	}
 
+	/* #nosec */
 	query := fmt.Sprintf("%s WHERE %s", triggerSelector, filterQuery)
 	query = ds.db.Rebind(query)
 	rows, err := ds.db.QueryxContext(ctx, query, args...)

api/datastore/sql/sqlite/sqlite.go

@@ -1,14 +1,14 @@
 package sqlite
 
 import (
-	"fmt"
-	"github.com/fnproject/fn/api/datastore/sql/dbhelper"
-	"github.com/jmoiron/sqlx"
-	"github.com/mattn/go-sqlite3"
 	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
+
+	"github.com/fnproject/fn/api/datastore/sql/dbhelper"
+	"github.com/jmoiron/sqlx"
+	"github.com/mattn/go-sqlite3"
 )
 
 type sqliteHelper int
@@ -24,7 +24,7 @@ func (sqliteHelper) Supports(scheme string) bool {
 func (sqliteHelper) PreConnect(url *url.URL) (string, error) {
 	// make all the dirs so we can make the file..
 	dir := filepath.Dir(url.Path)
-	err := os.MkdirAll(dir, 0755)
+	err := os.MkdirAll(dir, 0750)
 	if err != nil {
 		return "", err
 	}
@@ -38,12 +38,11 @@ func (sqliteHelper) PostCreate(db *sqlx.DB) (*sqlx.DB, error) {
 }
 
 func (sqliteHelper) CheckTableExists(tx *sqlx.Tx, table string) (bool, error) {
-	query := tx.Rebind(fmt.Sprintf(`SELECT count(*)
+	query := tx.Rebind(`SELECT count(*)
 		FROM sqlite_master
-		WHERE name = '%s'
-		`, table))
+		WHERE name = ?`)
 
-	row := tx.QueryRow(query)
+	row := tx.QueryRow(query, table)
 
 	var count int
 	err := row.Scan(&count)

api/mqs/bolt/bolt.go

@@ -70,12 +70,12 @@ func (boltProvider) String() string {
 func (boltProvider) New(url *url.URL) (models.MessageQueue, error) {
 	dir := filepath.Dir(url.Path)
 	log := logrus.WithFields(logrus.Fields{"mq": url.Scheme, "dir": dir})
-	err := os.MkdirAll(dir, 0755)
+	err := os.MkdirAll(dir, 0750)
 	if err != nil {
 		log.WithError(err).Errorln("Could not create data directory for mq")
 		return nil, err
 	}
-	db, err := bolt.Open(url.Path, 0666, &bolt.Options{Timeout: 1 * time.Second})
+	db, err := bolt.Open(url.Path, 0600, &bolt.Options{Timeout: 1 * time.Second})
 	if err != nil {
 		log.WithError(err).Errorln("Could not open BoltDB file for MQ")
 		return nil, err

api/server/init.go

@@ -5,6 +5,7 @@ import (
 	"io/ioutil"
 	"os"
 	"os/signal"
+	"path/filepath"
 	"runtime"
 	"strconv"
 	"strings"
@@ -24,7 +25,7 @@ func getEnv(key, fallback string) string {
 	if value, ok := os.LookupEnv(key); ok {
 		return value
 	} else if value, ok := os.LookupEnv(key + "_FILE"); ok {
-		dat, err := ioutil.ReadFile(value)
+		dat, err := ioutil.ReadFile(filepath.Clean(value))
 		if err == nil {
 			return string(dat)
 		}
@@ -42,7 +43,7 @@ func getEnvInt(key string, fallback int) int {
 		}
 		return i
 	} else if value, ok := os.LookupEnv(key + "_FILE"); ok {
-		dat, err := ioutil.ReadFile(value)
+		dat, err := ioutil.ReadFile(filepath.Clean(value))
 		if err == nil {
 			var err error
 			var i int

images/fn-test-utils/fn-test-utils.go

@@ -10,6 +10,7 @@ import (
 	"log"
 	"net/http"
 	"os"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -364,7 +365,7 @@ func getChunk(size int) []byte {
 
 func readFile(name string, size int) (string, error) {
 	// read the whole file into memory
-	out, err := ioutil.ReadFile(name)
+	out, err := ioutil.ReadFile(filepath.Clean(name))
 	if err != nil {
 		return "", err
 	}
@@ -376,7 +377,7 @@ func readFile(name string, size int) (string, error) {
 }
 
 func createFile(name string, size int) error {
-	f, err := os.Create(name)
+	f, err := os.Create(filepath.Clean(name))
 	if err != nil {
 		return err
 	}