jonny.tornbom at axis
Sep 12, 2012, 3:16 AM
Post #7 of 8
(289 views)
Permalink
|
Hi,
I did some investigation of this and here's my conclusions:
(We are currently using rsyslog 5.8.11 here, my comments below is targeted
towards that version if I don't specify otherwise)
SCM_CREDENTIALS is a control message type, not a socket option, so using
setsockopt(...SCM_CREDENTIALS...) is potentially dangerous and wrong and
should be deleted from the code. (SCM_CREDENTIALS is used in conjuction
with SO_PASSCRED which is the socket option to use.)
I'll give some inline comments below and some additional comments at the end:
On Wed, Sep 12, 2012 at 09:56:33AM +0200, Rainer Gerhards wrote:
> > -----Original Message-----
> > From: rsyslog-bounces [at] lists [mailto:rsyslog-
> > bounces [at] lists] On Behalf Of Cristian Ionescu-Idbohrn
> > Sent: Tuesday, August 28, 2012 3:43 PM
> > To: rsyslog [at] lists
> > Subject: Re: [rsyslog] SCM_CREDENTIALS saga
> >
> > Rainer,
> >
> > 2 weeks ago I stumbled on this problem:
> >
> > On Tue, 14 Aug 2012, Cristian Ionescu-Idbohrn wrote:
> > >
> > > Calling 'setsockopt' with 'optname' SCM_CREDENTIALS seems to be both
> > > wrong and not needed.
> >
> > and I've sent 2 patches. Bellow, a cleaner patch that even corrects
> > some warnings.
> >
> > I wonder if you could comment on the matter?
>
> I finally looked at the patch. Some comments below:
>
> >
> > diff --git a/rsyslog/plugins/imuxsock/imuxsock.c
> > b/rsyslog/plugins/imuxsock/imuxsock.c
> > index 9ad2421..64d8760 100644
> > --- a/rsyslog/plugins/imuxsock/imuxsock.c
> > +++ b/rsyslog/plugins/imuxsock/imuxsock.c
> > @@ -75,9 +75,11 @@ MODULE_TYPE_NOKEEP
> > #endif
> >
> > /* emulate struct ucred for platforms that do not have it */
> > +#if 0
> > #ifndef HAVE_SCM_CREDENTIALS
> > struct ucred { int pid; };
>
> This is needed by the BSD folks, so we cannot simply #if0 it.
>
This shall _not_ be #if 0 since several functions take a struct ucred
as argument. However, I saw you had intentions to apply a patch from
Brad Davis (FreeBSD) that extended the ucred struct with guid/uid, and
that shall not really be necessary since none of that is used. This only
generates some unused variables as far as I could see. (There's a second
issue with the other part of that patch, i'll comment on that at the end
of this mail).
> > #endif
> > +#endif
> >
> > /* handle some defines missing on more than one platform */
> > #ifndef SUN_LEN
> > @@ -376,7 +378,9 @@ static inline rsRetVal
> > openLogSocket(lstn_t *pLstn)
> > {
> > DEFiRet;
---
> > +# if HAVE_SCM_CREDENTIALS
> > int one;
> > +# endif /* HAVE_SCM_CREDENTIALS */
This looks OK on 5.8.11 and v7 too, its only used inside an
HAVE_SCM_CREDENTIALS block.
> >
> > if(pLstn->sockName[0] == '\0')
> > return -1;
> > @@ -417,10 +421,21 @@ openLogSocket(lstn_t *pLstn)
> > errmsg.LogError(errno, NO_ERRCODE, "set SO_PASSCRED
> > failed on '%s'", pLstn->sockName);
> > pLstn->bUseCreds = 0;
> > }
> > +#if 0
> > + /*
> > + * SCM_CREDENTIALS is not a socket option; its value is
> > usually
> > + * 0x2 which on most archs corresponds to socket option
> > + * SO_REUSEADDR, but on mips-arch there is no socket option
> > with
> > + * value 0x2 and SO_REUSEADDR = 0x4, so the call will fail
> > with
> > + * ENOPROTOOPT 'Protocol not available'; skip it.
> > + * There does not seem any other special setsockopt call is
> > + * needed anyway, besides the above.
> > + */
>
> I think that #if0 and the comment means I can remove that code. Will double-check and, if so, remove it.
>
> Rainer
The setsockopt(...SCM_CREDENTIALS...) part needs to be removed since its wrong
and potentially dangerous. This part of Cristians patch should be correct.
> > if(setsockopt(pLstn->fd, SOL_SOCKET, SCM_CREDENTIALS, &one,
> > sizeof(one)) != 0) {
> > errmsg.LogError(errno, NO_ERRCODE, "set
> > SCM_CREDENTIALS failed on '%s'", pLstn->sockName);
> > pLstn->bUseCreds = 0;
> > }
> > +#endif
> > }
> > # else /* HAVE_SCM_CREDENTIALS */
> > pLstn->bUseCreds = 0;
> > @@ -631,7 +646,9 @@ static rsRetVal readSocket(lstn_t *pLstn)
> > # endif
> > struct ucred *cred;
> > uchar bufRcv[4096+1];
---
> > +# if HAVE_SCM_CREDENTIALS
> > char aux[128];
> > +# endif
This part is correct in both 5.8.11 and v7, aux is not used outside
of an HAVE_SCM_CREDENTIALS block in this function.
> > uchar *pRcv = NULL; /* receive buffer */
> >
> > assert(pLstn->fd >= 0);
> >
> >
> > Cheers,
> >
> > --
> > Cristian
> > _______________________________________________
> > rsyslog mailing list
> > http://lists.adiscon.net/mailman/listinfo/rsyslog
> > http://www.rsyslog.com/professional-services/
> > What's up with rsyslog? Follow https://twitter.com/rgerhards
> _______________________________________________
> rsyslog mailing list
> http://lists.adiscon.net/mailman/listinfo/rsyslog
> http://www.rsyslog.com/professional-services/
> What's up with rsyslog? Follow https://twitter.com/rgerhards
Ok the patch from Brad Davis (FreeBSD) with my comments inline:
>--- plugins/imuxsock/imuxsock.c.orig 2012-06-18 02:13:18.000000000 -0600
>+++ plugins/imuxsock/imuxsock.c 2012-08-16 16:37:27.000000000 -0600
>@@ -81,7 +81,7 @@
>
> /* emulate struct ucred for platforms that do not have it */
> #ifndef HAVE_SCM_CREDENTIALS
>-struct ucred { int pid; };
>+struct ucred { int pid; uid_t uid; gid_t gid; };
This shall not be necessary, it would be nice with an explanation of why this
is added.
> #endif
>
> /* handle some defines missing on more than one platform */
>@@ -891,9 +891,7 @@
> int iMaxLine;
> struct msghdr msgh;
> struct iovec msgiov;
>-# if HAVE_SCM_CREDENTIALS
> struct cmsghdr *cm;
>-# endif
This part _needs_ to be addressed on current master (v7) since the cm variable
is being used outside of a HAVE_SCM_CREDENTIALS block further down in the same
function. However for 5.8.11 its only used inside of a HAVE_SCM_CREDENTIALS block
and the patch will only generate a compiler warning about an unused variable. So
I think this part of the patch is correct but its version-specific of rsyslog.
> struct ucred *cred;
> struct timeval *ts;
> uchar bufRcv[4096+1];
Another issue was how configure checked if the system had SCM_CREDENTIALS defined.
The current way its done in configure.ac looks correct to me. One could extend it
to check if both SO_PASSCRED and SCM_CREDENTIALS are defined in sys/socket.h
or sys/types.h. (If the macro is defined it should be usable, if not usable, its
outside the scope of rsyslog.)
Another thing I noticed while browsing through plugins/imuxsock/imuxsock.c was that
cred->pid gets assigned a value typecasted to (unsigned long) at some places, where
at other places its casted to an int. Is there a reason for this?
Best regards,
Jonny
_______________________________________________
rsyslog mailing list
http://lists.adiscon.net/mailman/listinfo/rsyslog
http://www.rsyslog.com/professional-services/
What's up with rsyslog? Follow https://twitter.com/rgerhards
NOTE WELL: This is a PUBLIC mailing list, posts are ARCHIVED by a myriad of sites beyond our control. PLEASE UNSUBSCRIBE and DO NOT POST if you DON'T LIKE THAT.
|
