Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

guava-16.0.1 and guav-19.0 vulnerability CVE-2018-10237 #50395

Closed
davydotcom opened this issue Dec 19, 2019 · 10 comments
Closed

guava-16.0.1 and guav-19.0 vulnerability CVE-2018-10237 #50395

davydotcom opened this issue Dec 19, 2019 · 10 comments
Labels
:Core/Infra/Core Core issues without another label high hanging fruit stalled Team:Core/Infra Meta label for core/infra team

Comments

@davydotcom
Copy link

CVE-2018-10237

ElasticSearch guava version needs updated to 24.1.1 to resolve another CVE
https://nvd.nist.gov/vuln/detail/CVE-2018-10237

@Tim-Brooks Tim-Brooks added the :Core/Infra/Core Core issues without another label label Dec 20, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Core)

jaymode added a commit to jaymode/elasticsearch that referenced this issue Jan 8, 2020
This commit upgrades the OWASP HTML sanitizer used by watcher to the
latest version and also upgrades guava, which it depends on. The guava
upgrade also requires the addition of a new dependency that guava
itself requires as of version 27.0.

Relates elastic#50395
@rjernst
Copy link
Member

rjernst commented Jan 8, 2020

I've been going through all the uses of guava. These are all indirect (we do not use guava directly in Elasticsearch). The following are uses of guava less than 24.1.1:

