[PATCH] avoid insecure use of mail in man page example

[PATCH] avoid insecure use of mail in man page example

[Joey Hess]

As recently seen in fail2ban's security hole (CVE-2021-32749),
piping user controlled input to mail is exploitable,
since a line starting with "~! foo" in the input will run command foo.

This example on the man page pipes to mail. It may not be exploitable.
git rev-list --pretty indents commit messages, which prevents the escape
sequence working there. It's less clear if it might be possible to embed
the escape sequence in a signed push certificate. The user reading the
man page might alter the example to do something more exploitable.
To encourage safe use of mail, add -E 'set escape'

Signed-off-by: Joey Hess <joeyh@joeyh.name>
---
 Documentation/git-receive-pack.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 014a78409b..cdaae75365 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -183,7 +183,7 @@ do
 		echo "New commits:"
 		git rev-list --pretty "$nval" "^$oval"
 	fi |
-	mail -s "Changes to ref $ref" commit-list@mydomain
+	mail -E 'set escape' -s "Changes to ref $ref" commit-list@mydomain
 done
 # log signed push certificate, if any
 if test -n "${GIT_PUSH_CERT-}" && test ${GIT_PUSH_CERT_STATUS} = G
@@ -191,7 +191,7 @@ then
 	(
 		echo expected nonce is ${GIT_PUSH_NONCE}
 		git cat-file blob ${GIT_PUSH_CERT}
-	) | mail -s "push certificate from $GIT_PUSH_CERT_SIGNER" push-log@mydomain
+	) | mail -E 'set escape' -s "push certificate from $GIT_PUSH_CERT_SIGNER" push-log@mydomain
 fi
 exit 0
 ----
-- 
2.33.0

Re: [PATCH] avoid insecure use of mail in man page example

[Jeff King]

On Tue, Sep 28, 2021 at 08:16:48AM -0400, Joey Hess wrote:

> As recently seen in fail2ban's security hole (CVE-2021-32749),
> piping user controlled input to mail is exploitable,
> since a line starting with "~! foo" in the input will run command foo.
> 
> This example on the man page pipes to mail. It may not be exploitable.
> git rev-list --pretty indents commit messages, which prevents the escape
> sequence working there. It's less clear if it might be possible to embed
> the escape sequence in a signed push certificate. The user reading the
> man page might alter the example to do something more exploitable.
> To encourage safe use of mail, add -E 'set escape'

Seems like a good goal, but is "-E" portable?

On my system, where "mail" comes from the bsd-mailx package, "-E" means
"do not send a message with an empty body" and your example command
barfs as it tries to deliver to the recipient "set escape".

At least we'd want to make a note in the documentation saying what the
mysterious "set escape" is doing, and that not all versions of mail
would need / want it.

-Peff

Re: [PATCH] avoid insecure use of mail in man page example

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 28, 2021 at 08:16:48AM -0400, Joey Hess wrote:
>
>> As recently seen in fail2ban's security hole (CVE-2021-32749),
>> piping user controlled input to mail is exploitable,
>> since a line starting with "~! foo" in the input will run command foo.
>> 
>> This example on the man page pipes to mail. It may not be exploitable.
>> git rev-list --pretty indents commit messages, which prevents the escape
>> sequence working there. It's less clear if it might be possible to embed
>> the escape sequence in a signed push certificate. The user reading the
>> man page might alter the example to do something more exploitable.
>> To encourage safe use of mail, add -E 'set escape'
>
> Seems like a good goal, but is "-E" portable?
>
> On my system, where "mail" comes from the bsd-mailx package, "-E" means
> "do not send a message with an empty body" and your example command
> barfs as it tries to deliver to the recipient "set escape".
>
> At least we'd want to make a note in the documentation saying what the
> mysterious "set escape" is doing, and that not all versions of mail
> would need / want it.

It is not the primary focus for this documentation page to teach how
to send e-mails in the first place.  Instead of risking confused
users rightly complain with "my 'mail' does not understand the -E
option---what does this do?", I wonder if it is better to just change it to

	git rev-list --pretty ...
-   fi |
-   mail -s ...    
+   fi >>/var/log/update.log

so that it illustrates what's available *out* *of* *us* to the
authors of the script, without having to teach them "mail" and other
things we are responsible for.

Re: [PATCH] avoid insecure use of mail in man page example

[Jeff King]

On Tue, Sep 28, 2021 at 04:46:52PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Sep 28, 2021 at 08:16:48AM -0400, Joey Hess wrote:
> >
> >> As recently seen in fail2ban's security hole (CVE-2021-32749),
> >> piping user controlled input to mail is exploitable,
> >> since a line starting with "~! foo" in the input will run command foo.
> >> 
> >> This example on the man page pipes to mail. It may not be exploitable.
> >> git rev-list --pretty indents commit messages, which prevents the escape
> >> sequence working there. It's less clear if it might be possible to embed
> >> the escape sequence in a signed push certificate. The user reading the
> >> man page might alter the example to do something more exploitable.
> >> To encourage safe use of mail, add -E 'set escape'
> >
> > Seems like a good goal, but is "-E" portable?
> >
> > On my system, where "mail" comes from the bsd-mailx package, "-E" means
> > "do not send a message with an empty body" and your example command
> > barfs as it tries to deliver to the recipient "set escape".
> >
> > At least we'd want to make a note in the documentation saying what the
> > mysterious "set escape" is doing, and that not all versions of mail
> > would need / want it.
> 
> It is not the primary focus for this documentation page to teach how
> to send e-mails in the first place.  Instead of risking confused
> users rightly complain with "my 'mail' does not understand the -E
> option---what does this do?", I wonder if it is better to just change it to
> 
> 	git rev-list --pretty ...
> -   fi |
> -   mail -s ...    
> +   fi >>/var/log/update.log
> 
> so that it illustrates what's available *out* *of* *us* to the
> authors of the script, without having to teach them "mail" and other
> things we are responsible for.

Yeah, I'd agree that side-stepping the issue entirely is a good
direction. Doing it right is probably best left to tools like
git-multimail.

-Peff

Re: [PATCH] avoid insecure use of mail in man page example

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Tue, Sep 28, 2021 at 04:46:52PM -0700, Junio C Hamano wrote:
>>> On Tue, Sep 28, 2021 at 08:16:48AM -0400, Joey Hess wrote:

>>>> As recently seen in fail2ban's security hole (CVE-2021-32749),
>>>> piping user controlled input to mail is exploitable,
>>>> since a line starting with "~! foo" in the input will run command foo.
[...]
>> It is not the primary focus for this documentation page to teach how
>> to send e-mails in the first place.  Instead of risking confused
>> users rightly complain with "my 'mail' does not understand the -E
>> option---what does this do?", I wonder if it is better to just change it to
>> 
>> 	git rev-list --pretty ...
>> -   fi |
>> -   mail -s ...    
>> +   fi >>/var/log/update.log
>> 
>> so that it illustrates what's available *out* *of* *us* to the
>> authors of the script, without having to teach them "mail" and other
>> things we are responsible for.
>
> Yeah, I'd agree that side-stepping the issue entirely is a good
> direction. Doing it right is probably best left to tools like
> git-multimail.

This makes sense to me.  Joey, are you planning to send an updated
version of the patch, or would you like us to take care of it?

Thanks,
Jonathan