jitsi / jibri

Jitsi BRoadcasting Infrastructure
Apache License 2.0
607 stars 313 forks source link

Logback CVE + link to Log4j #459

Closed Neustradamus closed 2 years ago

Neustradamus commented 2 years ago

Logback CVE + link to Log4j:

jbg commented 2 years ago

This is not a helpful way to file an issue — please provide some context, in particular describing how you believe this applies to Jibri, in particular which of the transitive dependencies depends on log4j (since Jibri does not).

Neustradamus commented 2 years ago

@jbg: https://github.com/jitsi/jibri/search?q=logback

And about Log4j:

jbg commented 2 years ago

OK, so mockk is the dep with the logback dep: https://github.com/jitsi/jibri/blob/master/pom.xml#L226 — only used in tests.

The logback dep in mockk was updated 9 days ago to 1.2.9 but there doesn't appear to have been a new release of mockk made yet. Filed https://github.com/mockk/mockk/issues/772

saghul commented 2 years ago

@jbg: https://github.com/jitsi/jibri/search?q=logback

And about Log4j:

We don't use log4j ourselves, the JVB was o my affected if one used Callstats. Is that the case here?

Just dropping CVE links on an issue is not helpful, it just creates unnecessary FUD. You linked an issue in the jitsi repo, this is jibri.

If you believe there is a security problem, check SECURITY.md and act appropriately next time please.

jbg commented 2 years ago

@saghul it looks like logback is used (only) in Jibri's tests, so there is a (fairly theoretical) risk of CI compromise (which may or may not have any actual impact depending on whether your CI has access to secrets etc) if tests are run on untrusted code (e.g. automatically running CI on pull requests).

AFAIK you don't automatically run CI on pull requests so this is probably pretty much a non-issue. Still worth updating the mockk dependency once they make a release though.

Neustradamus commented 2 years ago

@jbg: Thanks for your replies!

@saghul: I have specified logback for jibri and a little recall for log4j and Jitsi.

bgrozev commented 2 years ago

@saghul: I have specified logback for jibri and a little recall for log4j and Jitsi.

Jibri was never affected by any of the log4j issues, because jibri doesn't use or include log4j. Adding "a little recall" is unnecessary and adds confusion.

particular which of the transitive dependencies depends on log4j (since Jibri does not).

For clarity: none of them do.

jbg commented 2 years ago

@Neustradamus mockk has released v1.12.2 with the updated logback dependency. Will you be making a PR to update Jibri's mockk dependency?

bgrozev commented 2 years ago

mockk is the dep with the logback dep: https://github.com/jitsi/jibri/blob/master/pom.xml#L226 — only used in tests.

I think this is incorrect. mvn:dependency:tree shows that logback is a transitive dependency ofktor-server-test-host (and only it). Ktor updated the dep, but the only releae since then has been 2.0.0-beta-1 which isn't fully in maven yet (io.ktor:ktor-jackson:jar:2.0.0-beta-1 is not available).

Neustradamus commented 2 years ago

I have done a comment here:

And about slf4j and Bouncy Castle?

slf4j:

Bouncy Castle:

Note: I have done severals PRs in all Jitsi repositories since several years.

jbg commented 2 years ago

@Neustradamus slf4j and bouncycastle are not relevant to this repository. There's no value in "reminding" on random other repositories. Please don't do it. If you feel changes don't happen fast enough, you can always send a pull request...

Neustradamus commented 2 years ago

PRs have been done several years ago but the team does not want I think...

saghul commented 2 years ago

mockk is the dep with the logback dep: https://github.com/jitsi/jibri/blob/master/pom.xml#L226 — only used in tests.

I think this is incorrect. mvn:dependency:tree shows that logback is a transitive dependency ofktor-server-test-host (and only it). Ktor updated the dep, but the only releae since then has been 2.0.0-beta-1 which isn't fully in maven yet (io.ktor:ktor-jackson:jar:2.0.0-beta-1 is not available).

Based on @bgrozev's assessment I'm going to tentatively close this since there is nothing actionable.