Problem: medium shiftleft scan findings (fix #127) #199

leejw51crypto · 2020-10-21T03:23:43Z

Solution: change filemode to 600, check filepath validity, remove defer in file.close

codecov · 2020-10-21T03:27:08Z

Codecov Report

Merging #199 into master will increase coverage by 2.43%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
+ Coverage   26.96%   29.40%   +2.43%     
==========================================
  Files          32       32              
  Lines        5762     5768       +6     
==========================================
+ Hits         1554     1696     +142     
+ Misses       4031     3870     -161     
- Partials      177      202      +25

Flag	Coverage Δ
#integration_tests	`26.94% <0.00%> (-0.03%)`	⬇️
#unit_tests	`27.78% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/chain-maind/app/app.go	`56.59% <0.00%> (-0.95%)`	⬇️
x/chainmain/client/cli/testnet.go	`9.66% <0.00%> (ø)`
x/genutil/client/cli/gentx.go	`21.27% <0.00%> (-0.47%)`	⬇️
x/gov/client/cli/tx.go	`47.68% <0.00%> (+0.66%)`	⬆️
x/staking/client/cli/tx.go	`48.40% <0.00%> (+19.42%)`	⬆️
config/config.go	`66.66% <0.00%> (+20.83%)`	⬆️
x/bank/client/cli/tx.go	`75.58% <0.00%> (+32.55%)`	⬆️
x/staking/client/cli/utils.go	`38.46% <0.00%> (+38.46%)`	⬆️
app/coins.go	`61.76% <0.00%> (+52.94%)`	⬆️
x/gov/client/cli/parse.go	`100.00% <0.00%> (+100.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08d59d9...748b134. Read the comment docs.

devashishdxt · 2020-10-21T04:31:56Z

cmd/chain-maind/app/app.go

-		return json.NewEncoder(file).Encode(&genesis)
+
+		ret := json.NewEncoder(file).Encode(&genesis)
+		file.Close()


Can we remove the earlier file.Close()?

previously, it was defer flie.Close, so closed after function returns.
now it explicitly close file

defer can close the file in case of panic.

in golang, there is no exception,
so this is the same with previous version

in golang, there is no exception,
so this is the same with previous version

golang has panic, and defer can run when that happens.

ok, i'll check

leejw51crypto · 2020-10-21T05:27:35Z

i'll add unit test for the coverage

yihuang · 2020-10-22T00:59:35Z

x/genutil/client/cli/gentx.go

 	if err != nil {
 		return err
 	}
-	defer outputFile.Close()


why remove this defer line, isn't defer is better than close manually?

i think so, too.
but code analysis tool recommends not use defer to file.close in issue #127

https://www.joeshaw.org/dont-defer-close-on-writable-files/
I guess the objection to defer close method is it don't check and properly handle the return value of it, so if we don't handle the error of close method in new style, we don't solve the real issue. and we lost the handling of panic.

ok, i'll modify based upon the link

one other option: securego/gosec#512 (comment)

we can make a extension method file.CloseOrWarn() ? ;D

tomtau

defer for closing files is still OK -- it's a common pattern -- the problematic part is missing the error code -- the easiest way may be just to defer a function that will close the file, but also log any errors (if they happen)
for the path from env, keeping it simple with filepath.Clean

tomtau · 2020-10-22T02:56:55Z

cmd/chain-maind/app/app.go

+		file, err := os.OpenFile(path, os.O_RDWR, 0600)
+		if !chaingenutilcli.IsValidPath(path) {
+			return fmt.Errorf("insecure filepath %s", path)
+		}


For this file path finding, two things:

it's likely a false positive, as the scope of this is very limited -- at least in the case of genutil

this "fix" is a bit problematic:

it's done after opening the file

it does some custom input validation with a deny list

So I suggest keeping this simple and either:

suppress this warning (// nolint: gosec)

or just do a simple path cleanup / canonical path -- filepath.Clean(path)

tomtau · 2020-10-22T03:26:34Z

cmd/chain-maind/app/app.go

+			if err == nil {
+				err = cerr
+			}


and if err is not nil, it'll be lost? I'm not sure of the exact defer semantics, but perhaps logging the error (if cerr is not nil) won't hurt?

I guess assign to err at here don't have any effect.

yes, i changed to display the log

fix lint issue use closure for file.close fix manual file validity check display error in closing file fix lint issue tidy up

…rypto-org-chain#199) Solution: change filemode to 600, cleanup filepath, defer file.close with error logging

leejw51crypto requested review from calvinaco, devashishdxt, linfeng-crypto, tomtau and yihuang as code owners October 21, 2020 03:23

leejw51crypto force-pushed the cro-127 branch from 0032bbc to 51de149 Compare October 21, 2020 03:30

devashishdxt reviewed Oct 21, 2020

View reviewed changes

yihuang reviewed Oct 22, 2020

View reviewed changes

tomtau suggested changes Oct 22, 2020

View reviewed changes

leejw51crypto force-pushed the cro-127 branch from 5c612d5 to 02e11f6 Compare October 22, 2020 03:21

tomtau reviewed Oct 22, 2020

View reviewed changes

leejw51crypto force-pushed the cro-127 branch 2 times, most recently from 2cd75e8 to f3e377b Compare October 22, 2020 03:44

Problem: medium shiftleft scan findings (fix crypto-org-chain#127)

31544ac

fix lint issue use closure for file.close fix manual file validity check display error in closing file fix lint issue tidy up

leejw51crypto force-pushed the cro-127 branch from f3e377b to 31544ac Compare October 22, 2020 04:09

Merge branch 'master' into cro-127

748b134

tomtau approved these changes Oct 22, 2020

View reviewed changes

tomtau merged commit 4784eac into crypto-org-chain:master Oct 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem: medium shiftleft scan findings (fix #127) #199

Problem: medium shiftleft scan findings (fix #127) #199

leejw51crypto commented Oct 21, 2020 •

edited

Loading

codecov bot commented Oct 21, 2020 •

edited

Loading

devashishdxt Oct 21, 2020

leejw51crypto Oct 21, 2020

yihuang Oct 22, 2020

leejw51crypto Oct 22, 2020

yihuang Oct 22, 2020

leejw51crypto Oct 22, 2020

leejw51crypto commented Oct 21, 2020

yihuang Oct 22, 2020

leejw51crypto Oct 22, 2020

yihuang Oct 22, 2020 •

edited

Loading

leejw51crypto Oct 22, 2020

tomtau Oct 22, 2020

yihuang Oct 22, 2020

tomtau left a comment •

edited

Loading

tomtau Oct 22, 2020

tomtau Oct 22, 2020

yihuang Oct 22, 2020 •

edited

Loading

leejw51crypto Oct 22, 2020

Problem: medium shiftleft scan findings (fix #127) #199

Problem: medium shiftleft scan findings (fix #127) #199

Conversation

leejw51crypto commented Oct 21, 2020 • edited Loading

codecov bot commented Oct 21, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto commented Oct 21, 2020 •

edited

Loading

codecov bot commented Oct 21, 2020 •

edited

Loading

yihuang Oct 22, 2020 •

edited

Loading

tomtau left a comment •

edited

Loading

yihuang Oct 22, 2020 •

edited

Loading