-
-
Notifications
You must be signed in to change notification settings - Fork 620
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
File perms error for 0644 file perms #107
Comments
Well it really depends on the use case. For example 0644 would not be acceptable if the file contained any sort of credentials. So we have opted for a strict but secure default. FWIW - This is configurable (but not currently documented feature). github.com/GoAstScanner/gas master ✗ 28d ◒ ⍉
▶ ./gas -include=G302 ~/samples/fileperms1_sample.go
Results:
[/Users/gm/samples/fileperms1_sample.go:6] - Expect file permissions to be 0600 or less (Confidence: HIGH, Severity: MEDIUM)
> os.Chmod("/tmp/somefile", 0777)
[/Users/gm/samples/fileperms1_sample.go:7] - Expect file permissions to be 0600 or less (Confidence: HIGH, Severity: MEDIUM)
> os.Chmod("/tmp/someotherfile", 0644)
Summary:
Files: 1
Lines: 8
Nosec: 0
Issues: 2
github.com/GoAstScanner/gas master ✗ 28d ◒ ⍉
▶ cat config.json
{
"G302": "0644"
}
github.com/GoAstScanner/gas master ✗ 28d ◒
▶ ./gas -conf=config.json -include=G302 ~/samples/fileperms1_sample.go
Results:
[/Users/gm/samples/fileperms1_sample.go:6] - Expect file permissions to be 0644 or less (Confidence: HIGH, Severity: MEDIUM)
> os.Chmod("/tmp/somefile", 0777)
Summary:
Files: 1
Lines: 8
Nosec: 0
Issues: 1 |
You're not supposed to put credentials in code, right? Requiring stricter
perms than the OS default seems like overkill. Won't practically every Go
file out there fail this check?
This is used by gometalinter now. Is this intended to be a general-purpose
linter? Maybe that's my disconnect.
Thanks for replying.
…On Tue, Jan 10, 2017 at 5:11 PM Grant Murphy ***@***.***> wrote:
Well it really depends on the use case. For example 0644 would not be
acceptable if the file contained any sort of credentials. So we have opted
for a strict but secure default. FWIW - This is configurable (but not
currently documented feature).
github.com/GoAstScanner/gas master ✗ 28d ◒ ⍉
▶ ./gas -include=G302 ~/samples/fileperms1_sample.go
Results:
[/Users/gm/samples/fileperms1_sample.go:6] - Expect file permissions to be 0600 or less (Confidence: HIGH, Severity: MEDIUM)
> os.Chmod("/tmp/somefile", 0777)
[/Users/gm/samples/fileperms1_sample.go:7] - Expect file permissions to be 0600 or less (Confidence: HIGH, Severity: MEDIUM)
> os.Chmod("/tmp/someotherfile", 0644)
Summary:
Files: 1
Lines: 8
Nosec: 0
Issues: 2
github.com/GoAstScanner/gas master ✗ 28d ◒ ⍉
▶ cat config.json
{
"G302": "0644"
}
github.com/GoAstScanner/gas master ✗ 28d ◒
▶ ./gas -conf=config.json -include=G302 ~/samples/fileperms1_sample.go
Results:
[/Users/gm/samples/fileperms1_sample.go:6] - Expect file permissions to be 0644 or less (Confidence: HIGH, Severity: MEDIUM)
> os.Chmod("/tmp/somefile", 0777)
Summary:
Files: 1
Lines: 8
Nosec: 0
Issues: 1
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#107 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD5VrBGEr_iGYaD3E6e8QbnKsrmlERUks5rRCwogaJpZM4LgDB->
.
|
Sorry. Just to be clear this rule will trigger wherever Go source code is creating a file not on the actual permissions of Go source files within a project. From a security review / code auditing point of view I want to know where files are being created on the filesystems that aren't following principle of least privilege type practices. |
Oh, I misunderstood. Strange, the file it complained about doesn't write any files directly. ...And of course now it won't repro. Closing. Thanks. :) |
Hi @gcmurphy, The principle of least privilege type practices does not justify the error in the lint. The lint cannot know if I am giving or not the least privilege for my use case. Example: I am tooling a process where it would create a file that should/can be edit for anyone. The linter is raising issues when should not and trying to do a check that goes beyond its responsibility since it is unable to analyse the scenario and the business rules to know if the 644 is or nor the least privilege that should be applied, indeed. |
The aim of gosec is to find potential security problems and you could make a similar argument against most of the rules in gosec really as usually there isn't enough context to make a definitive decision. From a security researchers perspective knowing places like this in a code base you may be unfamiliar with is really useful. For this specific scenario if you are working closely with the tool you have a multitude of options:
|
The [gosec linter](https://github.com/securego/gosec) warns [by default on file permissions above 0600](securego/gosec#107). We need the permissions to be 0644 for this line (because it has to be written to), so we exclude it from linting.
The [gosec linter](https://github.com/securego/gosec) warns [by default on file permissions above 0600](securego/gosec#107) We need the permissions to be 0644 for this line (because it has to be written to), so we exclude it from linting.
The [gosec linter][1] warns by default on [file permissions above 0600][2]. We need the permissions to be 0644 for this line (because it has to be written to), so we exclude it from linting. [1]: https://github.com/securego/gosec [2]: securego/gosec#107
* Bump reviewdog golangci-lint action major version This is a necessary precursor to bumping the Go version from 1.21 to 1.22, which will be done in separate PR. * Remove deprecated deadcode linter See golangci/golangci-lint#3125 * Add missing period at end of comment As warned by golangci-lint. * Remove unnecessary trailing whitespace As per golangci-lint warning. * Change case of variable to mixed not snake As per golangci-lint warning. * Fix cuddled statements warned by golangci-lint * Fix return with no blank line before lint warnings * Fix unchecked error return value linting warnings * Exclude line from line length linter warning * Format files with gofumpt to fix linting warnings * Exclude file permissions line from gosec linting The [gosec linter][1] warns by default on [file permissions above 0600][2]. We need the permissions to be 0644 for this line (because it has to be written to), so we exclude it from linting. [1]: https://github.com/securego/gosec [2]: securego/gosec#107 * Remove depguard from Go linting Created #430 to consider reinstating it in the future. * Fix magic number warning by extracting constant * Fix cuddling linter warnings * Add required comment to exported function * Do not use deprecated ioutil package * Remove unused function
I'm getting for a main.go file when run through gometalinter:
The perms are 0644.
Is this saying main.go should have 0 for group and other perms? If so, why is this so stringent? 0644 are the default perms, on macOS at least, and they're perfectly fine.
The text was updated successfully, but these errors were encountered: