Three years ago, #FDroid had a similar kind of attempt as the #xz #backdoor. A new contributor submitted a merge request to improve the search, which was oft requested but the maintainers hadn't found time to work on. There was also pressure from other random accounts to merge it. In the end, it became clear that it added a #SQLinjection #vuln. In this case, we managed to catch it before it was merged. Since similar tactics were used, I think its relevant now
I'm going to ignore that...
@gentoobro @eighthave You could easily drop the last word and have another true-ism that is perhaps more relevant.
@sehe @gentoobro Free software passion projects are wonderful things. Payment often kills the passion that makes them great. Maintenance of infrastructure is not a passion project and that is what we all should be paying for. I see the #EU moving towards this kind of funding. There are many opportunities for doing this well: for example, orgs like #NSA get billions to improve #cyber-defense. But they are subordinate to the offensive side who want the 0days. This needs to be exactly the opposite.
@eighthave this doesn't read like there was intent. Could've been an honest mistake.
Apart from that it was exhausting reading the back and forth about what changes should be added. Not really a lesson in clear communication
@Optional clear communication definitely suffers when maintainers are overloaded, stressed out and feel ganged up on. I think that's another key takeaway from this current incident. For a well resourced actor, it is not too hard to social engineer themselves into a trusted position when projects get into that position. That happens all too often, unfortunately.
@Optional @eighthave somebody was jiggling the doorknob to gauge how soft the target is. I think at that point it could just be a spy's side project, and they can use success there to show their boss "hey I think I found a real opportunity here, give me a team and a budget to build something bigger".
@Optional @eighthave That's possible, but honestly, does it matter? Either way, you're introducing security problems, whether through incompetence or intention of the author of the PR.
@eighthave Similar things happened in the past to the Arch's AUR repository and not only one time. Something like that was also on github as well and not that long ago, as I remember correctly.
@eighthave Yeah, comments pushing for quick merges instead of adding actual code reviews, or for handing over maintainership altogether, should immediately raise some flags in maintainers' minds. Personally, I found that further probing and asking for actual contributions were good strategies to evaluate intentions.
@eighthave Interesting. However, with a project that relies on string concatenation for producing SQL queries rather than prepared statements – this kind of thing is to be expected, most likely it was an honest mistake. The issue was also fairly obvious, someone attempting to introduce it maliciously should have expected it to be caught.
@WPalant Ironically, it could be because I was so aware of the crappy state of things that I caught this. The author of the SQL code was no longer involved. I suck at SQL but was well aware that string concat to build SQL queries is a bad idea. So I was terrified to merge any changes to the SQL without really confirming them.
In any case, we've since replaced all that crazy code with libraries that provide much more protection.
@WPalant Because the submitter deleted their account as a response to the review, I think it could be an deliberate attempt to insert the vuln. Plus all the attention from random new accounts. If it had been a normal review process, I could see how it could have been an honest mistake. But that scenario also makes it more attractive to the attacker, since making a mistake there is quite plausible, and could serve as an easy cover story.
@eighthave
The suspicious part is not the SQL injection, that may have been an honest mistake. It's the pressure to merge the patch in a hurry, before anyone has time to review it.
@eighthave Thanks for being diligent!
@eighthave I have to say that at least for a non-developer like myself, the insistence that the patch should be merged just as-is without changes kind of stands out as a bit of a red flag. It seems like they basically said multiple times to just integrate as-is without further discussion before doing so.
@nazokiyoubinbou I agree it is a red flag, but it is also perfectly normal for people to want their changes to be merged, especially when they are doing it on a volunteer basis and they want to wrap up that piece of work. Software is so often a rabbit hole that can easily suck down all your time. That's why this is a vulnerability. Developers understand that people want things merged, and it is generally worthwhile to merge improvements if they don't cause problems, even if they are not "done".
@eighthave It was the "just merge it as-is without further discussion" part that gets me. Strong implications that it was desired there should be no changes.
@eighthave @davidgerard Honestly: I think if he had bad intentions he probably would have just complied. To me this person sounded like an eager developer with to much ego and to little knowledge. But it was really interesting to read how close this MR was to being merged and how much damage it could have done, intentional or unintentional.
