Exposing internal representation¶
ID: cs/expose-implementationKind: problemSecurity severity: Severity: recommendationPrecision: highTags: - quality - reliability - correctness - external/cwe/cwe-485Query suites: - csharp-security-and-quality.qls
Click to see the query in the CodeQL repository
Sometimes a method of a class can expose an internal field to change by other code if it returns a reference to a mutable private field.
Recommendation¶
There are several ways to address this problem depending on your situation. One of the best ways is to use an immutable object to store fields. References to this object can be passed outside the class but the objects are immutable so they cannot be changed.
Another good way of preventing external modification of private fields is to only ever return a copy of the object referred to by the field. This is called “defensive copying”. If the copy is changed then internal fields will not be affected.
Example¶
This example clearly demonstrates the problem with passing references to mutable objects outside a class. In this case it was possible to modify the values in the array despite theRange class not offering any method to do so.
usingSystem;classBad{classRange{privateint[]rarray=newint[2];publicRange(intmin,intmax){if(min<=max){rarray[0]=min;rarray[1]=max;}}publicint[]Get()=>rarray;}publicstaticvoidMain(string[]args){varr=newRange(1,10);varr_range=r.Get();r_range[0]=500;Console.WriteLine("Min: "+r.Get()[0]+" Max: "+r.Get()[1]);// prints "Min: 500 Max: 10"}}
Fixing with an immutable object¶
Here the example has been modified to prevent changes to the private field by using aReadOnlyCollection object.
usingSystem.Collections.ObjectModel;classGood1{classRange{privateReadOnlyCollection<int>rarray=newReadOnlyCollection<int>(newint[2]);publicRange(intmin,intmax){if(min<=max){int[]rarray=newint[2];rarray[0]=min;rarray[1]=max;this.rarray=newReadOnlyCollection<int>(rarray);}}publicReadOnlyCollection<int>Get()=>rarray;}}
Fixing with defensive copying¶
This is an example of the same class but this time it returns a defensive copy of the private field. There is also a short program showing what happens when an attempt is made to modify the data held by the field.
usingSystem;classGood2{classRange{privateint[]rarray=newint[2];publicRange(intmin,intmax){if(min<=max){rarray[0]=min;rarray[1]=max;}}publicint[]Get()=>(int[])rarray.Clone();}publicstaticvoidMain(string[]args){Rangea=newRange(1,10);int[]a_range=a.Get();a_range[0]=500;Console.WriteLine("Min: "+a.Get()[0]+" Max: "+a.Get()[1]);// prints "Min: 1 Max: 10"}}
References¶
MSDN, C# Programming Guide,Arrays as Objects.
MSDN,ReadOnlyCollection<T>.
Common Weakness Enumeration:CWE-485.