From f0ddbed44c332391ae6d9bbf6b07e2f06c4dd676 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Sat, 29 Nov 2008 07:22:48 +0100 Subject: security bugfix: $AllowedSender was not honored, ...all senders were permitted instead --- ChangeLog | 2 ++ plugins/imgssapi/imgssapi.c | 14 +++------ plugins/imtcp/imtcp.c | 7 ++--- plugins/imudp/imudp.c | 7 ++--- runtime/net.c | 69 +++++++++++++++++++++++++++++++++------------ runtime/net.h | 7 ++--- runtime/rsyslog.h | 1 + 7 files changed, 64 insertions(+), 43 deletions(-) diff --git a/ChangeLog b/ChangeLog index 82bccb14..1d0c5fa5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,7 @@ --------------------------------------------------------------------------- Version 3.20.1 [v3-stable] (rgerhards), 2008-11-?? +- security bugfix: $AllowedSender was not honored, all senders were + permitted instead - enhance: regex nomatch option "ZERO" has been added This allows to return the string 0 if a regular expression is not found. This is probably useful for storing numerical values into diff --git a/plugins/imgssapi/imgssapi.c b/plugins/imgssapi/imgssapi.c index 766cb519..d00c51d6 100644 --- a/plugins/imgssapi/imgssapi.c +++ b/plugins/imgssapi/imgssapi.c @@ -174,10 +174,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; @@ -656,14 +656,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 f01a9f0f..8e7d2905 100644 --- a/plugins/imtcp/imtcp.c +++ b/plugins/imtcp/imtcp.c @@ -89,7 +89,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); } @@ -212,10 +212,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 f8beb01f..57c5c02d 100644 --- a/plugins/imudp/imudp.c +++ b/plugins/imudp/imudp.c @@ -192,7 +192,7 @@ 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(fromHost, fromHostIP, pRcvBuf, l, MSG_PARSE_HOSTNAME, NOFLAG, eFLOWCTL_NO_DELAY); @@ -241,10 +241,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/runtime/net.c b/runtime/net.c index b9af700c..76dc185c 100644 --- a/runtime/net.c +++ b/runtime/net.c @@ -92,6 +92,30 @@ int ACLDontResolve = 0; /* add hostname to acl instead of resolving it /* ------------------------------ begin permitted peers code ------------------------------ */ +/* 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; + else if(!strcmp((char*)pszType, "GSS")) + *ppAllowRoot = pAllowedSenders_GSS; + 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; +} + + /* add a wildcard entry to this permitted peer. Entries are always * added at the tail of the list. pszStr and lenStr identify the wildcard * entry to be added. Note that the string is NOT \0 terminated, so @@ -507,27 +531,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, @@ -905,11 +934,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/runtime/net.h b/runtime/net.h index 0d36e824..092c3116 100644 --- a/runtime/net.h +++ b/runtime/net.h @@ -135,11 +135,11 @@ 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); /* permitted peer handling should be replaced by something better (see comments above) */ @@ -149,9 +149,6 @@ BEGINinterface(net) /* name must also be changed in ENDinterface macro! */ /* data members - 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 4 /* increment whenever you change the interface structure! */ diff --git a/runtime/rsyslog.h b/runtime/rsyslog.h index 7927af3a..06ffae86 100644 --- a/runtime/rsyslog.h +++ b/runtime/rsyslog.h @@ -248,6 +248,7 @@ enum rsRetVal_ /** return value. All methods return this if not specified oth RS_RET_CERTLESS = -2102, /**< state: we run without machine cert (this may be OK) */ RS_RET_QUEUE_FULL = -2103, /**< queue is full, operation could not be completed */ RS_RET_ACCEPT_ERR = -2104, /**< error during accept() system call */ + 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) */ -- cgit