-
Notifications
You must be signed in to change notification settings - Fork 992
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
Prevent Usage of Stdlib File/Dir Writing With Static Analysis #7685
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7685 +/- ##
==========================================
- Coverage 62.18% 62.13% -0.05%
==========================================
Files 429 429
Lines 30205 30248 +43
==========================================
+ Hits 18782 18794 +12
- Misses 8513 8530 +17
- Partials 2910 2924 +14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add more tests.
if err := fileutil.MkdirAll(dirPath); err != nil { | ||
return nil, err | ||
} | ||
} | ||
datafile := path.Join(dirPath, databaseFileName) | ||
boltDB, err := bolt.Open(datafile, params.BeaconIoConfig().ReadWritePermissions, &bolt.Options{Timeout: 1 * time.Second, InitialMmapSize: 10e6}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the directory already exists and contains a database? Shouldn't this be an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it already exists that means you already have a database and we should open it normally to start your beacon node
hasDir, err := fileutil.HasDir(dirPath) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if !hasDir { | ||
if err := fileutil.MkdirAll(dirPath); err != nil { | ||
return nil, err | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please write some tests?
hasDir, err := fileutil.HasDir(dirPath) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if !hasDir { | ||
if err := fileutil.MkdirAll(dirPath); err != nil { | ||
return nil, err | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please write some tests?
case *ast.CallExpr: | ||
// Check if any of disallowed functions have been used. | ||
for pkg, path := range aliases { | ||
for _, fn := range disallowedFns { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not that big of a deal, but since you have a nested loop you check MkdirAll
and WriteFile
for both packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is ok because it's better if it's extensible. In the future we can easily add more functions we want to check against near the top of the file
What type of PR is this?
What does this PR do? Why is it needed?
This PR prevents usage of ioutil.WriteFile and os.MkdirAll in our repository, as those are unsafe when it comes to checking permissions. For example, an attacker could create a directory
p
with 777 permissions, andos.MkdirAll(p)
will fail silently, without overriding the permissions of the attacker. Instead, we enforce Prysm to use our ownshared/fileutil
package that properly checks for permissions errors compared to the Go standard library. We enforce this using a static analyzer that preventsos.MkdirAll
andioutil.WriteFile
.Which issues(s) does this PR fix?
Part of #7410 and fixes #7410