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 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 @WPalant It sounds like the person got frustrated and left the project after trying for four months to have their changes approved. From reading the thread, I don’t blame them.
@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 @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.
