4 ms·

I like the comment that started with "way too many arm chair 'researchers' in this thread" and then goes on to rudely say that the maintainers are doing a bad j

I like the comment that started with "way too many arm chair 'researchers' in this thread" and then goes on to rudely say that the maintainers are doing a bad job because they merged in the original changes by Jia Tan. What are you sitting on, if not an arm chair? We all agree that the xz attack was of unparalleled sophistication and complexity, spread carefully over years, funded by a State. Many people were taken in so how is it helpful to pile on Jia Tan's primary victims?

prmoustache 2y ago
The question is still very relevant. Why are PR like this merged to begin with?
- joh6nn 2y ago
  The malicious commit was designed to be confusing, as noted in the first comment of the investigation: > but calls to safe_fprintf were replaced with calls to the unsafe fprintf. The diff doesn't make this obvious due to the removal of a newline in a parameter list. It wasn't noticed because it was specifically designed not to be obvious.
- pas 2y ago
  because there are hundreds of thousands of programmers who don't have the need for "better" and are willing to put up with C and shell scripts, and "small incremental changes"
freedomben 2y ago
I thought that was a good question, and certainly one I'd like to know the answer too, but I very much agree it was done rudely. It didn't seem like it was asked in good faith. Anyone who has maintained large/complex software like this knows that name recognition is worth a ton, and it kind of has to be that way. It's just not practical at all to scrutinize every commit/change as though the committer is an adversary, and particularly when you know the person it is not a reasonable ask. I would bet the truth is basically "yes, we knew him so it didn't get full scrutiny," and honestly that's an honest (but hard to give) answer. I do hope (perhaps naively) that this (security code reviews) is something AI can get really good at in the future, because that would be a real value add IMHO.
- BalinKing 2y ago
  This change doesn’t look like it would’ve required much scrutiny, though… it’s three lines long and seems (admittedly as an uninformed outsider) to be obviously wrong. Like, ignoring the fact that Jia Tan happened to be an adversary, I’m kinda shocked that their code review process let it through—unless the standard quite literally is “recognized contributors get rubber-stamped without further review.”
  - joh6nn 2y ago
    Right, but that was the exact nature of the attack: it's a small commit that doesn't look like it needs a lot of scrutiny. Like, I get that you meant "it wouldn't take much scrutiny to find this" but I mean "it doesn't look like it needs to be scrutinized". Especially because, as mentioned in the first comment of the investigation, the change to an unsafe behavior is deliberately obscured by the formatting of the diff. It's like Where's Wal(do|ly): once you know where to look, it's obvious, but if you don't even know you're supposed to be looking for it, you may never find it
    - chefandy 2y ago
      Right-- A busy maintainer sees a weird looking commit-- but it's three lines long, submitted from a known contributor, and the tests pass. It was very carefully planned to be innocuous-looking enough to not trigger any concerns with a casual once-over (oh, it just changes the way an error is printed) and obfuscated enough to not be obviously malicious because of the diff formatting, and submitted by a reliable known contributor. Each piece was designed to make a rigorous code review as unlikely as they could possibly make it. Sure, that's not how it's SUPPOSED to happen, but I'll eat my hat if at least 95% of people who've approved a PR at some point couldn't have been walked down that path by a dedicated attacker over time. Hopefully this has been enough of a jolt to make that less likely the next time someone tries it. People often cite death and taxes as the only certainties in life-- we could easily include human fallibility.
      - Bluestein 2y ago
pas 2y ago
I recently removed the arm rests from my shitty IKEA chair (to apply WD40 silicone, because the standard cheap mechanism was creaking every time I leaned back, even if it was locked), and decided not to put them back, because they incentivized slouching so akcheually some people are sitting on a block of concrete! theorizing that it was wrong to merge in itself is not victim blaming. but of course piling up in GH discussions and issues unconstructively, and just expressing opinions is bullying.
psychoslave 2y ago
It seems that blaming victims can really provide some power thrill, to which human can easily become addicted to. Raising competent empathic well balanced individuals is difficult, to say the least. And it’s not like the so called world leader elites really show they are some paragons of these traits.

prmoustache 2y ago

The question is still very relevant. Why are PR like this merged to begin with?

joh6nn 2y ago
The malicious commit was designed to be confusing, as noted in the first comment of the investigation: > but calls to safe_fprintf were replaced with calls to the unsafe fprintf. The diff doesn't make this obvious due to the removal of a newline in a parameter list. It wasn't noticed because it was specifically designed not to be obvious.
pas 2y ago
because there are hundreds of thousands of programmers who don't have the need for "better" and are willing to put up with C and shell scripts, and "small incremental changes"

freedomben 2y ago

I thought that was a good question, and certainly one I'd like to know the answer too, but I very much agree it was done rudely. It didn't seem like it was asked in good faith. Anyone who has maintained large/complex software like this knows that name recognition is worth a ton, and it kind of has to be that way. It's just not practical at all to scrutinize every commit/change as though the committer is an adversary, and particularly when you know the person it is not a reasonable ask. I would bet the truth is basically "yes, we knew him so it didn't get full scrutiny," and honestly that's an honest (but hard to give) answer. I do hope (perhaps naively) that this (security code reviews) is something AI can get really good at in the future, because that would be a real value add IMHO.

BalinKing 2y ago
This change doesn’t look like it would’ve required much scrutiny, though… it’s three lines long and seems (admittedly as an uninformed outsider) to be obviously wrong. Like, ignoring the fact that Jia Tan happened to be an adversary, I’m kinda shocked that their code review process let it through—unless the standard quite literally is “recognized contributors get rubber-stamped without further review.”
- joh6nn 2y ago
  Right, but that was the exact nature of the attack: it's a small commit that doesn't look like it needs a lot of scrutiny. Like, I get that you meant "it wouldn't take much scrutiny to find this" but I mean "it doesn't look like it needs to be scrutinized". Especially because, as mentioned in the first comment of the investigation, the change to an unsafe behavior is deliberately obscured by the formatting of the diff. It's like Where's Wal(do|ly): once you know where to look, it's obvious, but if you don't even know you're supposed to be looking for it, you may never find it
  - chefandy 2y ago
    Right-- A busy maintainer sees a weird looking commit-- but it's three lines long, submitted from a known contributor, and the tests pass. It was very carefully planned to be innocuous-looking enough to not trigger any concerns with a casual once-over (oh, it just changes the way an error is printed) and obfuscated enough to not be obviously malicious because of the diff formatting, and submitted by a reliable known contributor. Each piece was designed to make a rigorous code review as unlikely as they could possibly make it. Sure, that's not how it's SUPPOSED to happen, but I'll eat my hat if at least 95% of people who've approved a PR at some point couldn't have been walked down that path by a dedicated attacker over time. Hopefully this has been enough of a jolt to make that less likely the next time someone tries it. People often cite death and taxes as the only certainties in life-- we could easily include human fallibility.
    - Bluestein 2y ago

pas 2y ago

I recently removed the arm rests from my shitty IKEA chair (to apply WD40 silicone, because the standard cheap mechanism was creaking every time I leaned back, even if it was locked), and decided not to put them back, because they incentivized slouching so akcheually some people are sitting on a block of concrete! theorizing that it was wrong to merge in itself is not victim blaming. but of course piling up in GH discussions and issues unconstructively, and just expressing opinions is bullying.

psychoslave 2y ago

It seems that blaming victims can really provide some power thrill, to which human can easily become addicted to. Raising competent empathic well balanced individuals is difficult, to say the least. And it’s not like the so called world leader elites really show they are some paragons of these traits.