send-email garbled header with trailing doublequote in email

send-email garbled header with trailing doublequote in email

[Andrea Arcangeli]

Hello,

send-email gets confused with one trailing " at the end of the
email. Details and how to reproduce below, it breaks also with
upstream git version 2.10.2.dirty.

Feel free to CC me if you need any further info.

Thanks,
Andrea

----- Forwarded message from Andrea Arcangeli <aarcange@redhat.com> -----

Date: Wed, 2 Nov 2016 21:07:02 +0100
From: Andrea Arcangeli <aarcange@redhat.com>
To: linux-mm@kvack.org
Subject: Re: [PATCH 00/33] userfaultfd tmpfs/hugetlbfs/non-cooperative
User-Agent: Mutt/1.7.1 (2016-10-04)

FYI: apparently I hit a git bug in this submit... reproducible with
the below command:

git send-email -1 --to '"what ever" <--your--@--email--.com>"'

after replacing --your--@email--.com with your own email.

*snip*
Dry-OK. Log says:
To: "what ever" " <--your--@--email--.com>
*snip*
X-Mailer: git-send-email 2.7.3

Result: OK

It's not ok if the --dry-run outputs the above with a fine header, but
the actual header in the email data is different. Of course I tested
--dry-run twice and it was fine like the above is fine as well.

*snip*

----- End forwarded message -----

Re: send-email garbled header with trailing doublequote in email

[Jeff King]

On Wed, Nov 02, 2016 at 09:27:09PM +0100, Andrea Arcangeli wrote:

> send-email gets confused with one trailing " at the end of the
> email. Details and how to reproduce below, it breaks also with
> upstream git version 2.10.2.dirty.

I'm not quite sure what happened, and what you wanted to happen. In your
example:

> FYI: apparently I hit a git bug in this submit... reproducible with
> the below command:
> 
> git send-email -1 --to '"what ever" <--your--@--email--.com>"'

The "to" address is slightly bogus here because of the extra
double-quote. That gets turned into a slightly bogus rfc2822 header:

> *snip*
> Dry-OK. Log says:
> To: "what ever" " <--your--@--email--.com>

Which is funny, but matches what we do with other addresses that are
invalid according to the rfc. E.g., there was a discussion recently on:

  Stable <stable@vger.kernel.org> [4.8+]

which has historically been converted to:

  "Stable [4.8+]" <stable@vger.kernel.org>

