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

* Reduce false positives for rule * Make rule more comprehensive * Improve rule accuracy * Narrow rule to just find nil dereferences * add more examples
trailofbits · Oct 13, 2023 · aeb0cc8 · aeb0cc8
1 parent 331b19f
commit aeb0cc8
Show file tree

Hide file tree

Showing 2 changed files with 243 additions and 21 deletions.
diff --git a/go/invalid-usage-of-modified-variable.go b/go/invalid-usage-of-modified-variable.go
@@ -12,6 +12,10 @@ type Engineer struct {
 	Address *Address
 }
 
+func (e *Engineer) String() string {
+    return e.FName + " " + e.LName
+}
+
 type Address struct {
 	City  string
 	State string
@@ -24,40 +28,240 @@ type Log struct {
 
 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, err2 := getEngineerAtIndex(engineers, 5)
+	if err2 != nil {
+		fmt.Printf("Something")
+		fmt.Printf("Unable to obtain engineer with FName %s\n", eng11.FName)
+		fmt.Printf("Something")
+	}
+
+	var eng12 *Engineer
+    // ruleid: invalid-usage-of-modified-variable
+	eng12, err = getEngineerAtIndex(engineers, 5)
+	if err != nil {
+		fmt.Printf("Unable to obtain engineer with FName %s\n", eng12.FName)
+	}
+
+	var eng13 *Engineer
+	// ruleid: invalid-usage-of-modified-variable
+	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")
+	}
+
+	// ruleid: invalid-usage-of-modified-variable
+	eng14, addr1, err := getEngineerAndAddressByIndex(engineers, 5)
+	if err != nil {
+		fmt.Printf("Something")
+		fmt.Printf("Unable to obtain engineer with FName %s\n", eng14.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)
 	}
 
-	eng3 := Engineer{4, "Lee", "Renaldo", 50, nil}
 	// 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")
+	}
+
+	var eng22 *Engineer
+	// ruleid: invalid-usage-of-modified-variable
+	if eng22, err = getEngineerAtIndex(engineers, 6); err != nil {
+		fmt.Printf("Unable to obtain engineer with FName %s\n", eng22.FName)
+	}
+
+	var eng23 *Engineer
+	// ruleid: invalid-usage-of-modified-variable
+	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")
+	}
+
+	var eng24 *Engineer
+	// ruleid: invalid-usage-of-modified-variable
+	if eng24, err = getEngineerAtIndex(engineers, 6); err != nil {
+		fmt.Printf("Something")
+		fmt.Printf("Unable to obtain engineer with FName %s\n", eng24.FName)
+		fmt.Printf("Something")
+		eng24 = &Engineer{0, "N/A", "N/A", 0, nil}
+	}
+
+	// WHOLE VAR AND METHODS TESTS
+	// 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")
+	}
+
+
+	// FALSE POSITIVES
+	eng3 := Engineer{4, "Lee", "Renaldo", 50, nil}
+	// ok: 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
+	// ok: 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")
+	}
+
+	// ok: invalid-usage-of-modified-variable
 	eng4, err := getEngineerAtIndex(engineers, 5)
 	if err != nil {
-		log := Log{
-			Id:      eng4.Id,
-			Message: "Unable to obtain engineer",
-		}
-		fmt.Printf("%#v\n", log)
+		fmt.Printf("Something")
+		fmt.Printf("%#v\n", eng4)
+		fmt.Printf("Something")
+	}
+
+	// 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}
+		fmt.Printf("Unable to obtain engineer %d!\n", eng7.Id)
+	}
+
+	// ok: invalid-usage-of-modified-variable
+	eng71, err := getEngineerAtIndex(engineers, 8)
+	if err != nil {
+		fmt.Printf("Something")
+		eng71 := &Engineer{0, "N/A", "N/A", 0, nil}
+		fmt.Printf("Unable to obtain engineer %d!\n", eng71.Id)
+		fmt.Printf("Something")
+	}
+
+
+	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
-	if eng5, err := getEngineerAtIndex(engineers, 6); err == nil {
-		fmt.Printf("Obtained engineer with FName %s\n", eng5.FName)
+	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}
+	}
+
+	// ok: invalid-usage-of-modified-variable
+	eng92, addr2, err := getEngineerAndAddressByIndex(engineers, 5)
+	if err != nil {
+		fmt.Printf("Something")
+		eng92, err = getEngineerAtIndex(engineers, 6)
+		fmt.Printf("Something")
+	}
+
+    // ok: invalid-usage-of-modified-variable
+	eng93, err := getEngineerAtIndex(engineers, 8)
+    if err != nil {
+        eng93 = &Engineer{0, "N/A", "N/A", 0, nil}
+        eng93 = &Engineer{eng93.Id, "N/A", "N/A", 0, nil}
+    }
+
+
+	// TRUE POSITIVES
+
+	// ruleid: invalid-usage-of-modified-variable
+	eng11, err = getEngineerAtIndex(engineers, 8)
+	if err != nil {
+		eng11.Address = nil
+	}
+
+    // ruleid: invalid-usage-of-modified-variable
+	eng12, err = getEngineerAtIndex(engineers, 8)
+    if err != nil {
+        eng12 = &Engineer{eng12.Id, "N/A", "N/A", 0, nil}
+    }
+
+	// The following test case should match, but I was unable to find a way to
+	// match it without causing some of the false positives to match. This is
+	// largely due to the requirement that "<... $X.$Y ...>" be the last line in
+	// the if-block, i.e. the following causes an invalid pattern error:
+	//  - pattern: |
+	//      $X, err = ...
+	//      if err != nil {
+	//          ...
+	//          <... $X.$Y ...>
+	//          ...
+	//          $X = ...
+	//      }
+
+	// todoruleid: 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)
 	}
 
 	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 71: %s", fmt.Sprintf("%s %s", eng7.FName, eng71.LName))
