[java]: Timing attack

[ahmed-farid-dev]

Query PR

https://github.com/github/codeql/pull/8686

Language

Java

CVE(s) ID list

CVE-2021-38153 CVE-2021-31404

CWE

CWE-208

Report

A constant-time algorithm should be used for checking the value of info. In other words, the comparison time should not depend on the content of the input, Otherwise, an attacker may be able to implement a timing attacks that may reveal the value of sensitive info

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

• [X] Yes
• [ ] No

Blog post link

No response

[JarLob]

Hello @ahmed-farid-dev, we believe this is already covered by https://github.com/github/codeql/pull/6006 Did you try to validate that CVE-2021-38153 and CVE-2016-0790 are not already covered and your query successfully identifies the vulnerabilities? Could you please share codeql databases of the vulnerable source snapshots?

[ahmed-farid-dev]

i don't think that because https://github.com/github/codeql/pull/6006 didn't flagged this https://github.com/jenkinsci/jenkins/commit/79e0b64322a6b15e0b80ac6511c9aa74383642be or this https://github.com/apache/kafka/commit/00c086e9087c3163cb0502bf0067bae4d401d66e

[ahmed-farid-dev]

Hello @JarLob, what do you mean by the vulnerable source ?

[JarLob]

Hi @ahmed-farid-dev, I meant codeql databases built from source code snapshots that were vulnerable to CVE-2021-38153 and CVE-2016-0790. Sorry the issue went under my radar, I'll try to make the databases, but if you can provide them, it could speed things up.

[ahmed-farid-dev]

Sorry, i had noticed that https://github.com/github/codeql/pull/6006/files can detect https://github.com/advisories/GHSA-jgpr-qrw2-6gp3 .but it's not cover https://github.com/advisories/GHSA-3j6g-hxx5-3q26 and https://github.com/advisories/GHSA-xwg3-qrcg-w9x6

[ahmed-farid-dev]

https://github.com/ahmed-farid-dev/files/blob/main/db.zip

[ahmed-farid-dev]

Hi @JarLob , Any update?

[JarLob]

