MALICIOUS_CODE EI_EXPOSE_REP Medium

Question

I run findbugs against all of my code and only tackle the top stuff. I finally got the top stuff resolved and now am looking at the details. I have a simple entity, say a user:

public class User implements Serializable
{
    protected Date birthDate;

    public Date getBirthDate()
    {return(birthDate);}

    public void setBirthDate(final Date birthDate)
    {this.birthDate = birthDate;}
}

This class is incomplete, so don't harp me about it missing the serialVersionUID and other standard stuff, I am just concerned with the birthDate security hole.

Now, according to the findbugs report, since I am returning a reference to a mutable object, that is a potential security risk. In practice though, how much does that really matter?

http://findbugs.sourceforge.net/bugDescriptions.html#EI_EXPOSE_REP

I suppose I still don't really see what the problem is here in this case. Should I pass in a long and set the date from that?

Walter

Thamos

Answer 1

I think the key here is the if:

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.

So in other words, if you wanted an immutable object (i.e. you didn't have a setBirthdate() method), your code be incorrect, because someone could write:

Date date = user.getBirthDate();
date.setMonth(1);  // mutated!

So you would probably want the following instead:

public Date getBirthDate()
{return new Date(birthDate.getTime());}  // essentially a clone

Darra

I agree with both comments, for what I'm doing, I don't need to worry about this. This raises a good point though, getters may not always simply be return(property); I missed that security aspect before. Thanks for your comments Matt and Robert. – Thamos

Shouldnt we also have a null check if in case the class is instantiated with a null birthDate? – Particularity

Answer 2

Yeah, I wouldn't really call it a ‘security’ issue as such... I mean, what attacker exactly is going to be writing malicious code against your objects? The real problem would be that you're quite likely yourself to trip up by accidentally calling getBirthDate then modifying the result.

For this reason, it is common to have your getter clone mutable objects like Date for returning, when you're using them as value types.

(You could also argue that Java's Date shouldn't have been made mutable, but there's not much can be done about that now.)

Soulless

@Soulless - "... I mean, what attacker exactly is going to be writing malicious code against your objects?". Findbugs does not know anything about the environment in which the code will be executed. And neither do you! There ARE situations in which this particular "bug" COULD be a serious security issue. – Psychopathology

Sure, in as much as any bug can become a security issue. I find it a little odd the way many Java coders are so paranoid about other classes in the codebase being able to do things ‘they shouldn't’, given that 99% of real-world projects don't have an internal security boundary that actually requires this kind of water-tightness. – Soulless

Answer 3

Adding to the good answer of Matt Solnit, I've faced the same problem when setting a attribute, so I did the same:

public void setDataEmissaoNota (Date dataEmissaoNota)
{
    this.dataEmissaoNota = new Date(dataEmissaoNota.getTime());
}

Work's fine!

Dibble

Shouldnt we also have a null check if in case the Date passed to the setMethod is null? – Particularity

Answer 4

Well, I'd say that all depends. There are other non security-related reasons to return immutable objects, since it may also lead to some hard-to-find bugs in your code if the object is misused.

Is the class going to be accessed by untrusted code and/or data? If so, you need to have a clear idea of where the responsibility lies in your application with regards to validating input.

Also, what is the nature of the application? If it's e.g. an externally accessible network service then the input should almost certainly be considered potentially malicious. However if it's an application run locally with no priviliges which gets input from a trusted source, then probably no need to worry.

Pensile

Answer 5

I'd prefer to store Date as Long in EpochTime format and use that for persisting across my application layers. That does not need any extra overriding of Lombok getters. Finally while providing the response, I can have an util function which will convert the epoch timestamp to a date and then return it as a string. May be do something like below ::

private String Epoch_to_ISO8601(Long savedTimeStamp) {
Date passedDate = new Date(savedTimeStamp);
String ISO8601_date =
    DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ssX")
        .withZone(ZoneOffset.UTC)
        .format(passedDate.toInstant());
return ISO8601_date;

}

Conditional

Answer 6

So firstly why we get these bugs EI: May expose internal representation by returning reference to mutable object (EI_EXPOSE_REP) 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. EI2: May expose internal representation by incorporating reference to mutable object (EI_EXPOSE_REP2) This code stores a reference to an externally mutable object into 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. Storing a copy of the object is better approach in many situations.

So either you can clone the object(simply clone()) or public void setBirthDate(final Date birthDate) {this.birthDate = new Date(birthDate);}

Bwana

This question was asked 14 years ago and has several accepted answers. Even though your answer is correct, there was no need to play necromancer here – Chloechloette