Bug 5728 - [review] require -u with --sql-config or --ldap-config
Summary: [review] require -u with --sql-config or --ldap-config
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: spamc/spamd (show other bugs)
Version: 3.2.3
Hardware: Other other
: P5 normal
Target Milestone: Undefined
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-20 16:09 UTC by Duncan Findlay
Modified: 2007-11-28 16:04 UTC (History)
0 users



Attachment Type Modified Status Actions Submitter/CLA Status
Fix for 3.2 branch patch None Duncan Findlay [HasCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Duncan Findlay 2007-11-20 16:09:35 UTC
When using spamd with -q/--sql-config or --ldap-config, spamd currently will try
to use the default setuid behaviour and setuid to the passed in user. This
doesn't really make sense. If we wanted the setuid behaviour, we'd use
-Q/--setuid-with-sql and/or --setuid-with-ldap.

Now, if we're not going to setuid, we should also require -u as we do with
vpopmail or virtual-config-dir. (That said, should we require -u or should we
only require -u if spamd is not run as root?)
Comment 1 Duncan Findlay 2007-11-20 16:19:20 UTC
Sorry, I just realized that description is a little incorrect.

If we use --sql-config or --ldap-config, spamd won't try to use the default
setuid behavior, but it will try to fallback to nobody if it's running as root.
This doesn't really make sense, since we shouldn't be using the setuid behavior
to begin with.

Anyways, I've committed a patch. I'm still unsure whether we should drop the
requirement for '-u' if we're running spamd as a different (non-root) user.

Committed revision 596888.
Comment 2 Duncan Findlay 2007-11-20 16:25:40 UTC
Created attachment 4188 [details]
Fix for 3.2 branch
Comment 3 Michael Parker 2007-11-21 10:59:52 UTC
+1
Comment 4 Justin Mason 2007-11-22 02:49:51 UTC
+1

'sudo make test TEST_FILES=t/root*' passes.
Comment 5 Duncan Findlay 2007-11-22 11:36:59 UTC
Now, does anyone have an opinion on whether we should be checking for -u or that
$> != 0?
Comment 6 Justin Mason 2007-11-23 01:51:58 UTC
(In reply to comment #5)
> Now, does anyone have an opinion on whether we should be checking for -u or that
> $> != 0?

can you clarify?

what is the behaviour if run as non-root without -u?  spamd won't be able to
setuid anyway so it should be a moot point...
Comment 7 Duncan Findlay 2007-11-27 13:57:09 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > Now, does anyone have an opinion on whether we should be checking for -u or that
> > $> != 0?
> 
> can you clarify?

This whole section of code hurts my brain.

I believe under the current code, you can't run
"spamd --vpopmail" or "spamd --virtual-config-dir" as a non-root user. We
specifically check to see if these options are used without the "--username"
option and die.

With my change, this also applies to "spamd --sql-config" and "spamd --ldap-config".

The quick fix (I think) is to wrap this code (and the like)

if ( $opt{'vpopmail'} ) {
  if ( !$opt{'username'} ) {
    die "spamd: cannot use --vpopmail without -u\n";
  }
}

with an "if ($setuid_to_user) {}" or "if ($> == 0) {}" block.
Comment 8 Justin Mason 2007-11-27 16:06:04 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Now, does anyone have an opinion on whether we should be checking for -u
or that
> > > $> != 0?
> > 
> > can you clarify?
> 
> This whole section of code hurts my brain.
> 
> I believe under the current code, you can't run
> "spamd --vpopmail" or "spamd --virtual-config-dir" as a non-root user. We
> specifically check to see if these options are used without the "--username"
> option and die.
> 
> With my change, this also applies to "spamd --sql-config" and "spamd
--ldap-config".
> 
> The quick fix (I think) is to wrap this code (and the like)
> 
> if ( $opt{'vpopmail'} ) {
>   if ( !$opt{'username'} ) {
>     die "spamd: cannot use --vpopmail without -u\n";
>   }
> }
> 
> with an "if ($setuid_to_user) {}" or "if ($> == 0) {}" block.

I think it's ok, since $setuid_to_user is set to 1 if ($> != 0).
Comment 9 Duncan Findlay 2007-11-27 18:57:51 UTC
(In reply to comment #8)

> I think it's ok, since $setuid_to_user is set to 1 if ($> != 0).

I'm confused by your comment (what is ok?) -- what do you think is the right
thing to do?
Comment 10 Justin Mason 2007-11-28 03:02:16 UTC
(In reply to comment #9)
> (In reply to comment #8)
> 
> > I think it's ok, since $setuid_to_user is set to 1 if ($> != 0).
> 
> I'm confused by your comment (what is ok?) -- what do you think is the right
> thing to do?

actually, ignore my comment -- I misread both your intentions, and the code ;)
So you said:

'I believe under the current code, you can't run
"spamd --vpopmail" or "spamd --virtual-config-dir" as a non-root user. We
specifically check to see if these options are used without the "--username"
option and die.
With my change, this also applies to "spamd --sql-config" and "spamd
--ldap-config".'

that's almost correct.  Actually, it *IS* possible to run as non-root if the
user uses '-u $currentuser'; e.g. if you look at t/spamd_allow_user_rules.t, it
runs spamd with

  spamd --virtual-config-dir=log/virtualconfig/%u -L -u $spamd_run_as_user

where $spamd_run_as_user is set by the test framework to be whatever the username
of the currently-running uid is.  So the caller has to know what their current
username is, and use that in the invocation, and spamd will then run ok as
non-root with those options.

If you are suggesting that we should *automatically* deal with this case, by
doing something like

  if ($> != 0 && !$opt{'username'}) {
    # we are run as non-root; set username to whatever we're currently running as
    $opt{'username'} = currently_running_username();
  }

I would be in favour of this, as a new feature.  But it'd be a separate bug...

is that what you were talking about?
Comment 11 Duncan Findlay 2007-11-28 16:04:12 UTC
Committed to 3.2 branch (with fixed typo in comment) as r599208

Updated Summary to reflect description better.

Filed bug 5734 in response to Justin's comments.