In fact, it is not even git that does this, but rather what
Mail::Address happens to output (though git has fallback routines if
that module isn't available that do the same thing).

But in either case, in my test, the actual email address is still
extracted correctly and fed to the MTA, so the mail is delivered.

So I'm not sure what you wanted to happen that didn't. Did you want
--dry-run to complain about the bogus address? Did the message not get
delivered for you? Something else?

-Peff

Re: send-email garbled header with trailing doublequote in email

[Andrea Arcangeli]

Hello,

On Wed, Nov 02, 2016 at 05:11:18PM -0400, Jeff King wrote:
> In fact, it is not even git that does this, but rather what
> Mail::Address happens to output (though git has fallback routines if
> that module isn't available that do the same thing).

If it's something in perl it should be fixed there indeed. I didn't
debug what exactly was wrong, so I sent it here first.

> But in either case, in my test, the actual email address is still
> extracted correctly and fed to the MTA, so the mail is delivered.

The email is delivered to the right email for me too, but to see the
problem you need to check the email itself, and look how the To: field
looks in the actual email in your mail client or web interface.

Don't you see whatever@yourhostname without spaces and @yourhostname
instead of the email domain?

If you still can't reproduce, maybe it's a different perl
Mail::Address, I'm using dev-perl/MailTools-2.140.0

From who receives the email it just looks garbled, but --dry-run
showed a correct To/Cc list so it was undetectable that it would end
up garbled during the real email delivery.

> So I'm not sure what you wanted to happen that didn't. Did you want
> --dry-run to complain about the bogus address? Did the message not get
> delivered for you? Something else?

If --dry-run complained and failed instead of passing and then sending
an email with garbled To/Cc list, it'd be more than enough. Either
that or it should generate a mail header that matches the output of
--dry-run so the review of --dry-run becomes meaningful again.

Thanks,
Andrea

Re: send-email garbled header with trailing doublequote in email

[Jeff King]

On Wed, Nov 02, 2016 at 10:38:05PM +0100, Andrea Arcangeli wrote:

> > But in either case, in my test, the actual email address is still
> > extracted correctly and fed to the MTA, so the mail is delivered.
> 
> The email is delivered to the right email for me too, but to see the
> problem you need to check the email itself, and look how the To: field
> looks in the actual email in your mail client or web interface.
> 
> Don't you see whatever@yourhostname without spaces and @yourhostname
> instead of the email domain?

Nope, it looks exactly as --dry-run reports it.

To see exactly what is being sent out, try:

-- >8 --

cat >/tmp/foo <<\EOF
#!/bin/sh
echo "args: $*"
sed 's/^/stdin: /'
EOF

chmod +x /tmp/foo

git send-email --smtp-server=/tmp/foo --to=whatever

-- 8< --

Mine shows the same header as --dry-run. Which makes sense, because the
code is literally just dumping the $header variable, which is the same
thing that gets sent to the sendmail binary (or the SMTP server if that
is in use).

So my guess would be that either an MTA in the routing path is garbling
it (or possibly mailing list software). or maybe even the eventual MUA,
though it sounds like you checked the raw bytes.

> If you still can't reproduce, maybe it's a different perl
> Mail::Address, I'm using dev-perl/MailTools-2.140.0

Mine is 2.13, which I would imagine is comparable.

> If --dry-run complained and failed instead of passing and then sending
> an email with garbled To/Cc list, it'd be more than enough. Either
> that or it should generate a mail header that matches the output of
> --dry-run so the review of --dry-run becomes meaningful again.

OK. I think we get that first part right. The problem is that the
garbling is such that somebody in the middle is unhappy with it (which
makes sense; it's syntactically bogus). So the tool is there to see the
problem in --dry-run, but of course it's rather subtle.

In theory we should be able to detect and complain about bogus syntax
like this, but right now we don't re-parse the addresses at all. We rely
on Mail::Address to produce valid output, and it doesn't seem to be
doing so here.

-Peff

Re: send-email garbled header with trailing doublequote in email

[Andrea Arcangeli]

On Wed, Nov 02, 2016 at 06:04:37PM -0400, Jeff King wrote:
> Nope, it looks exactly as --dry-run reports it.

My sendmail is postfix 3.1.0.

> To see exactly what is being sent out, try:
> 
> -- >8 --
> 
> cat >/tmp/foo <<\EOF
> #!/bin/sh
> echo "args: $*"
> sed 's/^/stdin: /'
> EOF
> 
> chmod +x /tmp/foo
> 
> git send-email --smtp-server=/tmp/foo --to=whatever
> 
> -- 8< --

Right it's the same as --dry-run:

stdin: To: "what ever" " <.....>

There's not my hostname and not removed space. If I add more addresses
they also go in the second line with a leading space and they're not cut.

So this must be postfix then that out of the blue decided to garble it
in a strange way while parsing the input... The removal of all
whitespaces s/what ever/whatever/ especially I've no idea how it
decided to do so.

Can you reproduce with postfix as sendmail at least? If you can
reproduce also see what happens if you add another --to.

Re: send-email garbled header with trailing doublequote in email

[Jeff King]

On Wed, Nov 02, 2016 at 11:29:01PM +0100, Andrea Arcangeli wrote:

> So this must be postfix then that out of the blue decided to garble it
> in a strange way while parsing the input... The removal of all
> whitespaces s/what ever/whatever/ especially I've no idea how it
> decided to do so.
> 
> Can you reproduce with postfix as sendmail at least? If you can
> reproduce also see what happens if you add another --to.

Yes, I can easily reproduce without using git at all by installing
postfix in local-delivery mode and running:

sendmail peff@sigill.intra.peff.net <<\EOF
From: Jeff King <peff@peff.net>
To: "what ever" " <peff@sigill.intra.peff.net>
Subject: patch

This is the body
EOF

Many MTAs do this kind of header-rewriting. I don't necessarily agree
with it as a general concept, but the real problem is the syntactically
bogus header. The munging that postfix does makes things worse, but I
can see why it is confused and does what it does (the whole email is
inside a double-quoted portion that is never closed, so it probably
thinks there is no hostname portion at all).

So git is possibly at fault for passing along a bogus address. OTOH, the
user is perhaps at fault for providing the bogus address to git in the
first place. GIGO. :)

I think if any change were to be made, it would be to recognize this
bogosity and either clean it up or abort. That ideally would happen via
Mail::Address so git does not have to add a bunch of ad-hoc "is this
valid rfc822" checks. Reading the manpage for that module, though, it
says:

  [we do not handle all of rfc2822]
  Often requests are made to the maintainers of this code improve this
  situation, but this is not a good idea, where it will break zillions
  of existing applications.  If you wish for a fully RFC2822 compliant
  implementation you may take a look at Mail::Message::Field::Full, part
  of MailBox.

So it's possible that switching to a more robust module would improve
things.

-Peff

Re: send-email garbled header with trailing doublequote in email

[demerphq]

On 3 November 2016 at 15:18, Jeff King <peff@peff.net> wrote:
> On Wed, Nov 02, 2016 at 11:29:01PM +0100, Andrea Arcangeli wrote:
>
>> So this must be postfix then that out of the blue decided to garble it
>> in a strange way while parsing the input... The removal of all
>> whitespaces s/what ever/whatever/ especially I've no idea how it
>> decided to do so.
>>
>> Can you reproduce with postfix as sendmail at least? If you can
>> reproduce also see what happens if you add another --to.
>
> Yes, I can easily reproduce without using git at all by installing
> postfix in local-delivery mode and running:
>
> sendmail peff@sigill.intra.peff.net <<\EOF
> From: Jeff King <peff@peff.net>
> To: "what ever" " <peff@sigill.intra.peff.net>
> Subject: patch
>
> This is the body
> EOF
>
> Many MTAs do this kind of header-rewriting. I don't necessarily agree
> with it as a general concept, but the real problem is the syntactically
> bogus header. The munging that postfix does makes things worse, but I
> can see why it is confused and does what it does (the whole email is
> inside a double-quoted portion that is never closed, so it probably
> thinks there is no hostname portion at all).
>
> So git is possibly at fault for passing along a bogus address. OTOH, the
> user is perhaps at fault for providing the bogus address to git in the
> first place. GIGO. :)
>
> I think if any change were to be made, it would be to recognize this
> bogosity and either clean it up or abort. That ideally would happen via
> Mail::Address so git does not have to add a bunch of ad-hoc "is this
> valid rfc822" checks. Reading the manpage for that module, though, it
> says:
>
>   [we do not handle all of rfc2822]
>   Often requests are made to the maintainers of this code improve this
>   situation, but this is not a good idea, where it will break zillions
>   of existing applications.  If you wish for a fully RFC2822 compliant
>   implementation you may take a look at Mail::Message::Field::Full, part
>   of MailBox.
>
> So it's possible that switching to a more robust module would improve
> things.

There is an RFC2822 compliant email address validator in the perl test
suite if you guys want to use it.  We use it to test recursive
patterns.

http://perl5.git.perl.org/perl.git/blob/HEAD:/t/re/reg_email.t

Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: send-email garbled header with trailing doublequote in email

[Andrea Arcangeli]

On Thu, Nov 03, 2016 at 10:18:48AM -0400, Jeff King wrote:
> bogus header. The munging that postfix does makes things worse, but I

I agree it makes things worse.

> can see why it is confused and does what it does (the whole email is
> inside a double-quoted portion that is never closed, so it probably
> thinks there is no hostname portion at all).

I would see it too, if it actually sent the email to the garbled email
address it generated, but it actually got the right email address
(because it sent the email to the right address), but it decided to
show a different email address in the mail header than the one it sent
the email to. But I figure this is the wrong list for such questions :).

