How to check CWE-404 when throw exception

[ysuLihua]

Description of the issue

This is test code: test_throw.h

#define _O_RDONLY      0x0000  // open for reading only
namespace OCKIO {
namespace MSG {
void Throw(const char* func, const char* file)
{
    throw("error");
}
}
};

test_throw.cpp

#include "test_throw.h"
#include <fcntl.h>
#include <unistd.h> 
#include <sys/types.h> 
#include <sys/stat.h> 

namespace OCKIO {
namespace SHORE {
namespace PROCESSOR {
class TransPosix {
private:
    void ReadJewel();
};
void TransPosix::ReadJewel(){
    auto fdData = open("a.txt", 0x0000);
    if (fdData < 0) {
        MSG::Throw("error", "file opening failed");
    }
    if (lseek(fdData, 5, SEEK_SET) > 20){
        // no close
        **MSG::Throw("error", "file opening failed");**
    }
    close(fdData);
}
}
}
};

How can I check fdData is closed, before throw?

[rvermeulen]

@ysuLihua thanks for your question.

You have a few options to consider:

• Our Dominance allows you to determine if a node in the control flow graph is always preceded or followed by another node. This can for example be used to determine if MSG:Throw invocation that is dominated by an open call is not dominated by a close call. In other words, there is a path from open to MSG:Throw that doesn't contain a close. This doesn't yet consider the parameters to open and close so it might yield FN if those are different.
• You can write your own recursive predicate to determine a path between open and MSG::Throw that doesn't go through a close. The module GVN may help with determining that the arguments have the same value.

These options, however, only considers intra-procedural analysis. You can extend those options to inter-procedural, but you would need to handle the calls yourself.

The following query may provide a good example cpp/cert/src/rules/FIO51-CPP/CloseFilesWhenTheyAreNoLongerNeeded.ql. It implements the second option using the reachesTermination predicate.

Let me know if this helps.

[ysuLihua]

Thank you. I'll learn about this.

[ysuLihua]

@rvermeulen thanks for your suggestions! Now I define: Open file as FileStreamOpenExpr, and Throw terminate as TerminationCallExpr. I try to use TaintTracking to check it, but the check result is none. Can TaintTracking be used to detect this scenario?

Here's a code example：

class Config extends TaintTracking::Configuration {
  Config() { this = "Config: this name doesn't matter" }

  override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof FileStreamOpenExpr }
  
  override predicate isSink(DataFlow::Node sink) {
    sink.asExpr() instanceof TerminationCallExpr
  }
}

from Config cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink, source, sink, "open file stream flows to exception terminate."

[rvermeulen]

Hi @ysuLihua,

Unfortunately you cannot model this with data flow, because the value returned from open doesn't flow into the throw. This has to be checked with control flow.

The cpp/cert/src/rules/FIO51-CPP/CloseFilesWhenTheyAreNoLongerNeeded.ql uses control flow by finding a path using the recursive predicate reachesTermination.

The predicate can be adjusted in your case by starting at the open call and finding a path to throw that doesn't encounter a close call. Have a look at the recursive case in that predicate to see how you can stop it when it encounters a stop condition (in this case a close call).

Data flow can be used to determine if the value from open reaches an encountered close to ensure the same file descriptor is closed (to keep things simple you can skip this data flow step and implement it if there are too many false positives which I don't expect).

[github-actions[bot]]

This issue is stale because it has been open 14 days with no activity. Comment or remove the Stale label in order to avoid having this issue closed in 7 days.

[github-actions[bot]]

This issue was closed because it has been inactive for 7 days.