Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#9561] Check remote certificates for XMPP TLS #1147

Merged
merged 19 commits into from Jul 4, 2019

Conversation

@ralphm
Copy link
Contributor

commented May 27, 2019

https://twistedmatrix.com/trac/ticket/9561
Replaces #1084.

While setting up XMPP connections, during StartTLS, an unconfigured CertificationOptions instance is passed. These changes properly use optionsForClientTLS instead, using the remote server's domain (XmlStream.otherJID, taken from the JID the client is connecting with) to verify the remote certificate using the platform trust root and that domain.

Additionally, you can now pass custom certificate options to modify this new default, e.g. for using client certificates with SASL External.

ralphm added 8 commits May 7, 2019
Clean up connecting authenticators
This adds an option `required` argument to the inits of initializers
deriving from BaseFeatureInitiatingInitializer, to simplify setup.
Additionally it changes the requiredness of two initializers used by
XMPPAuthenticator:

* Setup of TLS is now required by default. This ensures that if StartTLS
is not advertized by the server, initialization fails instead of
silently proceeding to authentication without encryption.
* Binding a resource is required by default, because without it servers
will not allow any further meaningful interaction.
Provide a way to use custom certificate options for XMPP clients
This adds an optional `contextFactory` argument to `XMPPClientFactory`
that is passed on to `XMPPAuthenticator`, which in turn passes it to
`TLSInitiatingInitializer`.

