Do java's Inner classes pose a security risk?

Question

Recently the security team on my project released a secure code guidelines document, designed to be used as part of our code reviews. The first thing that struck me was an item that said "Do not use Inner classes". I thought this seemed like a very heavy handed and sweeping statement. Inner classes are good if used correctly right?, but i did a bit of googling and found this, quoted here for convenience.

Rule 5: Don't Use Inner Classes

Some Java language books say that inner classes can only be accessed by the outer classes that enclose them. This is not true. Java byte code has no concept of inner classes, so inner classes are translated by the compiler into ordinary classes that happen to be accessible to any code in the same package. And Rule 4 says not to depend on package scope for protection.

But wait, it gets worse. An inner class gets access to the fields of the enclosing outer class, even if these fields are declared private. And the inner class is translated into a separate class. In order to allow this separate class access to the fields of the outer class, the compiler silently changes these fields from private to package scope! It's bad enough that the inner class is exposed, but it's even worse that the compiler is silently overruling your decision to make some fields private. Don't use inner classes if you can help it. (Ironically, the new Java 2 doPrivileged() API usage guidelines suggest that you use an inner class to write privileged code. That's one reason we don't like the doPrivileged() API.)

My questions are

1. Does this behaviour still exist in java 5 / 6?
2. Is this actually a security risk, given that any class, other than the outer and inner classes, that tried to access the outer class' private members would not compile?
3. Does it pose enough of a security risk to warant the 'guideline' 'Do not use inner classes'?

Answer 1

1. Yes, this behavior still exists.
2. It is a security risk because the rogue class could be crafted with something else than the standard javac.
3. It depends of how much paranoid you are :) If you don't allow alien classes to run in your JVM, I don't see the problem though. And if you do, you have bigger problems (sandboxes and all)
4. I know you only had 3 questions, but like other people here, I think this is a stupid restriction.

Answer 2

The idea of this kind of security in code is kind of silly. If you want code level security, use an obfuscation tool. Like @skaffman said in the comments above, "Code visibility has never been a security feature. Even private members can be accessed using reflection.".

If you are distributing your compiled code and not obfuscating it, then using an inner class is your last worry if you are worried about people tinkering with your privates.

If you are hosting your code, then why are you worried about someone poking around your inner classes?

If you going to linking some 3rd party code you don't trust and can't check at run time, then sandbox it.

Like I said above, if this is really a policy at your company, please promptly report your company to thedailywtf.com

Answer 3

You should consider what kind of security your application has to provide. An application with a secure architecture won't run into these named issues.

If there is something an user is not allowed to do with your code, you have to seperate this functionality and run it on a server (where the user has no access to the class files).

Remember that you can always decompile java class files. And don't rely on "security by obscurity". Even obfuscated code can be analyzed, understood and modified.

Answer 4

Malicious code can use java reflection to get to any piece of information in the JVM unless a security manager is in place which prohibits this, this includes changing private fields to public and much more.

My personal opinion is that the reasons not to, are overwhelmed by the other possibilities, so if you need it, it makes sense, and it is readable, use inner classes.

Answer 5

Note that the drawbacks listed do not hold for static inner classes as they do not have implicit access to their enclosing class (or object really.)

So if this rule is going to help up in your company, it might be an idea to get static inner classes excempted as they offer a way for encapsulation which is useful in many cases.

@Tom, quoting the Java language specification, "Member classes may be static, in which case they have no access to the instance variables of the surrounding class"

Answer 6

This information is a around a decade out of date. The widespread use of anonymous inner classes with AccessController.doPrivileged should be a clue. (If you don't like the API, consider the proportion of try-finally blocks that are incorrectly missing in the JDK.)

The policy is that no two class can share the same package if they are loaded by different class loaders or have different certificates. For more protection, mark packages as sealed in the manifest of your jars. So, from a security standpoint, "Rule 4" is bogus and hence also this rule.

In any case, working out security policies you should understand what you are protecting against. These sorts of policies are for handling mobile code (code that moves) that may have different levels of trust. Unless you are handling mobile code, or your code is going into a library that may be required to, there is very little point in these sorts of precautions. However, it is almost always a good idea to use a robust programming style, for instance copying and validating arguments and return values.

Answer 7

Does this behaviour still exist in java 5 / 6?

You can use the javap tool to determine what your binaries are exposing and how.

package demo;
public class SyntheticAccessors {
  private boolean encapsulatedThing;

  class Inner {
    void doSomething() {
      encapsulatedThing = true;
    }
  }
}

The above code (compiled with Sun Java 6 javac) creates these methods in SyntheticAccessors.class:

Compiled from "SyntheticAccessors.java"
public class demo.SyntheticAccessors extends java.lang.Object{
    public demo.SyntheticAccessors();
    static void access$0(demo.SyntheticAccessors, boolean);
}

Note the new access$0 method.

Answer 8

Does this behaviour still exist in java 5 / 6?

Not exactly as described; I've never seen a compiler where this was true:

In order to allow this separate class access to the fields of the outer class, the compiler silently changes these fields from private to package scope!

Instead IIRC Sun Java 3/4 created an accessor rather than modifying the field.

Sun Java 6 (javac 1.6.0_16 ) creates a static accessor:

public class InnerExample {
    private int field = 42; 

    private class InnerClass {
        public int getField () { return field; };
    }

    private InnerClass getInner () { 
        return new InnerClass();
    }

    public static void main (String...args) {
        System.out.println(new InnerExample().getInner().getField());
    }
}


$ javap -classpath bin -private InnerExample
Compiled from "InnerExample.java"
public class InnerExample extends java.lang.Object{
    private int field;
    public InnerExample();
    private InnerExample$InnerClass getInner();
    public static void main(java.lang.String[]);
    static int access$000(InnerExample);
}


$ javap -classpath bin -c -private InnerExample
static int access$000(InnerExample);
  Code:
   0:   aload_0
   1:   getfield #1; //Field field:I
   4:   ireturn

Is this actually a security risk, given that any class, other than the outer and inner classes, that tried to access the outer class' private members would not com[p]ile?

I'm speculating a bit here, but if you compile against the class it doesn't, but if you add the access$000 then you can compile code which uses the accessor.

import java.lang.reflect.*;

public class InnerThief {
    public static void main (String...args) throws Exception {
        for (Method me : InnerExample.class.getDeclaredMethods()){
            System.out.println(me);
            System.out.printf("%08x\n",me.getModifiers());
        }

        System.out.println(InnerExample.access$000(new InnerExample()));
    }
}

The interesting thing is that the synthesised accessor has modifier flags 00001008 where if you add a package level static method it has flags 00000008. There's nothing in the second edition of the JVM spec for that flag value, but it seems to prevent the method being seen by javac.

So it appears that there's some security feature there, but I can't find any documentation on it.

(hence this post in CW, in case someone does know what 0x1000 means in a class file)

Answer 9

"Is this actually a security risk, given that any class, other than the outer and inner classes, that tried to access the outer class' private members would not compile?"

Even if it won't compile under normal circumstances, you can still generate your own bytecode. But that's no reason to avoid inner classes. All you would have to do is assume all your inner classes are public.

If you really want to be able to run untrusted code, learn how setup your own sandboxes and security levels using The Java Security Architecture, it's not that hard. But mostly, you should avoid running random code in a secure environment.

Answer 10

nonsense! follow the same logic, do not write public methods either, because they have access to private fields, gush!