+	fmt.Printf("Engineer 9: %s", fmt.Sprintf("%s %s", eng9.FName, eng9.LName))
+	fmt.Printf("Engineer 91: %s", fmt.Sprintf("%s %s", eng91.FName, eng91.LName))
+	fmt.Printf("Engineer 92: %s", fmt.Sprintf("%s %s", eng92.FName, eng92.LName))
+	fmt.Printf("Address 1: %v", addr1)
+	fmt.Printf("Address 2: %v", addr2)
 }
 
 func getEngineerAtIndex(slice []Engineer, idx int) (*Engineer, error) {
@@ -74,6 +278,13 @@ func getEngineerAddressByIndex(slice []Engineer, idx int) (*Address, error) {
 	return slice[idx].Address, nil
 }
 
+func getEngineerAndAddressByIndex(slice []Engineer, idx int) (*Engineer, *Address, error) {
+	if idx >= len(slice) {
+		return nil, nil, fmt.Errorf("invalid index")
+	}
+	return &slice[idx], slice[idx].Address, nil
+}
+
 func getEngineers() []Engineer {
 	return []Engineer{
 		{

diff --git a/go/invalid-usage-of-modified-variable.yaml b/go/invalid-usage-of-modified-variable.yaml
@@ -18,16 +18,27 @@ rules:
       - https://blog.trailofbits.com/2019/11/07/attacking-go-vr-ttps/
 
   patterns:
-    - pattern-either:
-        - pattern: |
-            $X, err = ...
-            if err != nil {
-              <... $X ...>
-            }
-        - pattern: |
-            $X, err := ...
-            if err != nil {
-              ...
-              <... $X.$Y ...>
-            }
+    - pattern: |
+        ..., $X, ..., $ERR = ...
+        if $ERR != nil {
+          ...
+          <... $X.$Y ...>
+        }
+
+    - pattern-not: |
+        ..., $X, ..., $ERR = ...
+        if $ERR != nil {
+          ...
+          $X, ... = ...
+          ...
+          <... $X.$Y ...>
+        }
+    - pattern-not: |
+        ..., $X, ..., $ERR = ...
+        if $ERR != nil {
+          ...
+          $X = ...
+          ...
+          <... $X.$Y ...>
+        }