Remove a timing channel in Password matching

[fredfeng]

Hi,

I found a timing channel in Password.java:
https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-util/src/main/java/org/eclipse/jetty/util/security/Password.java#L105

By using Arrays.equals, it actually violates the "constant-time-implementation" discipline.

For more information about timing attack:
https://codahale.com/a-lesson-in-timing-attacks/

[ebourg]

Is there a CVE ID assigned to this issue?

[fredfeng]

Not yet. Shall I create one?

[ebourg]

Ideally yes. A CVE ID raises the awareness of the security issue and allows downstream users like Linux distributions to notice it and quickly publish a fix.

[fredfeng]

Sure, I will do it later today and attach the CVE ID.

[carnil]

Downstream bug in Debian: https://bugs.debian.org/864898

[carnil]

This has been assigned CVE-2017-9735

[fredfeng]

Thanks!

[fredfeng]

But I got the following error message:
ERROR: Couldn't find 'CVE-2017-9735'

[carnil]

It only has been assigned around 10 minutes ago. It needs some time to update the MITRE website with the newest CVE entries AFAIK.

[adioss]

As it is a security issue with an associated CVE with High score, is anyone aware of a version 9.4.x potential release date?
My bad: I didn't see that it has been released with 9.4.6.v20170531.
Thanks!

[securinator]

@fredfeng @carnil Does this affect older versions of Jetty? Say 6-8? The code looks like it has barely changed for password.java between then and now but I am unfamiliar with the code base so I can only assume so.

[joakime]

@securinator all versions of Jetty 6 through 8 are EOL (End of Life).

[joakime]

@adioss look at the commit in github, you'll see that the commit exists in the releases:
jetty-9.4.6.v20170531
jetty-9.3.20.v20170531
jetty-9.2.22.v20170606

[securinator]

I know, I just wondered if anyone could confirm if they were affected or not.

[joakime]

@securinator nobody has confirmed that it's possible on Jetty 9 in a remote scenario.
Existing security auditing done against Jetty on a variety of tools and environments have failed to confirm a timing channel attack is possible from a remote client.

However, if you are local and are doing a timing channel attack on local passwords (in configuration files for example) then its very possible. But if you have this scenario you have much bigger issues then a timing channel attack on the Password class.

[msymons]

@joakime, can I echo the request of @securinator: does this threat affect jetty 6-8? The wording of the CVE would indicate that they are affected ("Jetty through 9.4.x").

I ask the question in full understanding that these versions are now way past EOL. But knowing for certain that an EOL component that one is using does have a security threat does help with motivating that an application be updated to use a secure version of the vulnerable component.

Ideally, I would like to see the CVE updated with accurate CPE information... this should help static analysis tools such as Dependency-Checker (or Black Duck, White Source, Nexus IQ, etc).

[joakime]

Jetty 6 through Jetty 8 are long ago declared EOL (End of Life) they shouldn't be used for production (unless heavily modified, like Google App Engine does).

We do not test archived / EOL products.

The question of If a specific CVE affects an EOL product is meaningless.
If CVE's are important to you, then you shouldn't be using EOL products in the first place.
It is misplaced to be concerned of CVEs and still be using EOL products (the minute the product is announced for EOL it should be treated as vulnerable and should be upgraded/replaced)

The various specs and protocols that define the general internet and web change and evolve, what was once considered acceptable is now considered a vulnerability. This evolution is not steady either, the number of changes is increasing year to year.

It is our opinion that you must keep your Jetty and JVM up to date.

If the CVE as written says "Jetty through 9.4.x" then that's wholly inaccurate statement, as there are Jetty 9.3.x and Jetty 9.2.x releases without the issue reported in the CVE.

[gregw]

@msymons We cannot make any statements about the vulnerability or otherwise of old releases without doing the due diligence to actually analyse the software and the particular vulnerability. We do such due diligence on our current releases, plus several old release branches for which we have commercial support relationships.

But we do not have the resources nor responsibility to do so for release branches that are many years old. I think it would be safest to assume that jetty 6, last release in 2010, will be vulnerable in some form or other, but we are not in a position to be able to say definitively that it is or is not vulnerable to a specific attack vector.

The software is open source and open to inspection, so if anybody is still running such old releases in production, then they are free to check the code and make their own assessment of vulnerability.

Finally, for this particular issue, we are not even sure the current release branches were vulnerable as no exploit was demonstrated and I expect it would only be possible in ideal circumstances unlikely to exist in any real deployment. However, we appreciate the issue being brought to our attention and were happy to make changes in order to be prudent. We can say that we have fixed all known timing issues in the current releases, but we cannot say if prior release were or were not vulnerable.

[ddillard]

I think this fix is broken. stringEquals() will return true if two strings are of equal length and any character at any position is the same in both strings. The fix appears to be pretty simple, result defaults to true and it's an &= instead of an |=. This way result is only true at the end if all characters in both strings are the same.

protected static boolean stringEquals(String s1, String s2)
 {
     if (s1 == s2)
         return true;
     if (s1 == null || s2 == null || s1.length() != s2.length())
         return false;
     boolean result = true;
     for (int i = 0; i < s1.length(); i++)
         result &= s1.charAt(i) == s2.charAt(i);
     return result;
 }

[fredfeng]

Yes, you are right. It should be '&' instead of '|'.

[sbordet]

@ddillard it's already been fixed using & in f3751d7.

[gregw]

I've updated the fix to always loop for precisely the length of the unknown credential, so we do not reveal the length of the real credential.

[sbordet]

@gregw have you applied this from 9.2.x onwards ?

[gregw]

oops no. will cherry pick back.

[praveenag]

@gregw Will there a patch release for 9.2.9.v20150224? Thanks.

[joakime]

There are no 9.2.x releases planned.

If security is important to you, then you should not be running Jetty 9.2.x or Java 1.7

Also, why are you not running the latest 9.2.x release?

[praveenag]

Yes you are right. We havent upgraded our jetty server in a while. We'll update it to the latest version of 9.2.

Thanks,
Praveena

[sbordet]

Jetty 9.2.x is old now. You should update to Jetty 9.4.x (and JDK 8).

[praveenag]

@sbordet We are already on JDK8 and when we faced issues trying to upgrade to 9.4.x which has been raised here

We'll revisit the upgrade in the near future :) Thanks for your help.