[pmg-devel] [PATCH pmg-api] fix #3734: scrub 'url' from style tags/attributes

[pmg-devel] [PATCH pmg-api] fix #3734: scrub 'url' from style tags/attributes

[Dominik Csapak]

if 'view images' for the quarantine is disabled, it is expected that
*no* images will be loaded. but in addition to img (src/href/etc.)
also css can load external images via the 'url' directive

since html scrubber does not parse/iterate over css, we simply remove
the url+protocol part of those tags/attributes. this technically leaves behind
invalid css, but the browsers should cope with that.
(we cannot 'cleanly' remove without much more effort because of quoting)

also we have to scrub the style tags in 'dump_html' since HTML::Scrubber
does not have a way to modify the *content* of a tag, only the
attributes...

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/HTMLMail.pm | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/src/PMG/HTMLMail.pm b/src/PMG/HTMLMail.pm
index b69a596..987dc39 100644
--- a/src/PMG/HTMLMail.pm
+++ b/src/PMG/HTMLMail.pm
@@ -15,8 +15,16 @@ use HTML::Scrubber;
 use PMG::Utils;
 use PMG::MIMEUtils;
 
+# $value is a ref to a string scalar
+my sub remove_urls {
+    my ($value) = @_;
+    # remove all urls with a protocol, this leaves partially invalid
+    # css, but prevents the browser from loading them
+    $$value =~ s|url\s*\(\s*(['"]?)[a-z]+://|($1|gi;
+}
+
 sub dump_html {
-    my ($tree, $cid_hash) = @_;
+    my ($tree, $cid_hash, $viewimages) = @_;
 
     my @html = ();
 
@@ -37,6 +45,11 @@ sub dump_html {
 			    $node->{src} = $datauri;
 			}
 		    }
+		} elsif ($tag eq 'style' && !$viewimages) {
+		    for my $el ($node->content_refs_list()) {
+			next if ref $$el;
+			remove_urls($el);
+		    }
 		}
 
 		if($start) { # on the way in
@@ -137,7 +150,13 @@ sub getscrubber {
 	    span => 1,
 	    src => $viewimages ? qr{^(?!(?:java)?script)}i : 0,
 	    start => 1,
-	    style => 1,
+	    style => $viewimages ? 1 : sub {
+		my ($obj, $tag_name, $attr_name, $value) = @_;
+
+		remove_urls(\$value);
+
+		return $value;
+	    },
 	    summary => 1,
 	    tabindex => 1,
 	    target => 1,
@@ -267,7 +286,7 @@ sub entity_to_html {
 	$tree->parse($raw);
 	$tree->eof();
 
-	my $whtml = dump_html($tree, $viewimages ? $cid_hash : {});
+	my $whtml = dump_html($tree, $viewimages ? $cid_hash : {}, $viewimages); #scrubs style tags
 	$tree->delete;
 
 	# remove dangerous/unneeded elements
-- 
2.30.2

Re: [pmg-devel] [PATCH pmg-api] fix #3734: scrub 'url' from style tags/attributes

[Thomas Lamprecht]

On 25.11.21 12:22, Dominik Csapak wrote:
> if 'view images' for the quarantine is disabled, it is expected that
> *no* images will be loaded. but in addition to img (src/href/etc.)
> also css can load external images via the 'url' directive
> 
> since html scrubber does not parse/iterate over css, we simply remove
> the url+protocol part of those tags/attributes. this technically leaves behind
> invalid css, but the browsers should cope with that.
> (we cannot 'cleanly' remove without much more effort because of quoting)
> 
> also we have to scrub the style tags in 'dump_html' since HTML::Scrubber
> does not have a way to modify the *content* of a tag, only the
> attributes...
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/HTMLMail.pm | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PMG/HTMLMail.pm b/src/PMG/HTMLMail.pm
> index b69a596..987dc39 100644
> --- a/src/PMG/HTMLMail.pm
> +++ b/src/PMG/HTMLMail.pm
> @@ -15,8 +15,16 @@ use HTML::Scrubber;
>  use PMG::Utils;
>  use PMG::MIMEUtils;
>  
> +# $value is a ref to a string scalar
> +my sub remove_urls {
> +    my ($value) = @_;
> +    # remove all urls with a protocol, this leaves partially invalid
> +    # css, but prevents the browser from loading them
> +    $$value =~ s|url\s*\(\s*(['"]?)[a-z]+://|($1|gi;

isn't the image() css function also problematic?
https://developer.mozilla.org/en-US/docs/Web/CSS/image/image()

And isn't this dangerous as an attacker could use this to craft the actual url,
for example:

urlurl(https://https:/my.super.evil.site.com/foo.png)

perl -we 'my $a = "urlurl(http://http:/my.super.evil.site.com/foo.png)"; $a =~ s|url\s*\(\s*(["]?)[a-z]+://|($1|gi; print "$a"'

prints:

url(https:/my.super.evil.site.com/foo.png)

So maybe rather just prefix the bad function names with __ or so, that would be safe
against such things, removing partial things won't ever be, at least not at the regex
level.

> +}
> +
>  sub dump_html {
> -    my ($tree, $cid_hash) = @_;
> +    my ($tree, $cid_hash, $viewimages) = @_;

I know this is the same name we use at caller side, but I'd still rather keep
local naming style: $view_images

>  
>      my @html = ();
>  
> @@ -37,6 +45,11 @@ sub dump_html {
>  			    $node->{src} = $datauri;
>  			}
>  		    }
> +		} elsif ($tag eq 'style' && !$viewimages) {
> +		    for my $el ($node->content_refs_list()) {
> +			next if ref $$el;
> +			remove_urls($el);
> +		    }

could be (and really just to let my line-bloat-reduction policy get out of hand):

		    remove_urls($_) for grep { !ref $$el } $node->content_refs_list();
>  		}
>  
>  		if($start) { # on the way in
> @@ -137,7 +150,13 @@ sub getscrubber {
>  	    span => 1,
>  	    src => $viewimages ? qr{^(?!(?:java)?script)}i : 0,
>  	    start => 1,
> -	    style => 1,
> +	    style => $viewimages ? 1 : sub {
> +		my ($obj, $tag_name, $attr_name, $value) = @_;
> +
> +		remove_urls(\$value);
> +
> +		return $value;
> +	    },

may be nicer do declare that separately and pass the code-ref directly by variable
or sub-ref-name here (not sure from top of my head if `my sub foo` can be passed along)

>  	    summary => 1,
>  	    tabindex => 1,
>  	    target => 1,
> @@ -267,7 +286,7 @@ sub entity_to_html {
>  	$tree->parse($raw);
>  	$tree->eof();
>  
> -	my $whtml = dump_html($tree, $viewimages ? $cid_hash : {});
> +	my $whtml = dump_html($tree, $viewimages ? $cid_hash : {}, $viewimages); #scrubs style tags

the comment is confusing, dump_html does more than just scrubbing style tags,
doesn't it?

Seems also like we split "show images" logic now a bit more, maybe always pass $cid_hash
and decide in dump_html if it's used or not, as it's just a ref it wouldn't be any
real overhead. But, no to hard feeling here and I did not looked at the full picture
to closely so I may miss something that would make that a bad idea.


>  	$tree->delete;
>  
>  	# remove dangerous/unneeded elements
>