6

Referring to this CWE-496, it mentions that because of the application assigning public data to a private array it is equivalent to giving public access to the array.

This is not clear to me because usually before you can use the object, we must declare the object. For example:

User user = new User();

I am going to make two assumptions, and correct me if I am wrong.

  1. Every new Object() will create different instance
  2. Inside User object, there is a private array userRoles and a public setter named setUserRoles(String[] userRoles)

My question is, since the object will declare new instances every time, how does the situation defined in CWE happen?

For example:

How come the user.setUserRoles() in a second request will overwrite the values of the userRoles variable in first request or the values of userRoles variable in other pages?

RoraΖ
  • 12,457
  • 4
  • 52
  • 84
overshadow
  • 361
  • 3
  • 5
  • 18
  • 4
    @Beguerad The OP is referencing a vulnerability, and asking why it occurs. It does involve coding, but I believe it falls into the scope of InfoSec. – RoraΖ Sep 17 '15 at 12:37
  • Be aware that CWE includes all sorts of coding weaknesses that are not vulnerabilities on their own - but could potentially be part of a multi-stage attack. – paj28 Sep 18 '15 at 10:01

2 Answers2

2

The issue is that the private data of the object becomes a reference to the data which is passed as parameter in the setter (which is a public one).

Arrays in Java are passed by reference (See this question), so that means that : 

public void setUserRoles(String[] userRoles) {
    this.userRoles = userRoles;
}

String[] param = ...;
User.setUserRoles(param);

Actually puts the reference of the param variable into the private member. If someone were to modify this param variable afterwards, it would modify the private member as well. The problem in this case is that the array is mutable as you can see in this SO question.

This behaviour defies the principle of encapsulation and therefore is wrong.

Finally, this has nothing to do with how many instances of the object you can create. It's a problem that is related to the object itself (so all object you would create would have the same problem).

A solution to the problem would be to make a full copy of the parameter array into the private member array (using clone() for example).

Community
  • 1
M'vy
  • 13,053
  • 3
  • 49
  • 70
  • This pattern is not wrong in and of itself. There is actually a valid use case in situations where the performance overhead of copying a huge array cannot be justified. – Pacerier Jan 28 '16 at 11:51
0

I feel like the CWE is pretty clear on the issue. A developer presumably marks a variable as private because he wants that variable to be private. Having a publicly accessible method that modifies the private variable effectively renders the variable public.

You ask about a specific situation where a class contains a private array that is modified by a public setter. It is true that this probably isn't a problem. However, remember that classes can in fact reference other objects not within the class scope.

Note that this isn't automatically a vulnerability. It may (or may not) lead to logical errors due to unintended modification of the variable scope which may (or may not) result in a security vulnerability.

  • 4
    Having private backing variables for publicly accessible (and even modifiable) fields is a common programming pattern. Also, access modifiers (such as private, protected and public in most object-oriented languages with inheritance) are essentially advisory; techniques like runtime reflection can be used to bypass such access modifiers. – user Sep 17 '15 at 13:28
  • "A developer presumably marks a variable as private because he wants that variable to be private" is circular... "Wellington is in New Zealand. Therefore, Wellington is in New Zealand." 99.9% of times such "private" access is for code beauty, not security. – Pacerier Jan 28 '16 at 11:51