G306 triggered on executable bit set

[matthewhughes-uw]

Summary

Steps to reproduce the behavior

Given the following in main.go in the current directory:

package main

import (
	"os"
)

func main() {
	err := os.WriteFile(
		"my_script.sh",
		[]byte("#!/bin/sh\n\necho 'hello world'\n"),
		0o500,
	)
	if err != nil {
		panic(err)
	}
}

Run:

$ gosec -quiet ./...
Results:


[/tmp/proj/main.go:8-12] - G306 (CWE-276): Expect WriteFile permissions to be 0600 or less (Confidence: HIGH, Severity: MEDIUM)
    7: func main() {
  > 8: 	err := os.WriteFile(
  > 9: 		"my_script.sh",
  > 10: 		[]byte("#!/bin/sh\n\necho 'hello world'\n"),
  > 11: 		0o500,
  > 12: 	)
    13: 	if err != nil {



Summary:
  Gosec  : dev
  Files  : 1
  Lines  : 17
  Nosec  : 0
  Issues : 1

gosec version

I built from source at 8fa46c1e3efd6ae6beb602dab5411604535ff578

$ gosec -version
Version: dev
Git tag: 
Build date: 

Go version (output of 'go version')

$ go version
go version go1.21.5 linux/amd64

Operating system / Environment

GNU/Linux 6.1.67-1-lts

Expected behavior

When creating a file with the executable bit this rule is not triggered, since this rule maps to CWE-276 which refers to "During installation, installed file permissions are set to allow anyone to modify those files" , however setting this executable bit on a file does not allow for any such modifications.

Though if I've misunderstood the meaning for this rule it might still be worth updating the error message to be more accurate: since 0o500 is less than 0o600 I find the error message confusing. From a quick search I see this was changed in https://github.com/securego/gosec/commit/cf63541008c411ba0b4b182bb2b1c058290d6ddb from a plain less-than check to a bit comparison.

Actual behavior

The rule is triggered, see output above

[ccojocar]

Creating a file with execute permission can lead to a RCE if the attack is able to control the input. This is not the case in the example above since everything seems to be hardcoded.

This rule is meant to be taken as a warning when creating a file with excessive permissions. It is recommended to either create a file with wither owner read-only or read-write file permissions. In this sense less means 0400.

I agree, in this case the rule message sounds confusing since 0500 permissions are less than 0600. I think a better wording might be helpful.

[matthewhughes-uw]

This rule is meant to be taken as a warning when creating a file with excessive permissions. It is recommended to either create a file with wither owner read-only or read-write file permissions. In this sense less means 0400.

:+1: make sense, I was confusing this rule with a strict mapping to CWE-276 (i.e. enforcing only file modification permissions), though the summary "Poor file permissions used when writing to a new file" makes clear its intent (and that it should cover the case of the executable bit too)

I agree, in this case the rule message sounds confusing since 0500 permissions are less than 0600. I think a better wording might be helpful.

Do you have any suggestions for an improvement? The first two ideas that came to my mind:

Expect WriteFile permissions to be a subset of 0600

or:

Expect WriteFile permissions to be provide no more capabilities than 0600

[ccojocar]

Do you have any suggestions for an improvement? The first two ideas that came to my mind:

Expect WriteFile permissions to be a subset of 0600

or:

Expect WriteFile permissions to be provide no more capabilities than 0600

I think it is a bit difficult to have a more expressive wording since the file mode can be configured https://github.com/securego/gosec/blob/8fa46c1e3efd6ae6beb602dab5411604535ff578/rules/fileperms.go#L83.

The former message sounds a bit more accurate to me.