Re: [Nmh-workers] Questionable code in m_chkids() in sbr/context_save.c

[Neil W Rickert]

Jon Steinhart <address@hidden> wrote on May 13, 2005:

>> Jon Steinhart <address@hidden> wrote on May 13, 2005:

>> >Saw this while looking for something else.

>> >m_chkids() forks a child process to run context_save() if the
>> >uid is not the same as the euid.  But, it ends up running as
>> >if the uid and euid are the same if the fork() fails.  Seems
>> >to me that this should be an error.  I realize that it will
>> >probably result in later errors from being unable to access
>> >the files, but those will be confusing since they won't indicate
>> >the real problem.

>> >Opinions?

>> You shouldn't be making mh commands setuid, so the situation is
>> unlikely to arise.  This probably isn't worth fixing, except as part
>> of a complete revamp of core code.

>So give me a clue here.  Why shouldn't they be made setuid?  Someone
>obviously thought enough about this to put this code there in the
>first place.  If running setuid is a bad thing and shouldn't be done
>would it be acceptable to just remove this whole piece of code?

It was probably put into the library for generality.

I perhaps overstated things.  Better would be that, with only one
exception, I cannot think of a reason to run the mh commands setuid.

The one exception is "slocal", which might be called from sendmail or
similar MTA to deliver, and might inherit root euid.  My memory of
the code is that slocal checks for this condition, and does a
setuid() to the actual recipient quite early in the code, so it
should not be setuid by the time it is looking at context.

What can go wrong if euid != ruid, and that fork() fails?

The possibilities I see are:

  1:  the euid in use does not have permission to update the
      context.  In this case the command fails anyway.

  2:  the update succeeds, rewriting an existing file.  This does
      not seem to pose a problem.

  3:  the update succeeds creating or re-creating the file.  The data
      is still saved, but the created file has the wrong ownership.

The third case is the more questionable.  Which is better -- save the
data under the wrong ownership, or lose the data?  I suspect it isn't
very important, especially since it probably won't happen anyway.

Maybe there is a security risk here.  However, I would think that
the risk is in allowing these commands (except slocal) to run setuid
in the first place.

In other words, I don't see a serious problem with just leaving the
code as it is for the present.  Or perhaps add a comment that this
should be reviewed if/when the code is revamped.

Feel free to disagree (that applies to anybody).

 -NWR