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.

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

Resolve most Gosec suggestions #338

Merged
merged 27 commits into from
Jul 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
4d20bd3
gosec: allow loading user defined file
jsirianni Jun 23, 2021
0e59836
gosec: limit stanza log to the user that is running stanza. stanza er…
jsirianni Jun 23, 2021
b55c96c
handle errors returned by stderr / stdout
jsirianni Jun 23, 2021
a957851
gosec: allow net/http/pprof's http webhook. handle errors from closin…
jsirianni Jun 23, 2021
c9e3b01
gosec: limit stanza database to user that is running stanza. stanza d…
jsirianni Jun 23, 2021
71a511a
gosec: allow user to pass in files. restrict buffer file to user runn…
jsirianni Jun 23, 2021
316448c
handle error from closing connection
jsirianni Jun 23, 2021
12f3f51
gosec: limit permissions for file output file to the user running stanza
jsirianni Jun 23, 2021
3072f52
handle errors from closing body
jsirianni Jun 23, 2021
c0ee888
handle errors from closing body
jsirianni Jun 23, 2021
1e53f35
return the error from t.ProcessWith
jsirianni Jun 23, 2021
d25c788
handle error from base.Set
jsirianni Jun 23, 2021
018a561
gosec: allow loading user defined file
jsirianni Jun 23, 2021
559bd13
gosec: allow loading user defined file
jsirianni Jun 23, 2021
39b0e1e
gosec: handle errors
jsirianni Jun 23, 2021
df00c72
database directory is fine with 0755 permissions but the database fil…
jsirianni Jun 23, 2021
722a259
Merge branch 'master' into resolve-gosec
Jun 30, 2021
623a6e3
remove return type error from handleEvent and handleEvents as the err…
Jun 30, 2021
7e0396b
return file.Close() error
Jun 30, 2021
db20e4a
log errors returned by file.Close()
Jun 30, 2021
955dab0
nosec, user defines the input file
Jun 30, 2021
67c0ce1
journalctl is an executable that is required for this operator to fun…
Jun 30, 2021
79df13f
gosec wants TLS 1.2/1.3 but Go defaults to 1.0. Some users may requir…
Jun 30, 2021
f972beb
Merge branch 'master' into resolve-gosec
Jul 8, 2021
8245996
Merge branch 'master' into resolve-gosec
Jul 8, 2021
53954d1
remove inappropriate use of failnow
Jul 8, 2021
e0ca031
close open files before test finishes
Jul 8, 2021
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 agent/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type Config struct {

// NewConfigFromFile will create a new agent config from a YAML file.
func NewConfigFromFile(file string) (*Config, error) {
contents, err := ioutil.ReadFile(file)
contents, err := ioutil.ReadFile(file) // #nosec - configs load based on user specified directory
if err != nil {
return nil, fmt.Errorf("could not find config file: %s", err)
}
Expand Down
3 changes: 2 additions & 1 deletion cmd/stanza/logging.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,5 +59,6 @@ func registerWindowsSink() {
}

func newWinFileSink(u *url.URL) (zap.Sink, error) {
return os.OpenFile(u.Path[1:], os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
// Ensure permissions restrict access to the running user only
return os.OpenFile(u.Path[1:], os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0600)
}
24 changes: 16 additions & 8 deletions cmd/stanza/offsets.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/observiq/stanza/operator/helper"
"github.com/spf13/cobra"
"go.etcd.io/bbolt"
"go.uber.org/zap"
)

var stdout io.Writer = os.Stdout
Expand All @@ -19,9 +20,6 @@ func NewOffsetsCmd(rootFlags *RootFlags) *cobra.Command {
Use: "offsets",
Short: "Manage input operator offsets",
Args: cobra.NoArgs,
Run: func(command *cobra.Command, args []string) {
stdout.Write([]byte("No offsets subcommand specified. See `stanza offsets help` for details\n"))
},
}

offsets.AddCommand(NewOffsetsClearCmd(rootFlags))
Expand All @@ -46,7 +44,10 @@ func NewOffsetsClearCmd(rootFlags *RootFlags) *cobra.Command {

if all {
if len(args) != 0 {
stdout.Write([]byte("Providing a list of operator IDs does nothing with the --all flag\n"))
_, err := stdout.Write([]byte("Providing a list of operator IDs does nothing with the --all flag\n"))
if err != nil {
exitOnErr("", err)
}
}

err := db.Update(func(tx *bbolt.Tx) error {
Expand All @@ -59,7 +60,10 @@ func NewOffsetsClearCmd(rootFlags *RootFlags) *cobra.Command {
exitOnErr("Failed to delete offsets", err)
} else {
if len(args) == 0 {
stdout.Write([]byte("Must either specify a list of operators or the --all flag\n"))
_, err := stdout.Write([]byte("Must either specify a list of operators or the --all flag\n"))
if err != nil {
exitOnErr("", err)
}
os.Exit(1)
}

Expand Down Expand Up @@ -101,8 +105,8 @@ func NewOffsetsListCmd(rootFlags *RootFlags) *cobra.Command {
}

return offsetBucket.ForEach(func(key, value []byte) error {
stdout.Write(append(key, '\n'))
return nil
_, err := stdout.Write(append(key, '\n'))
return err
})
})
if err != nil {
Expand All @@ -115,8 +119,12 @@ func NewOffsetsListCmd(rootFlags *RootFlags) *cobra.Command {
}

func exitOnErr(msg string, err error) {
var sugaredLogger *zap.SugaredLogger
if err != nil {
os.Stderr.WriteString(fmt.Sprintf("%s: %s\n", msg, err))
_, err := os.Stderr.WriteString(fmt.Sprintf("%s: %s\n", msg, err))
if err != nil {
sugaredLogger.Errorw("Failed to write to stdout", zap.Any("error", err))
}
os.Exit(1)
}
}
14 changes: 11 additions & 3 deletions cmd/stanza/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
"net/http"

// This package registers its HTTP endpoints for profiling using an init hook
_ "net/http/pprof"
_ "net/http/pprof" // #nosec
"os"
"runtime"
"runtime/pprof"
Expand Down Expand Up @@ -156,7 +156,11 @@ func startProfiling(ctx context.Context, flags *RootFlags, logger *zap.SugaredLo
if err != nil {
logger.Errorw("Failed to create CPU profile", zap.Error(err))
}
defer f.Close()
defer func() {
if err := f.Close(); err != nil {
logger.Errorf(err.Error())
}
}()

if err := pprof.StartCPUProfile(f); err != nil {
log.Fatal("could not start CPU profile: ", err)
Expand Down Expand Up @@ -185,7 +189,11 @@ func startProfiling(ctx context.Context, flags *RootFlags, logger *zap.SugaredLo
if err != nil {
logger.Errorw("Failed to create memory profile", zap.Error(err))
}
defer f.Close() // error handling omitted for example
defer func() {
if err := f.Close(); err != nil {
logger.Errorw("Failed to close file", zap.Error(err))
}
}()

runtime.GC() // get up-to-date statistics
if err := pprof.WriteHeapProfile(f); err != nil {
Expand Down
4 changes: 2 additions & 2 deletions database/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func OpenDatabase(file string) (Database, error) {

if _, err := os.Stat(filepath.Dir(file)); err != nil {
if os.IsNotExist(err) {
err := os.MkdirAll(filepath.Dir(file), 0755)
err := os.MkdirAll(filepath.Dir(file), 0755) // #nosec - 0755 directory permissions are okay
if err != nil {
return nil, fmt.Errorf("creating database directory: %s", err)
}
Expand All @@ -59,5 +59,5 @@ func OpenDatabase(file string) (Database, error) {
}

options := &bbolt.Options{Timeout: 1 * time.Second}
return bbolt.Open(file, 0666, options)
return bbolt.Open(file, 0600, options)
}
3 changes: 2 additions & 1 deletion operator/buffer/disk.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ func (d *DiskBuffer) Open(path string, sync bool) error {
if sync {
flags |= os.O_SYNC
}
if d.data, err = os.OpenFile(dataPath, flags, 0755); err != nil {
// #nosec - configs load based on user specified directory
if d.data, err = os.OpenFile(dataPath, flags, 0600); err != nil {
return err
}

Expand Down
6 changes: 3 additions & 3 deletions operator/buffer/disk_metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ func OpenMetadata(path string, sync bool) (*Metadata, error) {
if sync {
flags |= os.O_SYNC
}
if m.file, err = os.OpenFile(path, flags, 0755); err != nil {
// #nosec - configs load based on user specified directory
if m.file, err = os.OpenFile(path, flags, 0600); err != nil {
return &Metadata{}, err
}

Expand Down Expand Up @@ -108,8 +109,7 @@ func (m *Metadata) Close() error {
if err != nil {
return err
}
m.file.Close()
return nil
return m.file.Close()
}

// setDeadRange sets the dead range start and length, then persists it to disk
Expand Down
19 changes: 19 additions & 0 deletions operator/buffer/disk_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ func TestDiskBuffer(t *testing.T) {
err = b2.Open(dir, false)
require.NoError(t, err)
readN(t, b2, 20, 0)
err = b2.Close()
require.NoError(t, err)
})

t.Run("Write20Flush10CloseRead20", func(t *testing.T) {
Expand All @@ -160,6 +162,8 @@ func TestDiskBuffer(t *testing.T) {
err = b2.Open(dir, false)
require.NoError(t, err)
readN(t, b2, 10, 10)
err = b2.Close()
require.NoError(t, err)
})

t.Run("ReadWaitTimesOut", func(t *testing.T) {
Expand All @@ -178,6 +182,11 @@ func TestDiskBuffer(t *testing.T) {
b := NewDiskBuffer(100) // Enough space for 1, but not 2 entries
dir := testutil.NewTempDir(t)
err := b.Open(dir, false)
defer func() {
if err := b.Close(); err != nil {
t.Error(err.Error())
}
}()
require.NoError(t, err)

// Add a first entry
Expand Down Expand Up @@ -214,6 +223,11 @@ func TestDiskBuffer(t *testing.T) {
b := NewDiskBuffer(1 << 30)
dir := testutil.NewTempDir(t)
err := b.Open(dir, false)
defer func() {
if err := b.Close(); err != nil {
t.Error(err.Error())
}
}()
require.NoError(t, err)

writes := 0
Expand Down Expand Up @@ -247,6 +261,11 @@ func TestDiskBufferBuild(t *testing.T) {
cfg.Path = testutil.NewTempDir(t)
b, err := cfg.Build(testutil.NewBuildContext(t), "test")
require.NoError(t, err)
defer func() {
if err := b.Close(); err != nil {
t.Error(err.Error())
}
}()
diskBuffer := b.(*DiskBuffer)
require.Equal(t, diskBuffer.atEnd, false)
require.Len(t, diskBuffer.entryAdded, 1)
Expand Down
13 changes: 6 additions & 7 deletions operator/builtin/input/aws/cloudwatch/cloudwatch.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/service/cloudwatchlogs"
"github.com/observiq/stanza/errors"
"github.com/observiq/stanza/operator"
"github.com/observiq/stanza/operator/helper"
)
Expand Down Expand Up @@ -302,14 +301,14 @@ func (c *CloudwatchInput) filterLogEventsInputBuilder(nextToken string) cloudwat
}

// handleEvent is the handler for a AWS Cloudwatch Logs Filtered Event.
func (c *CloudwatchInput) handleEvent(ctx context.Context, event *cloudwatchlogs.FilteredLogEvent) error {
func (c *CloudwatchInput) handleEvent(ctx context.Context, event *cloudwatchlogs.FilteredLogEvent) {
e := map[string]interface{}{
"message": event.Message,
"ingestion_time": event.IngestionTime,
}
entry, err := c.NewEntry(e)
if err != nil {
return errors.Wrap(err, "Failed to create new entry from record")
c.Errorf("Failed to create new entry from record: %s", err)
}

entry.AddResourceKey("log_group", c.logGroupName)
Expand All @@ -327,15 +326,15 @@ func (c *CloudwatchInput) handleEvent(ctx context.Context, event *cloudwatchlogs
c.Debugf("Writing start time %d to database", *event.IngestionTime)
c.persist.Write(c.logGroupName, c.startTime)
}
return nil
}

func (c *CloudwatchInput) handleEvents(ctx context.Context, events []*cloudwatchlogs.FilteredLogEvent) error {
func (c *CloudwatchInput) handleEvents(ctx context.Context, events []*cloudwatchlogs.FilteredLogEvent) {
for _, event := range events {
c.handleEvent(ctx, event)
}
c.persist.DB.Sync()
return nil
if err := c.persist.DB.Sync(); err != nil {
c.Errorf("Failed to sync offset database: %s", err)
}
}

// Returns time.Now() as Unix Time in Milliseconds
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ func TestPersisterLoad(t *testing.T) {
tempDir := testutil.NewTempDir(t)
db, openDbErr := database.OpenDatabase(filepath.Join(tempDir, "test.db"))
require.NoError(t, openDbErr)
defer func() {
if err := db.Close(); err != nil {
t.Error(err.Error())
}
}()
persister := Persister{
DB: helper.NewScopedDBPersister(db, "test"),
}
Expand Down
6 changes: 4 additions & 2 deletions operator/builtin/input/file/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ func (f *InputOperator) makeReaders(filePaths []string) []*Reader {
}
f.SeenPaths[path] = struct{}{}
}
file, err := os.Open(path)
file, err := os.Open(path) // #nosec - operator must read in files defined by user
if err != nil {
f.Errorw("Failed to open file", zap.Error(err))
continue
Expand All @@ -238,7 +238,9 @@ OUTER:
for i := 0; i < len(fps); i++ {
fp := fps[i]
if len(fp.FirstBytes) == 0 {
files[i].Close()
if err := files[i].Close(); err != nil {
f.Errorf("problem closing file", "file", files[i].Name())
}
// Empty file, don't read it until we can compare its fingerprint
fps = append(fps[:i], fps[i+1:]...)
files = append(files[:i], files[i+1:]...)
Expand Down
3 changes: 2 additions & 1 deletion operator/builtin/input/journald/journald.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ func (c JournaldInputConfig) Build(buildContext operator.BuildContext) ([]operat
if cursor != nil {
args = append(args, "--after-cursor", string(cursor))
}
return exec.CommandContext(ctx, "journalctl", args...)
return exec.CommandContext(ctx, "journalctl", args...) // #nosec - ...
// journalctl is an executable that is required for this operator to function
},
json: jsoniter.ConfigFastest,
}
Expand Down
12 changes: 11 additions & 1 deletion operator/builtin/input/tcp/tcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,17 @@ func (t *TCPInput) configureListener() error {
return nil
}

config := tls.Config{Certificates: []tls.Certificate{t.tlsKeyPair}}
// TLS 1.0 is the package default since Go 1.2
// https://golang.org/pkg/crypto/tls/
// An issue has been filed to support modifyingn the minimum version
// https://github.com/observIQ/stanza/issues/349
var tlsVersion uint16 = tls.VersionTLS10

// #nosec - Go defaults to TLS 1.0, and some users may require it
config := tls.Config{
Certificates: []tls.Certificate{t.tlsKeyPair},
MinVersion: tlsVersion,
}
config.Time = func() time.Time { return time.Now() }
config.Rand = rand.Reader

Expand Down
4 changes: 3 additions & 1 deletion operator/builtin/input/udp/udp.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,9 @@ func (u *UDPInput) readMessage() (string, net.Addr, error) {
// Stop will stop listening for udp messages.
func (u *UDPInput) Stop() error {
u.cancel()
u.connection.Close()
if err := u.connection.Close(); err != nil {
u.Errorf("failed to close connection, got error: %s", err)
}
u.wg.Wait()
return nil
}
4 changes: 2 additions & 2 deletions operator/builtin/output/file/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ type FileOutput struct {
// Start will open the output file.
func (fo *FileOutput) Start() error {
var err error
fo.file, err = os.OpenFile(fo.path, os.O_RDWR|os.O_APPEND|os.O_CREATE, 0660)
fo.file, err = os.OpenFile(fo.path, os.O_RDWR|os.O_APPEND|os.O_CREATE, 0600)
if err != nil {
return err
}
Expand All @@ -88,7 +88,7 @@ func (fo *FileOutput) Start() error {
// Stop will close the output file.
func (fo *FileOutput) Stop() error {
if fo.file != nil {
fo.file.Close()
return fo.file.Close()
}
return nil
}
Expand Down
7 changes: 4 additions & 3 deletions operator/builtin/output/forward/forward.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,11 @@ func (f *ForwardOutput) handleResponse(res *http.Response) error {
if err != nil {
return errors.NewError("unexpected status code", "", "status", res.Status)
} else {
res.Body.Close()
if err := res.Body.Close(); err != nil {
f.Errorf(err.Error())
}
return errors.NewError("unexpected status code", "", "status", res.Status, "body", string(body))
}
}
res.Body.Close()
return nil
return res.Body.Close()
}
7 changes: 4 additions & 3 deletions operator/builtin/output/newrelic/newrelic.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,10 +237,11 @@ func (nro *NewRelicOutput) handleResponse(res *http.Response) error {
if err != nil {
return errors.NewError("unexpected status code", "", "status", res.Status)
} else {
res.Body.Close()
if err := res.Body.Close(); err != nil {
nro.Errorf(err.Error())
}
return errors.NewError("unexpected status code", "", "status", res.Status, "body", string(body))
}
}
res.Body.Close()
return nil
return res.Body.Close()
}
7 changes: 4 additions & 3 deletions operator/builtin/output/otlp/otlp.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,11 @@ func (o *OTLPOutput) handleResponse(res *http.Response) error {
if err != nil {
return errors.NewError("non-success status code", "", "status", fmt.Sprint(res.StatusCode))
} else {
res.Body.Close()
if err := res.Body.Close(); err != nil {
o.Errorf(err.Error())
}
return errors.NewError("non-success status code", "", "status", fmt.Sprint(res.StatusCode), "body", string(body))
}
}
res.Body.Close()
return nil
return res.Body.Close()
}
Loading