C++: New query for SSL certificates not checked

[geoffw0]

This is the second of two new queries for CWE-295: Improper Certificate Validation.

Results:

• on 132 LGTM projects; there are quite a few results.
• there's one obvious FP where the code is simply wrapping SSL_get_peer_certificate.
• the other results look like plausible TPs to me, but I may need to improve my familiarity with the library and figure out if some of these are in fact OK / safe? --- update: results LGTM on a second look as well.

Precision:

• provisionally set to @medium, for review after looking at the LGTM results in more depth.

Performance:

• tested locally on kamailio_kamailio and chakra-core (performance is great with a warmed up cache).
• no timeouts on the above LGTM run.

[MathiasVP]

I haven't looked at any of the LGTM results yet. But I have a couple of code comments.

[MathiasVP]

Intead of rolling our own control-flow analysis here, could we instead use StackVariableReachability? Then isSource could be the call to SSL_get_peer_certificate, isSink could be the exit of the function, and isBarrier could be a call to SSL_get_verify_result.

Would that work?

It may be difficult to fit the certIsZero barrier into SSL_get_peer_certificate, but I think it's doable by having one StackVariableReachability for each call to SSL_get_peer_certificate.

[geoffw0]

I tried a couple of libraries that didn't quite fit the problem, I may have overlooked StackVariableReachability.. Trying it now...

[geoffw0]

It may be difficult to fit the certIsZero barrier into SSL_get_peer_certificate, but I think it's doable by having one StackVariableReachability for each call to SSL_get_peer_certificate.

This turns out not to be a major issue, the existing GVN logic finds appropriate guards on certificates returned by SSL_get_peer_certificate, and I've never seen more than one certificate being used at once (in which case we'd have to worry about which certificate it is).

What is an issue is that the certIsZero barrier needs to be a barrier edge, not a barrier node. StackVariableReachability does not appear to support barrier edges and I haven't found a sufficient workaround for this yet. I suppose I could add it as a new feature, but that will then require a lot of testing.

[MathiasVP]

Fair enough. I appreciate that you looked into it! Let's postpone converting the logic to StackVariableReachability to a later point, then.

[MathiasVP]

Could these cases not be covered by using guard.controls? That might be able to solve the FP in test2_11 (and in principle the one in test2_9 as well, but IIRC the GuardConditions class specifically doesn't handle this case very well).

[geoffw0]

I've pushed an improved version using guard.controls (and fixing some other issues). It's a bit tricky because guards gives the basic blocks that may be entered only if the guard goes a particular way, whereas we want to be able to handle blocks that have more than one route in, e.g.

cert = SSL_get_peer_certificate(ssl);
if (cert)
{
  result = SSL_get_verify_result(ssl);
  ...
}
// here

In the case above here is good because one path to it has cert = 0, the other has a call to SSL_get_verify_result - not because of either fact on its own. As such the code is still somewhat special-cased rather than just letting guards do its thing.

[geoffw0]

Pushed some improvements:

• corrected test annotations.
• now using GuardCondition.controls, though sadly we still rely on special casing NotExpr somewhat rather than being fully general.
• in one path cert wasn't checked properly, that is now fixed as well.

[MathiasVP]

LGTM!