repository-azure plugin
One of the dependencies of the azure-storage client is azure-keyvault-core, which uses guava. There is an updated version of keyvault core which has a newer version of guava, but the latest version of azure-storage, at least with the api we use, still depends on azure-keyvault-core 1.0.0. There is a newer V11/12 client, but we will need to update our entire use of azure to upgrade, which we already have an issue for (#43309).

repository-hdfs plugin
The latest version of hdfs client uses an updated version of guava. @jbaiera is looking into the compatibility issues if we were to upgrade.

watcher
Watcher uses the owasp-java-html-sanitizer, which uses guava. The newest version of html sanitizer has a recent version of guava.

security
The opensaml dep of security uses guava in the shibboleth utilities. The latest version still uses guava 20.

@davydotcom
Copy link
Author

i manually updated most of 7.4.2 to latest guava without issue and built my own version successfully. even if indirect they still show on security scans. Anything we can do to get that to not be a security scan result even if indirect is always best. Thanks for digging!

@jbaiera
Copy link
Member

jbaiera commented Jan 13, 2020

regarding repository-hdfs plugin compatibility:

Everything that I've seen has been centered around HDFS v3.x being backwards compatible with HDFS client v2, but not the other way around. If we were to upgrade the HDFS Client in repository-hdfs we would most likely need to increase the minimum supported HDFS version for the plugin. Hadoop and HDFS v3 has been out for a while now, but some Hadoop users are still bound to the version 2 line, and this would likely constitute a breaking change for that reason.

@davydotcom
Copy link
Author

davydotcom commented Jan 13, 2020 via email

@rjernst
Copy link
Member

rjernst commented Jan 14, 2020

I was able to override and bump guava without bumping hdfs without issue

While it is great that it worked for you, the trickiness of bumping dependencies which were not developed against the newer version is there can be runtime issues that do not come up until a particular feature is used. While we may end up still bumping this as suggested, guava is particularly painful and error prone in this case, or at least was with older versions. Bumping this will need extensive testing.

jaymode added a commit that referenced this issue Jan 17, 2020
This commit upgrades the OWASP HTML sanitizer used by watcher to the
latest version and also upgrades guava, which it depends on. The guava
upgrade also requires the addition of a new dependency that guava
itself requires as of version 27.0. The sanitizer's behavior has changed to
re-write these templated values with a comment that results in this output
`{<!-- -->{ctx.metadata.name}}`. This would be an issue if we attempted to
sanitize the template, but the code that uses the sanitizer runs the rendered
string through the sanitizer, which means that the templated values have
been replaced already.

Relates #50395
jaymode added a commit to jaymode/elasticsearch that referenced this issue Jan 17, 2020
This commit upgrades the OWASP HTML sanitizer used by watcher to the
latest version and also upgrades guava, which it depends on. The guava
upgrade also requires the addition of a new dependency that guava
itself requires as of version 27.0. The sanitizer's behavior has changed to
re-write these templated values with a comment that results in this output
`{<!-- -->{ctx.metadata.name}}`. This would be an issue if we attempted to
sanitize the template, but the code that uses the sanitizer runs the rendered
string through the sanitizer, which means that the templated values have
been replaced already.

Relates elastic#50395
jaymode added a commit that referenced this issue Jan 17, 2020
This commit upgrades the OWASP HTML sanitizer used by watcher to the
latest version and also upgrades guava, which it depends on. The guava
upgrade also requires the addition of a new dependency that guava
itself requires as of version 27.0. The sanitizer's behavior has changed to
re-write these templated values with a comment that results in this output
`{<!-- -->{ctx.metadata.name}}`. This would be an issue if we attempted to
sanitize the template, but the code that uses the sanitizer runs the rendered
string through the sanitizer, which means that the templated values have
been replaced already.

Relates #50395
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this issue Jan 23, 2020
This commit upgrades the OWASP HTML sanitizer used by watcher to the
latest version and also upgrades guava, which it depends on. The guava
upgrade also requires the addition of a new dependency that guava
itself requires as of version 27.0. The sanitizer's behavior has changed to
re-write these templated values with a comment that results in this output
`{<!-- -->{ctx.metadata.name}}`. This would be an issue if we attempted to
sanitize the template, but the code that uses the sanitizer runs the rendered
string through the sanitizer, which means that the templated values have
been replaced already.

Relates elastic#50395
@rjernst rjernst added the stalled label Mar 2, 2020
@rjernst
Copy link
Member

rjernst commented Mar 2, 2020

I've marked this issue as stalled. There are 3 remaining production uses, which are blocked on the following changes:

@jbaiera
Copy link
Member

jbaiera commented Mar 27, 2020

Checking in on the hdfs side of things - Upgrading from 11.0.2 to 24.1.1 is not going well. Additionally, The HDFS 3.x client is not backward compatible with HDFS 2.x services, which according to artifact download numbers on maven are still the most popular versions. We may have to wait on upgrading Hadoop for a bit.

Taking a look at the CVE, the AtomicDoubleArray class does not seem to be used on the client side of things, but CompoundOrdering class may be used occasionally since it is so widely referenced through guava. Hadoop relies primarily on Protocol Buffers to handle serialization on the RPC interface. Since the vulnerability surfaces when using Java's built in serialization, these cases may have lower surface area for exploitation through the HDFS client.

@rjernst rjernst added the Team:Core/Infra Meta label for core/infra team label May 4, 2020
@rjernst rjernst added the needs:triage Requires assignment of a team area label label Dec 3, 2020
@pgomulka pgomulka added high hanging fruit and removed needs:triage Requires assignment of a team area label labels Dec 10, 2020
@sbrunot
Copy link

sbrunot commented May 25, 2022

In the latest 7.x version, guava dependencies with vulnerabilities are reported in modules/x-pack-security and modules/x-pack-watcher. What about providing the option to uninstall those modules (which were not present in the oss version of ES, if I'm not mistaken, on which there were no guava vulnerability reported...). That will give users the option to comply with "no reported vulnerability in production" rules without needing to "hack" the binaries. What do you think ?

@tvernum
Copy link
Contributor

tvernum commented Jun 3, 2022

As at Elasticsearch 7.17 the only remaining use of Guava < 24 is the transitive dependency via OpenSAML.

Unfortunately the situation with respect to OpenSAML + Guava + Elasticsearch is:

  • The final release of OpenSAML 3 was 3.4.6 that depends on Guava 20
  • Guava follows semantic versioning, so Guava 24+ is not a drop in replacement for Guava 20
  • OpenSAML 4 depends on a newer version of Guava, but also requires Java 11
  • Elasticsearch v7 needs to maintain compatibility with Java 8

There is no supported combination of Java 8 + OpenSAML + Guava that we can use except to continue shipping an older version Guava (such as 19, or 20).

We considered options to patch OpenSAML or Guava to avoid shipping a dependency with this CVE, however each option introduces additional risk.

The CVE in question does not have any practical issue for Elasticsearch because it is a serialization attack, and ES does not use Java serialization in conjunction with Guava.
We aim to upgrade all dependency that have published CVEs, however we prioritise that work based on whether the CVE in question affects Elasticsearch (and to what degree).

In this case, we have upgraded OpenSAML to v4.0 in Elasticsearch 8.0 (because ES8 depends on Java 17, which removes the upgrade obstacle we had).

Our commitment to our users is that we introduce new features and substantial changes in major or minor releases (x.y.0), and patch releases (x.y.1, x.y.2, etc) are for bug fixes and low risk changes.
So, while we aim to resolve out of date dependencies in all supported versions, we have to balance that against the risk of performing significant dependency upgrades in a patch release. No one wants to upgrade from 7.17.4 to 7.17.5 and find that a patch fix release has caused a change in behaviour that breaks their cluster in some way.

At this point in time, with the available information we have, our view is that attempting to replace or remove Guava 19 in a patch release of Elasticsearch 7.17 would introduced unwarranted risk that would outweigh the benefits of resolving this CVE. We have made this decision based on the published information about the CVE and the assessment that it is not exploitable in Elasticsearch.

This issue is entirely resolved in the most recent releases of Elasticsearch 8.

@tvernum tvernum closed this as completed Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label high hanging fruit stalled Team:Core/Infra Meta label for core/infra team
Projects
None yet
Development

No branches or pull requests

8 participants