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

CRM-14814 CRM-8744 breaks on SMTP svrs requiring secure authentication #66

Merged
merged 1 commit into from Dec 2, 2015

Conversation

@kenwest
Copy link
Contributor

commented Jun 7, 2014

See forum discussion http://forum.civicrm.org/index.php/topic,21960

The change made for CRM-8744 prevents CiviCRM connecting to a SMTP server unless that server provides authentication in the clear. This is insecure.

Notes:

  1. The latest version of http://pear.php.net/package/Net_SMTP doesn't include this patch (released 2013-07-05) which tells me it's not an important patch
  2. The source of the patch http://www.pear-forum.org/post-4935.html has gone off-air so we can't see why this patch was made or assess how well it was tested etc
  3. I can demonstrate (in the Forum discussion) that this patch breaks the code
  4. This patch creates a security issue, because it forces people to authenticate SMTP in the clear

@jszwedko

This comment has been minimized.

Copy link

commented Jul 2, 2014

Note, I was able to successfully authenticate over TLS by prefixing the 'SMTP Server' field with 'ssl://' (presumably this is handled Pear:Net_SMTP) though this should really be better documented.

@kenwest

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2014

@jszwedko

SSL is a different protocol to TLS. SSL uses port 465 and communications is always encrypted via SSL. TLS uses port 587 and communications starts off in the clear. A properly configured TLS port should only offer one SMTP verb in the clear: STARTTLS.

What the patch for CRM-8744 does is prevent STARTTLS working. One either needs to use SSL or allow authentication in the clear. As SSL is a weaker protocol than TLS neither of these options is palatable.

@jszwedko

This comment has been minimized.

Copy link

commented Jul 8, 2014

Gotcha! You are correct, I was actually using SSL rather than TLS.

@totten totten added the 4.6 label Aug 15, 2014
@totten totten added 4.7 and removed 4.6 labels Dec 2, 2015
totten added a commit that referenced this pull request Dec 2, 2015
CRM-14814 CRM-8744 breaks on SMTP svrs requiring secure authentication
@totten totten merged commit 50904cb into civicrm:master Dec 2, 2015
@totten

This comment has been minimized.

Copy link
Member

commented Dec 2, 2015

+1. Merged. Feedback on forums was positive, and this PR aligns us better with upstream's code.

It's not clear to me if this will undo whatever benefit/fix was originally provided by CRM-8744 / http://www.pear-forum.org/post-4935.html -- or if post-4935.html was just Really Bad(tm) to begin with. Worst case... it trades one bug for another, but it's no longer our bug... it would be a bug in the upstream library. If that happens, we should either refer the problem upstream or switch to swiftmailer.org.

Aside: It looks like upstream has made some other TLS-related fixes and improved the syntax (boo PHP-4, yay PHP-7), so I might send another PR to upgrade.

totten pushed a commit to totten/civicrm-packages that referenced this pull request Oct 12, 2017
Override title only when shortcode hijacks post
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.