Some of the inline code annotation stopped working

[roytman]

Summary

The code annotations in the format:

/* #nosec G306 */
// Avoid nosec "Expect WriteFile permissions to be 0600 or less" error
some code

or annotations without justification stopped working

Annotations with inline justification, work as expected.

// #nosec G306 -- Avoid nosec "Expect WriteFile permissions to be 0600 or less" error
some code

Steps to reproduce the behavior

run gosec with the following code

func main() {
	/* #nosec G306 */
	// Avoid nosec "Expect WriteFile permissions to be 0600 or less" error
	err := ioutil.WriteFile("test", []byte("someData"), 0644)
	if err != nil {}
}

gosec version

Just installed the latest version,

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

Go version (output of 'go version')

go version go1.16.3 linux/amd64

Operating system / Environment

Linux alexey-VB2 5.4.0-91-generic #102-Ubuntu SMP Fri Nov 5 16:31:28 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Expected behavior

Do not print security warnings

Actual behavior

Warnings are printed

[fdelbos]

I can reproduce with go 17.1 and the latest master version of gosec

this will raise an error:

/* #nosec G401 */
hash := md5.New()

this works:

// #nosec G401
hash := md5.New()

[mcauto]

It's occur in v2.9.4.

[Yiwei-Ding]

Apologize to bring such unhappy experience to you. Have submitted a PR #738 to fix this issue.

[strongjz]

Ingress-nginx is still having this issue even with v2.9.5 update.

Locally and in In github CI

Local env

± |main → origin {2} U:1 ✗| → go version
go version go1.17.2 darwin/amd64
± |main → origin {2} U:1 ✗| → uname -a
Darwin strongjz-macbook.local 20.6.0 Darwin Kernel Version 20.6.0: Mon Aug 30 06:12:21 PDT 2021; root:xnu-7195.141.6~3/RELEASE_X86_64 x86_64

Github Action

https://github.com/kubernetes/ingress-nginx/blob/main/.github/workflows/ci.yaml#L48

Error https://github.com/kubernetes/ingress-nginx/runs/4510742592?check_suite_focus=true#step:4:304

[/Users/strongjz/Documents/code/go/src/github.com/kubernetes/ingress-nginx/internal/ingress/controller/nginx.go:445] - G109 (CWE-190): Potential Integer overflow made by strconv.Atoi result conversion to int16/32 (Confidence: MEDIUM, Severity: HIGH)
    444:                        }
  > 445:                        port, err := strconv.Atoi(pb.Port.String()) /* #nosec G109 */
    446:                        if err != nil {



[/Users/strongjz/Documents/code/go/src/github.com/kubernetes/ingress-nginx/internal/ingress/controller/controller.go:1651] - G109 (CWE-190): Potential Integer overflow made by strconv.Atoi result conversion to int16/32 (Confidence: MEDIUM, Severity: HIGH)
    1650: func externalNamePorts(name string, svc *apiv1.Service) *apiv1.ServicePort {
  > 1651:       port, err := strconv.Atoi(name) // #nosec
    1652:       if err != nil {



[/Users/strongjz/Documents/code/go/src/github.com/kubernetes/ingress-nginx/internal/ingress/controller/controller.go:1651] - G109 (CWE-190): Potential Integer overflow made by strconv.Atoi result conversion to int16/32 (Confidence: MEDIUM, Severity: HIGH)
    1650: func externalNamePorts(name string, svc *apiv1.Service) *apiv1.ServicePort {
  > 1651:       port, err := strconv.Atoi(name) // #nosec
    1652:       if err != nil {



[/Users/strongjz/Documents/code/go/src/github.com/kubernetes/ingress-nginx/internal/ingress/controller/controller.go:400] - G109 (CWE-190): Potential Integer overflow made by strconv.Atoi result conversion to int16/32 (Confidence: MEDIUM, Severity: HIGH)
    399:                /* #nosec */
  > 400:                targetPort, err := strconv.Atoi(svcPort) // #nosec
    401:                if err != nil {



Summary:
  Gosec  : 2.9.5
  Files  : 124
  Lines  : 22112
  Nosec  : 14
  Issues : 4

[Yiwei-Ding]

@strongjz @ccojocar It seems the stack gosec.context.Ignores mismatches nodes that gosec visits.. Still in investigation.

[Yiwei-Ding]

Hi @ccojocar @strongjz , I guess the root cause is found!

From the error message "Potential Integer overflow made by strconv.Atoi result conversion to int16/32", we can see that the exact violation is converting the result of strconv.Atoi to int16/32, not strconv.Atoi itself. So when I removed all existing #nosec and added #nosec annotations for all int32(port) in nginx.go and controller.go (where the issues above appeared), the issues were gone.

In the former version of gosec, using #nosec will skip the whole AST that affected and thus the issues of ingress-nginx were totally thrown away. After adding the new feature, tracking suppressions, the AST was not skipped to scan issues. Then the unsuppressed issues jumped out.

You could verify the conclusion from another perspective. If you use #nosec G109 instead of #nosec at the current annotations in ingress-nginx with gosec v2.9.3, you will find the issues are not suppressed. Again, if you use #nosec G109 for all int32(port) with gosec v2.9.3, these issues will be suppressed.

So this should be another issue "G109 tells the location at strconv.Atoi instead of strconv.Atoi result conversion to int16/32".

[Yiwei-Ding]

A code sample could reproduce the G109 location issue:

package main

import (
    "fmt"
    "strconv"
)

func main() {
    a, err := strconv.Atoi("a")
    b := int32(a) // #nosec G109
    fmt.Println(b, err)
}