| Submitter | Katsunori FUJIWARA |
|---|---|
| Date | March 25, 2013, 5:32 p.m. |
| Message ID | <9e6f7b025beec226db65.1364232754@juju> |
| Download | mbox | patch |
| Permalink | /patch/1189/ |
| State | Accepted |
| Commit | 19d489404d79058d518473798423185bd1cd7558 |
| Delegated to: | Bryan O'Sullivan |
| Headers | show |
Comments
Series LGTM, but since it's security code I'd like someone else to take a quick look. Putting a couple of crew folks on To: in hopes that one of them will be willing to take a look. Sorry about the delay in looking at this. On Mar 25, 2013, at 1:32 PM, FUJIWARA Katsunori <foozy@lares.dti.ne.jp> wrote: > # HG changeset patch > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp> > # Date 1364232490 -32400 > # Node ID 9e6f7b025beec226db65af27bf9ed7dcbfdfa190 > # Parent 6148e84c076b4b660408d76a519daedf06a5255a > smtp: verify the certificate of the SMTP server for STARTTLS/SMTPS > > Before this patch, the certificate of the SMTP server for STARTTLS or > SMTPS isn't verified. > > This may cause man-in-the-middle security problem (stealing > authentication information), even though SMTP channel itself is > encrypted by SSL. > > When "[smtp] tls" is configured as "smtps" or "starttls", this patch: > > - uses classes introduced by preceding patches instead of "SMTP" or > "SMTP_SSL" of smtplib, and > > - verifies the certificate of the SMTP server, if "[smtp] > verifycert" is configured as other than False > > "[smtp] verifycert" can be configured in 3 levels: > > - "strict": > > This verifies peer certificate, and aborts if: > > - peer certification is not valid, or > - no configuration in "[hostfingerprints]" and "[web] cacerts" > > This is default value of "[smtp] verifycert" for security. > > - "loose": > > This verifies peer certificate, and aborts if peer certification is > not valid. > > This just shows warning message ("certificate not verified"), if > there is no configuration in "[hostfingerprints]" and "[web] > cacerts". > > This is as same as verification for HTTPS connection. > > - False(no verification): > > Peer certificate is not verified. > > This is as same as the behavior before this patch series. > > "hg email --insecure" uses "loose" level, and ignores "[web] cacerts" > as same as push/pull/etc... with --insecure. > > Ignoring "[web] cacerts" configuration for "hg email --insecure" is > already done in "dispatch._dispatch()" by looking "insecure" up in the > table of command options. > > diff --git a/hgext/patchbomb.py b/hgext/patchbomb.py > --- a/hgext/patchbomb.py > +++ b/hgext/patchbomb.py > @@ -540,7 +540,13 @@ > fp.close() > else: > if not sendmail: > - sendmail = mail.connect(ui, mbox=mbox) > + verifycert = ui.config('smtp', 'verifycert') > + if opts.get('insecure'): > + ui.setconfig('smtp', 'verifycert', 'loose') > + try: > + sendmail = mail.connect(ui, mbox=mbox) > + finally: > + ui.setconfig('smtp', 'verifycert', verifycert) > ui.status(_('sending '), subj, ' ...\n') > ui.progress(_('sending'), i, item=subj, total=len(msgs)) > if not mbox: > diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt > --- a/mercurial/help/config.txt > +++ b/mercurial/help/config.txt > @@ -1052,6 +1052,16 @@ > Optional. Method to enable TLS when connecting to mail server: starttls, > smtps or none. Default: none. > > +``verifycert`` > + Optional. Verification for the certificate of mail server, when > + ``tls`` is starttls or smtps. "strict", "loose" or False. For > + "strict" or "loose", the certificate is verified as same as the > + verification for HTTPS connections (see ``[hostfingerprints]`` and > + ``[web] cacerts`` also). For "strict", sending email is also > + aborted, if there is no configuration for mail server in > + ``[hostfingerprints]`` and ``[web] cacerts``. --insecure for > + :hg:`email` overwrites this as "loose". Default: "strict". > + > ``username`` > Optional. User name for authenticating with the SMTP server. > Default: none. > diff --git a/mercurial/mail.py b/mercurial/mail.py > --- a/mercurial/mail.py > +++ b/mercurial/mail.py > @@ -91,14 +91,25 @@ > smtps = tls == 'smtps' > if (starttls or smtps) and not util.safehasattr(socket, 'ssl'): > raise util.Abort(_("can't use TLS: Python SSL support not installed")) > - if smtps: > - ui.note(_('(using smtps)\n')) > - s = smtplib.SMTP_SSL(local_hostname=local_hostname) > - else: > - s = smtplib.SMTP(local_hostname=local_hostname) > mailhost = ui.config('smtp', 'host') > if not mailhost: > raise util.Abort(_('smtp.host not configured - cannot send mail')) > + verifycert = ui.config('smtp', 'verifycert', 'strict') > + if verifycert not in ['strict', 'loose']: > + if util.parsebool(verifycert) is not False: > + raise util.Abort(_('invalid smtp.verifycert configuration: %s') > + % (verifycert)) > + if (starttls or smtps) and verifycert: > + sslkwargs = sslutil.sslkwargs(ui, mailhost) > + else: > + sslkwargs = {} > + if smtps: > + ui.note(_('(using smtps)\n')) > + s = SMTPS(sslkwargs, local_hostname=local_hostname) > + elif starttls: > + s = STARTTLS(sslkwargs, local_hostname=local_hostname) > + else: > + s = smtplib.SMTP(local_hostname=local_hostname) > mailport = util.getport(ui.config('smtp', 'port', 25)) > ui.note(_('sending mail: smtp host %s, port %s\n') % > (mailhost, mailport)) > @@ -108,6 +119,9 @@ > s.ehlo() > s.starttls() > s.ehlo() > + if (starttls or smtps) and verifycert: > + ui.note(_('(verifying remote certificate)\n')) > + sslutil.validator(ui, mailhost)(s.sock, verifycert == 'strict') > username = ui.config('smtp', 'username') > password = ui.config('smtp', 'password') > if username and not password: > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > http://selenic.com/mailman/listinfo/mercurial-devel
Patch
diff --git a/hgext/patchbomb.py b/hgext/patchbomb.py --- a/hgext/patchbomb.py +++ b/hgext/patchbomb.py @@ -540,7 +540,13 @@ fp.close() else: if not sendmail: - sendmail = mail.connect(ui, mbox=mbox) + verifycert = ui.config('smtp', 'verifycert') + if opts.get('insecure'): + ui.setconfig('smtp', 'verifycert', 'loose') + try: + sendmail = mail.connect(ui, mbox=mbox) + finally: + ui.setconfig('smtp', 'verifycert', verifycert) ui.status(_('sending '), subj, ' ...\n') ui.progress(_('sending'), i, item=subj, total=len(msgs)) if not mbox: diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt --- a/mercurial/help/config.txt +++ b/mercurial/help/config.txt @@ -1052,6 +1052,16 @@ Optional. Method to enable TLS when connecting to mail server: starttls, smtps or none. Default: none. +``verifycert`` + Optional. Verification for the certificate of mail server, when + ``tls`` is starttls or smtps. "strict", "loose" or False. For + "strict" or "loose", the certificate is verified as same as the + verification for HTTPS connections (see ``[hostfingerprints]`` and + ``[web] cacerts`` also). For "strict", sending email is also + aborted, if there is no configuration for mail server in + ``[hostfingerprints]`` and ``[web] cacerts``. --insecure for + :hg:`email` overwrites this as "loose". Default: "strict". + ``username`` Optional. User name for authenticating with the SMTP server. Default: none. diff --git a/mercurial/mail.py b/mercurial/mail.py --- a/mercurial/mail.py +++ b/mercurial/mail.py @@ -91,14 +91,25 @@ smtps = tls == 'smtps' if (starttls or smtps) and not util.safehasattr(socket, 'ssl'): raise util.Abort(_("can't use TLS: Python SSL support not installed")) - if smtps: - ui.note(_('(using smtps)\n')) - s = smtplib.SMTP_SSL(local_hostname=local_hostname) - else: - s = smtplib.SMTP(local_hostname=local_hostname) mailhost = ui.config('smtp', 'host') if not mailhost: raise util.Abort(_('smtp.host not configured - cannot send mail')) + verifycert = ui.config('smtp', 'verifycert', 'strict') + if verifycert not in ['strict', 'loose']: + if util.parsebool(verifycert) is not False: + raise util.Abort(_('invalid smtp.verifycert configuration: %s') + % (verifycert)) + if (starttls or smtps) and verifycert: + sslkwargs = sslutil.sslkwargs(ui, mailhost) + else: + sslkwargs = {} + if smtps: + ui.note(_('(using smtps)\n')) + s = SMTPS(sslkwargs, local_hostname=local_hostname) + elif starttls: + s = STARTTLS(sslkwargs, local_hostname=local_hostname) + else: + s = smtplib.SMTP(local_hostname=local_hostname) mailport = util.getport(ui.config('smtp', 'port', 25)) ui.note(_('sending mail: smtp host %s, port %s\n') % (mailhost, mailport)) @@ -108,6 +119,9 @@ s.ehlo() s.starttls() s.ehlo() + if (starttls or smtps) and verifycert: + ui.note(_('(verifying remote certificate)\n')) + sslutil.validator(ui, mailhost)(s.sock, verifycert == 'strict') username = ui.config('smtp', 'username') password = ui.config('smtp', 'password') if username and not password:
# HG changeset patch # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp> # Date 1364232490 -32400 # Node ID 9e6f7b025beec226db65af27bf9ed7dcbfdfa190 # Parent 6148e84c076b4b660408d76a519daedf06a5255a smtp: verify the certificate of the SMTP server for STARTTLS/SMTPS Before this patch, the certificate of the SMTP server for STARTTLS or SMTPS isn't verified. This may cause man-in-the-middle security problem (stealing authentication information), even though SMTP channel itself is encrypted by SSL. When "[smtp] tls" is configured as "smtps" or "starttls", this patch: - uses classes introduced by preceding patches instead of "SMTP" or "SMTP_SSL" of smtplib, and - verifies the certificate of the SMTP server, if "[smtp] verifycert" is configured as other than False "[smtp] verifycert" can be configured in 3 levels: - "strict": This verifies peer certificate, and aborts if: - peer certification is not valid, or - no configuration in "[hostfingerprints]" and "[web] cacerts" This is default value of "[smtp] verifycert" for security. - "loose": This verifies peer certificate, and aborts if peer certification is not valid. This just shows warning message ("certificate not verified"), if there is no configuration in "[hostfingerprints]" and "[web] cacerts". This is as same as verification for HTTPS connection. - False(no verification): Peer certificate is not verified. This is as same as the behavior before this patch series. "hg email --insecure" uses "loose" level, and ignores "[web] cacerts" as same as push/pull/etc... with --insecure. Ignoring "[web] cacerts" configuration for "hg email --insecure" is already done in "dispatch._dispatch()" by looking "insecure" up in the table of command options.