jellyfin / TMDbLib

C#.Net library for TheMovieDB
MIT License
344 stars 128 forks source link

Security Vulnerability in Newtonsoft.Json 9.0.1 #441

Closed jakelandau closed 9 months ago

jakelandau commented 10 months ago

Versions of Newtonsoft.Json below 13.0.1 have a high severity exploit allowing for Denial of Service attacks. This is particularly impactful for TMDbLib as it is a dependency for numerous network services. I've submitted #439 since this should probably be solved sooner rather than later.

In the long term, is it feasible to transition TMDbLib over to System.Text.Json? Or is there something this project uses in Newtonsoft.Json that isn't possible with the Base Class Library? As we get closer to .NET 8 release there's getting to be fewer and fewer reasons to keep using an external dependency for JSON serialization.

LordMike commented 10 months ago

There are a number of converters that are used, which would have to be mapped, converted or reconsidered in order to use System.Text.Json. Other than that, it should be doable.

As for depending on a higher version of Newtonsoft - that's tricky. If any consumer of this library were to use f.ex. 10.x, if Tmdblib was depending on >= 13, they couldn't do that. In my opinion, a library should depend on the lowest versions it can work with, in order to allow the consumers to choose newer versions if they want to.

So it may not be the best approach to simply depend on the newest version here. :)

jakelandau commented 10 months ago

Do we have a different approach though? Every Newtonsoft version below 13 has a high severity vulnerability, to be frank we probably don't want anybody consuming this library to try and use a lower version of Newtonsoft than that.

What other options do we have to mitigate the security issue other than update the vulnerable dependency? If the concern is maintaining compatibility, Newtonsoft 13 still targets .NET Standard 1.0, which is lower than TMDbLib's 1.2. Downstream projects will need to rebuild but it'll be pretty trivial, and frankly if they're shipping a security vuln they need to rebuild.

cvium commented 9 months ago

Fixed by #439.

I agree that dependency resolving can be tricky, but given a security vulnerability, it's a no-brainer to me.