Malicious code vulnerability - May expose internal representation by incorporating reference to mutable object

Question

I have the following code in my dto class.

public void setBillDate(Date billDate) {
    this.billDate = billDate;
}

And I get an error in sonar stated as such and I'm not sure what I'm doing wrong here.

Malicious code vulnerability - May expose internal representation by incorporating reference to mutable object

The class is a dto and the method is automatically created setter method. What am I doing wrong here. if anyone could explain. it would be a great help.

You could trying using `this.billDate = new Date(billDate.getTime());` instead... — MadProgrammer, Sep 23 '13 at 08:35
Is dto; data transfer object? Undefined acronyms can only lead to confusion — Richard Tingle, Sep 23 '13 at 08:36

score 125 · Accepted Answer · edited Jul 07 '16 at 13:32

125

`Date` is mutable

Using that setter, someone can modify the date instance from outside unintentionally

Consider this

class MyClass {

   private Date billDate;


   public void setBillDate(Date billDate) {
      this.billDate = billDate;
   }

}

now some one can set it

MyClass m = new MyClass();

Date dateToBeSet = new Date();
m.setBillDate(dateToBeSet); //The actual dateToBeSet is set to m

dateToBeSet.setYear(...); 
//^^^^^^^^ Un-intentional modification to dateToBeSet, will also modify the m's billDate

To avoid this, you may want to Deep-copy before setting

public void setBillDate(Date billDate) {
    this.billDate = new Date(billDate.getTime());
}

edited Jul 07 '16 at 13:32

tk_

16,415
8
80
90

answered Sep 23 '13 at 08:36

sanbhat

17,522
6
48
64

how can this be achieved if my instance variable was an Array of Dates ? can i use Arrays.copyOf ? – yathirigan Nov 16 '15 at 16:44
2

@yathirigan yes. Basically you need to do deep copy, so that outside modification doesn't affect the object held within this instance – sanbhat Nov 17 '15 at 09:02
thank you.. am confused looking at few articles that state that Arrays.copyOf does not do a deep copy. am doing some reading around it. – yathirigan Nov 17 '15 at 09:15
also is clone useful, eg. billDate.clone() – nikli Oct 18 '16 at 06:23
What if we write `this.billDate = billDate.clone();` ? It is also the same – prem30488 May 23 '17 at 06:38
are we saying, sonar will actually look at this deep copy code and understand? – Kalpesh Soni Sep 19 '18 at 16:15

score 51 · Answer 2 · answered Jan 14 '14 at 12:49

51

I wonder why none of the solutions takes null into consideration. A general, null-safe solution should look like this:

public void setBillDate(Date billDate) {
    this.billDate = billDate != null ? new Date(billDate.getTime()) : null;
}

answered Jan 14 '14 at 12:49

m.bemowski

632
6
5

4

maybe because you should avoid using null values as it is considered by many as antipattern – Zavael Feb 04 '16 at 14:26
2

@Zavael The whole point of this is "you don't know what people outside the class are going to do with the values". You can't expect everyone to never pass you a null value. – Demonblack Mar 17 '16 at 15:32
@Demonblack I agree you can't excpect everyone to never pass a null, but you can "punish" them with appropriate exception :) if the caller does not have a value to set, he shouldn't call the method at all – Zavael Mar 17 '16 at 16:06
1

@Zavael Or, we can accept the Date to be a null instead of just punish them. Just like this answer. – Nier Mar 31 '16 at 07:12
@Nier yes we can, but I prefer to avoid it where I can – Zavael Mar 31 '16 at 07:30
1

@Zavael Most of time throw exception is a good choice. But recently I'm dealing with JPA with a nullable Date field in get / set functions. I need to consider the null case because it's legal to set / get a null Date. – Nier Mar 31 '16 at 07:36
Is there any open source utility method with this code? I wonder if apache commons lacks it. – nomad Sep 08 '16 at 21:41
I added a utility method to do this `public static Date dateCopy(Date d) { return d != null ? new Date(d.getTime()) : null; }` to avoid having null checks sprinkled everywhere in code. – anztenney Jun 14 '17 at 04:48

score 6 · Answer 3 · answered Aug 17 '17 at 03:11

A counter argument can be, why one would one unintentionally modify the date? If client sets the value and then modifies it, then our code should reflect it, isn't it? If not then is it not confusing?

