Reduce false positives for invalid-usage-of-modified-variable #33

risto-liftoff · 2023-07-10T23:45:38Z

Hi! When I ran this rule against my code, it generated a finding that I could tell was a false positive. Essentially, it was flagging this as an issue:

eng7, err := getEngineerAtIndex(engineers, 8)
if err != nil {
	eng7 = &Engineer{0, "N/A", "N/A", 0, nil}
}

which shouldn't be problematic since the only use of eng7 is assigning a new value.

CLAassistant · 2023-07-10T23:47:04Z

All committers have signed the CLA.

GrosQuildu · 2023-07-11T08:05:11Z

Thanks for the submission! Interesting false positive you have found. Looking again at the rule, it seems that we not only have the false positive, but also the original patterns are not comprehensive enough.

Wrote the following test that should catch more cases, including yours. Please let me know if the new test cases are valid. If so, we would greatly appreciate fixes that will make the test pass. Otherwise, we can merge the PR as it is and save outstanding not-passing tests in our TODO list.

func (e *Engineer) String() string {
    return e.FName + " " + e.LName
}

func main() {
	engineers := getEngineers()

	// BASIC TESTS
	// ruleid: invalid-usage-of-modified-variable
	eng1, err := getEngineerAtIndex(engineers, 5)
	if err != nil {
		fmt.Printf("Unable to obtain engineer with FName %s\n", eng1.FName)
	}

	// ruleid: invalid-usage-of-modified-variable
	eng11, err := getEngineerAtIndex(engineers, 5)
	if err != nil {
		fmt.Printf("Something")
		fmt.Printf("Unable to obtain engineer with FName %s\n", eng11.FName)
		fmt.Printf("Something")
	}

	// ruleid: invalid-usage-of-modified-variable
	var eng12 *Engineer
	eng12, err = getEngineerAtIndex(engineers, 5)
	if err != nil {
		fmt.Printf("Unable to obtain engineer with FName %s\n", eng12.FName)
	}

	// ruleid: invalid-usage-of-modified-variable
	var eng13 *Engineer
	eng13, err = getEngineerAtIndex(engineers, 5)
	if err != nil {
		fmt.Printf("Something")
		fmt.Printf("Unable to obtain engineer with FName %s\n", eng13.FName)
		fmt.Printf("Something")
	}

	// BASIC TESTS - inline if
	// ruleid: invalid-usage-of-modified-variable
	if eng2, err := getEngineerAtIndex(engineers, 6); err != nil {
		fmt.Printf("Unable to obtain engineer with FName %s\n", eng2.FName)
	}

	// ruleid: invalid-usage-of-modified-variable
	if eng21, err := getEngineerAtIndex(engineers, 6); err != nil {
		fmt.Printf("Something")
		fmt.Printf("Unable to obtain engineer with FName %s\n", eng21.FName)
		fmt.Printf("Something")
	}

	// ruleid: invalid-usage-of-modified-variable
	var eng22 *Engineer
	if eng22, err = getEngineerAtIndex(engineers, 6); err != nil {
		fmt.Printf("Unable to obtain engineer with FName %s\n", eng22.FName)
	}

	// ruleid: invalid-usage-of-modified-variable
	var eng23 *Engineer
	if eng23, err = getEngineerAtIndex(engineers, 6); err != nil {
		fmt.Printf("Something")
		fmt.Printf("Unable to obtain engineer with FName %s\n", eng23.FName)
		fmt.Printf("Something")
	}

	// WHOLE VAR AND METHODS TESTS
	// ruleid: invalid-usage-of-modified-variable
	eng4, err := getEngineerAtIndex(engineers, 5)
	if err != nil {
		fmt.Printf("Something")
		fmt.Printf("%#v\n", eng4)
		fmt.Printf("Something")
	}

	// ruleid: invalid-usage-of-modified-variable
	eng5, err := getEngineerAtIndex(engineers, 5)
	if err != nil {
		fmt.Printf("Something")
		fmt.Printf("%s\n", eng5.String())
		fmt.Printf("Something")
	}

	// FIELD ASSIGN TESTS
	eng3 := Engineer{4, "Lee", "Renaldo", 50, nil}
	// ruleid: invalid-usage-of-modified-variable
	eng3.Address, err = getEngineerAddressByIndex(engineers, 1)
	if err != nil {
		fmt.Printf("Unable to obtain address %#v!\n", eng3.Address)
	}

	// ruleid: invalid-usage-of-modified-variable
	eng3.Address, err = getEngineerAddressByIndex(engineers, 1)
	if err != nil {
		fmt.Printf("Something")
		fmt.Printf("Unable to obtain address %#v!\n", eng3.Address)
		fmt.Printf("Something")
	}

	// FALSE POSITIVES
	// ok: invalid-usage-of-modified-variable
	eng6, err := getEngineerAtIndex(engineers, 7)
	if err != nil {
		eng6 = &Engineer{0, "N/A", "N/A", 0, nil}
		fmt.Printf("Unable to obtain engineer %d!\n", eng6.Id)
	}

	// ok: invalid-usage-of-modified-variable
	eng61, err := getEngineerAtIndex(engineers, 7)
	if err != nil {
		fmt.Printf("Something")
		eng61 = &Engineer{0, "N/A", "N/A", 0, nil}
		fmt.Printf("Unable to obtain engineer %d!\n", eng61.Id)
		fmt.Printf("Something")
	}

	// ok: invalid-usage-of-modified-variable
	eng7, err := getEngineerAtIndex(engineers, 8)
	if err != nil {
		eng7 = &Engineer{0, "N/A", "N/A", 0, nil}
	}

	var eng8 *Engineer
	// ok: invalid-usage-of-modified-variable
	eng8, err = getEngineerAtIndex(engineers, 7)
	if err != nil {
		eng8 = &Engineer{0, "N/A", "N/A", 0, nil}
		fmt.Printf("Unable to obtain engineer %d!\n", eng8.Id)
	}

	var eng81 *Engineer
	// ok: invalid-usage-of-modified-variable
	eng81, err = getEngineerAtIndex(engineers, 7)
	if err != nil {
		fmt.Printf("Something")
		eng81 = &Engineer{0, "N/A", "N/A", 0, nil}
		fmt.Printf("Unable to obtain engineer %d!\n", eng81.Id)
		fmt.Printf("Something")
	}

	// ok: invalid-usage-of-modified-variable
	eng9, err := getEngineerAtIndex(engineers, 8)
	if err != nil {
		eng9 = &Engineer{0, "N/A", "N/A", 0, nil}
	}

	var eng91 *Engineer
	// ok: invalid-usage-of-modified-variable
	eng91, err = getEngineerAtIndex(engineers, 8)
	if err != nil {
		eng91 = &Engineer{0, "N/A", "N/A", 0, nil}
	}

	// TRUE POSITIVES
	// ruleid: invalid-usage-of-modified-variable
	eng10, err := getEngineerAtIndex(engineers, 10)
	if err != nil {
		fmt.Printf("Something")
		fmt.Printf("Unable to obtain engineer %d!\n", eng10.Id)
		eng10 = &Engineer{0, "N/A", "N/A", 0, nil}
		fmt.Printf("Unable to obtain engineer %d!\n", eng10.Id)
	}

	var eng101 *Engineer
	// ruleid: invalid-usage-of-modified-variable
	eng101, err = getEngineerAtIndex(engineers, 10)
	if err != nil {
		fmt.Printf("Something")
		fmt.Printf("Unable to obtain engineer %d!\n", eng101.Id)
		eng101 = &Engineer{0, "N/A", "N/A", 0, nil}
		fmt.Printf("Unable to obtain engineer %d!\n", eng101.Id)
	}

	// ruleid: invalid-usage-of-modified-variable
	eng11, err = getEngineerAtIndex(engineers, 8)
	if err != nil {
		eng11.Address = nil
	}

	fmt.Printf("Engineer 1: %s", fmt.Sprintf("%s %s", eng1.FName, eng1.LName))
	fmt.Printf("Engineer 7: %s", fmt.Sprintf("%s %s", eng7.FName, eng7.LName))
	fmt.Printf("Engineer 7: %s", fmt.Sprintf("%s %s", eng9.FName, eng9.LName))
	fmt.Printf("Engineer 7: %s", fmt.Sprintf("%s %s", eng91.FName, eng91.LName))
}

