In this blog series, we will share some findings from running iCR on open source code.

We ran iCR on Jenkins version jenkins-2.264 with the last commit hash of 4ff3e8d. This version had 1634 Java files and about 285 KLoC.

We will be giving examples of some bugs that iCR identified.

 

Null Pointer Dereference:

One of the possible null pointer dereference cases we found was in the Util.java file in the rawEncode method which has been used in many places in the program. We found 29 references to this method in the codebase. The rawEncode method takes a String parameter as input, creates a StringBuilder by encoding the information, converts the StringBuilder back to String, and returns it.

There is a @NonNull annotation for the String parameter that is passed to the rawEncode method. However, the String parameter may be an empty string. In that case, the StringBuilder that was created locally to encode (out) will remain null and then it will be referenced:

return escaped ? out.toString() : s;

This presents a null pointer exception opportunity if the method is not called properly. iCR provides a trace of where the variable is used before it is being referenced. In this case, it suggests that String.valueOf() should be used in place of toString() method since that can handle null parameters. The diff prepared by iCR is shown here.

--- /Users/munawar/Desktop/OR_PLUGIN_CMDLINE_JAVA/source/jenkins/core /src/main/java/hudson/Util.java
+++ /Users/munawar/Desktop/OR_PLUGIN_CMDLINE_JAVA/source/jenkins/core /src/main/java/hudson/Util.java
@@ -930,7 +930,26 @@
                 }
             }
         }
-        return escaped ? out.toString() : s;
+        /* ********OpenRefactory Warning********
+           Possible null pointer Dereference!
+           Path:
+               File: Util.java, Line: 880
+                   StringBuilder out=null;
+                   Variable out is initialized null.
+               File: Util.java, Line: 890
+                   out=new StringBuilder(i + (m - i) * 3);
+                   Variable out is allocated.
+               File: Util.java, Line: 916
+                   out=new StringBuilder(i + (m - i) * 3);
+                   Variable out is allocated.
+               File: Util.java, Line: 933
+                   return escaped ? out.toString() : s;
+                   out is referenced in method invocation.
+           Fix:
+               out is identified as null.
+               iCR fixes by using String.valueOf() method instead.
+         */
+         return escaped ? String.valueOf(out) : s;
     }
     private static char toDigit(int n) {

Path Manipulation Problem:

In file ClassicPluginStrategy.java, method resolve, there is a potential path traversal problem. A path is passed from outside in the parameter named relative. This path is used to create a file. But the path can be controlled by an attacker.

iCR suggests the following fix.

--- /Users/munawar/Desktop/OR_PLUGIN_CMDLINE_JAVA/source/jenkins/core
/src/main/java/hudson/ClassicPluginStrategy.java
+++ /Users/munawar/Desktop/OR_PLUGIN_CMDLINE_JAVA/source/jenkins/core
/src/main/java/hudson/ClassicPluginStrategy.java
@@ -64,6 +64,7 @@
 import java.net.URL;
 import java.nio.file.Files;
 import java.nio.file.InvalidPathException;
+import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -435,7 +436,13 @@     @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "Administrator action installing a plugin, which could do far worse.")
     private static File resolve(File base, String relative) {
-        File rel = new File(relative);
+        relative = Paths.get(relative).normalize().toString();
+        /* ********OpenRefactory Warning********
+           Possible Path manipulation attack!
+           Fix:
+               iCR sanitizes the tainted input.
+        */
+        File rel = new File(relative);
         if(rel.isAbsolute())
             return rel;
         else

This bug was also reported by FindBugs, which Jenkins developers use. There is an annotation for the method that suppresses the bug report:

@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification =
"Administrator action installing a plugin, which could do far worse.")

If the sanity check that is suggested by iCR is added, it should be making the method more secure. Besides, it appears that the method resolve is called once, but its caller (method loadLinkedManifest) is called from different contexts. Therefore, it should be fixed.

 

Error in return value:
iCR identified a case in FullDuplexHttpServiceTest in which there is a call to a read on an InputStream, and then using the return value in a write call. Here is the suggested fix.

--- /Users/munawar/Desktop/OR_PLUGIN_CMDLINE_JAVA/source/jenkins/test
/src/test/java/jenkins/util/FullDuplexHttpServiceTest.java
+++ /Users/munawar/Desktop/OR_PLUGIN_CMDLINE_JAVA/source/jenkins/test
/src/test/java/jenkins/util/FullDuplexHttpServiceTest.java
@@ -83,6 +83,9 @@
         @Override
         protected void run(InputStream upload, OutputStream download) throws IOException, InterruptedException {
             int x = upload.read();
+            // OR Warning: Handle Error Code
+            if (x == -1) {
+            }
             download.write(x * 2);
         }
     };

The read method call may return a negative number as an error message. This should probably be handled before it is used in the write call.

 

Improper Way to write finalize method:

In AtomicFileWriter.java file, there is an implementation of the finalize method inside the AtomicFileWriter class. Java secure coding practices recommend that finalizers should not be used [MET12-J].

There are several reasons for this:

• There is no fixed time at which finalizers must be executed because time of execution depends on the Java Virtual Machine (JVM). The only guarantee is that any finalizer method that executes will do so sometime after the associated object has become unreachable and sometime before the garbage collector reclaims the associated object’s storage. Often times it is not executed, so if someone is attempting to clean up resources (files, sockets, etc.,) inside finalize, it may never be executed.
• The finalize method may throw an exception while performing its task. This exception may be ignored if not caught.
• Finalizers increase garbage-collection time and introduce space overheads.
• Use of locks or other synchronization-based mechanisms within a finalizer can cause deadlock or starvation.

Any subclass that overrides finalize() must explicitly invoke the method for its superclass as well. There is no automatic chaining with finalize. The correct way to handle it is as follows:

  try {
      //...
  } finally {
      super.finalize();
  }

This is suggested by the iCR fix for one case found in Jenkins.

--- /Users/munawar/Desktop/OR_PLUGIN_CMDLINE_JAVA/source/jenkins/core
/src/main/java/hudson/util/AtomicFileWriter.java
+++ /Users/munawar/Desktop/OR_PLUGIN_CMDLINE_JAVA/source/jenkins/core
/src/main/java/hudson/util/AtomicFileWriter.java
@@ -223,8 +223,15 @@     @Override
     protected void finalize() throws Throwable {
-        closeAndDeleteTempFile();
-    }
+        /* ********OpenRefactory Warning********
+           Overriding the finalize method is problematic. Avoid if possible
+        */
+        try {
+            closeAndDeleteTempFile();
+        } finally {
+            super.finalize();
+        }
+    }     
     private void closeAndDeleteTempFile() throws IOException {
         // one way or the other, temporary file should be deleted.

It calls the finalize method of the Writer following the correct coding principles