Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms

[Jan Viktorin <viktorin@rehivetech.com>]

Authen::SASL gives:

No SASL mechanism found
 at /usr/share/perl5/vendor_perl/Authen/SASL.pm line 77.
 at /usr/share/perl5/core_perl/Net/SMTP.pm line 207.

The SASL library does not check validity of mechanisms'
names (or I did not find it). It just tries to load one
that matches both the ours and the server side ones.

I can see one possible weakness of this, however I doubt
whether there exists a successful attack vector. Imagine
that somebody gives me a malicious .gitconfig with
smtpauth = ~/ATTACK and redirects me to a fake mail
server that advertises ~/ATTACK as a working mechanism.
This might lead to an unwanted execution of ~/ATTACK.pm.
Should we consider this to be a threat?

Another thing that confuses me (I mentioned it in the
previous e-mail). I forced to use CRAM-MD5, however, it
dies with the above errors. The CRAM-MD5 is installed:

/usr/share/perl5/vendor_perl/Authen/SASL/CRAM_MD5.pm
/usr/share/perl5/vendor_perl/Authen/SASL/Perl/CRAM_MD5.pm

The same for DIGEST-MD5. On different PC with the same
set of libraries, OS, the CRAM-MD5 just works. Why? LOGIN
and PLAIN are OK. Environment? (I doubt.)

I would like to include the regex check based on RFC 4422
as I've already mentioned. at least, it filters out the
unwanted characters like '/', '.', etc.

Regards
Jan

On Sun, 2 Aug 2015 05:41:29 -0400
Eric Sunshine <sunshine@sunshineco.com> wrote:

> On Sat, Aug 1, 2015 at 2:19 PM, Jan Viktorin
> <viktorin@rehivetech.com> wrote:
> > On Sat, 1 Aug 2015 05:33:28 -0400 Eric Sunshine
> > <sunshine@sunshineco.com> wrote:
> >> On Fri, Jul 31, 2015 at 7:33 PM, Jan Viktorin
> >> <viktorin@rehivetech.com> wrote:
> >> At the very least, you will also want to update the documentation
> >> (Documentation/git-send-email.txt) and, if possible, add new tests
> >> (t/t9001-send-email.sh).
> >
> > I will update the documentation when it is clear, how the smtp-auth
> > works.
> >
> > I have no idea, how to test the feature. I can see something like
> > fake.sendmail in the file. How does it work? I can image a test
> > whether user inserts valid values. What more?
> 
> That's what I was thinking. You could test if the die() is triggered
> or if it emits warnings for bad values (assuming you implement that
> feature). As for testing the actual authentication, I'm not sure you
> can (and don't see any such testing in the script).
> 
> >> > diff --git a/git-send-email.perl b/git-send-email.perl
> >> > index ae9f869..b00ed9d 100755
> >> > --- a/git-send-email.perl
> >> > +++ b/git-send-email.perl
> >> > @@ -1129,6 +1134,16 @@ sub smtp_auth_maybe {
> >> >                 return 1;
> >> >         }
> >> >
> >> > +       # Do not allow arbitrary strings.
> >>
> >> Can you explain why this restriction is needed. What are the
> >> consequences of not limiting the input to this "approved" list?
> >
> > This is more a check of an arbitrary user input then a check
> > of an "approved list". It should be also used to inform user
> > about invalid methods (however, I didn't implemented it yet).
> 
> What I was really asking was whether this sort of checking really
> belongs in git-send-email or if it is better left to Net::SMTP (and
> Authen::SASL) to do so since they are in better positions to know what
> is valid and what is not. If the Perl module(s) generate suitable
> diagnostics for bad input, then it makes sense to leave the checking
> to them. If not, then I can understand your motivation for
> git-send-email doing the checking instead in order to emit
> user-friendly diagnostics.
> 
> So, that's what I meant when I asked 'What are the consequences of not
> limiting the input to this "approved" list?'.
> 
> The other reason I asked was that it increases maintenance costs for
> us to maintain a list of "approved" mechanisms, since the list needs
> to be updated when new ones are implemented (and, as brian pointed
> out, some may already exist which are not in your list).
>
> (...)
>
> >> Also, don't you want to warn the user about tokens that don't match
> >> one of the accepted (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5), rather
> >> than dropping them silently?
> >
> > Yes, this would be great (as I've already mentioned). It's a
> > question whether to include the check for the mechanisms or whether
> > to leave the $smtp_auth variable as it is... Maybe just validate by
> > a regex?
> >
> > The naming rules are defiend here:
> >  https://tools.ietf.org/html/rfc4422#page-8
> > So, this looks to me as a better way.
> 
> Maybe. This leads back to my original question of whether it's really
> git-send-email's responsibility to do validation or if that can be
> left to Net::SMTP/Authen::SASL. If the Perl module(s) emit suitable
> diagnostics for bad input, then validation can be omitted from
> git-send-email.



-- 
  Jan Viktorin                E-mail: Viktorin@RehiveTech.com
  System Architect            Web:    www.RehiveTech.com
  RehiveTech                  Phone: +420 606 201 868
  Brno, Czech Republic