Skip to content

Conversation

@tsmock
Copy link
Contributor

@tsmock tsmock commented Dec 27, 2021

Current CVE's in dependencies

With this patch series, the CVE list drops to the following:

@tsmock tsmock marked this pull request as draft December 27, 2021 23:12
@tsmock tsmock marked this pull request as ready for review December 28, 2021 00:15
@migurski
Copy link
Collaborator

Looking good, I’ll set aside some time to merge + release.

@tsmock
Copy link
Contributor Author

tsmock commented Dec 28, 2021

That would be appreciated.

I just started looking at what it would take to upgrade Netty, but that should probably be a different PR due to the number of code changes I've already had to do.

Additional notes:

  • Some tests fail on my system (both pre-PR and post-PR) both inside and outside the docker images (docker.sh)
  • There may be some binary compatibility issues -- I converted most compile dependencies to implementation, when api is technically a better match to what compile was. The only api dependency is in osmosis-core, and is commons-compress. When I was thinking it over last night, I think it would be better to make it an implementation dependency, and just re-add the dependency where required.
  • I did not upgrade to Gradle 7.x since maven was totally removed, and I've never used the maven/maven-publish plugins. I'll look into doing that conversion in a separate PR, once dependencies are updated.

@brettch
Copy link
Member

brettch commented Apr 3, 2022

Thanks for putting this together. I'm really keen to see this merged.

Unfortunately the CI process seems quite unstable at the moment (intermittently failing db tests, not sure why yet) so I'm hesitant to merge until that's addressed. I'll try to find some time soon to investigate.

tsmock added 9 commits April 4, 2022 09:18
Jump to 7.x was not done due to usage of `maven` plugin. This needs to
be migrated to `maven-publish`. `maven` was deprecated in Gradle 5.6,
along with `uploadArchives`.

Signed-off-by: Taylor Smock <[email protected]>
Signed-off-by: Taylor Smock <[email protected]>
TreeWalker is a submodule of Checker, so inherits tabWidth.
tabWidth was added to Checker in 8.19.

Signed-off-by: Taylor Smock <[email protected]>
Signed-off-by: Taylor Smock <[email protected]>
Signed-off-by: Taylor Smock <[email protected]>
@GeOSMRob
Copy link

Hello everyone,

We have recently found another vulnerability in osmosis via a scan.

(Spring4Shell​; CVE-2022-22965) here are the files:

  • osmosis/lib/default/spring-beans-5.1.0.RELEASE.jar
  • osmosis/lib/default/spring-core-5.1.0.RELEASE.jar)

Can you already say when an update of the software is planned?

Kind regards Robert

* mysql connector 8.0.28 -> 8.0.29
  * Avoids potential issues when using utf8mb3 server side
  * Now supports FIDO authentication (requires osmosis changes to
    actually use)
* postgresql 42.3.3 -> 42.3.5
  * Performance and bug fixes
* protobuf 3.19.4 -> 3.20.1
  * Report original exceptions from JSON parsing
  * Better guards against stack overflows
  * Removes deprecated functionality from protocol compiler
* spring 5.3.18 -> 5.3.20
  * Updates ASM to 9.3 (Java 19 support)
  * Various bug fixes

Signed-off-by: Taylor Smock <[email protected]>
@tsmock
Copy link
Contributor Author

tsmock commented May 12, 2022

@GeOSMRob : I'm not certain CVE-2022-22965 affects osmosis in the configuration(s) that most users will be using.

Specifically, that CVE requires the following (from the GitHub page), noting that people may work around some of the requirements:

  • JDK 9 or higher
  • Apache Tomcat as the Servlet container
  • Packaged as WAR
  • spring-webmvc or spring-webflux dependency

Anything that depends upon osmosis and has all those requirements fulfilled will be affected, unless they override osmosis' dependencies.

It should still be fixed though. While I don't like the conveyor belt of dependency updates, it is a necessity in today's world.