@ralphm ralphm changed the title (#9561) Check remote certificates for XMPP TLS [#9561] Check remote certificates for XMPP TLS May 27, 2019

ralphm and others added 4 commits May 27, 2019
@glyph
glyph approved these changes May 27, 2019
Copy link
Member

left a comment

Minor feedback inline; the biggest deal here is the public-interface changes (the naming and public-ness of contextFactory). Address as you see fit, and file follow-ups for anything you think should be dealt with later, then land!

src/twisted/words/protocols/jabber/client.py Outdated Show resolved Hide resolved
src/twisted/words/protocols/jabber/client.py Outdated Show resolved Hide resolved
src/twisted/words/protocols/jabber/client.py Outdated Show resolved Hide resolved
src/twisted/words/protocols/jabber/xmlstream.py Outdated Show resolved Hide resolved
src/twisted/words/protocols/jabber/xmlstream.py Outdated Show resolved Hide resolved
@@ -16,6 +16,14 @@
from twisted.words.protocols.jabber.sasl import SASLInitiatingInitializer
from twisted.words.xish import utility

try:
from twisted.internet import ssl
except ImportError:

This comment has been minimized.

Copy link
@glyph

glyph May 27, 2019

Member

There are lots of other places which do this conditional import. Can we piggyback on one of them rather than adding bespoke ImportError-handling logic? My concern here is that this type of exception handling is likely to mask cases where there's an actual import-time bug in twisted.internet.ssl and I don't want to have to go looking for 20 instances of this if we ever manage to go in and fix it to only fail on defined import errors (i.e. those where cryptography isn't available).

Worst case, I'd rather literally import it from another test_ module for the time being.

This comment has been minimized.

Copy link
@ralphm

ralphm May 28, 2019

Author Contributor

I suppose this is due to the unconditional import of the OpenSSL module in twisted.internet.ssl and twisted.internet._sslverify. The former module does have a supported attribute, but it seems to (at least now) always be set to True, even though for example twisted.words.protocols.jabber.xmlstream checks for that.

If the OpenSSL module cannot be imported, the whole import fails, wheres I think my tests probably don't even use it.

Can we not make twisted.internet.ssl do this conditional import and have it set supported to False, and SSL to None?

This comment has been minimized.

Copy link
@ralphm

ralphm Jun 16, 2019

Author Contributor

@glyph I'm not sure if this should be addressed as part of this ticket?

src/twisted/words/test/test_jabberclient.py Outdated Show resolved Hide resolved
@alex

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

FYI I have requested a CVE for this.

@alex

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

Please use CVE-2019-12855

@alex

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

@ralphm can you include the CVE in the newsfragment?

@ralphm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@alex Done! Thanks for that.

@alex

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

ralphm added 2 commits Jun 16, 2019

@ralphm ralphm merged commit cf8ed69 into trunk Jul 4, 2019

7 checks passed

Tests #966 succeeded
Details
ci/circleci: documentation Your tests passed on CircleCI!
Details
ci/circleci: linux-wheels Your tests passed on CircleCI!
Details
ci/circleci: osx-wheels Your tests passed on CircleCI!
Details
ci/circleci: static_checkers Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.005%) to 91.347%
Details
@ralphm ralphm referenced this pull request Jul 5, 2019
4 of 4 tasks complete
@greysteil

This comment has been minimized.

Copy link

commented Jul 18, 2019

I work on GitHub's security workflows team and am looking into how we can make it easier for maintainers to publicise security vulnerabilities like this one. I'd love your feedback if you have 5 mins?

Specifically:

  • GitHub sent alerts to Twisted users about this vulnerability, and in some cases automated security fix PRs. Could we have done anything better there?
  • did you consider using the "Maintainer Security Advisories" feature to publicise this vulnerability (it's in the "Security" tab on this repo)? What would make that feature useful to you in future?
  • do you have any other thoughts on the process of handling a security vulnerability, and how GitHub can help?

We're trying to make the process easier so if you have any feedback at all please let me know. You can email me on greysteil@github.com if you'd like to discuss anything privately.

@alex

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

Hi Grey,

Here's some off the cuff thoughts:

  • I'd be surprised if GitHub could send a fix PR for this, I don't believe this PR has been included in a release yet.
  • We need to obtain CVEs for vulnerabilities, so unless GitHub can obtain them for us, using the Maintainer Security Advisories feature is duplicative.
  • Twisted is a large project and only a fraction of its users use XMPP, if there was some way to only notify users who were impacted by this specific vulnerability, as opposed to using an effected version, that'd be fantastic.

In general I'm super thrilled about GitHub automatically issuing notifications on insecure dependencies.

@exarkun exarkun deleted the 9561-xmpp-tls-verify-cert branch Jul 18, 2019

@greysteil

This comment has been minimized.

Copy link

commented Jul 18, 2019

Really useful feedback - thanks!

I'd be surprised if GitHub could send a fix PR for this, I don't believe this PR has been included in a release yet.

Oops! Thought it had been. We'll create fix PRs when it is :-)

We need to obtain CVEs for vulnerabilities, so unless GitHub can obtain them for us, using the Maintainer Security Advisories feature is duplicative.

Working on it!

Twisted is a large project and only a fraction of its users use XMPP, if there was some way to only notify users who were impacted by this specific vulnerability, as opposed to using an effected version, that'd be fantastic.

Interesting. I'm super keen to reduce our false positive rate - it's a way off but we'd like to be able to do that.

@glyph

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

Oops! Thought it had been. We'll create fix PRs when it is :-)

One thing that would make this kind of thing easier to track is explicit "present in releases" state on issues and PRs. If even people who work at github can't tell whether a PR has made it into a release or not, that says something about the UI for this 😄 .

(In fairness, Twisted's processes predate like, neolithic information technology so we don't lean particularly heavily on the Github notion of a release, or use Github issues for tracking in this project; still, if the ability to track what releases an issue was fixed in or a PR was present in, that would be a big motivation to migrate.)

@glyph

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

  • Could we have done anything better there?

For projects hosted on github, or even just with an official mirror here, it would be helpful if whatever notification-automation were integrated directly into the Maintainer Security Advisories tab. For example, if Github were to misidentify some element of the state associated with the PR (as you potentially did with the mistake around this being included in a release), we could go in and edit the advisory directly.

Based on the workflow I've seen so far, what I think I am asking for more specifically here is "please create a draft maintainer security advisory that we can see and comment on with at least some window before you put us on blast to our entire user base".

@greysteil

This comment has been minimized.

Copy link

commented Jul 18, 2019

Really helpful feedback, thanks @glyph. That's on our list of potential improvements for the next 3 months.

@alex

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

"The earliest tag that includes this PR is X" would be amazing. When I was at Mozilla the Firefox bug tracker had this and it was super handy.

You can always figure it out by looking at the merge commit page of course, but highlighting it would be dope.

@greysteil

This comment has been minimized.

Copy link

commented Jul 18, 2019

Agreed - have passed that one on to the PR team :octocat:

@glyph

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

(If this functionality went both ways — "what are all the PRs this tag (release) is the first to include" — that would go a long way to automating the production of changelogs…)

@ralphm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

For completeness. Twisted 19.7.0 (2019-07-28) contains this fix.

@glyph

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Thanks for the note, ralph!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.