Bug#872646: qa.debian.org: [debcheck] Escape some HTML before outputting

[Colin Watson]

On Sun, Aug 20, 2017 at 02:53:09PM +0200, gregor herrmann wrote:
> On Sun, 20 Aug 2017 10:13:47 +0200, Mattia Rizzolo wrote:
> > On Sat, Aug 19, 2017 at 11:20:50AM -0700, Chris Lamb wrote:
> > > Updated patch attached, although the last hunk is probably unnecessary
> > > anyway.
> > 
> > Although, I'm not a perl guy so I must ask before applying:
> >  * shouldn't that function be `escape_html()` instead of `html_escape()` ?
> >(i.e., what is cited in the `use`)
> >  * does that thing requires a new dependency?  The closer thing I could
> >find on the net is
> >http://search.cpan.org/dist/HTML-Escape/lib/HTML/Escape.pm which
> >doesn't seem like something that is in the standard lib…
> >  * in the last chunk you escaped the first $me but not the latter in the
> >line below, probably best to at least be consistent?
> 
> AFAICS HTML::Escape is not packaged for Debian. 
> (Ack on the other points).
> 
> HTML::Entities might be an option:
> 
> https://metacpan.org/pod/HTML::Entities
> 
> Included in the libhtml-parser-perl package.

Yes, I think that's probably the right [1] answer.  quantz already has
libhtml-parser-perl installed too.

I've tidied this up along those lines and made a merge request:

  https://salsa.debian.org/qa/qa/merge_requests/27

Note that I didn't bother to escape things that have already been
checked as conforming to a syntax that doesn't require HTML-escaping,
e.g. priority names, just because it was getting unwieldy.  Ordinarily
I'd make sure to put all substitutions through an escaping mechanism
just to guard against future mistakes, but well, see [1].

[1] I might have written it this way myself when I first started writing
Perl in 1999 or so, so no criticism of the original author intended,
but I've very distinctly gone off the idea of assembling HTML out of
lots of little string fragments in the intervening two decades.
These days I'd use a templating system (e.g. Perl's Template Toolkit
looks reasonable enough) as a bare minimum.  But right now I really
don't feel like rewriting almost all of debcheck to achieve that ...

-- 
Colin Watson   [cjwat...@debian.org]

Bug#872646: qa.debian.org: [debcheck] Escape some HTML before outputting

[Chris Lamb]

Hi Mattia,

> Although, I'm not a perl guy so I must ask before applying:

[…]

ACK all those issues. Tbh the patch was more of a demonstration; I'm sure
the real author will have a much cleaner solution. :)


Regards,

-- 
  ,''`.
 : :'  : Chris Lamb
 `. `'`  la...@debian.org / chris-lamb.co.uk
   `-

Bug#872646: qa.debian.org: [debcheck] Escape some HTML before outputting

[gregor herrmann]

On Sun, 20 Aug 2017 10:13:47 +0200, Mattia Rizzolo wrote:

> On Sat, Aug 19, 2017 at 11:20:50AM -0700, Chris Lamb wrote:
> > Updated patch attached, although the last hunk is probably unnecessary
> > anyway.
> 
> Although, I'm not a perl guy so I must ask before applying:
>  * shouldn't that function be `escape_html()` instead of `html_escape()` ?
>(i.e., what is cited in the `use`)
>  * does that thing requires a new dependency?  The closer thing I could
>find on the net is
>http://search.cpan.org/dist/HTML-Escape/lib/HTML/Escape.pm which
>doesn't seem like something that is in the standard lib…
>  * in the last chunk you escaped the first $me but not the latter in the
>line below, probably best to at least be consistent?

AFAICS HTML::Escape is not packaged for Debian. 
(Ack on the other points).

HTML::Entities might be an option:

https://metacpan.org/pod/HTML::Entities

Included in the libhtml-parser-perl package.


Cheers,
gregor


-- 
 .''`.  https://info.comodo.priv.at/ - Debian Developer https://www.debian.org
 : :' : OpenPGP fingerprint D1E1 316E 93A7 60A8 104D  85FA BB3A 6801 8649 AA06
 `. `'  Member of VIBE!AT & SPI, fellow of the Free Software Foundation Europe
   `-   NP: Bob Dylan: This Dream Of You


signature.asc
Description: Digital Signature

Bug#872646: qa.debian.org: [debcheck] Escape some HTML before outputting

[Mattia Rizzolo]

Control: tag -1 moreinfo

On Sat, Aug 19, 2017 at 11:20:50AM -0700, Chris Lamb wrote:
> Updated patch attached, although the last hunk is probably unnecessary
> anyway.

Thanks for the patch!

Although, I'm not a perl guy so I must ask before applying:
 * shouldn't that function be `escape_html()` instead of `html_escape()` ?
   (i.e., what is cited in the `use`)
 * does that thing requires a new dependency?  The closer thing I could
   find on the net is
   http://search.cpan.org/dist/HTML-Escape/lib/HTML/Escape.pm which
   doesn't seem like something that is in the standard lib…
 * in the last chunk you escaped the first $me but not the latter in the
   line below, probably best to at least be consistent?

-- 
regards,
Mattia Rizzolo

GPG Key: 66AE 2B4A FCCF 3F52 DA18  4D18 4B04 3FCD B944 4540  .''`.
more about me:  https://mapreri.org : :'  :
Launchpad user: https://launchpad.net/~mapreri  `. `'`
Debian QA page: https://qa.debian.org/developer.php?login=mattia  `-


