Bug 7267 - no way to set SSL_VERIFY_PEER in spamd
Summary: no way to set SSL_VERIFY_PEER in spamd
Status: NEW
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: spamc/spamd (show other bugs)
Version: 3.4.1
Hardware: All All
: P2 enhancement
Target Milestone: 4.0.0
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
: 7092 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-11-16 23:33 UTC by Curtis Villamizar
Modified: 2021-05-30 05:19 UTC (History)
6 users (show)



Attachment Type Modified Status Actions Submitter/CLA Status
preliminary patch for spamd - not tested patch None Curtis Villamizar [HasCLA]
patch to spamd/spamd.raw patch None Curtis Villamizar [HasCLA]
patch to spamc/spamc.pod patch None Curtis Villamizar [HasCLA]
patch for spamc/spamc,c patch None Curtis Villamizar [HasCLA]
patch to spamc/libspamc.h patch None Curtis Villamizar [HasCLA]
patch to spamc/libspamc.c patch None Curtis Villamizar [HasCLA]
consolidated patches to five files into one patch file patch None Curtis Villamizar [HasCLA]
Let spamc compile without ssl support, check return values patch None Giovanni Bechis [HasCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Curtis Villamizar 2015-11-16 23:33:43 UTC
There is no server side SSL verification in spamd.

The fix on the spamd side is simple.  Not so simple in spamc.

On spamd there needs to be three options, "--ssl-ca-file=file", "-ssl-ca-path=path", and "--ssl-verify".

Setting any one of them sets "$sockopt->SSL_verify_mode =
SSL_VERIFY_PEER;" on about line 1077 in spamd.  If either ssl-ca-file or ssl-ca-path are set, then set sockopt->SSL_ca_file or sockopt->SSL_ca_path.

That's it for spamd.  I haven't looked at spamc, but it should not be much more difficult to add options for key and cert.  Just C rather than perl.

Diffs to follow.
Comment 1 Curtis Villamizar 2015-11-17 00:23:24 UTC
Created attachment 5347 [details]
preliminary patch for spamd - not tested

Looking into spamc shortly.  Will test after that.
Comment 2 Curtis Villamizar 2015-11-20 20:21:37 UTC
Now that this is all debugged (took longer than I thought due to an oversight on my part in looking over the spamc code) I will be attaching five patch files and explaining the changes in each one.  The five files are:

patch-spamc__libspamc.c
patch-spamc__libspamc.h
patch-spamc__spamc.c
patch-spamc__spamc.pod
patch-spamd__spamd.raw

This is an overview of the patches.  Details will come with the files.

Spamd has three options added: B<--ssl-verify>, B<--ssl-ca-file>=I<cafile>, and B<--ssl-ca-path>=I<capath>.  The first just turns on client certificate verification and without the other two would use the built in CA certs.  The other two allow a CA file or CA path to be added as in IO::Socket::SSL.

The handling of ssl-version has changed.  The default is now tlsv12 (you can change back if you want).  The code now makes it so sslv3 allows sslv3 or better, tlsv1 allows tlsv1 or better, etc.  The options to specific tlsv11 and tlsv12 are added (with tlsv11 allowing "or better").

If SSL is requested, fallback to plain text socket is not allowed.

If any one of the three new options are specified, then the client will be required to provide a certificate and it has to be signed by a CA (which only really makes sense if a CA cert is specified and its your own CA cert (which could be self signed, or a domain signing certificate).

Since spamc didn't support sending a client cert, it had to be changed.

The pod file just has changed docs.  The .h file has a few lines to support the new options.  The spamc.c file has a small amount of change to support the help and the setting the options.  The new options for spamc are B<--ssl-cert>=I<cert-file> and B<--ssl-key>=I<key-file> to specify a client cert and client key.  Here also the default is changed to tlsv1.  The code is changed so sslv3 allows sslv3 or better and tlsv1 allows tlsv1 or better.

The real work in spamc is all in libspamc.c.  Two new functions were created.  Each takes a few identical lines formerly pasted into two places and expands on that code (rather than expanding in four places all together).  Now fallback to plain text is not allowed.  If client cert and key are provided, then spamc can provide a client cert if asked for one by spamd.

It seems like a real bad idea to have a server cert verification fail and fall back to doing the same thing in the clear.  I'm assuming that was a bug and not intentional.  It also seems best to change the default from the known flawed sslv3 to at least tlsv1.0 and even better to tlsv1.2.  A few words of code can change the defaults back.  Since spamc and spamd are distributed together for most users thats not an issue.  If a transition is needed either end can get upgraded first specifying sslv3 to support the transition and changing configs to tlsv1 after transition is done.
Comment 3 Curtis Villamizar 2015-11-20 20:33:19 UTC
Created attachment 5349 [details]
patch to spamd/spamd.raw

The $ssl_version_mapping allows simple sslv3, tlsv1, tlsv[12] options to be specified by the user but having the effect of that version or better.  The full syntax in IO::Socket::SSL is more flexible but this is easier on the user.

The $have_ssl_module block is earlier in the code so things like 0x01 be replaced with Net::SSLeay::VERIFY_PEER().  Also Net::SSLeay::VERIFY_FAIL_IF_NO_PEER_CERT() is or'd in so fallback to plain text is not allowed.

Setting SSL_verifycn_scheme to 'none' is OK if the CA cert is your own and you only sign for things you trust.  Otherwise an rDNS lookup and CN check against it would be needed and this would have to be done in a SSL_verifycn_scheme callback.  I turned off CRL check though supporting a CRL file would be a possible further enhancement.

Nothing else at all non-obvious in the changes.
Comment 4 Curtis Villamizar 2015-11-20 20:37:31 UTC
Created attachment 5350 [details]
patch to spamc/spamc.pod

Changes to documentation adding new options, updating the interpretation of the B<--ssl>=I<sslversion> handling, and saying bad things about using SSLv3 which need to be said.
Comment 5 Curtis Villamizar 2015-11-20 20:42:47 UTC
Created attachment 5351 [details]
patch for spamc/spamc,c

Nothing at all non-obvious.  Just added code to handle new options.
Comment 6 Curtis Villamizar 2015-11-20 20:44:02 UTC
Created attachment 5352 [details]
patch to spamc/libspamc.h
Comment 7 Curtis Villamizar 2015-11-20 20:57:38 UTC
Created attachment 5353 [details]
patch to spamc/libspamc.c

All changes are inside #ifdef SPAMC_SSL / #endif pairs.

Two functions are added.  Each function is called in two places where a few lines are replaced.

It helps to look at the replaced code first.  The SSLv3_client_method() and TLSv1_client_method() functions require exact match to SSLv3 or TLSv1, rather than "or better".  Since "or better" is assumed to have been the intent, SSLv23_client_method() is used in the new code, with SSL_CTX_set_options used to restrict the version negotiation (SSL_OP_NO_SSLv2, SSL_OP_NO_SSLv3).

The first new function replaced code did some init and "ctx = SSL_CTX_new(meth);".  The new code just replaces the code restricting which SSL/TLS versions are allowed.

The second function supports the new options.  First the CA certs are loaded if provided on the command line.  Then the client cert and key are loaded if provided on the command line and then checked.  The rest is like the original code but with a bit of error checking.

The "ssl = SSL_new(ctx);" is run and "ssl" is copied back to the caller.  Then "SSL_set_fd(ssl, sock)" is done, only this code checks the error return and logs.  Then "SSL_connect(ssl)" is run, again with error checking (which in this case is disabled).  The return value of SSL_connect(ssl) was ignored in the original code so it is ignored here.
Maybe that should change.
Comment 8 Kevin A. McGrail 2015-11-23 17:33:11 UTC
Curtis,

These are no trivial patches that will require a license agreement.  Can you please take a look at http://apache.org/licenses/#clas ?

Regards,
KAM
Comment 9 Kevin A. McGrail 2018-08-24 02:00:33 UTC
Checking if we have an ICLA from Curtis Villamizar otherwise very little can be done with these patches.
Comment 10 Curtis Villamizar 2018-08-24 14:53:44 UTC
Please consider the patches in this bug report to be the property of Apache Foundation to be made available under the Apache Licence Agreement and any other agreement spammassassin is made available under.

I hope that is sufficient for a CLA.  If not please let me know.
Comment 11 Kevin A. McGrail 2018-08-24 14:56:16 UTC
Thank you for responding but sorry, it's really not.  Can you take a pass at https://www.apache.org/licenses/icla.pdf?

If you can give some clarity about which patches to look at, which order. which are tested, etc. that would be helpful.  KAM
Comment 12 Curtis Villamizar 2018-08-28 22:26:12 UTC
I just sent in the ICLA.  Second time was a charm.  First time files was too big.

As so which patches, all of them except the one that is marked "preliminary" and "not tested":

patch-spamc__libspamc.c
patch-spamc__libspamc.h
patch-spamc__spamc.c
patch-spamc__spamc.pod
patch-spamd__spamd.raw

The comment trail describes the changes to each file.

These have not changed since Nov 2015 and have been used as local patches since then.  I'm using the FreeBSD ports collections and have built most recently on July 25 of this year.  So they have been tested for three years now.

If you prefer I can just concatonate the individual patch files into one file.
Comment 13 Kevin A. McGrail 2018-08-28 22:29:27 UTC
(In reply to Curtis Villamizar from comment #12)
> I just sent in the ICLA.  Second time was a charm.  First time files was too
> big.
> 
> As so which patches, all of them except the one that is marked "preliminary"
> and "not tested":
> 
> patch-spamc__libspamc.c
> patch-spamc__libspamc.h
> patch-spamc__spamc.c
> patch-spamc__spamc.pod
> patch-spamd__spamd.raw
> 
> The comment trail describes the changes to each file.
> 
> These have not changed since Nov 2015 and have been used as local patches
> since then.  I'm using the FreeBSD ports collections and have built most
> recently on July 25 of this year.  So they have been tested for three years
> now.
> 
> If you prefer I can just concatonate the individual patch files into one
> file.

If you can review the patches and made a consolidated patch and take a good pass at the documentation, I'll add this for 3.4.2.

Upgrading to blocker.
Comment 14 Curtis Villamizar 2018-08-29 01:10:18 UTC
Created attachment 5588 [details]
consolidated patches to five files into one patch file

This is a consolidated patch file that is just a concatonation of the five patch files.

Documentation is in spamc.pod and the pod comments in spamd.raw.  All new command options are described in the help as well as the man pages from pod.

I've tried to match coding style.

I'm going to bring up a test spamd server and test spamc client and further test to make sure the TLS version options all work.
Comment 15 Bill Cole 2018-08-31 22:55:03 UTC
*** Bug 7092 has been marked as a duplicate of this bug. ***
Comment 16 Kevin A. McGrail 2018-09-04 13:53:40 UTC
Pushing to 3.4.3.  Too big a change for this late in the game.
Comment 17 Giovanni Bechis 2019-05-05 10:16:47 UTC
Created attachment 5653 [details]
Let spamc compile without ssl support, check return values

Polished version of the diff, it seems not to work correctly anyway for me, "openssl s_client" is able to connect to spamd but when spamc connects gives a "bad protocol: header error: (closed before headers)" warning message.
Comment 18 Henrik Krohns 2019-06-15 20:54:13 UTC
Targeting 4.0.0 which seems much more appropriate