Skip to content
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

Revert malicious commits #95

Closed
wants to merge 11 commits into from
Closed

Conversation

Alcaro
Copy link

@Alcaro Alcaro commented Mar 29, 2024

Some of them claim to fix actual bugs and/or improve test coverage, but it's unclear to me if they're bugs in the actual XZ project, or if they're fallout from the backdoor.

For this kind of situation, I'd rather err on the side of deleting too much. If they were helpful, the bugs can be re-reported and re-fixed.

I only checked up to and including https://github.com/tukaani-project/xz/commits?author=JiaT75&after=af071ef7702debef4f1d324616a0137a5001c14c+104 ; everything on that page looks clean to me, so I believe the user only became malicious somewhere after that point.

convert doc/xz-logo.png xz-logo.rgba
convert -depth 8 -size 67x54 xz-logo.rgba doc/xz-logo.png

This ensures it contains only image data, no unpleasant surprises.

I don't think it contains anything bad, but better safe than sorry.
Copy link

@dirkmueller dirkmueller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look sensible to be reverted based on my personal analysis of this incident

@jumps-are-op
Copy link

LGTM! :P

@thesamesam
Copy link
Member

I wouldn't do 71ff4e2. As I mention in the commits somewhere, the change is right to begin with. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114115.

But I don't object strenuously - we can always redo it later.

I haven't checked the reverts do what they say they do, but the list sounds right to me.

@Alcaro
Copy link
Author

Alcaro commented Mar 29, 2024

Yes, this PR is bigger than necessary. It's intentional; for issues of this nature, I'd rather purge everything related with a wide margin. Better safe than sorry.

The bug report tells how to trigger that issue; once the dust settles, it will be easy to rerun the reproduction steps and see if it still crashes, and if yes, someone uninvolved can create a new fix, or revert the revert.

Copy link

@TruncatedDinoSour TruncatedDinoSour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ( fr this time )

@ItzSwirlz
Copy link

Honestly, might be best to revert all commits in the past 3 months or so, or just find an older tagged version that hasn't probably been force pushed. It's possible that the bad actor modified old commit logs and whatnot

@naikel
Copy link

naikel commented Mar 29, 2024

@Alcaro why are you changing the correct word "function" to "funcion" and also naming the commit as "fixing a typo"?

You're introducing a typo instead.

EDIT: I can see you're only reverting commits.

@theCapypara
Copy link

@naikel Those are reverts. The original commits fixed the typo. They were reverted.

@Alcaro
Copy link
Author

Alcaro commented Mar 29, 2024

It was easier to revert the entire commit than only revert parts of it.

That entire line is removed in a subsequent commit (1efea8d), so the end result is the same.

@karak1974
Copy link

Most useful PR I saw today!

@JustinBoxemDEV
Copy link

JustinBoxemDEV commented Mar 29, 2024

May be overkill to suggest, but before this get merged we should get confirmation one way or another that the threat is no longer active/functional on this puReq

That said, @Alcaro , great initiative!

@TruncatedDinoSour
Copy link

TruncatedDinoSour commented Mar 29, 2024

Honestly, might be best to revert all commits in the past 3 months or so, or just find an older tagged version that hasn't probably been force pushed. It's possible that the bad actor modified old commit logs and whatnot

3 months of work though, oof, 685094b
better than a backdoor ig

@lcarilla
Copy link

also LGTM based on what i know so far
lets get this merged, every state of xz is better than what we have now

@TruncatedDinoSour
Copy link

also LGTM based on what i know so far lets get this merged, every state of xz is better than what we have now

yeah, if anything we can fix later, this at least rules out the malicious commits, i +1 the 'merge it now and see later' idea rn

@lcarilla
Copy link

@TruncatedDinoSour your a maintainer of this, right? can you remove jia from the repo if not done yet

@TruncatedDinoSour
Copy link

@TruncatedDinoSour your a maintainer of this, right? can you remove jia from the repo if not done yet

i'm not, i'm just trying to be active and stay up to date with the situation whilst giving my input on this, looking for ways to contribute to resolving this asap as i also use this project,, i've looked through jias commits trying to find anything suspicious, skimming through it didn't uncover much and paying a bit more attention to it didn't uncover much either, i did found an xz-related kernel patch though fairly recently : https://lore.kernel.org/lkml/20240320183846.19475-1-lasse.collin@tukaani.org/t/

@theCapypara
Copy link

There is nobody that could merge this at the moment, as the accounts of both maintainers have been suspended appereantly:
#92 (comment)

@lcarilla
Copy link

is there anyone here who is able to merge this damn PR 😄 ?
or has jia completely taken over all this

@lcarilla
Copy link

There is nobody that could merge this at the moment, as the accounts of both maintainers have been suspended appereantly: #92 (comment)

damn.

@lcarilla
Copy link

lcarilla commented Mar 29, 2024

ill contact github support, maybe they can help 😄 , but getting this merged is not gonna fix all the already infected systems. crazy how one github account can take over an OSS project basically every linux distro relies onj

@MammaUauua
Copy link

Why did you revert 8c9b8b2 🤣

@MammaUauua
Copy link

7c8ad80 is not necessary

@skull-squadron
Copy link

@Alcaro You win a -LoC award in any event. A code base should be as small as possible, but no smaller. Every add'l line of code is a place for a bug to hide, UB, or even possibly a malicious implant. By far, corporate FOSS megaprojects tend to be the worst.

@@ -135,12 +135,12 @@ typedef uint32_t (*crc32_func_type)(
// This resolver is shared between all three dispatch methods. It serves as
// the ifunc resolver if ifunc is supported, otherwise it is called as a
// regular function by the constructor or first call resolution methods.
// The function attributes are needed for safe IFUNC resolver usage with GCC.
// The funcion attributes are needed for safe IFUNC resolver usage with GCC.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this pr just reverts changes lol, including 8c9b8b2

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was easier to revert the entire commit than only revert parts of it.

That entire line is removed in a subsequent commit (1efea8d), so the end result is the same.

@Larhzu Larhzu closed this Apr 5, 2024
@tukaani-project tukaani-project locked as resolved and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet