Hi! On Wed, 2024-09-11 at 16:43:24 +0100, Simon McVittie wrote: > 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»!) > 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. > 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