risto-liftoff · 2023-07-11T15:29:39Z

Those test cases all look valid to me. Would you like me to update the PR with a pattern that will pass all these test cases?

GrosQuildu · 2023-07-12T09:25:06Z

Sure, would be great if you could do that. Thanks.

risto-liftoff · 2023-07-12T19:08:09Z

I wasn't able to come up with a pattern that perfectly matched the test cases due to what seems to be a limitation in the semgrep rule syntax. I included a thorough comment in the test cases file explaining why I wasn't able to write a pattern to catch 3 of the test cases.

risto-liftoff · 2023-07-13T16:08:52Z

I found another instance in our code that I would consider a false positive:

eng, err := getEngineerAtIndex(slice, idx)
if err != nil {
	return eng, err
}

I've updated the rule to exclude this pattern.

GrosQuildu · 2023-07-14T10:26:26Z

Hm, I would consider that example a valid finding - if a method returned error and variable, then the variable is probably not meant to be used. It should be reviewed manually to confirm that returning variable instead of nil is fine.

risto-liftoff · 2023-07-14T14:57:08Z

All right, I've removed my last commit.

GrosQuildu · 2023-07-17T10:12:11Z

Thanks, we are almost good to merge. Only signing the CLA is left, please do it when you have time. https://cla-assistant.io/trailofbits/semgrep-rules?pullRequest=33

