-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
CVEs / Vulnerabilities coming up in scans #6769
Comments
Thanks for filing this in a well organized way. Trino has many dependencies, many of which are difficult or impractical to upgrade, for various reasons. For example, we depend on an old version of the Elasticsearch client, since newer versions of the client are not compatible with older versions of the server that people still use. While it feels like a general good practice to stay up to date with libraries, upgrades have a cost in terms of developer time and risk to users. As such, it would be rare for a library to be upgraded without a specific benefit. From a security perspective, we take a practical approach to security. Is the vulnerability in question applicable to Trino? In almost all cases, the vulnerability in a library is for a feature that we don't use, or requires using a library in a specific way. If the vulnerability did affect us, then it would be a high priority to resolve. |
@electrum thank you for the reply. I definitely and understand (and on a personal level, 110% agree) that it is important to look at these vulnerabilities in detail and understand whether they truly represent an attack surface area or not. Unfortunately, that's not how the overall enterprise security space has coalesced around this, and in our case, we have downstream consumers of ours which refuse to accept binaries (or will only accept them for a limited time) that have known CVEs against them, even if they are not directly relevant. Don't get me wrong - I understand the cost in terms of developer time and the risk upgrades pose in general - our software exists in a similar situation on the Java side, and it is a big effort to respond to these CVEs. Let me follow up with some more specific questions:
|
The vulnerability is in
These are for development mode where plugins can be resolved as POMs. It uses the Maven libraries to resolve dependencies. The server must be configured to point at local POMs, which contain or reference arbitrary code. The Netty vulnerabilities seem to require a malicious server or intermediary. However, the server is providing Maven artifacts containing code to be run by the server, it has to be trusted anyway. I'd like to upgrade to a newer version of the embedded Maven libraries, but when I looked last it appeared non trivial, and it has not been a priority, since it's for a rarely used development mode and the existing version works.
It's not clear if this applies to Trino. It seems that this is a memory consumption issue for resources that accept XML documents. To my knowledge, none of our JAX-RS endpoints accept XML, since we use JSON for everything. It is possible the parsing happens before the check for the accepted content type, but this would require more investigation. Unfortunately, while it would be nice to upgrade to later Jersey versions, the fix version is after the switch from the
I'm not sure why this is pulled in at all. I hope that Coral is not actually using Gradle. We could try excluding it. There is a related issue #5604
The check for For
Fixed in #6660 by upgrading to Jackson
It looks like this would be solved by updating |
For these three:
Is it possible to build a version of Trino that excludes them (i.e. if we were to do a custom build)? Or even delete them from the packaged tgz (again, we would do this ourselves)? If there's a way to just build without the airlift-resolver functionality, that seems like it would resolve a host of issues. I'm not familiar enough with this part of Trino to say. Regarding some of the other notes - we've tried explaining the case that the vulnerability doesn't exist (e.g. we were once asked to fix a CVE that only existed on ARM, even though we only support x86/x64), but the reality is that for these consumers, this is simply binary - it either passes or doesn't, and if it doesn't, it needs to be fixed. |
We are happy to accept contributions. The You could certainly modify the code the to remove the resolver. It would be an easy patch to maintain. |
This removes the old Maven dependencies from the server: #6834 |
@electrum thank you, that is very helpful! |
@itay is it safe to close this? |
@bitsondatadev yep, I don't think this is going to move forward in any other way. I'll close it. Appreciate the original engagement! |
We've done a scan of 350 (looking for the last non-Trino version for now) using Snyk and also received a scan from another tool, and wanted to share the findings. There are more that exist but I've limited this to the plugins we specifically care about. I've broken it up into what is happening in
presto-main
andpresto-hive
, and for each tried to include both the CVE (or a link to VulnDB) as well as themvn dependency:tree
output (though some of these dependencies exist in multiple places, I just picked the first one).Some of these look like relatively straightforward upgrades, but since many of them are in downstream dependencies, I wasn't sure what the Trino policy on updating them are. It's quite challenging to have these come up given the focus that many organizations are putting on not accepting any CVEs marked as High/Critical, which the below all are.
presto-main:
presto-hive:
The text was updated successfully, but these errors were encountered: