Bug#1081434: dpkg-source: should refuse to apply indented diffs, or at least give a warning

[Simon McVittie]

Package: dpkg-dev
Version: 1.22.11
Severity: normal

gtk+3.0_3.24.43-3 contains a patch in the `git format-patch` dialect of
DEP-3 format, whose long description contains a diff illustrating how
to test the change (patch attached for reference).

The author of this patch clearly meant for the machine-readable part
of the patch to apply changes to gtk/gtkmessagedialog.c only.
`git apply` and `git am` have this behaviour.

However, patch(1) and therefore dpkg-source looks for anything in the
patch text that looks vaguely diff-like, even if it's indented (!), and
applies it. The result is that in the uploaded gtk+3.0_3.24.43-3 package,
both gtk/gtkmessagedialog.c and demos/gtk-demo/dialog.c have been edited
(reported as #1081179).

I think the same thing could equally well happen with the
more-Debian-specific dialect of DEP-3 where the long human-readable
message is in an indented deb822-style "Description", although I don't have
a reproducer for that. However, the old dpatch framework would not have been
susceptible to this, because it prepended "#" to all the non-diff content.

Ideally, I think dpkg-source would (configure patch(1) to) refuse to apply
diffs that are indented in this way - applying them seems like a violation
of the principle of least astonishment.

Or, if that's considered to be too much of a compatibility break, I think
it would be useful for dpkg-source to issue a warning on such diffs.
patch(1) does output "(Patch is indented 4 spaces.)" when I apply that
diff, but it seems that dpkg-source suppresses that output.

I could even imagine this becoming a security issue, if the long
description of a patch contains instructions for changes to be made
during testing that are not suitable for production (for example if the
long description describes how to stub out authentication in order to
test something).

smcv

[Guillem Jover]

Hi!

Post by Simon McVittie
Package: dpkg-dev
Version: 1.22.11
Severity: normal
However, patch(1) and therefore dpkg-source looks for anything in the
patch text that looks vaguely diff-like, even if it's indented (!), and
applies it. The result is that in the uploaded gtk+3.0_3.24.43-3 package,
both gtk/gtkmessagedialog.c and demos/gtk-demo/dialog.c have been edited
(reported as #1081179).

Yes, I realized this some time ago, which had security implications:

CVE-2017-8283 <https://www.openwall.com/lists/oss-security/2017/04/20/2>

(I think this is even worse than the indented problem though, GNU patch(1)
also accepts hunks prefixed with an «X»!)

Post by Simon McVittie
Ideally, I think dpkg-source would (configure patch(1) to) refuse to apply
diffs that are indented in this way - applying them seems like a violation
of the principle of least astonishment.

When I checked this at the time of the above CVE, I didn't find any
way to configure patch(1) to either ignore these or reject them, the
only way I found to avoid this was to reject them from
Dpkg::Source::Patch's parser, which seemed enticing as at the time
there was no indented patch in the archive, but was also a way more
intrusive change to add into a stable system (AFAIR). I'm not sure
what would be the status now, but there are two categories of potential
breakage:

- indented patches that are intended to be applied (I'd assume this
is very rare or non-existing, but would be legitimate breakage).
- indented patches within the leading text which would not be
intended to be applied (those should already be able to apply or
patch(1) would fail), and rejecting them while fixing the
unintended application would make packages FTBFS, which I suppose
might not be a bad thing, but it's going to be fallout to deal
with.

So, I'd be fine with rejecting these. And I had a draft patch at the time:

https://git.hadrons.org/cgit/debian/dpkg/dpkg.git/commit/?h=pu/perl-Dpkg-Source-Patch-parse-indented&id=531e42e025f1346b234c331791ded926c5adde50

Which I could turn into making this fatal.

Post by Simon McVittie
Or, if that's considered to be too much of a compatibility break, I think
it would be useful for dpkg-source to issue a warning on such diffs.
patch(1) does output "(Patch is indented 4 spaces.)" when I apply that
diff, but it seems that dpkg-source suppresses that output.
I could even imagine this becoming a security issue, if the long
description of a patch contains instructions for changes to be made
during testing that are not suitable for production (for example if the
long description describes how to stub out authentication in order to
test something).

I think this POSIX behavior is completely broken, and I agree the git
one is way better, but right now we are limited by what patch(1)
offers us. :(

I've been rather unsatisfied with having to delegate the patch
application to the system patch(1), because of this kind of
misbehavior and because each system patch(1) implementation differs
in how to handle patches securely, most of them for example do not
properly avoid path traversal issues, so I'm currently forced to
require GNU patch on the system. I might need to explore again
perhaps implementing the patch application fully in Perl. :/

Thanks,
Guillem

[Tj]

Package: dpkg-dev
Followup-For: Bug #1081434
X-Debbugs-Cc: ***@proton.me

Excuse me for butting in; I'm not sure if this could be a solution or
not. I just proposed a patch in bug #1099170 "dpkg-source: Source/Patch:
fix parsing of patch header" and was pointed to this bug as a possible
duplicate - it is not.

However after working with the code I wonder if there might be an
elegant way to avoid the spawned 'patch' tool mis-reading the header in
this way:

analyze() keeps track of the entire header:

$patch_header .= "$line\n";
...
*$self->{analysis}{$destdir}{patchheader} = $patch_header;
return *$self->{analysis}{$destdir};

apply() and check_apply() both do:

my $analysis = $self->analyze($destdir, %opts);
...
$self->ensure_open('r');

At this point the spawned process is about to be passed its input via
standard input. Could the solution be to simply seek() beyond the header
before spawn() dup's the file descriptor - something like:

seek($self, length( %{$analysis->{patchheader}} ), 0);

[Tj]

Package: dpkg-dev
Followup-For: Bug #1081434
X-Debbugs-Cc: ***@proton.me

Seeking beyond the patch header before spawning the system patch tool solves
this in all my local testing, including using Simon's example patch that
triggered this.