archive links broken with obfuscate=true

archive links broken with obfuscate=true

[Kyle Meyer]

I've been testing out obfuscate=true a bit (which won't be a surprise to
Eric, given a private email that was sent to both of us).  One issue I
noticed is that it breaks archive links.  I've posted an example at
<https://yhetil.org/obf/20201204120929.GA22736@dcvr/>:

  Reported-by: Kyle Meyer <kyle@kyleam•com>
  Link: https://public-inbox.org/meta/87360nlc44.fsf@kyleam•com/

Re: archive links broken with obfuscate=true

[Eric Wong]

Kyle Meyer <kyle@kyleam.com> wrote:
> I've been testing out obfuscate=true a bit (which won't be a surprise to
> Eric, given a private email that was sent to both of us).  One issue I
> noticed is that it breaks archive links.  I've posted an example at
> <https://yhetil.org/obf/20201204120929.GA22736@dcvr/>:
> 
>   Reported-by: Kyle Meyer <kyle@kyleam•com>
>   Link: https://public-inbox.org/meta/87360nlc44.fsf@kyleam•com/

Oops, I think the following fixes it, but not sure if there's a
better way to accomplish the same thing....

I worry the regexp change is susceptible to performance problems
from malicious inputs.  I can't remember if something like this
triggers a pathological case or not, or if I'm confusing this
with another quirk that does (or quirks of another RE engine)

------------8<--------
Subject: [WIP] www: do not perform address obfuscation on URLs

---
 lib/PublicInbox/Hval.pm | 10 ++++++----
 t/hval.t                |  4 ++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Hval.pm b/lib/PublicInbox/Hval.pm
