[java]: Timing attack

[ahmed-farid-dev]

Query PR

github/codeql#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).

• Yes
• No

Blog post link

No response

[JarLob]

Hello @ahmed-farid-dev, we believe this is already covered by github/codeql#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 github/codeql#6006 didn't flagged this jenkinsci/jenkins@79e0b64 or this apache/kafka@00c086e

[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 GHSA-jgpr-qrw2-6gp3 .but it's not cover GHSA-3j6g-hxx5-3q26 and 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