Hi @ahmed-farid-dev, Thank you for providing the Vaadin codeql database. Your query provides 260 results on the database. Many of them flag any variable named "key" comparison and thus are false positive. Like in:

    static Key of(String key, String... additionalKeys) {
        Objects.requireNonNull(key);
        if ("".equals(key)) {
            throw new IllegalArgumentException("'key' cannot be empty");
        }

This is way too many to be acceptable (See "My Pull Request has been merged but my bounty was rejected. Is this normal?" in the bounty rules. Please fix the false positives to proceed further.

As a side note I have noticed your query displays raw results instead of path results, that makes it harder to read. By modifying it to select sink.getNode(), source, sink, "Possible timing attack against $@ validation.", source.getNode(), "time constant" you will get a better view of the results to analyze.

[ahmed-farid-dev]

Hi @JarLob, sorry for the delay. I made small changes to reduce the number of the false positives. you can take a look now

[JarLob]

The query doesn't compile. Please fix it.

[ahmed-farid-dev]

Sorry, the problem is fixed .

[JarLob]

Hi @ahmed-farid-dev, the PossibleTimingAttackAgainstSensitiveInfo.ql returned 5136 results on 35000 scanned repositories. It basically treats any expression flowing into comparison as potentially vulnerable. For example from virtualMachine.load(inputStream); to virtualMachine.getProperty("processId").equalsIgnoreCase(processId). We do not consider it as medium precision quality. Please delete it.

The TimingAttackAgainstSensitiveInfo.ql returned 2304 results on 35000 repositories. Results look better, but the weak part is that the query tries to guess what is secret based on the name. For example:

https://lgtm.com/projects/g/apache/flex-blazeds/snapshot/0b3f6088fec6fadde1834a7b403571f524ab47f5/files/proxy/src/main/java/flex/messaging/services/http/proxy/RequestUtil.java?sort=name&dir=ASC&mode=heatmap#L125 It compares the header name to "credentials" which is not security sensitive.

https://lgtm.com/projects/g/apache/qpid-jms-amqp-0-x/snapshot/d8522ef2fc0d9e8070c470dacfefec47050fa73c/files/client/src/main/java/org/apache/qpid/jms/failover/FailoverExchangeMethod.java?sort=name&dir=ASC&mode=heatmap#L132 Token in this case is just a result of splitting url to it components - tokens.

Similarly in https://lgtm.com/projects/g/apache/incubator-retired-slider/snapshot/20088758ae83fe9ee6c52663a6d6d6eb1801466a/files/slider-core/src/main/java/org/apache/slider/providers/agent/AgentProviderService.java?sort=name&dir=ASC&mode=heatmap#L3172 tokenMap is marked as something sensitive https://lgtm.com/projects/g/apache/incubator-retired-slider/snapshot/20088758ae83fe9ee6c52663a6d6d6eb1801466a/files/slider-core/src/main/java/org/apache/slider/providers/agent/AgentProviderService.java?sort=name&dir=ASC&mode=heatmap#L3172

Same in https://lgtm.com/projects/g/apache/pluto/snapshot/7665185af21d6e88fd376c166f82c792b8adde43/files/pluto-portal-driver-impl/src/main/java/org/apache/pluto/driver/url/impl/PortalURLParserImpl.java?sort=name&dir=ASC&mode=heatmap#L144 case where token variable name flows into window and https://lgtm.com/projects/g/apache/pluto/snapshot/7665185af21d6e88fd376c166f82c792b8adde43/files/pluto-portal-driver-impl/src/main/java/org/apache/pluto/driver/services/container/PortletURLProviderImpl.java?sort=name&dir=ASC&mode=heatmap#L75 is erroneously marked as vulnerable.

In https://lgtm.com/projects/g/apache/ofbiz-framework/snapshot/95e169497fab8a15762f6ef17a9bf1541e0c2474/files/applications/accounting/src/main/java/org/apache/ofbiz/accounting/invoice/InvoiceServices.java?sort=name&dir=ASC&mode=heatmap#L302 case passwordHint flows into billToRole

[JarLob]

In my opinion at least "token", "credential", "mac" should be removed. "%password%" should be changed to "%password" (because otherwise it matches things like "passwordHint"). Also "password" comparison needs sanitizer that would check if the plain text password or an encrypted/hashed password is compared. like in https://lgtm.com/projects/g/melin/super-diamond/snapshot/98f74816023fcf74e090d503cff87424c94c8585/files/super-diamond-server/src/main/java/com/github/diamond/web/service/impl/UserServiceImpl.java?sort=name&dir=ASC&mode=heatmap#L37

[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

[ahmed-farid-dev]

Hi @JarLob , Sorry for the delay i was very busy with the study.

I'm thinking to match the name of the function also, like in this case authenticate()

    protected boolean authenticate(String username, char[] password) throws IOException {
        if (username == null)
            return false;
        else {
            String expectedPassword = JaasContext.configEntryOption(jaasConfigEntries,
                    JAAS_USER_PREFIX + username,
                    PlainLoginModule.class.getName());
            return expectedPassword != null && Arrays.equals(password, expectedPassword.toCharArray());
            return expectedPassword != null && Utils.isEqualConstantTime(password, expectedPassword.toCharArray());
        }
    }

this is can be a good idea ?

[ahmed-farid-dev]

Hi @JarLob , i updated the query with the suggested line

[JarLob]

Hi @ahmed-farid-dev, thank you for pinging me. I will look into this today or tomorrow.

[JarLob]

1. The query doesn't compile.
2. I tried to make it compile locally, but it still flags https://lgtm.com/projects/g/melin/super-diamond/snapshot/98f74816023fcf74e090d503cff87424c94c8585/files/super-diamond-server/src/main/java/com/github/diamond/web/service/impl/UserServiceImpl.java?sort=name&dir=ASC&mode=heatmap#L37 so I'm not sure if my fixes were correct.
3. I'm not in favor of guessing what function does by its name, so I would work on eliminating already identified False Positives to see where it leads.

[JarLob]

Hi @ahmed-farid-dev, did you have a chance to look at it?

[ahmed-farid-dev]

Hi @JarLob, I'm trying to resolve this problem, when i match md5passwd the query still select the detect this line because of pwd

md5passwd.equals(pwd)

[ahmed-farid-dev]

Hi @JarLob , sorry for the delay, the problem is fixed now. the query doesn't select False Positive anymore.

[JarLob]

Hi Ahmed, since LGTM was discontinued there are technical difficulties re-running the query for evaluation. Sorry to say, but it is likely the feedback will be available only the next week.

[JarLob]

Hello Ahmed, the query still doesn't compile, it looks like you just edited it online without testing if it works. I have made these changes locally to be able to evaluate it. Hopefully this is what you intended:

@@ -45,8 +43,8 @@ class CredentialExpr extends Expr {
   CredentialExpr() {
     exists(Variable v | this = v.getAnAccess() |
       v.getName().toLowerCase().matches(suspicious()) and
-      not v.getName().toLowerCase().matches(nonSuspicious())
-      not v.isFinal()
+      not v.getName().toLowerCase().matches(nonSuspicious()) and
+      not v.isFinal()
     )
   }
 }

[JarLob]

Hello, Unfortunately the precision of the query doesn't satisfy the requirements for All for one, one for all bounty. However, we see its potential for security researches. Despite the high ratio of false positives, it also generates a lot of plausible findings. We propose you to consider applying to the The Bug Slayer bounty instead. To be eligible for the Slayer bounty you need to disclosure to maintainers and wait for the fix of minimum four high or two critical severity CVEs. Bigger number of high severity findings increases the amount of the potential bounty. Top 200 java projects could be a starting point, but more projects are eligible as described in the rules. For the full list of requirements please refer to https://securitylab.github.com/bounties/

[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