risto-liftoff · 2023-07-17T15:57:55Z

Yep, I'll sign the CLA once my legal department gives me the thumbs up to do so.

0xDC0DE · 2023-07-18T07:35:09Z

Hi!

Could you change the hardcoded variable name for err in the rule to a metavariable? e.g. $ERR .
While it is convention to call this variable err, it can still have a different name, and the current version of the rule would not flag those cases.

GrosQuildu · 2023-07-18T08:11:56Z

Good catch. Actually we could try to support more than 2 return values, if that is possible with Semgrep.

go/invalid-usage-of-modified-variable.yaml

risto-liftoff · 2023-07-19T18:23:02Z

All right, I've made the requested changes:

Use $ERR instead of err
Support more than 2 return values

hex0punk · 2023-08-07T22:52:35Z

IMO the real issue that this rule is looking for should be potential nil derefences. Otherwise, most of the results from this rule will be FPs. Unless you could use metavariable-type and specify whitelisted types that cannot have nil such as Int and String (not possible at the moment), then I'd argue that is best to only look for cases where a likely nil object is deferenced. So I suggest to ToB also updating this:

    - pattern: |
        ..., $X, ..., $ERR = ...
        if $ERR != nil {
          ...
          <... $X ...>
        }

to

    - pattern: |
        ..., $X, ..., $ERR = ...
        if $ERR != nil {
          ...
          <... $X.$Y ...>
        }

This would greatly reduce FPs and give you likely cases where a nil dereference could occur.

risto-liftoff · 2023-08-08T15:36:15Z

    - pattern: |
        ..., $X, ..., $ERR = ...
        if $ERR != nil {
          ...
          <... $X ...>
        }

to

    - pattern: |
        ..., $X, ..., $ERR = ...
        if $ERR != nil {
          ...
          <... $X.$Y ...>
        }