But as @brettch said, "the CI process seems quite unstable at the moment", and he is not wanting merge anything until that is fixed. With that said, everything seems to be working well for me locally (it didn't use to work for me, see my comment from 2021-12-28), so that may be fixed.

Anyway, with this patch series, the CVE list is:
guava-29.0-jre.jar (from checkstyle) (pkg:maven/com.google.guava/[email protected], cpe:2.3:a:google:guava:29.0:::::::) : CVE-2020-8908
netty-3.10.6.Final.jar (from osmosis-replication-http) (pkg:maven/io.netty/[email protected], cpe:2.3:a:netty:netty:3.10.6:
::::::) : CVE-2019-20444, CVE-2019-20445, CVE-2019-16869, CVE-2021-37136, CVE-2021-37137, CVE-2021-43797, CVE-2021-21295, CVE-2021-21409, CVE-2021-21290

Netty is going to involve a lot of code changes, and I'll start working on that in a separate PR when this one is merged.

@brettch
Copy link
Member

brettch commented May 12, 2022

Thanks for the comments, @tsmock. You have a much better understanding of this than I do :-)

Re the CI instability, I had a lot of trouble reliably reproducing the problem and parked it due to lack of time. My suspicion was that there might be race conditions between parallel database tests but I didn't get to the bottom of it. I think I should just merge your changes and hope it makes things better and not worse. At least we have a CI process again now which will make it easier to keep an eye on things.

Regarding the Netty changes, I think we should just delete those features. I created it to provide a streaming mechanism that could go below 1 minute updates. But in the many years since I wrote it I haven't heard of anybody using that feature.

I'll try to take a look at this again in the next couple of days and merge it.

@tsmock
Copy link
Contributor Author

tsmock commented May 12, 2022

Regarding the Netty changes, I think we should just delete those features. I created it to provide a streaming mechanism that could go below 1 minute updates. But in the many years since I wrote it I haven't heard of anybody using that feature.

A good way to find out if someone uses a feature is to remove it. 😄

I don't know anyone that uses the osmosis-replication-http module, but I mostly work on JOSM/JOSM plugins, along with some work on the atlas tools. So I'm definitely not someone who should be ripping code out of osmosis.

EDIT: It is definitely possible that this PR might fix the CI DB issues, since mysql, postgis, and postgres were all updated.

@brettch
Copy link
Member

brettch commented May 12, 2022

A good way to find out if someone uses a feature is to remove it. 😄

Hehe, this is true.

I don't know anyone that uses the osmosis-replication-http module, but I mostly work on JOSM/JOSM plugins, along with some work on the atlas tools. So I'm definitely not someone who should be ripping code out of osmosis.

Yep, that's fair enough. I think it's reasonable for me to take the position that if something has known security vulnerability issues, doesn't appear to be used, and doesn't have anybody volunteering to maintain it that we take it out. I think that's better than putting in a heap of work to upgrade something that's never used. If it's the wrong call we can restore it later.

EDIT: It is definitely possible that this PR might fix the CI DB issues, since mysql, postgis, and postgres were all updated.

🤞

@brettch brettch merged commit a13e92a into openstreetmap:main May 25, 2022
@brettch
Copy link
Member

brettch commented May 25, 2022

Thanks for your patience, @tsmock . It's merged now. The GitHub Actions build failed the first time on my fork but succeeded when I re-ran it. It seems that there are still some intermittent issues but this hasn't made them worse (or better).

@tsmock
Copy link
Contributor Author

tsmock commented May 25, 2022

I'm sorry to hear that. I hate intermittent issues (they are often not intermittent when debugging...)

I wonder if it only fails the first time it is run? (I've got a bug for JOSM ElevationProfile where the tests fail on first run, but succeed thereafter).

@tsmock tsmock deleted the dependency-updates branch May 25, 2022 12:55
@brettch
Copy link
Member

brettch commented May 26, 2022

Yah, intermittent issues can be frustrating. It's limited to pgsnapshot tasks so the blast radius is fairly small. I'll probably put my head in the sand for now and come back to it if/when I have more time and enthusiasm.

I'm not sure if there's a pattern yet. I haven't found one but I haven't been methodical in investigating it either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants