{"thread":{"id":"56599","subject":"[PATCH] avoid insecure use of mail in man page example","startedAt":"2021-09-28T12:27:10Z","lastAt":"2021-10-18T00:55:10Z","messageCount":5,"participants":["Joey Hess","Jeff King","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"437281","messageId":"20210928121648.1390915-1-joeyh@joeyh.name","threadId":"56599","inReplyTo":null,"subject":"[PATCH] avoid insecure use of mail in man page example","fromName":"Joey Hess","fromEmail":"joeyh@joeyh.name","sentAt":"2021-09-28T12:16:48Z","receivedAt":"2021-09-28T12:27:10Z","isPatch":true,"sender":{"key":"joeyh@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"As recently seen in fail2ban's security hole (CVE-2021-32749),\npiping user controlled input to mail is exploitable,\nsince a line starting with \"~! foo\" in the input will run command foo.\n\nThis example on the man page pipes to mail. It may not be exploitable.\ngit rev-list --pretty indents commit messages, which prevents the escape\nsequence working there. It's less clear if it might be possible to embed\nthe escape sequence in a signed push certificate. The user reading the\nman page might alter the example to do something more exploitable.\nTo encourage safe use of mail, add -E 'set escape'\n\nSigned-off-by: Joey Hess <joeyh@joeyh.name>\n---\n Documentation/git-receive-pack.txt | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt\nindex 014a78409b..cdaae75365 100644\n--- a/Documentation/git-receive-pack.txt\n+++ b/Documentation/git-receive-pack.txt\n@@ -183,7 +183,7 @@ do\n \t\techo \"New commits:\"\n \t\tgit rev-list --pretty \"$nval\" \"^$oval\"\n \tfi |\n-\tmail -s \"Changes to ref $ref\" commit-list@mydomain\n+\tmail -E 'set escape' -s \"Changes to ref $ref\" commit-list@mydomain\n done\n # log signed push certificate, if any\n if test -n \"${GIT_PUSH_CERT-}\" && test ${GIT_PUSH_CERT_STATUS} = G\n@@ -191,7 +191,7 @@ then\n \t(\n \t\techo expected nonce is ${GIT_PUSH_NONCE}\n \t\tgit cat-file blob ${GIT_PUSH_CERT}\n-\t) | mail -s \"push certificate from $GIT_PUSH_CERT_SIGNER\" push-log@mydomain\n+\t) | mail -E 'set escape' -s \"push certificate from $GIT_PUSH_CERT_SIGNER\" push-log@mydomain\n fi\n exit 0\n ----\n-- \n2.33.0\n\n"},{"id":"437344","messageId":"YVNi91WYyj3Le6UF@coredump.intra.peff.net","threadId":"56599","inReplyTo":"20210928121648.1390915-1-joeyh@joeyh.name","subject":"Re: [PATCH] avoid insecure use of mail in man page example","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-28T18:46:15Z","receivedAt":"2021-09-28T18:46:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 28, 2021 at 08:16:48AM -0400, Joey Hess wrote:\n\n> As recently seen in fail2ban's security hole (CVE-2021-32749),\n> piping user controlled input to mail is exploitable,\n> since a line starting with \"~! foo\" in the input will run command foo.\n> \n> This example on the man page pipes to mail. It may not be exploitable.\n> git rev-list --pretty indents commit messages, which prevents the escape\n> sequence working there. It's less clear if it might be possible to embed\n> the escape sequence in a signed push certificate. The user reading the\n> man page might alter the example to do something more exploitable.\n> To encourage safe use of mail, add -E 'set escape'\n\nSeems like a good goal, but is \"-E\" portable?\n\nOn my system, where \"mail\" comes from the bsd-mailx package, \"-E\" means\n\"do not send a message with an empty body\" and your example command\nbarfs as it tries to deliver to the recipient \"set escape\".\n\nAt least we'd want to make a note in the documentation saying what the\nmysterious \"set escape\" is doing, and that not all versions of mail\nwould need / want it.\n\n-Peff\n"},{"id":"437392","messageId":"xmqqtui4gt5f.fsf@gitster.g","threadId":"56599","inReplyTo":"YVNi91WYyj3Le6UF@coredump.intra.peff.net","subject":"Re: [PATCH] avoid insecure use of mail in man page example","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-28T23:46:52Z","receivedAt":"2021-09-28T23:50:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Sep 28, 2021 at 08:16:48AM -0400, Joey Hess wrote:\n>\n>> As recently seen in fail2ban's security hole (CVE-2021-32749),\n>> piping user controlled input to mail is exploitable,\n>> since a line starting with \"~! foo\" in the input will run command foo.\n>> \n>> This example on the man page pipes to mail. It may not be exploitable.\n>> git rev-list --pretty indents commit messages, which prevents the escape\n>> sequence working there. It's less clear if it might be possible to embed\n>> the escape sequence in a signed push certificate. The user reading the\n>> man page might alter the example to do something more exploitable.\n>> To encourage safe use of mail, add -E 'set escape'\n>\n> Seems like a good goal, but is \"-E\" portable?\n>\n> On my system, where \"mail\" comes from the bsd-mailx package, \"-E\" means\n> \"do not send a message with an empty body\" and your example command\n> barfs as it tries to deliver to the recipient \"set escape\".\n>\n> At least we'd want to make a note in the documentation saying what the\n> mysterious \"set escape\" is doing, and that not all versions of mail\n> would need / want it.\n\nIt is not the primary focus for this documentation page to teach how\nto send e-mails in the first place.  Instead of risking confused\nusers rightly complain with \"my 'mail' does not understand the -E\noption---what does this do?\", I wonder if it is better to just change it to\n\n\tgit rev-list --pretty ...\n-   fi |\n-   mail -s ...    \n+   fi >>/var/log/update.log\n\nso that it illustrates what's available *out* *of* *us* to the\nauthors of the script, without having to teach them \"mail\" and other\nthings we are responsible for.\n\n"},{"id":"437402","messageId":"YVOy0HLvManYQdGo@coredump.intra.peff.net","threadId":"56599","inReplyTo":"xmqqtui4gt5f.fsf@gitster.g","subject":"Re: [PATCH] avoid insecure use of mail in man page example","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-29T00:26:56Z","receivedAt":"2021-09-29T00:27:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 28, 2021 at 04:46:52PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Tue, Sep 28, 2021 at 08:16:48AM -0400, Joey Hess wrote:\n> >\n> >> As recently seen in fail2ban's security hole (CVE-2021-32749),\n> >> piping user controlled input to mail is exploitable,\n> >> since a line starting with \"~! foo\" in the input will run command foo.\n> >> \n> >> This example on the man page pipes to mail. It may not be exploitable.\n> >> git rev-list --pretty indents commit messages, which prevents the escape\n> >> sequence working there. It's less clear if it might be possible to embed\n> >> the escape sequence in a signed push certificate. The user reading the\n> >> man page might alter the example to do something more exploitable.\n> >> To encourage safe use of mail, add -E 'set escape'\n> >\n> > Seems like a good goal, but is \"-E\" portable?\n> >\n> > On my system, where \"mail\" comes from the bsd-mailx package, \"-E\" means\n> > \"do not send a message with an empty body\" and your example command\n> > barfs as it tries to deliver to the recipient \"set escape\".\n> >\n> > At least we'd want to make a note in the documentation saying what the\n> > mysterious \"set escape\" is doing, and that not all versions of mail\n> > would need / want it.\n> \n> It is not the primary focus for this documentation page to teach how\n> to send e-mails in the first place.  Instead of risking confused\n> users rightly complain with \"my 'mail' does not understand the -E\n> option---what does this do?\", I wonder if it is better to just change it to\n> \n> \tgit rev-list --pretty ...\n> -   fi |\n> -   mail -s ...    \n> +   fi >>/var/log/update.log\n> \n> so that it illustrates what's available *out* *of* *us* to the\n> authors of the script, without having to teach them \"mail\" and other\n> things we are responsible for.\n\nYeah, I'd agree that side-stepping the issue entirely is a good\ndirection. Doing it right is probably best left to tools like\ngit-multimail.\n\n-Peff\n"},{"id":"438976","messageId":"YWzF6deqfffBM7ub@gmail.com","threadId":"56599","inReplyTo":"YVOy0HLvManYQdGo@coredump.intra.peff.net","subject":"Re: [PATCH] avoid insecure use of mail in man page example","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2021-10-18T00:55:05Z","receivedAt":"2021-10-18T00:55:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Tue, Sep 28, 2021 at 04:46:52PM -0700, Junio C Hamano wrote:\n>>> On Tue, Sep 28, 2021 at 08:16:48AM -0400, Joey Hess wrote:\n\n>>>> As recently seen in fail2ban's security hole (CVE-2021-32749),\n>>>> piping user controlled input to mail is exploitable,\n>>>> since a line starting with \"~! foo\" in the input will run command foo.\n[...]\n>> It is not the primary focus for this documentation page to teach how\n>> to send e-mails in the first place.  Instead of risking confused\n>> users rightly complain with \"my 'mail' does not understand the -E\n>> option---what does this do?\", I wonder if it is better to just change it to\n>> \n>> \tgit rev-list --pretty ...\n>> -   fi |\n>> -   mail -s ...    \n>> +   fi >>/var/log/update.log\n>> \n>> so that it illustrates what's available *out* *of* *us* to the\n>> authors of the script, without having to teach them \"mail\" and other\n>> things we are responsible for.\n>\n> Yeah, I'd agree that side-stepping the issue entirely is a good\n> direction. Doing it right is probably best left to tools like\n> git-multimail.\n\nThis makes sense to me.  Joey, are you planning to send an updated\nversion of the patch, or would you like us to take care of it?\n\nThanks,\nJonathan\n"}]}