Go : Add query to detect timing attacks

Query PR

github/codeql#13119

Language

GoLang

CVE(s) ID list

CVE-2022-24912
I have a couple more I have found. I will add them later.

CWE

CWE-203

Report

This query detects instances where a non-contact comparision is used to compare two sensitive strings.

I have found multiple CVE's through this query. I don't know if those will qualify for Bug Slayer since some of them look like medium severity issues to me. I will open one if I can manage to meet the program requirements.

Are you planning to discuss this vulnerability submission publicly? (Blog Post, social networks, etc).

• Yes
• No

Blog post link

tba

atlantisVulnDb.zip
@Kwstubbs The PR for this ticket has now been merged. I have attached the db for the CVE in question below. This might help speed up your assessment.

[Kwstubbs]

@porcupineyhairs Thanks, will look into this in the next couple days.

[ghsecuritylab]

Your submission is now in status Test run.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

[ghsecuritylab]

Your submission is now in status Initial triage.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

[ghsecuritylab]

Your submission is now in status Test run.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

[Kwstubbs]

@porcupineyhairs
Found two things that could be improved.

1. Adding strings.Compare as an additional sink. This may increase the number of results.
2. Remove comparisons to constant numbers (secret ==1) and strings (secret == "password123") to reduce FPs.
   Please open another PR with these changes and link here. Let me know if changes make sense.

@Kwstubbs I have opened a new PR github/codeql#13645 with the proposed changes.

Remove comparisons to constant numbers (secret ==1) and strings (secret == "password123") to reduce FPs.

Won't this defeat the purpose of the query? remote to remote comparisions are already handled by the ConditionalBypass.ql query.

[Kwstubbs]

hi @porcupineyhairs I am thinking of comparisons where the left hand side/ right hand side is quite literally a number or string literal (secret ==1) (secret == "password123"), as these are 99% of time FP. A comparison that is a variable that has the type of string, (secret == config_specified_secret), and config_specified_secret is read from a config file or something would be allowed. I think BasicLit class might help.

[owen-mc]

When I change the rhs of the sinks in the tests to string literals then I no longer get any results. So I'm surprised that you're seeing that in the test run, @Kwstubbs .

Note that there is already EqualityTestBarrier which aims to do something similar. It should remove taint from a variable when it is compared successfully with a compile-time constant value (which isn't nil, for performance reasons, as there are so many comparisons with nil).

@owen-mc I have added the missing sink to my query now.

@Kwstubbs I just ran the query against top 1000 repos and I couldn't find any pattern similar to what you talk about above.

[Kwstubbs]

@porcupineyhairs looks good to me. Will move the bounty to the next stage.