I think it would be beneficial to not consider MaxVersion here; is there any case where a bad MaxVersion is not accompanied by a bad MinVersion?

I guess the only case would be if MinVersion is left as its implicit default, which we wouldn't otherwise spot. I haven't seen any cases in the wild (yet) where MaxVersion is a problem.

Trivial comment (feel free to ignore): I think it looks slightly nicer to use suiteName in [...] instead of suiteName = [...].

Would it be better to put these in the definition of the sink? I assume it would be slightly more efficient to rule out some sinks earlier in the calculation.

Would it be neater to just inline this in the one place it's used?

This is used in secureTlsVersionFlowsToSink and secureTlsVersionFlowsToField

I was talking about secureTlsVersionFlowsToSink, which I've just double-checked is only used in one place. I'm guessing you thought I was talking about secureTlsVersionFlow.

This should be in the previous commit, I guess, but it doesn't really matter

Is there an already existing change note for this query?
If not, why mention that it is promoted from experimental status?

I suppose to inform anyone that has used it before that their query has moved, but I defer to anyone that knows the proper convention to describe a promotion.

We had a similar change note when go/email-injection was promoted from the experimental folder, so I think it's fine: https://github.com/github/codeql-go/blob/master/change-notes/2020-06-16-email-injection.md .

Hm, is there a reason we're checking the implementation-specific tls enum values here? If it's because there's not way to get them, that's probably something that we should have.

Some people use an uint16 (eg 0x0301 for tls.VersionTLS10) instead of the hardcoded version consts from the tls package; this approach catches them both.

Those version values are not golang-specific, but part of the tls protocol spec.

This seems like it would be useful in general, though I'm not entirely sure where it might go.

I think the predicate below should be replaced by instanceof TestFile a la DisabledCertificateCheck.

We could make files that match the heuristic into TestFiles, or if we don't think that's a good idea, move the heuristic logic into frameworks/Testing and add a predicate we can use in all queries where we exclude test file results.

Why do we want to keep this filter at all? I think filtering out boring results in tests is the job of the presentation layer (Code Scanning or LGTM), not of the query. There are exceptions (usually if the thing the query flags is legitimately OK in tests), but I'm not sure this is one of them.

For some reason I can't add a comment to the discussion above, but FWIW in my opinion it's fine to flag any project with an insecure TLS MinVersion, including the default, since it is actually vulnerable to a downgrade attack and therefore a valid result.

That would mean we could probably not worry about MaxVersion at all and simplify the query, as a bonus.

You mean the discussion with @gagliardetto? I agree we should flag the default, it's just harder to detect since we're looking for a negative property (that no write reaches that field or any of its aliases), so it's more likely to throw false-positives. That's why I wanted to pursue that separately.

It doesn't seem to me like fieldWrite is used anywhere, can it be removed?

I can't comment on it directly, but I think the message on line 239 doesn't need to mention InsecureCipherSuites().

Seems like this is missing results for the tests you marked bad on lines 336, 346, and 355?

Ah, thanks -- that's because they were using a variable name that suggests deliberate insecurity. Now fixed.

Out of interest, what kinds of blocks are you excluding with this as opposed to the above checks logic?

Lexically related but not dominated by a test:

insecureSetup := getBadCiphers()

Dominated but not lexically surrounded:

if !insecureConfig {
  return
}
config.MinVersion = 0

Feels a bit odd to expose both this and isFeatureFlagName.

Maybe you could have a utility predicate like getFlagFunction for the use of isFeatureFlagName above? Or maybe you could just do any(FeatureFlag f).getAFlagName(), though that's a bit clunkier

I think it might be nicer to add this and getAFlag as a member predicates of Flag, like so:

  ControlFlow::ConditionGuardNode getACheck() { result.ensures(this.getAFlag().getANode(), _) }

I think that would also eliminate the need for featureFlag and legacyFlag.

🤨

Workaround for github breakage, will drop

Why do we want to keep this filter at all? I think filtering out boring results in tests is the job of the presentation layer (Code Scanning or LGTM), not of the query. There are exceptions (usually if the thing the query flags is legitimately OK in tests), but I'm not sure this is one of them.

So, does this mean we are now tracking all integer constants anywhere? That seems like it might easily blow up.

I was concerned too, but the all-go-projects run on lgtm.com doesn't seem to have extraordinary cost. Discussing with Pavel suggests a broad source and a narrow sink performs better than you might suppose. I can restrict it to more specific constants, but we'll always care about zero since that specifies the default (TLS1.0)

Fair enough. I note, though, that all-projects runs are a very coarse indication of performance at best; all they tell you is whether something timed out, but not whether it went from very fast to almost-timing-out. I personally wouldn't rely on the data-flow library being clever enough to prune the search space aggressively enough, particularly since it seems that in this case we could help it fairly easily be restricting the set of sources ourselves.

Do you mean just restricting to zero + the interesting values, or do you have something cleverer in mind?

Pushed a commit just now doing the simple restriction. Will do a distcompare once my current distcompare for SSH-host-keys is done.

Why is this a query predicate? Having extra query predicates confuses parts of our toolchain, so I don't think this is safe to add.

Doh, debugging stuff left over

What is this a workaround for? I'd rather fix it properly.

I can't remember 100% anymore, but it has to do with isSink using writesField which in turn will return both the implicit-deref and the underlying as base candidates. A secure-version flow then needs to eliminate them both. An alternative would be to filter out implicit-deref nodes as base candidates in isSink or its caller?

Where is the latter bit ("no parent (...) is named (...)") checked?

This moved into the top-level query; fixed.

Having a predicate Node.getEnclosingFunction() would be convenient, I think. With that in hand, I'd suggest rewriting this to

(which looks slightly redundant, but isn't equivalent to getEnclosingFunction+(), since the two getEnclosingFunction() predicates are actually different)

and then just use is*FlagName(fn.getName()) below.

Makes sense, but seems very specific to the TLS query. Move there instead?

I've split the flags into 3 pieces. I can't see an easy way to split the code actually into the TLS query and yet allow using the shared infrastructure though.

Why is that? The shared infrastructure just needs to know about FlagKind, the concrete subclass can be moved into the query.

Why do you need this? It's an abstract class, so this should just fall out from the usual semantics.

Of course, since it extends string you'll still have to put a binding-set annotation on the (trivial) characteristic predicate, like this: https://github.com/github/codeql-go/blob/master/ql/src/semmle/go/dataflow/internal/DataFlowImpl.qll#L51.

Aha, that's the very demonic incantation I was looking for, thanks

Why do you need this? Couldn't you just use any(SecurityFeatureFlag f)?

Outdated comment.

Why do you need this both as a top-level predicate and as a member predicate of SecurityFeatureFlag below?

Why is that? The shared infrastructure just needs to know about FlagKind, the concrete subclass can be moved into the query.

cf #234 (comment)