Go : Add query to detect timing attacks

[porcupineyhairs]

Query PR

https://github.com/github/codeql/pull/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).

• [X] Yes
• [ ] No

Blog post link

tba

[porcupineyhairs]

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.

[porcupineyhairs]

@Kwstubbs I have opened a new PR https://github.com/github/codeql/pull/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).

[porcupineyhairs]

@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.

[porcupineyhairs]

@Kwstubbs @owen-mc I think you forgot actually moving this ticket to the final stage. The review is done and the pr is merged but I don't see the bot update this ticket to reflect the new state.

[ghsecuritylab]

Your submission is now in status Final decision.

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

[Kwstubbs]

@porcupineyhairs yup no problem also thanks @owen-mc for your help 😄

[porcupineyhairs]

@Kwstubbs @xcorail Before the final decision is taken, I would like to inform that there are currently four vulnerabilities fixed in

• https://github.com/sourcegraph/sourcegraph
• https://github.com/fatedier/frp
• https://github.com/ginuerzh/gost
• https://github.com/Dreamacro/clash

Most of these are high or above typically having CVSS CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:C/C:H/I:L/A:N. There are more places where I have reported the vulnerability but I am yet to here back. As for the four listed above, only one maintainer has requested CVE via GHSA. For the rest, I am waiting for an update from MITRE for the CVE's to be assigned before creating a bug slayer issue.

[Kwstubbs]

@porcupineyhairs we will await the CVEs. Please tag me here once everything is processed.

[porcupineyhairs]

@Kwstubbs For the bug slayer right? This is against All for One. This only requires one valid CVE detected in any project. The CVE I referred above should be sufficient for processing this ticket.

[porcupineyhairs]

@Kwstubbs I have received a weird response from MITRE regarding the CVE's.

This is in reponse to a new CVE application.

Hello,

Regarding your CVE service request, logged on 2023-05-11T17:04:32, we have the following question or update:

CVE's that are ** RESERVED ** will remain in a pending state until we are provided with at least one public reference that follows the CVE Entry Reference Requirement rules in section 8.3 (https://www.cve.org/ResourcesSupport/AllResources/CNARules#section_8-3_cve_record_reference_requirements).
When the candidates are publicized, please send us the link to the advisory using https://cveform.mitre.org/ with "Notify CVE about a publication" as the request type.

Please do not hesitate to contact the CVE Team by replying to this email if you have any questions, or to provide more details.

Please do not change the subject line, which allows us to effectively track your request.

CVE Assignment Team

M/S M300, 202 Burlington Road, Bedford, MA 01730 USA

I have tried contacting them for further clarification but the thread has gone cold.

Do you have any concerns with processing this ticket without the new CVEs?

[ghsecuritylab]

Your submission is now in status Closed.

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

[xcorail]

Created Hackerone report 2099402 for bounty 502693 : [757] Go : Add query to detect timing attacks

[Kwstubbs]

@porcupineyhairs many people at securitylab are OOO currently. I will get back to you in one or two weeks after we can make a group decision