Bug#696230: lintian: Signed Debian control block parsing can be fooled
Control: tags -1 patch
On 2012-12-18 13:24, Guillem Jover wrote:
> Package: lintian
> Version: 2.5.12
> Severity: important
> File: lib/Lintian/Util.pm
> User: ansgar@debian.org
> Usertags: gpg-clearsign
>
> Hi!
>
Hi,
Thanks for the report.
> The current parsing code in visit_dpkg_paragraph() does not correctly
> parse Armor Header Lines (as per RFC4880), which can make it get very
> confused on hostile files, like external .dsc or .changes. An example
> bogus file is attached, other variants are possible by changing the
> structure of the bogus markers and their content. Compare lintian
> ouput with what gpg outputs with:
>
> [...]
>
> Ansgar has been filing this kind of bugs, and pointed out to #695855.
>
> Thanks,
> Guillem
Lintian, in the particular case, would discard the second (and any
later) paragraphs. So even without the all PGP headers you can probably
have it do "fun" results for dsc and changes files.
I have created a patch that wil make Lintian much stricter in this
regard. I figured that any "sane" control is either completely unsigned
or completely signed by a single signature. So with this patch, Lintian
will reject files with more than one signed message.
Review/comments welcome.
~Niels
>From 1d9c03ff72e7a504e29b02e6b9056dd138690e6d Mon Sep 17 00:00:00 2001
From: Niels Thykier <niels@thykier.net>
Date: Tue, 18 Dec 2012 21:04:21 +0100
Subject: [PATCH] L::Util: Be very strict around PGP markers
Be very strict in parsing and handling of PGP headers. Particularly,
reject any file with a "suspicious" use of PGP messages (like multiple
signed messages).
Signed-off-by: Niels Thykier <niels@thykier.net>
---
frontend/lintian | 8 +-
lib/Lintian/Util.pm | 152 +++++++++++++++++++--
t/scripts/Lintian/Util/data/pgp-eof-missing-sign | 5 +
t/scripts/Lintian/Util/data/pgp-leading-unsigned | 14 ++
t/scripts/Lintian/Util/data/pgp-malformed-header | 11 ++
t/scripts/Lintian/Util/data/pgp-no-end-pgp-header | 7 +
t/scripts/Lintian/Util/data/pgp-sig-before-start | 7 +
t/scripts/Lintian/Util/data/pgp-trailing-unsigned | 14 ++
t/scripts/Lintian/Util/data/pgp-two-signatures | 16 +++
t/scripts/Lintian/Util/data/pgp-two-signed-msgs | 19 +++
t/scripts/Lintian/Util/data/pgp-unexpected-header | 6 +
t/scripts/Lintian/Util/dctrl-parser.t | 54 ++++++++
12 files changed, 304 insertions(+), 9 deletions(-)
create mode 100644 t/scripts/Lintian/Util/data/pgp-eof-missing-sign
create mode 100644 t/scripts/Lintian/Util/data/pgp-leading-unsigned
create mode 100644 t/scripts/Lintian/Util/data/pgp-malformed-header
create mode 100644 t/scripts/Lintian/Util/data/pgp-no-end-pgp-header
create mode 100644 t/scripts/Lintian/Util/data/pgp-sig-before-start
create mode 100644 t/scripts/Lintian/Util/data/pgp-trailing-unsigned
create mode 100644 t/scripts/Lintian/Util/data/pgp-two-signatures
create mode 100644 t/scripts/Lintian/Util/data/pgp-two-signed-msgs
create mode 100644 t/scripts/Lintian/Util/data/pgp-unexpected-header
create mode 100644 t/scripts/Lintian/Util/dctrl-parser.t
diff --git a/frontend/lintian b/frontend/lintian
index e8e02ef..a64b0bc 100755
--- a/frontend/lintian
+++ b/frontend/lintian
@@ -915,7 +915,13 @@ while (my $arg = shift) {
# file?
if (-f $arg) {
if ($arg =~ m/\.(?:u?deb|dsc|changes)$/o){
- $pool->add_file($arg);
+ eval {
+ $pool->add_file($arg);
+ };
+ if ($@) {
+ print STDERR "Skipping $arg: $@";
+ $exit_code = 2;
+ }
} else {
fail("bad package file name $arg (neither .deb, .udeb, .changes or .dsc file)");
}
diff --git a/lib/Lintian/Util.pm b/lib/Lintian/Util.pm
index a12d93a..5369dce 100644
--- a/lib/Lintian/Util.pm
+++ b/lib/Lintian/Util.pm
@@ -242,8 +242,14 @@ refer to L</CONSTANTS> for the list of constants and their meaning.
The default value for FLAGS is 0.
If the file is empty (i.e. it contains no paragraphs), the method will
-contain an I<empty> list. Lines looking like a GPG-signature is
-ignored when parsing the file.
+contain an I<empty> list. The may contain the contents inside a PGP
+message with a signature.
+
+visit_dpkg_paragraph will require the PGP headers to be correct (if
+present) and require that the entire file is covered by the signature.
+However, it will I<not> validate the signature (in fact anything in
+the PGP SIGNATURE part can be empty). The signature should be
+validated separatedly.
visit_dpkg_paragraph will pass paragraphs to CODE as they are
completed. If CODE can process the paragraphs as they are seen, very
@@ -310,6 +316,45 @@ underscores.
A comment line appeared and FLAGS contained DCTRL_NO_COMMENTS.
+=item PGP signature seen before start of signed message
+
+A "BEGIN PGP SIGNATURE" header is seen and a "BEGIN PGP MESSAGE" has
+not been seen yet.
+
+=item Two PGP signatures (first one at line %d)
+
+Two "BEGIN PGP SIGNATURE" headers are seen in the same file.
+
+=item Unexpected %s header
+
+A valid PGP header appears (e.g. "BEGIN PUBLIC KEY BLOCK").
+
+=item Malformed PGP header
+
+An invalid or malformed PGP header appears.
+
+=item Expected at most one signed message (previous at line %d)
+
+Two "BEGIN PGP MESSAGE" headers appears in the same message.
+
+=item End of file but expected a "END PGP SIGNATURE" header
+
+The file ended after a "BEGIN PGP SIGNATURE" header without being
+followed by a "END PGP SIGNATURE".
+
+=item PGP MESSAGE header must be first content if present
+
+The file had content before PGP MESSAGE.
+
+=item Data after the PGP SIGNATURE
+
+The file had data after the PGP SIGNATURE block ended.
+
+=item End of file before "BEGIN PGP SIGNATURE"
+
+The file had a "BEGIN PGP MESSAGE" header, but no signature was
+present.
+
=back
=cut
@@ -322,13 +367,13 @@ sub visit_dpkg_paragraph {
my $open_section = 0;
my $last_tag;
my $debconf = $flags & DCTRL_DEBCONF_TEMPLATE;
+ my $signed = 0;
+ my $signature = 0;
local $_;
while (<$CONTROL>) {
chomp;
- # FIXME: comment lines are only allowed in debian/control and should
- # be an error for other control files.
if (/^\#/) {
next unless $flags & DCTRL_NO_COMMENTS;
die "syntax error at line $.: Comments are not allowed.\n";
@@ -343,18 +388,101 @@ sub visit_dpkg_paragraph {
$open_section = 0;
}
}
- # pgp sig?
- elsif (m/^-----BEGIN PGP SIGNATURE/) { # skip until end of signature
+ # pgp sig? Be strict here (due to #696230)
+ # According to http://tools.ietf.org/html/rfc4880#section-6.2
+ # The header MUST start at the beginning of the line and MUST NOT have
+ # any other text (except whitespace) after the header.
+ elsif (m/^-----BEGIN PGP SIGNATURE-----\s*$/) { # skip until end of signature
+ my $saw_end = 0;
+ if (not $signed or $signature) {
+ die "syntax error at line $.: PGP signature seen before start of signed message\n"
+ if not $signed;
+ die "syntax error at line $.: Two PGP signatures (first one at line $signature)\n";
+ }
+ $signature = $.;
while (<$CONTROL>) {
- last if m/^-----END PGP SIGNATURE/o;
+ if (m/^-----END PGP SIGNATURE-----\s*$/o) {
+ $saw_end = 1;
+ last;
+ }
}
+ # The "at line X" may seem a little weird, but it keeps the
+ # message format identical.
+ die "syntax error at line $.: End of file but expected a \"END PGP SIGNATURE\" header\n"
+ unless $saw_end;
}
# other pgp control?
- elsif (m/^-----BEGIN PGP/) { # skip until the next blank line
+ elsif (m/^-----(?:BEGIN|END) PGP/) {
+ # At this point it could be a malformed PGP header or one
+ # of the following valid headers (RFC4880):
+ # * BEGIN PGP MESSAGE
+ # - Possibly a signed Debian CTRL, so okay (for now)
+ # * BEGIN PGP {PUBLIC,PRIVATE} KEY BLOCK
+ # - Valid header, but not a Debian CTRL file.
+ # * BEGIN PGP MESSAGE, PART X{,/Y}
+ # - Valid, but we don't support partial messages, so
+ # bail on those.
+
+ unless (m/^-----BEGIN PGP SIGNED MESSAGE-----\s*$/) {
+ # Not a (full) PGP MESSAGE; reject.
+
+ my $key = qr/(?:BEGIN|END) PGP (?:PUBLIC|PRIVATE) KEY BLOCK/;
+ my $msgpart = qr{BEGIN PGP MESSAGE, PART \d+(?:/\d+)?};
+ my $msg = qr/(?:BEGIN|END) PGP (?:(?:COMPRESSED|ENCRYTPED) )?MESSAGE/;
+
+ if (m/^-----($key|$msgpart|$msg)-----\s*$/o) {
+ die "syntax error at line $.: Unexpected $1 header\n";
+ } else {
+ die "syntax error at line $.: Malformed PGP header\n";
+ }
+ } else {
+ if ($signed) {
+ die "syntax error at line $.: Expected at most one signed message" .
+ " (previous at line $signed)\n"
+ }
+ if ($sline > -1) {
+ # NB: If you remove this, keep in mind that it may allow two paragraphs to
+ # merge. Consider:
+ #
+ # Field-P1: some-value
+ # -----BEGIN PGP SIGANTURE----
+ #
+ # Field-P2: another value
+ #
+ # At the time of writing: If $open_section is
+ # true, it will remain so until the empty line
+ # after the PGP header.
+ die "syntax error at line $.: PGP MESSAGE header must be first" .
+ " content if present\n";
+ }
+ $signed = $.;
+ }
+
+ # skip until the next blank line
while (<$CONTROL>) {
last if /^\s*$/o;
}
}
+ # did we see a signature already? We allow all whitespace/comment lines
+ # outside the signature.
+ elsif ($signature) {
+ # Accept empty lines after the signature.
+ next if m/^\s*$/;
+
+ #NB: If you remove this, keep in mind that it may allow two paragraphs to
+ # merge. Consider:
+ #
+ # Field-P1: some-value
+ # -----BEGIN PGP SIGANTURE----
+ # [...]
+ # -----END PGP SIGANTURE----
+ # Field-P2: another value
+ #
+ # At the time of writing: If $open_section is
+ # true, it will remain so until the empty line
+ # after the PGP header.
+ die "syntax error at line $.: Data after the PGP SIGNATURE\n";
+ }
# new empty field?
elsif (m/^([^: \t]+):\s*$/o) {
$sline = $. if not $open_section;
@@ -410,6 +538,14 @@ sub visit_dpkg_paragraph {
}
# pass the last section (if not already done).
$code->($section, $sline) if $open_section;
+
+ # Given the API, we cannot use this check to prevent any paragraphs from being
+ # emitted to the code argument, so we might as well just do this last.
+ if ($signed and not $signature) {
+ # The "at line X" may seem a little weird, but it keeps the
+ # message format identical.
+ die "syntax error at line $.: End of file before \"BEGIN PGP SIGNATURE\"\n";
+ }
}
=item read_dpkg_control (FILE[, FLAGS[, LINES]])
diff --git a/t/scripts/Lintian/Util/data/pgp-eof-missing-sign b/t/scripts/Lintian/Util/data/pgp-eof-missing-sign
new file mode 100644
index 0000000..78e5ee9
--- /dev/null
+++ b/t/scripts/Lintian/Util/data/pgp-eof-missing-sign
@@ -0,0 +1,5 @@
+-----BEGIN PGP SIGNED MESSAGE-----
+
+Package: lintian
+
+# Missing signature block
diff --git a/t/scripts/Lintian/Util/data/pgp-leading-unsigned b/t/scripts/Lintian/Util/data/pgp-leading-unsigned
new file mode 100644
index 0000000..0b6b949
--- /dev/null
+++ b/t/scripts/Lintian/Util/data/pgp-leading-unsigned
@@ -0,0 +1,14 @@
+Package: dpkg
+
+# Unsigned above, signed below (bad)
+
+-----BEGIN PGP SIGNED MESSAGE-----
+
+Package: lintian
+
+-----BEGIN PGP SIGNATURE-----
+
+Some signature.
+
+-----END PGP SIGNATURE-----
+
diff --git a/t/scripts/Lintian/Util/data/pgp-malformed-header b/t/scripts/Lintian/Util/data/pgp-malformed-header
new file mode 100644
index 0000000..f83fb77
--- /dev/null
+++ b/t/scripts/Lintian/Util/data/pgp-malformed-header
@@ -0,0 +1,11 @@
+# Missing a dash in the end
+
+-----BEGIN PGP SIGNED MESSAGE----
+
+Package: linitan
+
+-----BEGIN PGP SIGNATURE----
+
+Some signature.
+
+-----END PGP SIGNATURE----
diff --git a/t/scripts/Lintian/Util/data/pgp-no-end-pgp-header b/t/scripts/Lintian/Util/data/pgp-no-end-pgp-header
new file mode 100644
index 0000000..6d15d98
--- /dev/null
+++ b/t/scripts/Lintian/Util/data/pgp-no-end-pgp-header
@@ -0,0 +1,7 @@
+-----BEGIN PGP SIGNED MESSAGE-----
+
+Package: lintian
+
+-----BEGIN PGP SIGNATURE-----
+
+Some signature, missing an end marker.
diff --git a/t/scripts/Lintian/Util/data/pgp-sig-before-start b/t/scripts/Lintian/Util/data/pgp-sig-before-start
new file mode 100644
index 0000000..1d7cf12
--- /dev/null
+++ b/t/scripts/Lintian/Util/data/pgp-sig-before-start
@@ -0,0 +1,7 @@
+Package: lintian
+
+-----BEGIN PGP SIGNATURE-----
+
+Some signature.
+
+-----END PGP SIGNATURE-----
diff --git a/t/scripts/Lintian/Util/data/pgp-trailing-unsigned b/t/scripts/Lintian/Util/data/pgp-trailing-unsigned
new file mode 100644
index 0000000..1d29d87
--- /dev/null
+++ b/t/scripts/Lintian/Util/data/pgp-trailing-unsigned
@@ -0,0 +1,14 @@
+-----BEGIN PGP SIGNED MESSAGE-----
+
+Package: lintian
+
+-----BEGIN PGP SIGNATURE-----
+
+Some signature.
+
+-----END PGP SIGNATURE-----
+
+# Signed above, unsigned below (bad)
+
+Package: dpkg
+
diff --git a/t/scripts/Lintian/Util/data/pgp-two-signatures b/t/scripts/Lintian/Util/data/pgp-two-signatures
new file mode 100644
index 0000000..0c0b7d6
--- /dev/null
+++ b/t/scripts/Lintian/Util/data/pgp-two-signatures
@@ -0,0 +1,16 @@
+-----BEGIN PGP SIGNED MESSAGE-----
+
+Package: lintian
+
+-----BEGIN PGP SIGNATURE-----
+
+Some signature.
+
+-----END PGP SIGNATURE-----
+
+-----BEGIN PGP SIGNATURE-----
+
+Another signature.
+
+-----END PGP SIGNATURE-----
+
diff --git a/t/scripts/Lintian/Util/data/pgp-two-signed-msgs b/t/scripts/Lintian/Util/data/pgp-two-signed-msgs
new file mode 100644
index 0000000..c8fcf9d
--- /dev/null
+++ b/t/scripts/Lintian/Util/data/pgp-two-signed-msgs
@@ -0,0 +1,19 @@
+-----BEGIN PGP SIGNED MESSAGE-----
+
+Package: lintian
+
+-----BEGIN PGP SIGNATURE-----
+
+Some signature.
+
+-----END PGP SIGNATURE-----
+
+-----BEGIN PGP SIGNED MESSAGE-----
+
+Package: dpkg
+
+-----BEGIN PGP SIGNATURE-----
+
+Aother signature.
+
+-----END PGP SIGNATURE-----
diff --git a/t/scripts/Lintian/Util/data/pgp-unexpected-header b/t/scripts/Lintian/Util/data/pgp-unexpected-header
new file mode 100644
index 0000000..743ac85
--- /dev/null
+++ b/t/scripts/Lintian/Util/data/pgp-unexpected-header
@@ -0,0 +1,6 @@
+-----BEGIN PGP MESSAGE-----
+
+We are expecting a "SIGNED" message.
+
+-----END PGP MESSAGE-----
+
diff --git a/t/scripts/Lintian/Util/dctrl-parser.t b/t/scripts/Lintian/Util/dctrl-parser.t
new file mode 100644
index 0000000..0f7897e
--- /dev/null
+++ b/t/scripts/Lintian/Util/dctrl-parser.t
@@ -0,0 +1,54 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Test::More;
+
+# Lintian::Util exports fail, which clashes with Test::More, so we
+# have to be explicit about the import(s).
+use Lintian::Util qw(visit_dpkg_paragraph);
+
+my $syntax_error = qr/syntax error at line \d+/;
+my %TESTS_BAD = (
+ 'pgp-sig-before-start' => qr/${syntax_error}: PGP signature seen before start of signed message/,
+ 'pgp-two-signatures' => qr/${syntax_error}: Two PGP signatures \(first one at line \d+\)/,
+ 'pgp-unexpected-header' => qr/${syntax_error}: Unexpected .+ header/,
+ 'pgp-malformed-header' => qr/${syntax_error}: Malformed PGP header/,
+ 'pgp-two-signed-msgs' => qr/${syntax_error}: Expected at most one signed message \(previous at line \d+\)/,
+ 'pgp-no-end-pgp-header' => qr/${syntax_error}: End of file but expected a "END PGP SIGNATURE" header/,
+ 'pgp-leading-unsigned' => qr/${syntax_error}: PGP MESSAGE header must be first content if present/,
+ 'pgp-trailing-unsigned' => qr/${syntax_error}: Data after the PGP SIGNATURE/,
+ 'pgp-eof-missing-sign' => qr/${syntax_error}: End of file before "BEGIN PGP SIGNATURE"/,
+);
+
+my $DATADIR = $0;
+$DATADIR =~ s,[^/]+$,,o;
+if ($DATADIR) {
+ # invokved in some other dir
+ $DATADIR = "$DATADIR/data";
+} else {
+ # current dir
+ $DATADIR = 'data';
+}
+
+plan skip_all => 'Data files not available'
+ unless -d $DATADIR;
+
+plan tests => scalar keys %TESTS_BAD;
+
+foreach my $filename (sort keys %TESTS_BAD) {
+ my $fail_regex = $TESTS_BAD{$filename};
+ my $path = "$DATADIR/$filename";
+ open my $fd, '<', $path or die "open $path: $!";
+ eval {
+ visit_dpkg_paragraph (sub {}, $fd);
+ };
+ close $fd;
+ if (my $err = $@) {
+ like ($err, $fail_regex, $filename);
+ } else {
+ fail ("$path was parsed successfully");
+ }
+}
+
--
1.7.10.4
Reply to: