[git-multimail] smtplib, check certificate

[git-multimail] smtplib, check certificate

[Simon P]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

Hi,

It seems that smtplib doesn't check if a certificate is valid (signed by
a trusted CA).

For my personal usage, I patched the starttls code in git-multimail:
only for starttls with smtplib.

This patch is inspired from

https://github.com/graingert/secure-smtplib/blob/master/src/secure_smtplib/__init__.py

It could be easy to add support cert check in for smtps (see
secure_smtplib).

This patch was tested only on git-multimail (v1.2)

It introduces two new options:
  - multimailhook.smtpcheckcert (default false)
  - multimailhook.smtpcacerts (default
                               /etc/ssl/certs/ca-certificates.crt)

Best regards,
Simon P.

[-- Attachment #2: git-multimail-secure-smtplib.patch --]
[-- Type: text/x-patch, Size: 4235 bytes --]

diff --git a/git-multimail/git_multimail.py b/git-multimail/git_multimail.py
index fae5c91..b49ed9d 100755
--- a/git-multimail/git_multimail.py
+++ b/git-multimail/git_multimail.py
@@ -57,6 +57,7 @@ import subprocess
 import shlex
 import optparse
 import smtplib
+import ssl
 import time
 import cgi
 
@@ -1945,6 +1946,7 @@ class SMTPMailer(Mailer):
                  smtpservertimeout=10.0, smtpserverdebuglevel=0,
                  smtpencryption='none',
                  smtpuser='', smtppass='',
+                 smtpcacerts='/etc/ssl/certs/ca-certificates.crt',smtpcheckcert=False
                  ):
         if not envelopesender:
             sys.stderr.write(
@@ -1974,13 +1976,43 @@ class SMTPMailer(Mailer):
             if self.security == 'none':
                 self.smtp = call(smtplib.SMTP, self.smtpserver, timeout=self.smtpservertimeout)
             elif self.security == 'ssl':
+                if smtpcheckcert:
+                    msg = "Checking certificate is not supported for ssl, prefer starttls"
+                    raise smtplib.SMTPException(msg)
                 self.smtp = call(smtplib.SMTP_SSL, self.smtpserver, timeout=self.smtpservertimeout)
             elif self.security == 'tls':
                 if ':' not in self.smtpserver:
                     self.smtpserver += ':587'  # default port for TLS
                 self.smtp = call(smtplib.SMTP, self.smtpserver, timeout=self.smtpservertimeout)
-                self.smtp.ehlo()
-                self.smtp.starttls()
+                if smtpcheckcert:
+                    # inspired form:
+                    #   https://github.com/graingert/secure-smtplib/blob/master/src/secure_smtplib/__init__.py
+                    # but add the path to trusted ca, and force ceritficate verification.
+                    self.smtp.ehlo_or_helo_if_needed()
+                    if not self.smtp.has_extn("starttls"):
+                        msg = "STARTTLS extension not supported by server"
+                        raise smtplib.SMTPException(msg)
+                    (resp, reply) = self.smtp.docmd("STARTTLS")
+                    if resp == 220:
+                        self.smtp.sock = ssl.wrap_socket(
+                            self.smtp.sock,
+                            ca_certs=smtpcacerts,
+                            cert_reqs=ssl.CERT_REQUIRED
+                        )
+                        if not hasattr(self.smtp.sock, "read"):
+                            # using httplib.FakeSocket with Python 2.5.x or earlier
+                            self.smtp.sock.read = self.smtp.sock.recv
+                        self.smtp.file = smtplib.SSLFakeFile(self.smtp.sock)
+                        self.smtp.helo_resp = None
+                        self.smtp.ehlo_resp = None
+                        self.smtp.esmtp_features = {}
+                        self.smtp.does_esmtp = 0
+                    else:
+                        msg = "Wrong answer to the STARTTLS command"
+                        raise smtplib.SMTPException(msg)
+                else:
+                    self.smtp.ehlo()
+                    self.smtp.starttls()
                 self.smtp.ehlo()
             else:
                 sys.stdout.write('*** Error: Control reached an invalid option. ***')
@@ -3500,6 +3532,8 @@ def choose_mailer(config, environment):
         smtpencryption = config.get('smtpencryption', default='none')
         smtpuser = config.get('smtpuser', default='')
         smtppass = config.get('smtppass', default='')
+        smtpcacerts = config.get('smtpcacerts', default='/etc/ssl/certs/ca-certificates.crt')
+        smtpcheckcert = config.get_bool('smtpcheckcert', default='false')
         mailer = SMTPMailer(
             envelopesender=(environment.get_sender() or environment.get_fromaddr()),
             smtpserver=smtpserver, smtpservertimeout=smtpservertimeout,
@@ -3507,6 +3541,8 @@ def choose_mailer(config, environment):
             smtpencryption=smtpencryption,
             smtpuser=smtpuser,
             smtppass=smtppass,
+            smtpcacerts=smtpcacerts,
+            smtpcheckcert=smtpcheckcert
             )
     elif mailer == 'sendmail':
         command = config.get('sendmailcommand')

Re: [git-multimail] smtplib, check certificate

[Matthieu Moy]

Simon P <simon.git@le-huit.fr> writes:

> Hi,

Hi, and thanks for the patch.

Please, add your sign-off and a proper commit message to your patch,
see:

https://github.com/git-multimail/git-multimail/blob/master/CONTRIBUTING.rst

I'm OK with patches by email, but you may prefer using a pull-request
(among other things, creating a pull-request triggers a Travis-CI build
and would have noticed the absence of sign-off and a minor PEP8 issue in
your code.

The patch obviously lacks documentation, and some way to test it.
Actually, the testsuite will fail if you document the configuration
variable and they don't appear somewhere in the testsuite. A fully
automatic test would be hard to write, but I have a semi-automated
testsuite for smtp: some configurations in t/*.config.in, and a script
test-email-config to run a test with each of the configurations (then I
check my mailbox). There should be one configuration with a valid
certificate and another with a buggy one so that we can check that the
certificate is actually checked.

> @@ -1945,6 +1946,7 @@ class SMTPMailer(Mailer):
>                   smtpservertimeout=10.0, smtpserverdebuglevel=0,
>                   smtpencryption='none',
>                   smtpuser='', smtppass='',
> +                 smtpcacerts='/etc/ssl/certs/ca-certificates.crt',smtpcheckcert=False

Do you need a default for smtpcheckcert if you already have one in
config.get(smtpcheckcert)? In any case, I'd rather avoid having two
hardcoded path in the code. If you need
'/etc/ssl/certs/ca-certificates.crt' in two places, please define a
constant elsewhere in the code and use it here.

Missing space after ,.

> +                if smtpcheckcert:
> +                    # inspired form:
> +                    #   https://github.com/graingert/secure-smtplib/blob/master/src/secure_smtplib/__init__.py
> +                    # but add the path to trusted ca, and force ceritficate verification.
> +                    self.smtp.ehlo_or_helo_if_needed()
> +                    if not self.smtp.has_extn("starttls"):
> +                        msg = "STARTTLS extension not supported by server"
> +                        raise smtplib.SMTPException(msg)
> +                    (resp, reply) = self.smtp.docmd("STARTTLS")

Parenthesis around (resp, reply) are not needed, I prefer to omit them.

Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [git-multimail] smtplib, check certificate

[Michael Haggerty]

On 04/22/2016 08:05 AM, Matthieu Moy wrote:
> Simon P <simon.git@le-huit.fr> writes:
> > This patch is inspired from
> > 
> > https://github.com/graingert/secure-smtplib/blob/master/src/secure_smtplib/__init__.py
> 
> Please, add your sign-off and a proper commit message to your patch,
> see:
> 
> https://github.com/git-multimail/git-multimail/blob/master/CONTRIBUTING.rst

I hate that we even have to worry about this stuff, but
graingert/secure-smtplib looks to be GPLv3, whereas git-multimail is
GPLv2 (like the Git project and Linux); *not* "GPLv2 or later". So if
"inspired" means "incorporated copyrightable content" then this patch
might be problematic.

Michael

Re: [git-multimail] smtplib, check certificate

[Simon Pontié]

Le 22/04/2016 08:05, Matthieu Moy a écrit :
> Hi, and thanks for the patch.

Hi.

Thanks for your tool, it is very useful!


> Please, add your sign-off and a proper commit message to your patch,
> see:

Done, I also signed my commit via PGP.

> I'm OK with patches by email, but you may prefer using a pull-request
> (among other things, creating a pull-request triggers a Travis-CI build
> and would have noticed the absence of sign-off and a minor PEP8 issue in
> your code.

I don't like github, but I understand your requirement. I submitted a
pull-request of a modified version of the patch:
  https://github.com/git-multimail/git-multimail/pull/150
I am not a python developer and I am not a Travis-CI user, so I cannot
understand failure messages at:
  https://travis-ci.org/git-multimail/git-multimail/builds/125406555

> The patch obviously lacks documentation

I have added a description in the README file.

> and some way to test it.
> Actually, the testsuite will fail if you document the configuration
> variable and they don't appear somewhere in the testsuite. A fully
> automatic test would be hard to write, but I have a semi-automated
> testsuite for smtp: some configurations in t/*.config.in
> test-email-config to run a test with each of the configurations (then I
> check my mailbox). There should be one configuration with a valid
> certificate and another with a buggy one so that we can check that the
> certificate is actually checked.

I have added some test: firstly, I renamed the file `smtp-tls.config.in`
to `smtp-tls-nocheckcert.config.in` because this configuration do not
check the server certificate. I also added to test files:
  - `smtp-tls-checkcert-unverifiedcert.config.in`
  - `smtp-tls-checkcert-verifiedcert.config.in`

The first one (unverifiedcert) uses a fake trusted CA list to check the
unverified server certificate detection (that can be tested with the
gmail server for example).

The second one (verifiedcert), assumes that your system have a file
`/etc/ssl/certs/ca-certificates.crt` with a list of all CA trusted by
your system (this file exist in Debian systems). It should succeed with
the gmail server.

> Do you need a default for smtpcheckcert if you already have one in
> config.get(smtpcheckcert)? In any case, I'd rather avoid having two
> hardcoded path in the code. If you need
> '/etc/ssl/certs/ca-certificates.crt' in two places, please define a
> constant elsewhere in the code and use it here.

I have modified the configuration, there is now only one configuration
var: smtpCACerts. If it is empty (default), the server certificate is
not verified (like before the patch) but a warning is emitted. If the
var is set, the targeted file is used to verify the server certificate.

For now, only the tls configuration is supported.

Simon P.

Re: [git-multimail] smtplib, check certificate

[Simon Pontié]

Le 22/04/2016 08:41, Michael Haggerty a écrit :
> I hate that we even have to worry about this stuff, but
> graingert/secure-smtplib looks to be GPLv3, whereas git-multimail is
> GPLv2 (like the Git project and Linux); *not* "GPLv2 or later". So if
> "inspired" means "incorporated copyrightable content" then this patch
> might be problematic.

https://github.com/git-multimail/git-multimail/pull/150#issuecomment-214020193

There is a GPLv3 on the secure-smtplib implementation:
https://github.com/graingert/secure-smtplib
but I also found an older code equivalent to my patch under MIT license
here: https://github.com/cybercase/django-smtp-starttls (code in
django_smtp_starttls.py, licence in setup.py)

I think there is not problem to re-use MIT code.

Simon.