threads / discuss / 21301

git-send-email.perl defect: address missing trailing > accepted

Subject: git-send-email.perl defect: address missing trailing > accepted

## tl;dr

6 messages between Oct 20, 2009 and Oct 20, 2009.

replies: 5people: 3as markdown or json

Joe Perches· Oct 20, 2009, 22:12 UTC · lore

I typo cut/pasted an invalid email address, neglecting to copy the trailing ">".

was: "Name <addr.org" needed: "Name <addr.org>"

Anyone have suggestions on how to get git-send-email.perl to notify and abort sending on more invalid address styles?

Erik Faye-Lund· Oct 20, 2009, 22:29 UTC · re: Joe Perches · lore

Re: git-send-email.perl defect: address missing trailing > accepted

On Wed, Oct 21, 2009 at 12:12 AM, Joe Perches <joe@perches.com> wrote:
Show 9 quoted lines
> I typo cut/pasted an invalid email address,
> neglecting to copy the trailing ">".
>
> was:    "Name <addr.org"
> needed: "Name <addr.org>"
>
> Anyone have suggestions on how to get
> git-send-email.perl to notify and abort
> sending on more invalid address styles?

Something along these lines? Of course, the error message is, uhm, less than helpful :)

--->8---
diff --git a/git-send-email.perl b/git-send-email.perl
index f5ba4e7..83f5e80 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -787,6 +787,10 @@ sub is_rfc2047_quoted {
 sub sanitize_address
 {
 	my ($recipient) = @_;
+	if ($recipient =~ m/.*<[^>]*$/) {
+		die "EEK!"
+	}
+
 	my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/);

 	if (not $recipient_name) {

-- 
Erik "kusma" Faye-Lund
Joe Perches· Oct 20, 2009, 22:48 UTC · re: Erik Faye-Lund · lore

Re: git-send-email.perl defect: address missing trailing > accepted

On Wed, 2009-10-21 at 00:29 +0200, Erik Faye-Lund wrote:
Show 22 quoted lines
> On Wed, Oct 21, 2009 at 12:12 AM, Joe Perches <joe@perches.com> wrote:
> > I typo cut/pasted an invalid email address,
> > neglecting to copy the trailing ">".
> > was:    "Name <addr.org"
> > needed: "Name <addr.org>"
> > Anyone have suggestions on how to get
> > git-send-email.perl to notify and abort
> > sending on more invalid address styles?
> 
> Something along these lines? Of course, the error message is, uhm,
> less than helpful :)
> 
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -787,6 +787,10 @@ sub is_rfc2047_quoted {
>  sub sanitize_address
>  {
>  	my ($recipient) = @_;
> +	if ($recipient =~ m/.*<[^>]*$/) {
> +		die "EEK!"
> +	}
> +
Maybe this?  Seems to work.
diff --git a/git-send-email.perl b/git-send-email.perl
index a0279de..52ddd9e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -374,15 +374,18 @@ my ($repoauthor, $repocommitter);
 # Verify the user input
 
 foreach my $entry (@to) {
-	die "Comma in --to entry: $entry'\n" unless $entry !~ m/,/;
+	die "Comma in --to entry: '$entry'\n" unless $entry !~ m/,/;
+	die "Invalid --to entry: '$entry'\n"  unless $entry !~ m/.*<[^>]*$/;
 }
 
 foreach my $entry (@initial_cc) {
-	die "Comma in --cc entry: $entry'\n" unless $entry !~ m/,/;
+	die "Comma in --cc entry: '$entry'\n" unless $entry !~ m/,/;
+	die "Invalid --cc entry: '$entry'\n" unless $entry !~ m/.*<[^>]*$/;
 }
 
 foreach my $entry (@bcclist) {
-	die "Comma in --bcclist entry: $entry'\n" unless $entry !~ m/,/;
+	die "Comma in --bcclist entry: '$entry'\n" unless $entry !~ m/,/;
+	die "Invalid --bcclist entry: '$entry'\n" unless $entry !~ m/.*<[^>]*$/;
 }
 
 sub parse_address_line {
Erik Faye-Lund· Oct 20, 2009, 22:56 UTC · re: Joe Perches · lore

Re: git-send-email.perl defect: address missing trailing > accepted

On Wed, Oct 21, 2009 at 12:48 AM, Joe Perches <joe@perches.com> wrote:
Show 5 quoted lines
>> Something along these lines? Of course, the error message is, uhm,
>> less than helpful :)
>
> Maybe this?  Seems to work.
>
Didn't my version work for you? It worked for me.

I find it a bit cleaner to make it a part of the address-sanitizion, since that needs to be performed for all addresses. I might miss something vital, though. I don't really speak perl all that well ;)

-- 
Erik "kusma" Faye-Lund
Joe Perches· Oct 20, 2009, 23:00 UTC · re: Erik Faye-Lund · lore

Re: git-send-email.perl defect: address missing trailing > accepted

On Wed, 2009-10-21 at 00:56 +0200, Erik Faye-Lund wrote:
> Didn't my version work for you? It worked for me.
Hi Erik.

It worked, but an unexplained die isn't great, so I put it where the other validations are done.

It seems that the regex for address validation isn't very good and perhaps there could/should be a stronger validation done for each address entered.

cheers, Joe
Junio C Hamano· Oct 20, 2009, 23:28 UTC · re: Joe Perches · lore

Re: git-send-email.perl defect: address missing trailing > accepted

Joe Perches <joe@perches.com> writes:
> It seems that the regex for address validation
> isn't very good and perhaps there could/should
> be a stronger validation done for each address
> entered.

The existing ones are actually already harmful. It would trigger on a valid addresses like this, wouldn't it?

    To: "Hamano, Jun" <gitster@pobox.com>
It is worse than that.

The "# Verify the user input" block is in a wrong place in the codepath. After @to goes through this bogus "verification" step, it then is given to expand_aliases(), which may expand to real addresses. And then they pass through sanitize_address() before getting used.

Three implications that come from this wrong code structure are:
 (1) The stricter checks you added on top of the existing bogus
     verification step may prevent @to to reach expand_aliases() step,
     even if the tokens in @to may expand to correct addresses by this
     expand_aliases() step if they were allowed to reach here;
 (2) The result from expand_aliases() is never checked.
 (3) The result from sanitize_address() is not checked either.

← back to recent threads