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

Hi!

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»!)

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.

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

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);

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.

Es gibt eine Familienspende in Höhe von 1.850.000,00 USD von Cheng Charlie
Saephan. Bitte antworten Sie für weitere Informationen. Denken Sie daran,
Ihrer Familie und den Bedürftigen in Ihrer Umgebung Gutes zu tun.

Dies ist bereits der zweite Versuch, Sie zu erreichen. Bitte antworten Sie
für weitere Details.

I was reminded of this by an indented diff similar to my GTK example
accidentally getting into Debian's i3 package recently
(https://bugs.debian.org/1126992). See also
https://bugs.debian.org/1081436 in which I suggested that `gbp pq
export` could avoid this indented-diff-in-commit-message pattern.

This seems like quite an elegant solution to this - if dpkg is already
aware of the boundary between header and patch, it adds very little new
code to be able to do this.

Guillem, would you be willing to consider Tj's patch? At least for
unstable?

     smcv

Hi!

Sorry, was meaning to comment on this at the time but it seems I
missed doing that.

While I agree the patch in itself is pretty elegant in the dpkg
context, there is still the problem that this would hide the
underlying deficiencies with patch(1) not handling this correctly,
where I'm assuming things like quilt would be problematic anyway.
(Unless quilt it is also already stripping the header?) And even
besides quilt, I'm not sure what else might be using patch(1)
directly to apply things. :/

Thanks,
Guillem

Having said that, I'm not sure whether what Tj proposed here would have
avoided the i3 bug or not. In the GTK bug the part of the commit message
that looked like a patch was indented and would not have been applied by
`git am`, but in i3 it was unindented and in a Github-style fenced code
block (``` before and after) which *would* have been applied by `git
am`, and I'm not sure the format is strict enough to distinguish that
from the actual patch content.

(Really this is a general problem with the patch format and in-band
signalling, rather than anything specific to dpkg or git.)

     smcv