I would favor making this change. When I run this rule as this PR currently has it against our codebase, it returns 97 matches, most of which are false positives (they wouldn't cause a panic). With the change suggested by hex0punk, it returns 3 matches, all of which are true positives.

If ToB agrees this would be a good change, I'll update the PR.

GrosQuildu · 2023-08-10T10:31:32Z

Lets make the change. While I think that the more noisy rule could help finding business logic bugs, reducing false positives rate is important for automated tooling.

After change, the rule should find cases where assignment to variable (current pattern-not cases) follows the nil dereference call.

hex0punk · 2023-08-17T16:26:49Z

just checking on the status of this. Is the only thing pending for @risto-liftoff to sign a CLA?

risto-liftoff · 2023-08-17T16:50:14Z

just checking on the status of this. Is the only thing pending for @risto-liftoff to sign a CLA?

Yep, still waiting on my legal department to approve the CLA. Once that's approved, I believe I'll also need an approval for the PR by a repo maintainer.

GrosQuildu · 2023-10-04T09:25:36Z

Hey, any updated on the CLA?

risto-liftoff · 2023-10-04T17:44:00Z

Yes, I finally got word back that I am allowed to sign the CLA.

…m:risto-liftoff/semgrep-rules into invalid-usage-of-modified-variable-fp-fix

GrosQuildu · 2023-10-13T15:50:31Z

Thanks a lot for the contribution! Really appreciated.

For a note: there is one true positive type we are currently missing - will try to work on it in the future.

Reduce false positives for rule

74c76ae

risto-liftoff requested review from GrosQuildu and Vasco-jofra as code owners July 10, 2023 23:45

Make rule more comprehensive

96893ac

risto-liftoff force-pushed the invalid-usage-of-modified-variable-fp-fix branch from 915d140 to 96893ac Compare July 14, 2023 14:56

Merge branch 'main' into invalid-usage-of-modified-variable-fp-fix

40967dd

GrosQuildu previously approved these changes Jul 17, 2023

View reviewed changes

0xDC0DE reviewed Jul 18, 2023

View reviewed changes

go/invalid-usage-of-modified-variable.yaml Outdated Show resolved Hide resolved

Improve rule accuracy

1be8bb3

risto-liftoff dismissed GrosQuildu’s stale review via 1be8bb3 July 19, 2023 18:21

Narrow rule to just find nil dereferences

db280ad

risto-liftoff and others added 4 commits October 4, 2023 10:44

Merge branch 'main' into invalid-usage-of-modified-variable-fp-fix

e2bd7a5

Merge branch 'main' into invalid-usage-of-modified-variable-fp-fix

990ddc0

add more examples

c77af69

Merge branch 'invalid-usage-of-modified-variable-fp-fix' of github.co…

df64c97

…m:risto-liftoff/semgrep-rules into invalid-usage-of-modified-variable-fp-fix

GrosQuildu self-requested a review October 13, 2023 15:48

GrosQuildu approved these changes Oct 13, 2023

View reviewed changes

GrosQuildu merged commit aeb0cc8 into trailofbits:main Oct 13, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce false positives for invalid-usage-of-modified-variable #33

Reduce false positives for invalid-usage-of-modified-variable #33

risto-liftoff commented Jul 10, 2023

CLAassistant commented Jul 10, 2023 •

edited

Loading

GrosQuildu commented Jul 11, 2023 •

edited

Loading

risto-liftoff commented Jul 11, 2023

GrosQuildu commented Jul 12, 2023

risto-liftoff commented Jul 12, 2023

risto-liftoff commented Jul 13, 2023

GrosQuildu commented Jul 14, 2023

risto-liftoff commented Jul 14, 2023

GrosQuildu commented Jul 17, 2023

risto-liftoff commented Jul 17, 2023

0xDC0DE commented Jul 18, 2023

GrosQuildu commented Jul 18, 2023

risto-liftoff commented Jul 19, 2023

hex0punk commented Aug 7, 2023 •

edited

Loading

risto-liftoff commented Aug 8, 2023

GrosQuildu commented Aug 10, 2023

hex0punk commented Aug 17, 2023

risto-liftoff commented Aug 17, 2023

GrosQuildu commented Oct 4, 2023

risto-liftoff commented Oct 4, 2023

GrosQuildu commented Oct 13, 2023

Reduce false positives for invalid-usage-of-modified-variable #33

Reduce false positives for invalid-usage-of-modified-variable #33

Conversation

risto-liftoff commented Jul 10, 2023

CLAassistant commented Jul 10, 2023 • edited Loading

GrosQuildu commented Jul 11, 2023 • edited Loading

risto-liftoff commented Jul 11, 2023

GrosQuildu commented Jul 12, 2023

risto-liftoff commented Jul 12, 2023

risto-liftoff commented Jul 13, 2023

GrosQuildu commented Jul 14, 2023

risto-liftoff commented Jul 14, 2023

GrosQuildu commented Jul 17, 2023

risto-liftoff commented Jul 17, 2023

0xDC0DE commented Jul 18, 2023

GrosQuildu commented Jul 18, 2023

risto-liftoff commented Jul 19, 2023

hex0punk commented Aug 7, 2023 • edited Loading

risto-liftoff commented Aug 8, 2023

GrosQuildu commented Aug 10, 2023

hex0punk commented Aug 17, 2023

risto-liftoff commented Aug 17, 2023

GrosQuildu commented Oct 4, 2023

risto-liftoff commented Oct 4, 2023

GrosQuildu commented Oct 13, 2023

CLAassistant commented Jul 10, 2023 •

edited

Loading

GrosQuildu commented Jul 11, 2023 •

edited

Loading

hex0punk commented Aug 7, 2023 •

edited

Loading