From b0317d31d98b17cd8b9b5d29f438191ac045cd33 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Mon, 8 Dec 2008 12:26:36 +0100 Subject: backport of $AllowedSender security fix - security bugfix: $AllowedSender was not honored, all senders were permitted instead (see http://www.rsyslog.com/Article322.phtml) (backport from v3-stable, v3.20.9) - minor bugfix: dual close() call on tcp session closure --- ChangeLog | 6 ++++ configure.ac | 2 +- doc/manual.html | 2 +- net.c | 72 +++++++++++++++++++++++++++++++++------------ net.h | 7 ++--- plugins/imgssapi/imgssapi.c | 14 +++------ plugins/imtcp/imtcp.c | 7 ++--- plugins/imudp/imudp.c | 22 +++++++++----- rsyslog.h | 1 + tcps_sess.c | 1 - tcpsrv.c | 10 +++---- 11 files changed, 90 insertions(+), 54 deletions(-) diff --git a/ChangeLog b/ChangeLog index 77d7be77..d86cbbea 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,4 +1,10 @@ --------------------------------------------------------------------------- +Version 3.18.6 (rgerhards), 2008-12-08 +- security bugfix: $AllowedSender was not honored, all senders were + permitted instead (see http://www.rsyslog.com/Article322.phtml) + (backport from v3-stable, v3.20.9) +- minor bugfix: dual close() call on tcp session closure +--------------------------------------------------------------------------- Version 3.18.5 (rgerhards), 2008-10-09 - bugfix: imudp input module could cause segfault on HUP It did not properly de-init a variable acting as a linked list head. diff --git a/configure.ac b/configure.ac index 892a2ab9..501cca56 100644 --- a/configure.ac +++ b/configure.ac @@ -2,7 +2,7 @@ # Process this file with autoconf to produce a configure script. AC_PREREQ(2.61) -AC_INIT([rsyslog],[3.18.5],[rsyslog@lists.adiscon.com]) +AC_INIT([rsyslog],[3.18.6],[rsyslog@lists.adiscon.com]) AM_INIT_AUTOMAKE AC_CONFIG_SRCDIR([syslogd.c]) AC_CONFIG_HEADERS([config.h]) diff --git a/doc/manual.html b/doc/manual.html index f0c02fd2..c84b095b 100644 --- a/doc/manual.html +++ b/doc/manual.html @@ -16,7 +16,7 @@ relay chains while at the same time being very easy to setup for the novice user. And as we know what enterprise users really need, there is also professional rsyslog support available directly from the source!

-

This documentation is for version 3.18.5 (v3-stable branch) of rsyslog. +

This documentation is for version 3.18.6 (v3-stable branch) of rsyslog. Visit the rsyslog status page to obtain current version information and project status.

If you like rsyslog, you might diff --git a/net.c b/net.c index 4054b863..54d162bf 100644 --- a/net.c +++ b/net.c @@ -80,6 +80,33 @@ static struct AllowedSenders *pLastAllowedSenders_GSS = NULL; int ACLAddHostnameOnFail = 0; /* add hostname to acl when DNS resolving has failed */ int ACLDontResolve = 0; /* add hostname to acl instead of resolving it to IP(s) */ + +/* sets the correct allow root pointer based on provided type + * rgerhards, 2008-12-01 + */ +static inline rsRetVal +setAllowRoot(struct AllowedSenders **ppAllowRoot, uchar *pszType) +{ + DEFiRet; + + if(!strcmp((char*)pszType, "UDP")) + *ppAllowRoot = pAllowedSenders_UDP; + else if(!strcmp((char*)pszType, "TCP")) + *ppAllowRoot = pAllowedSenders_TCP; +#ifdef USE_GSSAPI + else if(!strcmp((char*)pszType, "GSS")) + *ppAllowRoot = pAllowedSenders_GSS; +#endif + else { + dbgprintf("program error: invalid allowed sender ID '%s', denying...\n", pszType); + ABORT_FINALIZE(RS_RET_CODE_ERR); /* everything is invalid for an invalid type */ + } + +finalize_it: + RETiRet; +} + + /* Code for handling allowed/disallowed senders */ static inline void MaskIP6 (struct in6_addr *addr, uint8_t bits) { @@ -164,27 +191,32 @@ static rsRetVal AddAllowedSenderEntry(struct AllowedSenders **ppRoot, struct All } /* function to clear the allowed sender structure in cases where - * it must be freed (occurs most often when HUPed. - * TODO: reconsider recursive implementation - * I think there is also a memory leak, because only the last entry - * is acutally deleted... -- rgerhards, 2007-12-25 + * it must be freed (occurs most often when HUPed). + * rgerhards, 2008-12-02: revamped this code when we fixed the interface + * definition. Now an iterative algorithm is used. */ -void clearAllowedSenders (struct AllowedSenders *pAllow) +static void +clearAllowedSenders(uchar *pszType) { - if (pAllow != NULL) { - if (pAllow->pNext != NULL) - clearAllowedSenders (pAllow->pNext); - else { - if (F_ISSET(pAllow->allowedSender.flags, ADDR_NAME)) - free (pAllow->allowedSender.addr.HostWildcard); - else - free (pAllow->allowedSender.addr.NetAddr); - - free (pAllow); - } + struct AllowedSenders *pPrev; + struct AllowedSenders *pCurr; + + if(setAllowRoot(&pCurr, pszType) != RS_RET_OK) + return; /* if something went wrong, so let's leave */ + + while(pCurr != NULL) { + pPrev = pCurr; + pCurr = pCurr->pNext; + /* now delete the entry we are right now processing */ + if(F_ISSET(pPrev->allowedSender.flags, ADDR_NAME)) + free(pPrev->allowedSender.addr.HostWildcard); + else + free(pPrev->allowedSender.addr.NetAddr); + free(pPrev); } } + /* function to add an allowed sender to the allowed sender list. The * root of the list is caller-provided, so it can be used for all * supported lists. The caller must provide a pointer to the root, @@ -566,11 +598,15 @@ static inline int MaskCmp(struct NetAddr *pAllow, uint8_t bits, struct sockaddr * returns 1, if the sender is allowed, 0 otherwise. * rgerhards, 2005-09-26 */ -static int isAllowedSender(struct AllowedSenders *pAllowRoot, struct sockaddr *pFrom, const char *pszFromHost) +static int isAllowedSender(uchar *pszType, struct sockaddr *pFrom, const char *pszFromHost) { struct AllowedSenders *pAllow; - + struct AllowedSenders *pAllowRoot; + assert(pFrom != NULL); + + if(setAllowRoot(&pAllowRoot, pszType) != RS_RET_OK) + return 0; /* if something went wrong, we denie access - that's the better choice... */ if(pAllowRoot == NULL) return 1; /* checking disabled, everything is valid! */ diff --git a/net.h b/net.h index 0f5b0bc1..dc89137e 100644 --- a/net.h +++ b/net.h @@ -92,19 +92,16 @@ BEGINinterface(net) /* name must also be changed in ENDinterface macro! */ /* things to go away after proper modularization */ rsRetVal (*addAllowedSenderLine)(char* pName, uchar** ppRestOfConfLine); void (*PrintAllowedSenders)(int iListToPrint); - void (*clearAllowedSenders) (); + void (*clearAllowedSenders)(uchar*); void (*debugListenInfo)(int fd, char *type); int *(*create_udp_socket)(uchar *hostname, uchar *LogPort, int bIsServer); void (*closeUDPListenSockets)(int *finet); - int (*isAllowedSender)(struct AllowedSenders *pAllowRoot, struct sockaddr *pFrom, const char *pszFromHost); + int (*isAllowedSender)(uchar *pszType, struct sockaddr *pFrom, const char *pszFromHost); rsRetVal (*getLocalHostname)(uchar**); int (*should_use_so_bsdcompat)(void); /* data memebers - these should go away over time... TODO */ int *pACLAddHostnameOnFail; /* add hostname to acl when DNS resolving has failed */ int *pACLDontResolve; /* add hostname to acl instead of resolving it to IP(s) */ - struct AllowedSenders *pAllowedSenders_UDP; - struct AllowedSenders *pAllowedSenders_TCP; - struct AllowedSenders *pAllowedSenders_GSS; ENDinterface(net) #define netCURR_IF_VERSION 2 /* increment whenever you change the interface structure! */ diff --git a/plugins/imgssapi/imgssapi.c b/plugins/imgssapi/imgssapi.c index 74d5d5c5..f2b00d9d 100644 --- a/plugins/imgssapi/imgssapi.c +++ b/plugins/imgssapi/imgssapi.c @@ -172,10 +172,10 @@ isPermittedHost(struct sockaddr *addr, char *fromHostFQDN, void *pUsrSrv, void*p pGSess = (gss_sess_t*) pUsrSess; if((pGSrv->allowedMethods & ALLOWEDMETHOD_TCP) && - net.isAllowedSender(net.pAllowedSenders_TCP, addr, (char*)fromHostFQDN)) + net.isAllowedSender((uchar*)"TCP", addr, (char*)fromHostFQDN)) allowedMethods |= ALLOWEDMETHOD_TCP; if((pGSrv->allowedMethods & ALLOWEDMETHOD_GSS) && - net.isAllowedSender(net.pAllowedSenders_GSS, addr, (char*)fromHostFQDN)) + net.isAllowedSender((uchar*)"GSS", addr, (char*)fromHostFQDN)) allowedMethods |= ALLOWEDMETHOD_GSS; if(allowedMethods && pGSess != NULL) pGSess->allowedMethods = allowedMethods; @@ -645,14 +645,8 @@ ENDmodExit BEGINafterRun CODESTARTafterRun /* do cleanup here */ - if (net.pAllowedSenders_TCP != NULL) { - net.clearAllowedSenders (net.pAllowedSenders_TCP); - net.pAllowedSenders_TCP = NULL; - } - if (net.pAllowedSenders_GSS != NULL) { - net.clearAllowedSenders (net.pAllowedSenders_GSS); - net.pAllowedSenders_GSS = NULL; - } + net.clearAllowedSenders((uchar*)"TCP"); + net.clearAllowedSenders((uchar*)"GSS"); ENDafterRun diff --git a/plugins/imtcp/imtcp.c b/plugins/imtcp/imtcp.c index 7baa95f2..9b4d49f5 100644 --- a/plugins/imtcp/imtcp.c +++ b/plugins/imtcp/imtcp.c @@ -66,7 +66,7 @@ static int isPermittedHost(struct sockaddr *addr, char *fromHostFQDN, void __attribute__((unused)) *pUsrSrv, void __attribute__((unused)) *pUsrSess) { - return net.isAllowedSender(net.pAllowedSenders_TCP, addr, fromHostFQDN); + return net.isAllowedSender((uchar*) "TCP", addr, fromHostFQDN); } @@ -158,10 +158,7 @@ ENDwillRun BEGINafterRun CODESTARTafterRun /* do cleanup here */ - if(net.pAllowedSenders_TCP != NULL) { - net.clearAllowedSenders(net.pAllowedSenders_TCP); - net.pAllowedSenders_TCP = NULL; - } + net.clearAllowedSenders((char*)"TCP"); ENDafterRun diff --git a/plugins/imudp/imudp.c b/plugins/imudp/imudp.c index 81e7d7a4..3cdd8dd6 100644 --- a/plugins/imudp/imudp.c +++ b/plugins/imudp/imudp.c @@ -49,6 +49,10 @@ DEF_IMOD_STATIC_DATA DEFobjCurrIf(errmsg) DEFobjCurrIf(net) +static time_t ttLastDiscard = 0; /* timestamp when a message from a non-permitted sender was last discarded + * This shall prevent remote DoS when the "discard on disallowed sender" + * message is configured to be logged on occurance of such a case. + */ static int *udpLstnSocks = NULL; /* Internet datagram sockets, first element is nbr of elements * read-only after init(), but beware of restart! */ static uchar *pszBindAddr = NULL; /* IP to bind socket to */ @@ -189,15 +193,22 @@ CODESTARTrunInput * configured to do this). * rgerhards, 2005-09-26 */ - if(net.isAllowedSender(net.pAllowedSenders_UDP, + if(net.isAllowedSender((uchar*) "UDP", (struct sockaddr *)&frominet, (char*)fromHostFQDN)) { parseAndSubmitMessage((char*)fromHost, (char*) pRcvBuf, l, MSG_PARSE_HOSTNAME, NOFLAG, eFLOWCTL_NO_DELAY); } else { dbgprintf("%s is not an allowed sender\n", (char*)fromHostFQDN); if(option_DisallowWarning) { - errmsg.LogError(NO_ERRCODE, "UDP message from disallowed sender %s discarded", - (char*)fromHost); + time_t tt; + + time(&tt); + if(tt > ttLastDiscard + 60) { + ttLastDiscard = tt; + errmsg.LogError(NO_ERRCODE, + "UDP message from disallowed sender %s discarded", + (char*)fromHost); + } } } } @@ -238,10 +249,7 @@ ENDwillRun BEGINafterRun CODESTARTafterRun /* do cleanup here */ - if (net.pAllowedSenders_UDP != NULL) { - net.clearAllowedSenders (net.pAllowedSenders_UDP); - net.pAllowedSenders_UDP = NULL; - } + net.clearAllowedSenders((uchar*)"UDP"); if(udpLstnSocks != NULL) { net.closeUDPListenSockets(udpLstnSocks); udpLstnSocks = NULL; diff --git a/rsyslog.h b/rsyslog.h index 88429631..90b1ea10 100644 --- a/rsyslog.h +++ b/rsyslog.h @@ -172,6 +172,7 @@ enum rsRetVal_ /** return value. All methods return this if not specified oth RS_RET_MAIL_NO_FROM = -2072, /**< sender for mail destination is missing */ RS_RET_INVALID_PRI = -2073, /**< PRI value is invalid */ RS_RET_QUEUE_FULL = -2074, /**< queue is full, operation could not be completed */ + RS_RET_CODE_ERR = -2109, /**< program code (internal) error */ /* RainerScript error messages (range 1000.. 1999) */ RS_RET_SYSVAR_NOT_FOUND = 1001, /**< system variable could not be found (maybe misspelled) */ diff --git a/tcps_sess.c b/tcps_sess.c index 001f32f0..74eac3af 100644 --- a/tcps_sess.c +++ b/tcps_sess.c @@ -99,7 +99,6 @@ CODESTARTobjDestruct(tcps_sess) /* now destruct our own properties */ if(pThis->fromHost != NULL) free(pThis->fromHost); - close(pThis->sock); ENDobjDestruct(tcps_sess) diff --git a/tcpsrv.c b/tcpsrv.c index 955fb9b5..924426af 100644 --- a/tcpsrv.c +++ b/tcpsrv.c @@ -415,7 +415,7 @@ static rsRetVal SessAccept(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) { DEFiRet; - tcps_sess_t *pSess; + tcps_sess_t *pSess = NULL; int newConn; int iSess = -1; struct sockaddr_storage addr; @@ -464,8 +464,6 @@ SessAccept(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) * configured to do this). * rgerhards, 2005-09-26 */ -RUNLOG_VAR("%p", ppSess); -RUNLOG_VAR("%p", pSess); if(!pThis->pIsPermittedHost((struct sockaddr*) &addr, (char*) fromHostFQDN, pThis->pUsr, pSess->pUsr)) { dbgprintf("%s is not an allowed sender\n", (char *) fromHostFQDN); if(option_DisallowWarning) { @@ -492,12 +490,12 @@ RUNLOG_VAR("%p", pSess); *ppSess = pSess; pThis->pSessions[iSess] = pSess; + pSess = NULL; finalize_it: if(iRet != RS_RET_OK) { - if(iSess != -1) { - if(pThis->pSessions[iSess] != NULL) - tcps_sess.Destruct(&pThis->pSessions[iSess]); + if(pSess != NULL) { + tcps_sess.Destruct(&pSess); } iSess = -1; // TODO: change this to be fully iRet compliant ;) } -- cgit