42

Effective java says:

// Potential security hole!

static public final Thing[] VALUES = { ... };

Can somebody tell me what is the security hole?

2
  • No more a security hole than private would be, if you can read the source code and/or bytecode either way... Commented May 16, 2010 at 0:14
  • 1
    The key word is "potential"! Everything publicly accessible in a class must be understood and intended. It is a contract! As with any contract, it should be well thought out, clear and understandable to all, and its usage and implementation in all circumstances acceptable because it's inevitable that any and all scenarios will be attempted. The key to creating any class is to start with the most restrictive interface. It's almost always possible to expose additional features and almost always difficult to remove them. Commented May 16, 2010 at 4:21

9 Answers 9

58

Declaring static final public fields is usually the hallmark of a class constant. It's perfectly fine for primitive types (ints, doubles etc..), and immutable classes, like strings and java.awt.Color. With arrays, the problem is that even though the array reference is constant, the elements of the array can still be changed, and as it's a field, changes are unguarded, uncontrolled, and usually unwelcome.

To combat this, the visibility of the array field can be restricted to private or package private, so you have a smaller body of code to consider when looking for suspicious modification. Alternatively, and often better, is to do away with the array together and use a 'List', or other appropriate collection type. By using a collection, you control if updates are allowed, since all updates go through methods. You can prevent updates by wrapping your collection using Collections.unmodifiableList(). But beware that even though the collection is immutable, you must also be sure that the types stored in it are also immutable, or the risk of unsolicited changes on a supposed constant will reappear.

Sign up to request clarification or add additional context in comments.

1 Comment

The lack of immutable arrays is one of my least favorite things about Java.
48

To understand why this is a potential security hole and not just poor encapsulation, consider the following example:

public class SafeSites {
    // a trusted class with permission to create network connections
    public static final String[] ALLOWED_URLS = new String[] {
        "http://amazon.com", "http://cnn.com"};

    // this method allows untrusted code to connect to allowed sites (only)
    public static void doRequest(String url) {
        for (String allowed : ALLOWED_URLS) {
            if (url.equals(allowed)) {
                 // send a request ...
            }
        }
    }
}

public class Untrusted {
     // An untrusted class that is executed in a security sandbox.

     public void naughtyBoy() {
         SafeSites.ALLOWED_URLS[0] = "http://myporn.com";
         SafeSites.doRequest("http://myporn.com");
     }
}

As you can see, the mistaken use of a final array means that untrusted code can subvert the restriction that the trusted code / sandbox is trying to impose. In this case, this is clearly a security issue.

If your code is not part of a security critical application, then you could ignore this issue. But IMO this is a bad idea. At some point in the future you (or someone else) might reuse your code in a context where security is a concern. At any rate, this is why the author calls public final arrays a security issue.


Amber said this in a comment:

No more a security hole than private would be, if you can read the source code and/or bytecode either way...

This is not true.

The fact that a "bad guy" can use source code / bytecodes to determine that a private exists and refers to an array is not sufficient to break security. The bad guy also has to inject code into a JVM that has the required permissions to use reflection. This permission is not available to untrusted code running in a (properly implemented) security sandbox.

2 Comments

naughtyBoy :D. Nice name for function.
I laughed out loud on myPorn on a meeting, thank you.
16

Note that a nonzero-length array is always mutable, so it is wrong for a class to have a public static final array field, or an accessor that returns such a field. If a class has such a field or accessor, clients will be able to modify the contents of the array.

-- Effective Java, 2nd Ed. (page 70)

1 Comment

This is the correct answer; the problem is that the type gives the impression of a constant, but it isn't
2

An outside class can modify the contents of the array, which is probably not what you want users of your class to do (you want them to do it through a method). It's sounds like the author meant it violates encapsulation, and not security.

I guess someone declaring this line could think other classes can't modify the array contents since it's marked as final, but this is not true, final only stops you from re-assigning the attribute.

3 Comments

.. if you are running untrusted code in a sandbox, and the public static final String[] is in trusted code, then this is a security issue. I think that the author meant what he wrote!
@Stephen C: If one's intent is to have an array who's contents are immutable, then yes this is a security issue. If the programmer did this purposely, then letting anyone modify the contents of the array is part of the array's interface. The former case maybe be a security issue, the latter is probably not. Or am I missing something here?
I think that the author's point is that this is potential security hole because it is not clear what the design / security analysis requires. (I was about to say "what the coder's intention was", but I realized that the coder's intention is secondary. Consider possibilities: 1) the coder simply didn't think about the issue and had no clear thoughts on it, or 2) the coder deliberately did this to introduce a security hole.)
1

In this declaration, a client can modify Thing[0], Thing[1], etc (i.e. elements in the array).

Comments

1

I would also add what Joshua Bloch proposed in Effective Java 3rd edition. Of course we can easily change the value of the array if it is declared as:

public static final String[] VALUES = { "a", "b" }; 

a.VALUES[0] = "changed value on index 0";
System.out.println(String.format("Result: %s", a.VALUES[0]));

and we get Result: changed value on index 0

Joshua Bloch proposed to return copy of array:

private static final String[] VALUES = { "a", "b" };   
public static final String[] values()
{
    return VALUES.clone();
}

so now when we try:

a.values()[0] = "changed value on index 0";
System.out.println(String.format("Result: %s", a.values()[0]));

we get Result: a and that's what we wanted to achieve - the VALUES are immutable.

There is also nothing bad in declaring public static final a primitives values, Strings or other immutable objects like public static final int ERROR_CODE = 59;

Comments

0

Think it just means the whole public vs private thing. It's supposed to be good practice to have local variables declared private, then using get and set methods, rather than accessing them directly. Makes them a little harder to mess with outside of your program. About it as far as I'm aware.

1 Comment

It is more subtle than that. A public static final String is fine, but a public static final String[] is not, because the contents of the array can be changed.
0

Because, final keyword assures only reference value (assume it as memory location for example) but not the content in it.

Comments

0
 private List<Integer> numbers = Collections.unmodifiableList(Arrays.asList(10, 12, 14, 16, 1, 2, 5, 3, 1));

And how to threat:

try{
    Collections.sort(numbersImutable);
}catch (UnsupportedOperationException e){
    ....
}

Explication:

private static final List<Integer> numbers = Arrays.asList(10, 12, 14, 16, 1, 2, 5, 3, 1);

What Does This Mean? private: This makes the variable numbers accessible only within the class it's declared in. No other classes can directly access it.

static: This means that the variable numbers belongs to the class itself rather than to any specific instance of the class. All instances of the class share the same numbers list.

final: This means that once the variable numbers is assigned, it cannot be reassigned to point to a different list. The reference to the list is constant.

Why Can You Still Modify the List? Arrays.asList(): This method returns a list that is mutable but of fixed size. This means you can change the elements within the list (e.g., sorting the list or updating an element), but you cannot add or remove elements from the list.

final Does Not Make the List Immutable: The final keyword only ensures that the reference to the list cannot be changed. However, it does not make the contents of the list immutable. You can still modify the elements of the list, even though you cannot reassign numbers to point to a different list.

Summary: final makes the reference to the list constant but does not affect the mutability of the list's contents. Arrays.asList() returns a list that allows element modification but not size modification. If you need the list to be truly immutable (i.e., no modifications to its content), you should wrap it with Collections.unmodifiableList() like this:

private List<Integer> numbers = Collections.unmodifiableList(Arrays.asList(10, 12, 14, 16, 1, 2, 5, 3, 1));

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.