I prefer to just ignore this FindBugs warning.

In case if you want to do that, just add following Maven dependencies in your pom.xml:

<!-- Findbugs -->
        <dependency>
            <groupId>com.google.code.findbugs</groupId>
            <artifactId>annotations</artifactId>
            <version>3.0.1</version>
            <scope>provided</scope>
        </dependency>
        <dependency>
            <groupId>com.google.code.findbugs</groupId>
            <artifactId>annotations</artifactId>
            <version>3.0.1</version>
            <scope>provided</scope>
        </dependency>
        <dependency>
            <groupId>com.google.code.findbugs</groupId>
            <artifactId>jsr305</artifactId>
            <version>3.0.1</version>
            <scope>provided</scope>
        </dependency>

and then these annotations at class or member field level in your POJO:

@SuppressFBWarnings(value = { "EI_EXPOSE_REP", "EI_EXPOSE_REP2" }, justification = "I prefer to suppress these FindBugs warnings")

Cheers

Akshay

score 3 · Answer 4 · answered Sep 23 '13 at 08:39

`Date` is mutable

and you are not creating a copy of Date that came in to you are parameter. So if the client code will change the value of the Date object, it will affect your class too.

Solution is to create a copy of Date

public setBillDate(Date billDate){
   this.billDate = new Date(billDate.getTime());
}

ashish p · Answer 5 · 2015-04-30T11:05:31.823

3

Consider using a clone as well. Don't forget null check.

public void setBillDate(Date billDate) {
    this.billDate = billDate == null ? null : billDate.clone();
}

edited Apr 30 '15 at 11:05

answered Apr 27 '15 at 11:44

ashish p

202
3
10

This might be an option for `java.util.Date` (for Java 7+, see the comments on http://stackoverflow.com/a/18954948/1098673), see: http://stackoverflow.com/a/7082614/1098673 But it is not the right solution in general, see: http://stackoverflow.com/questions/869033/how-do-i-copy-an-object-in-java – Eric Apr 30 '15 at 11:23
1

billDate.clone() returns an Object, you will have to cast (which I think is bad) it to a Date Object to set the field. – Akash May 04 '16 at 20:29

score 3 · Answer 6 · answered Jul 27 '17 at 06:52

In addition to the existing answers, I propose a new version based on Optional class from Java 8.

public void setBillDate(Date billDate) {
    this.billDate = Optional
            .ofNullable(billDate)
            .map(Date::getTime)
            .map(Date::new)
            .orElse(null);
}

score 1 · Answer 7 · answered Sep 23 '13 at 08:38

1

Date is not immutable, i.e. your billDate can be changed after it has been set on your DTO object. Or, in code:

Date billDate = new Date();
dto.setBillDate(billDate);
billDate.setYear(1990);
// now, dto.getBillDate().getYear() == 1990

You can make your setter more secure:

public void setBillDate(Date billDate) {
    this.billDate = (Date)billDate.clone();
}

answered Sep 23 '13 at 08:38

isnot2bad

24,105
2
29
50

The default `clone()` will create shallow copy, so it might not fix the problem – sanbhat Sep 23 '13 at 08:41
In Oracle's JDK 7 it's overwritten to create a deep copy. But you're right, the API doc only says: 'Return a copy of this object.' - that does not fully explain the type of copy it creates. – isnot2bad Sep 23 '13 at 08:51

score 1 · Answer 8 · answered Sep 21 '15 at 10:42

1

Top answer number 37 is not the correct answer : nobody cares about NullPointerExceptions???

You should try this instead :

public void setBillDate(Date billDate) {
    this.billDate = billDate == null ? billDate : new Date(billDate.getTime());
}

answered Sep 21 '15 at 10:42

Stéphane Essayie

11
1

Null is already mentioned in [this answer](http://stackoverflow.com/a/21113988/923794). – cfi Sep 21 '15 at 11:04

Malicious code vulnerability - May expose internal representation by incorporating reference to mutable object

8 Answers8

`Date` is mutable

`Date` is mutable

Linked

Related

Malicious code vulnerability - May expose internal representation by incorporating reference to mutable object

8 Answers8

Date is mutable

Date is mutable

Linked

Related

`Date` is mutable

`Date` is mutable