smfreegard / DecodeShortURLs

SpamAssassin DecodeShortURLs plug-in repository
32 stars 15 forks source link

Plugin fails if spamd/spamassassin runs setuid and cache DB not world-writable #6

Open cepheid666 opened 6 years ago

cepheid666 commented 6 years ago

On systems where spamassassin is run with per-user settings, if the url_shortener_cache is not world-writable, the following error is generated and all DecodeShortURLs plugin rules fail: warn: plugin: eval failed: DBD::SQLite::st execute failed: attempt to write a readonly database at /etc/mail/spamassassin/DecodeShortURLs.pm line 591, <GEN15> line 58. Subsequent calls also generate a second error: warn: ) statement handle DBI: [...]:st=HASH(0xc5d9514) still Active at /etc/mail/spamassassin/DecodeShortURLs.pm line 565

When this error occurs, HAS_SHORT_URL fails to trigger, and therefore all subsequent rules also fail.

spamd: When run as root, spamd will setuid to the user invoking spamc, or to "nobody" if invoked by root. If the DB is not writable by the setuid user, this error will occur. If the DB owner is root, which is typical, then this error will occur on every call to spamd, even by root.

spamassassin: When manually invoked, this error will occur for all users other than DB owner (typically root).

If the DB is world-writable, this error doesn't occur and everything works as expected. (This error is also not applicable on systems where spamd/spamassassin are always run as a single user, assuming the DB is properly owner by that user. But, many systems operate in per-user mode.)

Note that this error is generated EVEN IF the URL in the spam is already cached, i.e., the cache file only needs to be read, not written to.

Requiring a world-writable DB seems undesirable.

1) Since the DB is just a cache, if it's not writable, it would seem that the plugin should be able to just fail gracefully, i.e., perform all read operations as needed, still perform all checks, and just omit writing to the cache. This may be less efficient but would allow proper operation, rather than outright failure as currently happens. (If desired, perhaps a user-enabled rule DECODESHORTURLS_CACHE_READONLY could be triggered to warn the sysadmin or user about this issue. Default setting on but could be disabled.)

2) Since running with a world-writable DB is a security concern, the plugin should ideally use per-user cache files, preferably in the user's user-pref directory (i.e., ~/.spamassassin). Since spamd runs setuid, it can already create and modify files in that directory (e.g., the Bayes DB).

This could be a configurable setting at the sysadmin level, i.e., one could have something like:

url_shortener_peruser [yes|no]
url_shortener_defaultdir [/tmp]
url_shortener_cache [DecodeShortURLs.sq3]

In the above scheme, if peruser = yes, the cache file used is <setuid-userhome>/<sa-pref>/<cache>, i.e., $HOME/.spamassassin/DecodeShortURLs.sq3 in this example. If the setuid user has no shell or no home directory (e.g., when spamd is running as "nobody"), if the user-pref directory isn't readable, or if peruser = no, the cache file used is <defaultdir>/<cache>, i.e., /tmp/DecodeShortURLs.sq3 in this example. (If the user-pref directory is readable but not writable, then the same READONLY trigger rule from item 1 above could fire.)

cepheid666 commented 5 years ago

@smfreegard Is there anything I can do to help with this? I'm unfortunately not sufficiently skilled to hack the plugin directly to do this. @orlitzky made some suggestions in issue #7 regarding using the userstate_dir variable, though I'm not sure where to insert that. But it would be great to get this resolved so that per-user configs can work securely. (In principle this could become a default plugin within the SA distro, if it can be robust to both single-user and per-user installs.)

smfreegard commented 5 years ago

Sorry - but I stopped using SpamAssassin quite a long time ago and switched to rspamd, so I don't really have much inclination to spend time on this.

I never used this for per-user configs so I never had this problem myself. Running a cache database per-user is really silly though, it needs to be shared across all users otherwise you're going to be doing needless HTTP calls to unshorten the links. Same problem if ignore writes etc.

cepheid666 commented 5 years ago

Indeed, having a system-wide DB does make more sense from a lookup perspective. Then the question is how to enable this securely when spamc is run per-user. But, I understand it's no longer a priority for you... thanks regardless for getting it started!