log4rs icon indicating copy to clipboard operation
log4rs copied to clipboard

Fix dependency on unmaintained and vulnerable trait

Open aryanbdps9 opened this issue 2 years ago • 14 comments

Hey, this trait depends on typemap, which is unmaintained, and depends on unmaintained crate "unsafe-any", which depends on another unmaintained crate AND vulnerable crate "traitobject".

Are there any mitigation steps that we can take?

aryanbdps9 avatar Mar 11 '22 05:03 aryanbdps9

+1 I discovered this when my minimal-versions test failed.

johalun avatar Apr 07 '22 22:04 johalun

Yes we need to replace typemap.

estk avatar Apr 19 '22 18:04 estk

@estk is there any work planned regarding this issue?

GiorgioBertolotti avatar Jun 07 '22 06:06 GiorgioBertolotti

@GiorgioBertolotti it's not currently a vulnerability, it may become one in the future. The exploit is possible given a different memory layout since rust's layout is not stabilized. I would love to move away from it but... https://github.com/reem/rust-typemap/issues/45 has discouraged me.

Searching crates.io I see https://crates.io/crates/typemap_rev which seems to be maintained by the discord folks, so I guess that seems fine. I or someone else needs to then just do the migration. I will not have time in the next few weeks but I encourage anyone interested to take a crack at it, I will of course try to do timely code reviews and provide any mentorship necessary.

estk avatar Jun 07 '22 07:06 estk

@estk Thank you for the detailed explanation. I tried to take a look at the typemap_rev crate you recommended but honestly I'm afraid that as of today it is not mature enough to cover all the typemap features.

For example typemap_rev does not implement in trait Clone which is currently used in log4rs. There is an open issue but it has been there since April 2021....

This is just one example but could be limiting in several other ways.

Unfortunately not knowing the codebase of log4rs I can't tell if there are other possibilities to replace the crate typemap, do you have any suggestions?

GiorgioBertolotti avatar Jun 07 '22 08:06 GiorgioBertolotti

FYI As traitobject, unsafeany and typemap are unmaintained I've pulled https://github.com/philip-peterson/destructure_traitobject/ into unsafe-any here https://github.com/No9/rust-unsafe-any/commit/e0c79e23f0265597f6ab92b296296ff6f474e174
and updated typemap here https://github.com/No9/rust-typemap/commit/f15452a41e34ecdee89852b7727a0cac221ef3e7 I've then pulled typemap into a fork of log4rs here https://github.com/estk/log4rs/compare/master...No9:destructure_traitobject_fix The tests in all the repos pass but it's probably not a good idea to create a PR from a set of repos under my account.

While this isn't ideal it might be a useful interim measure and I'm happy to move the code to a shared place or whatever makes sense.

No9 avatar Jun 12 '22 13:06 No9

Hey @No9 we should probably file unmaintained advisory for unsafe-any and could point to your fork ?

Also if you have typemap fork going we could include it as alternative - there is also another one

  • Add unmaintained typemap - https://github.com/rustsec/advisory-db/pull/1406
  • Add unmaintained traitobject - https://github.com/rustsec/advisory-db/pull/1390

Would you be publishing typemap and unsafe-any fork's to crates.io ?

pinkforest avatar Sep 01 '22 20:09 pinkforest

@No9 @pinkforest thanks for staying on this one. @No9 it seems there is an appetite for having access to your forks via crates.io. One question tho, what do you mean by this?

The tests in all the repos pass but it's probably not a good idea to create a PR from a set of repos under my account. While this isn't ideal it might be a useful interim measure and I'm happy to move the code to a shared place or whatever makes sense.

If the authors/maintainers of those crates are unresponsive I suggest you just create new crates.io projects so long as you have the bandwidth for long-term maintenance of said resources.

My only objection to using forks is that I prefer not to directly depend on a git repo.

estk avatar Sep 01 '22 21:09 estk

Happy to publish these to crates.io I'll look at it over the weekend and update my fork. I can take the maintenance for the short/medium term but I think it would also be a good idea to give @estk or someone else admin access to the forked repo too so this doesn't happen again.

No9 avatar Sep 01 '22 23:09 No9

You could always start a github organisation and then give publish right to team in the github organisation That's what we do e.g. with cargo-geiger where we got cargo-geiger admins team under wg-secure-code I've been meaning to start orphanage-rs GH org to cater these crates that need new adoptees and that could use maintainer bus

pinkforest avatar Sep 01 '22 23:09 pinkforest

@pinkforest I really like the idea of orphanage-rs. I'd be happy to pitch in where i can.

estk avatar Sep 01 '22 23:09 estk

k. I started orphanage-rs and we can create teams based cargo publish from there and move the forks there I invited you both there and we can share maintainer duties on these forked crates If the original maintainer wakes up we can always transfer back to original crate

pinkforest avatar Sep 01 '22 23:09 pinkforest

Dependency repos have moved to orphanage-rs with some additional housekeeping done (clippy/fmt/CI). https://github.com/orphanage-rs/rust-unsafe-any https://github.com/orphanage-rs/rust-typemap

Crates published and invites sent to @pinkforest and @estk https://crates.io/crates/unsafe-any-ors https://crates.io/crates/typemap-ors

I've PRd updates to this repo to use typemap-ors https://github.com/estk/log4rs/pull/275

No9 avatar Sep 03 '22 15:09 No9

Are we all set on this now that we use typemap-ors?

bconn98 avatar Jan 26 '24 23:01 bconn98