backport of $AllowedSender security fixv3.18.6

- 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

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 <a href="professional_support.html">professional
 rsyslog support</a> available directly from the source!</p>
-<p><b>This documentation is for version 3.18.5 (v3-stable branch) of rsyslog.</b>
+<p><b>This documentation is for version 3.18.6 (v3-stable branch) of rsyslog.</b>
 Visit the <i> <a href="http://www.rsyslog.com/doc-status.html">rsyslog status page</a></i></b> to obtain current
 version information and project status.
 </p><p><b>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 ;)
 	}