When clean code intended to help security can do the opposite

by   in CyberRes by OpenText

An example of a challenging scenario came up recently with try-with-resource statements in Java. This type of statement was introduced as a clean way to prevent accidentally leaving resources unreleased, which can lead to denial of service vulnerabilities, along with generally unreliable software (more information can be found here). While having been defined in JDK 7, try-with-resource statements have some ambiguity that can lead to security vulnerabilities even though part of the reason behind them is to prevent vulnerabilities.

The sample provided by Oracle is as follows:

1 (2).png






In this scenario, ThrowingStream and NonThrowingStream can be defined as (inner classes and other functionality missing for the sake of brevity):

2 (3).png

For this case, there is a resource leak, and the underlying stream is never deallocated.

Wait, what? Why?

According to the Java Specification,

“Resources are initialized in left-to-right order. If a resource fails to initialize (that is, its initializer expression throws an exception), then all resources initialized so far by the try-with-resources statement are closed. If all resources initialize successfully, the try block executes as normal and then all non-null resources of the try-with-resources statement are closed.”

This effectively means that for the pseudocode:

3 (2).png

If new ThrowingStream() throws an exception, nothing will be closed, because the Java compiler only sees a single try to allocate something.

This can be tested by running the sample and checking to see if a resource is allocated:

4 (3).png









5 (2).png

Here we set allocated to true in the constructor for NonThrowingStream and throw an exception in the constructor for ThrowingStream.

If the resources are allocated and then closed, we should see the output:

NonThrowingStream Allocated



No resource leak!

Instead we see the output:

NonThrowingStream Allocated

Resource leaked!

But why can’t Java make sure everything is closed?

A natural question from this point is, why don’t they close the underlying stream?

For starters, this would actually create some ambiguity. If someone were to pass a variable referencing some resource opened elsewhere, should this be closed too?

Secondly, this may cause functional problems. What if ThrowingStream.close() intentionally does not close the underlying resource? The developer would expect that the underlying resource would not be closed, and that it would have to be closed elsewhere. If Java automatically closed NonThrowingStream, then this would be confusing for the developer and may break additional code that relies upon the stream being still allocated.

So how is the code being compiled?

Try-with-resource statements can effectively be converted into regular try-catch-finally block, such as the following:

6 (2).png

This code is safe because new BufferedReader() cannot throw an exception. So, in scenarios where the constructor cannot throw an exception, these are safe to inline an allocation in the constructor this way.

Ok, I now understand. I get that it’s all about the constructor throwing an exception, but how do I fix this?

There are 2 ways to fix this scenario.

The first is to convert the try-with-resource statement into the older style try-catch-finally block as shown above.

The second option is to simply order the allocations one after another so that the first allocation gets closed if the second one fails:

8 (3).png 





Now if new ThrowingStream(nts) throws an exception, nts will still be closed(). From the compilation point of view, these will be treated as nested try-catch-finally blocks, to easily open them from left to right and close them in the opposite way.

One useful thing to note though is that if neither constructor throws an exception, and ThrowingStream.close() closes the underlying allocation, then nts will attempt to be closed twice! Once during the closing of the ThrowingStream instance and once during the NonThrowingStream instance. Therefore, if the code for closing the resource does not account for the resource already being closed, then there will also be problems. However, this is a general problem and you should always write close() methods that take into account that the resource may have already been closed.

In the end, it is a better practice to allocate the resources one at a time from left to right instead of doing an inline call, unless you are aware that the close() method is resource-intensive (even when the resource is already closed), but this would be a very uncommon scenario.

About Micro Focus Fortify

Fortify offers an end-to-end application security solution that secures and protects code throughout the entire development lifecycle of any type of software—from development to testing, release to production and every iteration in between. Fortify static, dynamic, interactive, and runtime security testing technologies are available on premise or on demand, offering organizations the flexibility needed to build an end-to-end software security assurance program.