G304: Issue when passing in perm or flag as variable to os.OpenFile() #1383
Conversation
…n and safe Join; fix nil-context panic\n\n- Limit G304 checks to first arg (path) for os.Open/OpenFile/ReadFile, avoiding false positives when flag/perm are variables\n- Track filepath.Clean so cleaned identifiers are treated as safe\n- Consider safe joins: filepath.Join(const|resolvedBase, Clean(var)|cleanedIdent)\n- Record Join(...) assigned to identifiers and allow if later cleaned\n- Fix panic by passing non-nil context in trackJoinAssignStmt\n- All rules tests: 42 passed
|
@ccojocar please review |
ccojocar
changed the title
ccojocar
left a comment
ccojocar
left a comment
There was a problem hiding this comment.
Can you add please the sample code for these use cases in https://github.com/securego/gosec/blob/master/testutils/g304_samples.go?
…\n- Ensure G304 does not fire when only non-path args (flag/perm) are variables\n- Both samples use filepath.Clean on the path arg\n- Rules suite remains green (42 passed)
|
@ccojocar I have added them, please do check. |
rules/readfile.go
Outdated
| // We don't have direct access to the enclosing assignment here (since Match receives call exprs), | ||
| // so the practical approach is: when Match sees a Join call used in an assignment, it should call this helper | ||
| // with the assignment node. For simplicity, we will expect the caller to pass an AssignStmt. | ||
| _ = joinCall // caller should call trackJoin on the AssignStmt (see Match where we call it) |
There was a problem hiding this comment.
Why this empty variable assignment needed here? This doesn't perform any action.
|
It seems that there are some lint issues. Please could you fix them? |
|
On it, I shall get back to you asap. |
|
@ccojocar I applied the changes, please check now. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1383 +/- ##
==========================================
- Coverage 68.49% 63.28% -5.22%
==========================================
Files 75 74 -1
Lines 4384 5267 +883
==========================================
+ Hits 3003 3333 +330
- Misses 1233 1800 +567
+ Partials 148 134 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
What
-Limit G304 to analyze only the path argument (arg[0]) for file APIs (os.Open, os.OpenFile, os.ReadFile, ioutil.ReadFile, os.Create).
-Ignore flag/perm variables to avoid false positives when non-path args are variable.
-Track filepath.Clean to treat cleaned identifiers as safe.
-Recognize safe joins: filepath.Join(const|resolvedBase, Clean(var)|cleanedIdent).
-Record Join(...) assigned to an identifier and consider it safe if that identifier is later cleaned.
-Fix a nil-context panic in trackJoinAssignStmt.
Why
-Prior behavior flagged G304 when perm or flag were variables, even though only the first argument is the file path. This caused false positives like:
-os.OpenFile(filepath.Clean(fn), os.O_RDONLY, perm)
-os.OpenFile(filepath.Clean(fn), flag, 0o600)Also fixed a panic where a nil analysis context was used in a call-list check.
How
-In readfile.Match, inspect only node.Args[0] (path).
-Add trackFilepathClean and isFilepathClean to respect filepath.Clean.
-Add isSafeJoin for filepath.Join(base, Clean(...)) or Join(constBase, cleanedIdent).
-Record Join(...) assignments via trackJoinAssignStmt and allow if the identifier is cleaned later.
Pass non-nil context to ContainsPkgCallExpr in trackJoinAssignStmt.
TestsRan rules suite: 42 passed, 0 failed.
Repro’d prior panic and confirmed it’s fixed.
Verified the three user examples:
-Static perm: OK, no G304.
-Var perm: OK, no G304.
-Var flag: OK, no G304.
-Still flags unsafe filepath.Join with variable path when not cleaned.
Impact
-Reduces false positives on G304 without weakening detection of unsafe path construction.
-Increases stability by removing nil-context panic.
Tests:
-go test ./rules -v
CLI:
-go build -o ./bin/gosec ./cmd/gosec
-./bin/gosec -include=G304 -fmt text -quiet /path/to/your/code
-JSON: ./bin/gosec -include=G304 -fmt json /path/to/your/code | jq .
fixes #1318