index d20f70ae..6f1a046c 100644
--- a/lib/PublicInbox/Hval.pm
+++ b/lib/PublicInbox/Hval.pm
@@ -82,15 +82,17 @@ sub obfuscate_addrs ($$;$) {
 	my $repl = $_[2] // '&#8226;';
 	my $re = $ibx->{-no_obfuscate_re}; # regex of domains
 	my $addrs = $ibx->{-no_obfuscate}; # { $address => 1 }
-	$_[1] =~ s/(([\w\.\+=\-]+)\@([\w\-]+\.[\w\.\-]+))/
-		my ($addr, $user, $domain) = ($1, $2, $3);
-		if ($addrs->{$addr} || ((defined $re && $domain =~ $re))) {
+	$_[1] =~ s#(\S*?)(([\w\.\+=\-]+)\@([\w\-]+\.[\w\.\-]+))#
+		my ($beg, $addr, $user, $domain) = ($1, $2, $3, $4);
+		if (index($beg, '://') > 0) {
+			$beg.$addr;
+		} elsif ($addrs->{$addr} || ((defined $re && $domain =~ $re))) {
 			$addr;
 		} else {
 			$domain =~ s!([^\.]+)\.!$1$repl!;
 			$user . '@' . $domain
 		}
-		/sge;
+		#sge;
 }
 
 # like format_sanitized_subject in git.git pretty.c with '%f' format string
diff --git a/t/hval.t b/t/hval.t
index 9d0dab7a..5afc2052 100644
--- a/t/hval.t
+++ b/t/hval.t
@@ -47,6 +47,10 @@ EOF
 
 is($html, $exp, 'only obfuscated relevant addresses');
 
+$exp = 'https://example.net/foo@example.net';
+PublicInbox::Hval::obfuscate_addrs($ibx, my $res = $exp);
+is($res, $exp, 'does not obfuscate URL with Message-ID');
+
 is(PublicInbox::Hval::to_filename('foo bar  '), 'foo-bar',
 	'to_filename has no trailing -');
 

Re: archive links broken with obfuscate=true

[Kyle Meyer]

Eric Wong writes:

> Oops, I think the following fixes it, but not sure if there's a
> better way to accomplish the same thing....

Thanks.  Jumping around a bit with that installed, I haven't spotted any
remaining issues.

> I worry the regexp change is susceptible to performance problems
> from malicious inputs.  I can't remember if something like this
> triggers a pathological case or not, or if I'm confusing this
> with another quirk that does (or quirks of another RE engine)

Hmm...

> diff --git a/lib/PublicInbox/Hval.pm b/lib/PublicInbox/Hval.pm
> index d20f70ae..6f1a046c 100644
> --- a/lib/PublicInbox/Hval.pm
> +++ b/lib/PublicInbox/Hval.pm
> @@ -82,15 +82,17 @@ sub obfuscate_addrs ($$;$) {
>  	my $repl = $_[2] // '•';
>  	my $re = $ibx->{-no_obfuscate_re}; # regex of domains
>  	my $addrs = $ibx->{-no_obfuscate}; # { $address => 1 }
> -	$_[1] =~ s/(([\w\.\+=\-]+)\@([\w\-]+\.[\w\.\-]+))/
> -		my ($addr, $user, $domain) = ($1, $2, $3);
> -		if ($addrs->{$addr} || ((defined $re && $domain =~ $re))) {
> +	$_[1] =~ s#(\S*?)(([\w\.\+=\-]+)\@([\w\-]+\.[\w\.\-]+))#
> +		my ($beg, $addr, $user, $domain) = ($1, $2, $3, $4);

... what about allowing the first match to be {0,N}, where N is some not
so huge value?  It'd risk incorrectly obfuscating some really long
links, but given that it's just the HTML presentation, that seems
acceptable.

Re: archive links broken with obfuscate=true

[Eric Wong]

Kyle Meyer <kyle@kyleam.com> wrote:
> Eric Wong writes:
> 
> > Oops, I think the following fixes it, but not sure if there's a
> > better way to accomplish the same thing....
> 
> Thanks.  Jumping around a bit with that installed, I haven't spotted any
> remaining issues.

Thanks for the report.  Have you run any performance tests?

> > I worry the regexp change is susceptible to performance problems
> > from malicious inputs.  I can't remember if something like this
> > triggers a pathological case or not, or if I'm confusing this
> > with another quirk that does (or quirks of another RE engine)
> 
> Hmm...
> 
> > diff --git a/lib/PublicInbox/Hval.pm b/lib/PublicInbox/Hval.pm
> > index d20f70ae..6f1a046c 100644
> > --- a/lib/PublicInbox/Hval.pm
> > +++ b/lib/PublicInbox/Hval.pm
> > @@ -82,15 +82,17 @@ sub obfuscate_addrs ($$;$) {
> >  	my $repl = $_[2] // '&#8226;';
> >  	my $re = $ibx->{-no_obfuscate_re}; # regex of domains
> >  	my $addrs = $ibx->{-no_obfuscate}; # { $address => 1 }
> > -	$_[1] =~ s/(([\w\.\+=\-]+)\@([\w\-]+\.[\w\.\-]+))/
> > -		my ($addr, $user, $domain) = ($1, $2, $3);
> > -		if ($addrs->{$addr} || ((defined $re && $domain =~ $re))) {
> > +	$_[1] =~ s#(\S*?)(([\w\.\+=\-]+)\@([\w\-]+\.[\w\.\-]+))#
> > +		my ($beg, $addr, $user, $domain) = ($1, $2, $3, $4);
> 
> ... what about allowing the first match to be {0,N}, where N is some not
> so huge value?  It'd risk incorrectly obfuscating some really long
> links, but given that it's just the HTML presentation, that seems
> acceptable.

I'm actually more worried about the '0' (of '{0,}') or '*' being
combined with '?'.  I can't remember if there's a pathological
case in that...

The upper bound of N is a smaller concern, especially for
non-spam messages which only have non-space tokens of reasonable
length.

Maybe changing the three existing '+' to {1,M} would be a way to
ameliorate the problem (though I'm not sure what a good value of
M would be, 255?).

OTOH, it may not be an actual problem at all and I'm just
confusing this with something else.

Re: archive links broken with obfuscate=true

[Kyle Meyer]

Eric Wong writes:

> Have you run any performance tests?

No.  To get an idea of how to approach that, would you suggest I look at
xt/perf-msgview.t?

> I'm actually more worried about the '0' (of '{0,}') or '*' being
> combined with '?'.  I can't remember if there's a pathological
> case in that...

Ah, okay, sorry for missing that.

> The upper bound of N is a smaller concern, especially for
> non-spam messages which only have non-space tokens of reasonable
> length.
>
> Maybe changing the three existing '+' to {1,M} would be a way to
> ameliorate the problem (though I'm not sure what a good value of
> M would be, 255?).

Me neither, though I suspect 255 would be sufficient.

Re: archive links broken with obfuscate=true

[Eric Wong]

Kyle Meyer <kyle@kyleam.com> wrote:
> Eric Wong writes:
> 
> > Have you run any performance tests?
> 
> No.  To get an idea of how to approach that, would you suggest I look at
> xt/perf-msgview.t?

Yeah, probably that with some tweaks; or running -httpd with ab,
wrk or some other HTTP benchmark that uses persistent connections.

I'm OK with things being slower with this option enabled, but
not with trivial denial-of-service vectors.

> > I'm actually more worried about the '0' (of '{0,}') or '*' being
> > combined with '?'.  I can't remember if there's a pathological
> > case in that...
> 
> Ah, okay, sorry for missing that.

No worries.  I've dealt with some nasty pathological slowdowns
in perl(-inspired) regex engines over the years and forget most
people haven't.  I recall perl itself seemed less susceptible to
pathological cases than engines inspired by it, but also wasn't
immune to them.

Maybe there's a compilation of known DoS-able regexp examples
which affect Perl somewhere.

> > The upper bound of N is a smaller concern, especially for
> > non-spam messages which only have non-space tokens of reasonable
> > length.
> >
> > Maybe changing the three existing '+' to {1,M} would be a way to
> > ameliorate the problem (though I'm not sure what a good value of
> > M would be, 255?).
> 
> Me neither, though I suspect 255 would be sufficient.

Alright.  There may be some other limits common to what SMTP
servers accept for line limits and such.  RFC 5322 limits
raw lines to 998, but that doesn't account for lengths after
decoding.

Re: archive links broken with obfuscate=true

[Kyle Meyer]

Eric Wong writes:

> Kyle Meyer <kyle@kyleam.com> wrote:
>> Eric Wong writes:
>> 
>> > Have you run any performance tests?
>> 
>> No.  To get an idea of how to approach that, would you suggest I look at
>> xt/perf-msgview.t?
>
> Yeah, probably that with some tweaks; or running -httpd with ab,
> wrk or some other HTTP benchmark that uses persistent connections.
>
> I'm OK with things being slower with this option enabled, but
> not with trivial denial-of-service vectors.

Below is my initial attempt at tweaking xt/perf-msgview.t.
xt/perf-obfuscate.t enables obfuscation in the inbox if PI_OBFUSCATE is
set in the environment:

  $ PI_OBFUSCATE=1 GIANT_INBOX_DIR=/tmp/test perl xt/perf-obfuscate.t

Here are my unscientific timings for three cases: 1) no obfuscation, 2)
obfuscation on the current master (ea4e9025dd), and 3) obfuscation with
the patch upthread.  The inbox is set to the Org mode's list archives
(<https://yhetil.org/orgmode/>), which at the time of execution
contained 135,885 messages.

| obfuscate   | run | wall |    usr |  sys |
|-------------+-----+------+--------+------|
| no          |   1 |   50 |  49.14 | 0.57 |
| no          |   2 |   49 |  47.76 | 0.58 |
|             |     |      |        |      |
| yes, master |   1 |   56 |  54.47 | 0.58 |
| yes, master |   2 |   55 |  54.24 | 0.58 |
|             |     |      |        |      |
| yes, patch  |   1 |  175 | 174.71 | 0.52 |
| yes, patch  |   2 |  176 | 174.33 | 0.56 |


diff --git a/xt/perf-obfuscate.t b/xt/perf-obfuscate.t
new file mode 100644
index 00000000..2a8d5c1e
--- /dev/null
+++ b/xt/perf-obfuscate.t
@@ -0,0 +1,64 @@
+# Copyright (C) 2019-2021 all contributors <meta@public-inbox.org>
+# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
+use strict;
+use warnings;
+use Test::More;
+use Benchmark qw(:all);
+use PublicInbox::Inbox;
+use PublicInbox::View;
+use PublicInbox::TestCommon;
+
+my $inboxdir = $ENV{GIANT_INBOX_DIR};
+plan skip_all => "GIANT_INBOX_DIR not defined for $0" unless $inboxdir;
+
+my $obfuscate = $ENV{PI_OBFUSCATE} ? 1 : 0;
+print "obfuscate=$obfuscate\n";
+
+my @cat = qw(cat-file --buffer --batch-check --batch-all-objects);
+if (require_git(2.19, 1)) {
+	push @cat, '--unordered';
+} else {
+	warn
+"git <2.19, cat-file lacks --unordered, locality suffers\n";
+}
+require_mods qw(Plack::Util);
+use_ok 'Plack::Util';
+my $ibx = PublicInbox::Inbox->new({ inboxdir => $inboxdir, name => 'name' ,
+				    obfuscate => $obfuscate});
+my $git = $ibx->git;
+my $fh = $git->popen(@cat);
+my $vec = '';
+vec($vec, fileno($fh), 1) = 1;
+select($vec, undef, undef, 60) or die "timed out waiting for --batch-check";
+
+my $ctx = {
+	env => { HTTP_HOST => 'example.com', 'psgi.url_scheme' => 'https' },
+	ibx => $ibx,
+	www => Plack::Util::inline_object(style => sub {''}),
+};
+my ($mime, $res, $oid, $type);
+my $n = 0;
+my $obuf = '';
+my $m = 0;
+
+my $cb = sub {
+	$mime = PublicInbox::Eml->new(shift);
+	PublicInbox::View::multipart_text_as_html($mime, $ctx);
+	++$m;
+	$obuf = '';
+};
+
+my $t = timeit(1, sub {
+	$ctx->{obuf} = \$obuf;
+	$ctx->{mhref} = '../';
+	while (<$fh>) {
+		($oid, $type) = split / /;
+		next if $type ne 'blob';
+		++$n;
+		$git->cat_async($oid, $cb);
+	}
+	$git->cat_async_wait;
+});
+diag 'multipart_text_as_html took '.timestr($t)." for $n <=> $m messages";
+is($m, $n, 'rendered all messages');
+done_testing();

[PATCH v2] www: do not obfuscate addresses in URLs

[Eric Wong]

Kyle Meyer <kyle@kyleam.com> wrote:
> | obfuscate   | run | wall |    usr |  sys |
> |-------------+-----+------+--------+------|
> | no          |   1 |   50 |  49.14 | 0.57 |
> | no          |   2 |   49 |  47.76 | 0.58 |
> |             |     |      |        |      |
> | yes, master |   1 |   56 |  54.47 | 0.58 |
> | yes, master |   2 |   55 |  54.24 | 0.58 |
> |             |     |      |        |      |
> | yes, patch  |   1 |  175 | 174.71 | 0.52 |
> | yes, patch  |   2 |  176 | 174.33 | 0.56 |

Wow, that's horribly slow.  Probably not pathological, but still
bad.  The following might be slightly faster (or roughly the
same, hard to tell due to system noise).

-------------8<------------
Subject: [PATCH] www: do not obfuscate addresses in URLs

As they are likely Message-IDs.   If an email address ends up in
a URL, then it's likely public, so there's even less reason to
obfuscate that particular address.

[km: add xt/perf-obfuscate.t]
[ew: modernize perf test (5.10.1), use diag instead of print]

This version of the patch the massive slowdown noted by Kyle in
<https://public-inbox.org/meta/87wnt9or6t.fsf@kyleam.com/>.
Performance remains roughly the same, if not slightly faster
(which may be due to me testing this on a busy server).  Results
from xt/perf-obfuscate.t against 6078 messages on a local mirror
of <https://public-inbox.org/meta/>:

	before: 6.67 usr + 0.04 sys = 6.71 CPU
	 after: 6.64 usr + 0.04 sys = 6.68 CPU

Reported-by: Kyle Meyer <kyle@kyleam.com>
Helped-by: Kyle Meyer <kyle@kyleam.com>
Link: https://public-inbox.org/meta/87a6q8p5qa.fsf@kyleam.com/
---
 MANIFEST                |  1 +
 lib/PublicInbox/Hval.pm | 21 +++++++++-----
 t/hval.t                |  4 +++
 xt/perf-obfuscate.t     | 64 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 7 deletions(-)
 create mode 100644 xt/perf-obfuscate.t

diff --git a/MANIFEST b/MANIFEST
index b663c2a2..12247ad2 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -504,6 +504,7 @@ xt/net_writer-imap.t
 xt/nntpd-validate.t
 xt/perf-msgview.t
 xt/perf-nntpd.t
+xt/perf-obfuscate.t
 xt/perf-threading.t
 xt/solver.t
 xt/stress-sharedkv.t
diff --git a/lib/PublicInbox/Hval.pm b/lib/PublicInbox/Hval.pm
index d20f70ae..eab4738e 100644
--- a/lib/PublicInbox/Hval.pm
+++ b/lib/PublicInbox/Hval.pm
@@ -82,15 +82,22 @@ sub obfuscate_addrs ($$;$) {
 	my $repl = $_[2] // '&#8226;';
 	my $re = $ibx->{-no_obfuscate_re}; # regex of domains
 	my $addrs = $ibx->{-no_obfuscate}; # { $address => 1 }
-	$_[1] =~ s/(([\w\.\+=\-]+)\@([\w\-]+\.[\w\.\-]+))/
-		my ($addr, $user, $domain) = ($1, $2, $3);
-		if ($addrs->{$addr} || ((defined $re && $domain =~ $re))) {
-			$addr;
+	$_[1] =~ s#(\S+)\@([\w\-]+\.[\w\.\-]+)#
+		my ($pfx, $domain) = ($1, $2);
+		if (index($pfx, '://') > 0 || $pfx !~ s/([\w\.\+=\-]+)\z//) {
+			"$pfx\@$domain";
 		} else {
-			$domain =~ s!([^\.]+)\.!$1$repl!;
-			$user . '@' . $domain
+			my $user = $1;
+			my $addr = "$user\@$domain";
+			if ($addrs->{$addr} || ((defined($re) &&
+						$domain =~ $re))) {
+				$pfx.$addr;
+			} else {
+				$domain =~ s!([^\.]+)\.!$1$repl!;
+				$pfx . $user . '@' . $domain
+			}
 		}
-		/sge;
+		#sge;
 }
 
 # like format_sanitized_subject in git.git pretty.c with '%f' format string
diff --git a/t/hval.t b/t/hval.t
index 9d0dab7a..5afc2052 100644
--- a/t/hval.t
+++ b/t/hval.t
@@ -47,6 +47,10 @@ EOF
 
 is($html, $exp, 'only obfuscated relevant addresses');
 
+$exp = 'https://example.net/foo@example.net';
+PublicInbox::Hval::obfuscate_addrs($ibx, my $res = $exp);
+is($res, $exp, 'does not obfuscate URL with Message-ID');
+
 is(PublicInbox::Hval::to_filename('foo bar  '), 'foo-bar',
 	'to_filename has no trailing -');
 
diff --git a/xt/perf-obfuscate.t b/xt/perf-obfuscate.t
new file mode 100644
index 00000000..d4e7fb99
--- /dev/null
+++ b/xt/perf-obfuscate.t
@@ -0,0 +1,64 @@
+#!perl -w
+# Copyright (C) 2021 all contributors <meta@public-inbox.org>
+# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
+use strict;
+use v5.10.1;
+use PublicInbox::TestCommon;
+use Benchmark qw(:all);
+use PublicInbox::Inbox;
+use PublicInbox::View;
+
+my $inboxdir = $ENV{GIANT_INBOX_DIR};
+plan skip_all => "GIANT_INBOX_DIR not defined for $0" unless $inboxdir;
+
+my $obfuscate = $ENV{PI_OBFUSCATE} ? 1 : 0;
+diag "obfuscate=$obfuscate\n";
+
+my @cat = qw(cat-file --buffer --batch-check --batch-all-objects);
+if (require_git(2.19, 1)) {
+	push @cat, '--unordered';
+} else {
+	warn
+"git <2.19, cat-file lacks --unordered, locality suffers\n";
+}
+require_mods qw(Plack::Util);
+use_ok 'Plack::Util';
+my $ibx = PublicInbox::Inbox->new({ inboxdir => $inboxdir, name => 'name' ,
+				    obfuscate => $obfuscate});
+my $git = $ibx->git;
+my $fh = $git->popen(@cat);
+my $vec = '';
+vec($vec, fileno($fh), 1) = 1;
+select($vec, undef, undef, 60) or die "timed out waiting for --batch-check";
+
+my $ctx = {
+	env => { HTTP_HOST => 'example.com', 'psgi.url_scheme' => 'https' },
+	ibx => $ibx,
+	www => Plack::Util::inline_object(style => sub {''}),
+};
+my ($mime, $res, $oid, $type);
+my $n = 0;
+my $obuf = '';
+my $m = 0;
+
+my $cb = sub {
+	$mime = PublicInbox::Eml->new(shift);
+	PublicInbox::View::multipart_text_as_html($mime, $ctx);
+	++$m;
+	$obuf = '';
+};
+
+my $t = timeit(1, sub {
+	$ctx->{obuf} = \$obuf;
+	$ctx->{mhref} = '../';
+	while (<$fh>) {
+		($oid, $type) = split / /;
+		next if $type ne 'blob';
+		++$n;
+		$git->cat_async($oid, $cb);
+	}
+	$git->cat_async_wait;
+});
+diag 'multipart_text_as_html took '.timestr($t)." for $n <=> $m messages";
+is($m, $n, 'rendered all messages');
+done_testing();

Re: [PATCH v2] www: do not obfuscate addresses in URLs

[Eric Wong]

> This version of the patch the massive slowdown noted by Kyle in
                           ^--avoids

Re: [PATCH v2] www: do not obfuscate addresses in URLs

[Kyle Meyer]

Eric Wong writes:

> Kyle Meyer <kyle@kyleam.com> wrote:
>> | obfuscate   | run | wall |    usr |  sys |
>> |-------------+-----+------+--------+------|
>> | no          |   1 |   50 |  49.14 | 0.57 |
>> | no          |   2 |   49 |  47.76 | 0.58 |
>> |             |     |      |        |      |
>> | yes, master |   1 |   56 |  54.47 | 0.58 |
>> | yes, master |   2 |   55 |  54.24 | 0.58 |
>> |             |     |      |        |      |
>> | yes, patch  |   1 |  175 | 174.71 | 0.52 |
>> | yes, patch  |   2 |  176 | 174.33 | 0.56 |
>
> Wow, that's horribly slow.  Probably not pathological, but still
> bad.

Yeah.  The difference was big enough that I was getting ready to kill
the run and say "dunno, much longer" (or, rather, try with fewer
messages) :)

> The following might be slightly faster (or roughly the
> same, hard to tell due to system noise).
>
> -------------8<------------
> Subject: [PATCH] www: do not obfuscate addresses in URLs

Looking good on my end too:

 (54.53 usr +  0.47 sys = 55.00 CPU) @  0.02/s (n=1) for 135885 <=> 135885 messages

Thanks.