signature.asc
Description: PGP signature

Bug#872646: qa.debian.org: [debcheck] Escape some HTML before outputting

[Chris Lamb]

Hi,

> qa.debian.org: [debcheck] Escape some HTML before outputting

Updated patch attached, although the last hunk is probably unnecessary
anyway.


Regards,

-- 
  ,''`.
 : :'  : Chris Lamb
 `. `'`  la...@debian.org / chris-lamb.co.uk
   `-
>From b57aea649dd0ee90d6f7e2bf44f6d6119ed71815 Mon Sep 17 00:00:00 2001
From: Chris Lamb 
Date: Sat, 19 Aug 2017 10:59:07 -0700
Subject: [PATCH] debcheck: Escape some HTML before outputting.

Discovered as the parser doesn't support Build-Profiles, which end up as
literal < and > chars in the error message:

  build time dependency on 'tcl ' which is broken Syntax
^^

Signed-off-by: Chris Lamb 
---
 data/debcheck/debcheck | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/data/debcheck/debcheck b/data/debcheck/debcheck
index 863afe9..aabffe2 100755
--- a/data/debcheck/debcheck
+++ b/data/debcheck/debcheck
@@ -7,6 +7,7 @@ use strict;
 
 use Dpkg::ErrorHandling;
 use Dpkg::Version;
+use HTML::Escape qw{escape_html};
 report_options (quiet_warnings => 1);
 
 my $VERBOSE = -t 1; # output is a terminal
@@ -619,7 +620,7 @@ sub malformedbuilddepends() {
 	my ($pkg, $prob, $maint, $section) = @_;
 	print FILE "Malformed Build-Depends";
 	for my $dependency (keys %$prob) {
-		print FILE "Package declares a build time dependency on '$dependency' which is broken Syntax.\n";
+		print FILE "Package declares a build time dependency on '" . html_escape($dependency) . "' which is broken Syntax.\n";
 	};
 	registerSummaryItem('malformed-build-depends', undef, $pkg, $maint);
 	registerSummaryItem('main-only-malformed-build-depends', undef, $pkg, $maint) if ($section eq 'main');
@@ -628,7 +629,7 @@ sub malformedbuilddepends() {
 sub standardversion() {
 	my ($pkg, $prob, $maint, $section) = @_;
 	print FILE "Standards-Version";
-	print FILE "Package has a Standards-Version of $prob which is pretty old.\n";
+	print FILE "Package has a Standards-Version of " . html_escape($prob) . " which is pretty old.\n";
 	registerSummaryItem('Standards-Version', undef, $pkg, $maint);
 	registerSummaryItem('main-only-Standards-Version', undef, $pkg, $maint) if ($section eq 'main');
 };
@@ -636,7 +637,7 @@ sub standardversion() {
 sub wrongstandardversion() {
 	my ($pkg, $prob, $maint, $section) = @_;
 	print FILE "Wrong-Standards-Version-Syntax";
-	print FILE "Package has a Standards-Version of '$prob' which is broken Syntax.\n";
+	print FILE "Package has a Standards-Version of '" . html_escape($prob) . "' which is broken Syntax.\n";
 	registerSummaryItem('Wrong-Standards-Version-Syntax', undef, $pkg, $maint);
 	registerSummaryItem('main-only-Wrong-Standards-Version-Syntax', undef, $pkg, $maint) if ($section eq 'main');
 };
@@ -677,7 +678,7 @@ EOF
 	my $them = $2;
 	for my $arch (keys %{$prob->{$depType}->{$depTarget}->{$partdepTarget}->{$priType}}) {
 		if ($depTarget eq $partdepTarget) {
-			print FILE "Package is $me and has a $depType on $depTarget which is $them on $arch.\n";
+			print FILE "Package is " . html_escape($me) . " and has a $depType on $depTarget which is $them on $arch.\n";
 		} else {
 			print FILE "Package is $me and has a $depType on $partdepTarget (within $depTarget) which is $them on $arch.\n";
 		};
-- 
2.14.1

Bug#872646: qa.debian.org: [debcheck] Escape some HTML before outputting

[Chris Lamb]

Package: qa.debian.org
Severity: important
Tags: patch

Hi,

Can someone review and release the attached patch:

  commit b57aea649dd0ee90d6f7e2bf44f6d6119ed71815
  Author: Chris Lamb 
  Date:   Sat Aug 19 10:59:07 2017 -0700
  
  debcheck: Escape some HTML before outputting.
  
  Discovered as the parser doesn't support Build-Profiles, which end up as
  literal < and > chars in the error message:
  
build time dependency on 'tcl ' which is broken Syntax
  ^^
  
  Signed-off-by: Chris Lamb 
  
   data/debcheck/debcheck | 9 +
   1 file changed, 5 insertions(+), 4 deletions(-)

Probably exploitable with:

  Standards-Version: alert('XSS')

*g*


Regards,

-- 
  ,''`.
 : :'  : Chris Lamb
 `. `'`  la...@debian.org / chris-lamb.co.uk
   `-
>From b57aea649dd0ee90d6f7e2bf44f6d6119ed71815 Mon Sep 17 00:00:00 2001
From: Chris Lamb 
Date: Sat, 19 Aug 2017 10:59:07 -0700
Subject: [PATCH] debcheck: Escape some HTML before outputting.

Discovered as the parser doesn't support Build-Profiles, which end up as
literal < and > chars in the error message:

  build time dependency on 'tcl ' which is broken Syntax
^^

Signed-off-by: Chris Lamb 
---
 data/debcheck/debcheck | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/data/debcheck/debcheck b/data/debcheck/debcheck
index 863afe9..aabffe2 100755
--- a/data/debcheck/debcheck
+++ b/data/debcheck/debcheck
@@ -7,6 +7,7 @@ use strict;
 
 use Dpkg::ErrorHandling;
 use Dpkg::Version;
+use HTML::Escape qw{escape_html};
 report_options (quiet_warnings => 1);
 
 my $VERBOSE = -t 1; # output is a terminal
@@ -619,7 +620,7 @@ sub malformedbuilddepends() {
 	my ($pkg, $prob, $maint, $section) = @_;
 	print FILE "Malformed Build-Depends";
 	for my $dependency (keys %$prob) {
-		print FILE "Package declares a build time dependency on '$dependency' which is broken Syntax.\n";
+		print FILE "Package declares a build time dependency on '" . html_escape($dependency) . "' which is broken Syntax.\n";
 	};
 	registerSummaryItem('malformed-build-depends', undef, $pkg, $maint);
 	registerSummaryItem('main-only-malformed-build-depends', undef, $pkg, $maint) if ($section eq 'main');
@@ -628,7 +629,7 @@ sub malformedbuilddepends() {
 sub standardversion() {
 	my ($pkg, $prob, $maint, $section) = @_;
 	print FILE "Standards-Version";
-	print FILE "Package has a Standards-Version of $prob which is pretty old.\n";
+	print FILE "Package has a Standards-Version of " . html_escape($prob) . " which is pretty old.\n";
 	registerSummaryItem('Standards-Version', undef, $pkg, $maint);
 	registerSummaryItem('main-only-Standards-Version', undef, $pkg, $maint) if ($section eq 'main');
 };
@@ -636,7 +637,7 @@ sub standardversion() {
 sub wrongstandardversion() {
 	my ($pkg, $prob, $maint, $section) = @_;
 	print FILE "Wrong-Standards-Version-Syntax";
-	print FILE "Package has a Standards-Version of '$prob' which is broken Syntax.\n";
+	print FILE "Package has a Standards-Version of '" . html_escape($prob) . "' which is broken Syntax.\n";
 	registerSummaryItem('Wrong-Standards-Version-Syntax', undef, $pkg, $maint);
 	registerSummaryItem('main-only-Wrong-Standards-Version-Syntax', undef, $pkg, $maint) if ($section eq 'main');
 };
@@ -677,7 +678,7 @@ EOF
 	my $them = $2;
 	for my $arch (keys %{$prob->{$depType}->{$depTarget}->{$partdepTarget}->{$priType}}) {
 		if ($depTarget eq $partdepTarget) {
-			print FILE "Package is $me and has a $depType on $depTarget which is $them on $arch.\n";
+			print FILE "Package is " . html_escape($me and has a $depType on $depTarget which is $them on $arch.\n";
 		} else {
 			print FILE "Package is $me and has a $depType on $partdepTarget (within $depTarget) which is $them on $arch.\n";
 		};
-- 
2.14.1