Hi!
On Wed, 2014-04-30 at 00:12:56 +0200, Jakub Wilk wrote:
> * Guillem Jover <guil...@debian.org>, 2014-04-29, 23:40:
> >>>1. Simply revert the patch, and ignore issues w/ partial upgrades (at
> >>>least for now?).
> >>>2. Revert the patch and add versioned depdendencies against the
> >>>working patch package. This might require some dist-upgrade tests,
> >>>though.
> >>>3. Fix the patch to take into account the old behaviour, by checking
> >>>if either of the filenames (escaped and unescaped) are unsafe.
> >>>
> >>>I guess the last one is the “safest option”.
> >>
> >>For a quick fix, 3 is probably the best.
> >
> >Did you mean 1? After having checked to implement 3, there's many parts of
> >the code that need to be changed and moved around, I'll try to cook an
> >actual patch to see how bad it is though.
>
> I had assumed that the patch for 3 would we straightforward. If this is not
> the case, then I'd go for 1 for now, and maybe we'll figure out something
> better later. Of course, Security Team's option may vary.
Well it looks a bit gross, because it might end up generating strange
directories, as those are recorded by the same code analyzing and
deciding if a pathname is safe or not. So if we have to check both
we don't know which one will end up being used by patch and would
need to create them all. :/
And actually, after having pondered about this for a bit, it just
occurred to me that the most strightforward and safe solution of all is
to simply and completely ban C-style strings, which would be option 4.
Because we avoid any discrepancy in behaviour from dpkg-dev and patch,
avoid any strange directory creation side-effects, and works everywhere,
even on systems with old/new or non-GNU patch programs. Also any such
new patches would not extract correctly on old suites anyway. And why
should we support those encoded characters at all, when we've lived
w/o them all this time.
Attached a non-tested quick patch implementing this. I'll start
testing it and preparing packages for all suites.
> >There's also the newly supported git formatted patches now recognized by
> >patch, “fortunately” Dpkg::Source::Patch does not understand them and
> >because it creates any necessary directories (or what looks like one), I
> >don't see a way it can be exploited. But I might be short on imagination
> >at this moment.
>
> Oh, I completely forgot about git patches. I have a hunch that there's a
> clever way to exploit them. :\
That's what I fear too, but I've been trying to concoct some PoC and
failed.
Thanks,
Guillem
diff --git a/scripts/Dpkg/Source/Patch.pm b/scripts/Dpkg/Source/Patch.pm
index 712e743..2d8fb81 100644
--- a/scripts/Dpkg/Source/Patch.pm
+++ b/scripts/Dpkg/Source/Patch.pm
@@ -332,31 +332,6 @@ sub _getline {
return $line;
}
-my %ESCAPE = ((
- 'a' => "\a",
- 'b' => "\b",
- 'f' => "\f",
- 'n' => "\n",
- 'r' => "\r",
- 't' => "\t",
- 'v' => "\cK",
- '\\' => '\\',
- '"' => '"',
-), (
- map { sprintf('%03o', $_) => chr($_) } (0..255)
-));
-
-sub _unescape {
- my ($diff, $str) = @_;
-
- if (exists $ESCAPE{$str}) {
- return $ESCAPE{$str};
- } else {
- error(_g('diff %s patches file with unknown escape sequence \\%s'),
- $diff, $str);
- }
-}
-
# Fetch the header filename ignoring the optional timestamp
sub _fetch_filename {
my ($diff, $header) = @_;
@@ -366,12 +341,7 @@ sub _fetch_filename {
# Is it a C-style string?
if ($header =~ m/^"/) {
- $header =~ m/^"((?:[^\\"]|\\.)*)"/;
- error(_g('diff %s patches file with unbalanced quote'), $diff)
- unless defined $1;
-
- $header = $1;
- $header =~ s/\\([0-3][0-7]{2}|.)/_unescape($diff, $1)/eg;
+ error(_g('diff %s patches file using unsupported C-style string'), $diff);
} else {
# Tab is the official separator, it's always used when
# filename contain spaces. Try it first, otherwise strip on space
