3

FindBugs raises a bug called EI_EXPOSE_REP with the following description :

EI: May expose internal representation by returning reference to mutable object

Returning a reference to a mutable object value stored in one of the object's fields exposes the internal representation of the object. If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Returning a new copy of the object is better approach in many situations.

Several questions on SO (1, 2 and 3) have already addressed how to avoid such bug and I understand that it is a development best practice to prevent modifications of immutable objects however it is not clear to me why such bug belongs to the MALICIOUS_CODE category.

What is the real threat behind this ?

If it's a malicious code problem, the attacker can do almost anything he wants and mutability wouldn't be the biggest problem. If it is a vulnerability, it can be exploited only if untrusted code is executed also and I can't see any usecase where this is true.

Any perspective on this ?

Thanks !

Community
  • 1
  • 1
Nibbler
  • 503
  • 5
  • 10
  • 2
    Findbugs "security" rules are based on old-ish security guidelines from Sun and are targeting primarily applet environments. They do not have much use for, say, webapps or server apps. – Vitaly Osipov Jan 03 '13 at 01:05
  • @agelastic: I think your comment is a bit misleading. It's probably far better to say that the "security" rules are more for environments that may be running unapproved code. – NotMe Jan 03 '13 at 01:42
  • Chris Lively - what are those environments? In webapp world it's applets. What are the others? – Vitaly Osipov Jan 03 '13 at 23:11

2 Answers2

2

The point is that any time you open up an implementation you run the risk of code doing damage, intentionally or otherwise. Obviously a malicious library user, for example, could just disassemble your jar and learn details that way–the point is to minimize the risk of exposure: it cannot be eliminated.

A trivial example of external code accessing your library:

Consider something simple like an object that holds an access level. If it was mutable, it's conceivable a library user could set their own access level. Something this trivial would rarely be exposed by any reasonable library, but it's a clear example of when an internal representation might be abused.

The bottom line is that exposed mutable state makes code difficult to reason about, and difficult to protect. Your code or others may accidentally, or deliberately, modify something your own code/library uses. If your library then changes its behavior without taking that into account, you may introduce a subtle (or not so subtle) bug.

Dave Newton
  • 158,873
  • 26
  • 254
  • 302
  • Indeed, it is possible if you open your implementation as a library or other however what is the real risk by having such bug in the main class of your own software ? – Nibbler Jan 02 '13 at 16:02
  • @Nibbler Not sure what you mean by "in the main class of your own software". Over-exposed mutable state allows any code, yours or others, to mess with internal representations. Any time that level of representation is exposed, it makes code difficult to protect and reason about. – Dave Newton Jan 02 '13 at 16:18
  • Let's say you implement an FTP server in Java with immutable classes without correctly handling mutable parameters. Is there really a risk with it? And how can this be classified as malicious code? – Nibbler Jan 02 '13 at 16:21
  • @Nibbler It can be classified as "malicious" because it's *possible* your code, or the code of others, could take advantage of internal representation. I don't understand what your issue is: it's called "malicious" because it could be maliciously abused. How would FindBugs know it's analysing code only you will ever use?! There's risk in the sense that mutable state makes code difficult to reason about. – Dave Newton Jan 02 '13 at 16:38
1

The ability to have an object to hide state and provide a strong, static interface is core to Java mobile code security. There are a number of ways that a programmer messing this up can cause a vulnerability.

For value objects, consider String. We trust any instance of String not to change. We don't want to validate [check] a particular file name, for example, we don't want it to change when we actually use it (note, java.io.File doesn't work well in this sense). Also a mutable internals may have malicious equals method (say), that is called by the enclosing class' equals, but maliciously acquires references to internal objects of other enclosing class instances compared to.

Reference types are often object capabilities. Typically they will attenuate a capability of an object they were endowed with (passed through the constructor). Say, you can only write a file to a particular directory through the instance you have access to, but it has a field with a capability to write to an entire filesystem.

Then there's the Java 2 Security Model, and that's never a good thing. The inner object may have methods called in a privileged context, which isn't usually a problem. Now a malicious party places in object (of a trusted type) that does something that it shouldn't in that context.

Of course, watch out for randomly subclassing classes. They may acquire a get method in future versions.

Having said that, in my opinion this warning from Findbugs is not helpful. The code probably wasn't intented to hide the object.

(For a verbose description of the problem of exposing internal objects see Chapter 13 (and 14) of Michael Feathers Working Effectively with Legacy Code.)

Tom Hawtin - tackline
  • 145,806
  • 30
  • 211
  • 305