views:

606

answers:

10

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'?
+5  A: 
  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.
Jerome
I'd emphasize the 3. If the perpetrator gets into your machine/VM, you've already lost and no amount of "secure coding" can get around that.
Esko
Esko: Have you lost if you run an unsigned applet? I think not!
Tom Hawtin - tackline
@Esko except if you have a proper security model ready to handle it, and you control the way alien code is launched.
Jerome
+5  A: 

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

Zac Bowling
I don't understand your comment: the security guys in mR_fr0g's company do not want private fields to become public, whatever the way, be it realistic or not. An obfuscation tool will not change any level of security, and every method to obtain access to a private field will still work after obfuscation.
Jerome
Yeah, this is terrible advice. Obfuscation won't make anything more secure, it will just mean that hackers need to do a lot more work. But ultimately anything they could do before they can still do.
Chad Okere
You can anyhow access private fields through reflection
nos
FYI, you can not access private fields trhough reflection if you do not have a ReflectPermission("suppressAccessChecks") permission.
Jerome
(Jerome: Quite. Well, unless you are doing reflection from within that class, which is pretty much pointless.)
Tom Hawtin - tackline
+3  A: 

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.

tangens
+3  A: 

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.

Thorbjørn Ravn Andersen
A: 

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"

rsp
Static nested classes to have private access to the enclosing class (and vice versa).
Tom Hawtin - tackline
+14  A: 

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.

Tom Hawtin - tackline
Copying arguments won't always help to make your code more secure, eg when you call toArray() on an argument collection you trust the argument's toArray() implementation!
Adrian
Adrian: Yes, as noted in SUn's Secure Code Guidelines for Java.
Tom Hawtin - tackline
+4  A: 

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.

McDowell
+6  A: 

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)

Pete Kirkham
What about synthetic modifier?
serge_bg
I think it is, but didn't find any reference from Sun, nor anything which says 'if a method has a synthetic modifier, it can't be called through reflection without first setting it accessible', which would remove the perceived security risk.
Pete Kirkham
+2  A: 

"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.

Chad Okere
A: 

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

irreputable
This is not a language issue. The fact that nested classes have access to privates in the enclosing class and vice versa is not the issue under discussion. The (non-)issue is that adding nested classes can lead to privates being accessible to any code injected into the same package (if malicious code can actually be injected into the package).
Tom Hawtin - tackline