> I think if any change were to be made, it would be to recognize this
> bogosity and either clean it up or abort. That ideally would happen via

If postfix continues to do what it does, a strict checking would be my
preference of course, assuming it won't break anything that currently
works...

If not it's fine too, as nothing particularly "unexpected" happened in
git.

At this point double checking that what postfix does is legit is
perhaps more worthwhile than adding strictness to git.

Thanks,
Andrea

Re: send-email garbled header with trailing doublequote in email

[Jeff King]

On Fri, Nov 04, 2016 at 01:03:51AM +0100, Andrea Arcangeli wrote:

> > can see why it is confused and does what it does (the whole email is
> > inside a double-quoted portion that is never closed, so it probably
> > thinks there is no hostname portion at all).
> 
> I would see it too, if it actually sent the email to the garbled email
> address it generated, but it actually got the right email address
> (because it sent the email to the right address), but it decided to
> show a different email address in the mail header than the one it sent
> the email to. But I figure this is the wrong list for such questions :).

It has to do with the "envelope recipient" versus the RFC822 header. Git
provides the envelope recipient on the command-line, and that is what is
used in the SMTP conversation. In theory the MTA does not need to ever
even look at the contents of the message itself. Some do not, but some
have features like rewriting the in-message headers (e.g., to turn
"to: user" into "to: user@hostname"). You can probably disable that
header-rewriting feature in your config, but I don't know very much
about configuring postfix.

-Peff