Improved Socket File Permissions and Crash Prevention Checks (#760)

* Update sslciphersuite setting in omiserver.conf (#55) * Update TLS,SSL and sslciphersuite settings in omiserver.conf * Update omiserver.conf --------- * Fixes for MSRC issues * Null check to prevent crash --------- * 84333 * Correct typo in messages.c * Fixes socket permission. Fixes socket permission. * Fixes CodeQL SM02323 in xml.c * Improves socket permission. * Fixes socket permission for handle sig. * Socket file permission for omiserver.sock
microsoft · Mar 12, 2024 · 3566da8 · 3566da8
1 parent ffca75c
commit 3566da8
Show file tree

Hide file tree

Showing 11 changed files with 102 additions and 13 deletions.
diff --git a/Unix/base/batch.c b/Unix/base/batch.c
@@ -411,7 +411,8 @@ MI_Boolean Batch_FixPointer(
     Batch* self,
     const Header_BatchInfoItem* ptrAdjustmentInfo,
     size_t ptrAdjustmentInfoCount,
-    void** ptrInOut)
+    void** ptrInOut,
+    size_t size)
 {
     /* see if pointer matches old blocks */
     size_t index;
@@ -432,6 +433,10 @@ MI_Boolean Batch_FixPointer(
         if ( old_ptr >= old_page &&
             old_ptr < (old_page + ptrAdjustmentInfo[index].pageSize) )
         {
+            if( size > 0 && (old_page + ptrAdjustmentInfo[index].pageSize - old_ptr) < size)
+            {
+                return MI_FALSE;
+            }
             *ptrInOut = ((char*)(p + 1)) + (old_ptr - old_page);
             return MI_TRUE;
         }

diff --git a/Unix/base/batch.h b/Unix/base/batch.h
@@ -220,7 +220,8 @@ MI_Boolean Batch_FixPointer(
     Batch* batch,
     const Header_BatchInfoItem* ptrAdjustmentInfo,
     size_t ptrAdjustmentInfoCount,
-    void** ptrInOut);
+    void** ptrInOut,
+    size_t size);
 
 /* Add this block to list of individual blocks */
 MI_INLINE void Batch_AttachPage(

diff --git a/Unix/base/messages.c b/Unix/base/messages.c
@@ -477,7 +477,8 @@ static MI_Result _RestoreMessage(
                     batch,
                     ptrAdjustmentInfo,
                     ptrAdjustmentInfoCount,
-                    ptr))
+                    ptr,
+                    (size_t)0))
                 {
                     trace_RestoreMsgFailed_PointersForMstPointer();
                     return MI_RESULT_INVALID_PARAMETER;
@@ -505,7 +506,8 @@ static MI_Result _RestoreMessage(
                         batch,
                         ptrAdjustmentInfo,
                         ptrAdjustmentInfoCount,
-                        ptrPacked))
+                        ptrPacked,
+                        (size_t)packedSize))
                     {
                         trace_RestoreMsgFailed_PointersForMstInstance();
                         return MI_RESULT_INVALID_PARAMETER;
@@ -680,7 +682,8 @@ MI_Result __MessageFromBatch(
         batch,
         ptrAdjustmentInfo,
         ptrAdjustmentInfoCount,
-        (void*)&msg))
+        (void*)&msg,
+        sizeof(Message)))
     {
         trace_BatchFixPointerFailed();
         return MI_RESULT_INVALID_PARAMETER;

diff --git a/Unix/http/httpauth.c b/Unix/http/httpauth.c
@@ -1652,7 +1652,8 @@ static MI_Result _ServerAuthenticateCallback(PamCheckUserResp *msg)
     char *auth_response = NULL;
     int response_len = 0;
 
-    Http_SR_SocketData *handler = (Http_SR_SocketData*)(uintptr_t)msg->handle;
+    Handler* handlerIn = (Handler*)(uintptr_t)msg->handle;
+    Http_SR_SocketData* handler = FromOffset( Http_SR_SocketData, handler, handlerIn );
     HttpHeaders *headers = &handler->recvHeaders;
 
     if (!msg->result)
@@ -1794,7 +1795,7 @@ Http_CallbackResult IsClientAuthorized(_In_ Http_SR_SocketData * handler)
 
                 if (0 != AskServerToAuthenticate(headers->username, 
                                                  headers->password, 
-                                                 (MI_Uint64)(uintptr_t)handler,
+                                                 (MI_Uint64)(uintptr_t)(&handler->handler),
                                                  _ServerAuthenticateCallback))
                 {
                     handler->httpErrorCode = HTTP_ERROR_CODE_UNAUTHORIZED;

diff --git a/Unix/installbuilder/conf/omiserver.conf b/Unix/installbuilder/conf/omiserver.conf
@@ -63,6 +63,13 @@ httpsport=0
 #NoSSLv2=true
 #NoSSLv3=false
 
+##
+## This section is for TLS enabled ciphers
+## The prioritized list of allowed SSL/TLS ciphers. For example, set `sslciphersuite=ALL:!SSLv2:!SSLv3:!TLSv1:!TLSv0:!CBC:!RC4-MD5:!RC4-SHA:!SEED-SHA` in `/etc/opt/omi/conf/omiserver.conf` to disable all SSLv2,SSLv3,TLSv1,TLSv0 ciphers and other weak ciphers: ##CBC,RC4-MD5,RC4-SHA,SEED-SHA; then run `sudo /opt/omi/bin/service_control restart` to take effect, for more information, check `man ciphers` or search internet with `openssl man ciphers` 
+## Note : Disabling TLSv1 and SSLv3 on some older implemetation of openssl doesn't work well. In such cases, either update the openssl version or update the below sslciphersuite value accordingly
+##
+sslciphersuite=ALL:!SSLv2:!SSLv3:!TLSv1:!TLSv0:!CBC:!RC4-MD5:!RC4-SHA:!SEED-SHA
+
 # Enabling this will cause each provider to run under it's own omiagent
 # process. This will take considerably more memory, but is useful for
 # diagnosing omiagent cores due to a provider fault. Setting of `true`

diff --git a/Unix/protocol/protocol.c b/Unix/protocol/protocol.c
@@ -1790,6 +1790,8 @@ static MI_Boolean _VerifyMessage(
 
     return MI_TRUE;
 }
+
+
 /*
     Processes incoming message, including:
         - decoding message from batch
@@ -1863,7 +1865,8 @@ static Protocol_CallbackResult _ProcessReceivedMessage(
         }
         else if (msg->tag == PamCheckUserRespTag)
         {
-            if( _ProcessPamCheckUserResp(handler, msg) )
+            ProtocolBase* protocolBase = (ProtocolBase*)handler->base.data;
+            if(Selector_ContainsHandlerId(protocolBase->selector, ((PamCheckUserResp*)msg)->handle) == MI_RESULT_OK &&  _ProcessPamCheckUserResp(handler, msg) )
                 ret = PRT_CONTINUE;
         }
 #if defined(CONFIG_ENABLE_PREEXEC)
@@ -1882,6 +1885,12 @@ static Protocol_CallbackResult _ProcessReceivedMessage(
                 BinProtocolNotification* binMsg = (BinProtocolNotification*) msg;                    
                 if (binMsg->type == BinNotificationConnectRequest)
                 {
+                    //Check if already forwarded to server 
+                    if(handler->engineHandler != NULL){
+                            trace_FailedNewServerConnection();
+                            return PRT_RETURN_FALSE;
+                    }
+
                     // forward to server
 
                     uid_t uid = INVALID_ID;
@@ -2064,6 +2073,12 @@ static Protocol_CallbackResult _ReadHeader(
 
     for ( ; ; )
     {
+        // Check if page count exceeds the PROTOCOL_HEADER_MAX_PAGES , if so this is an invalid request 
+        if (handler->recv_buffer.base.pageCount > PROTOCOL_HEADER_MAX_PAGES)
+        {
+            trace_Socket_ReadingHeader_ErrorPageCount(s_type, handler);
+            return PRT_RETURN_FALSE;
+        }
         buf = (char*)&handler->recv_buffer;
         buf_size = (sizeof(HeaderBase) + sizeof(Header_BatchInfoItem) * handler->recv_buffer.base.pageCount);
         received = 0;

diff --git a/Unix/server/servercommon.c b/Unix/server/servercommon.c
@@ -9,6 +9,7 @@
 #include <base/user.h>
 #include <sock/selector.h>
 #include <server/server.h>
+#include <grp.h>
 
 // We are allowing max 50 cipher suite with each being maximum of 100 length
 #define SSLCIPHERSUITE_LIMIT 5000

diff --git a/Unix/sock/selector.c b/Unix/sock/selector.c
@@ -245,7 +245,11 @@ MI_Result Selector_AddHandler(
 MI_Result Selector_RemoveHandler(
     Selector* self,
     Handler* handler)
-{
+{    
+    if(handler == NULL){
+        return MI_RESULT_NOT_FOUND;
+    }
+
     SelectorRep* rep = (SelectorRep*)self->rep;
     Handler* p;
 
@@ -468,6 +472,29 @@ MI_Result Selector_ContainsHandler(
     return MI_RESULT_NOT_FOUND;
 }
 
+MI_Result Selector_ContainsHandlerId(
+    Selector* self,
+    MI_Uint64 handlerId)
+{
+    SelectorRep* rep = (SelectorRep*)self->rep;
+    Handler* p;
+
+    Lock_Acquire(&rep->listLock);
+
+    for (p = (Handler*)rep->head; p; p = (Handler*)p->next)
+    {
+        if ((MI_Uint64)(uintptr_t)p == handlerId)
+        {
+            Lock_Release(&rep->listLock);
+            return MI_RESULT_OK;
+        }
+    }
+
+    Lock_Release(&rep->listLock);
+
+    return MI_RESULT_NOT_FOUND;
+}
+
 void Selector_SetAllowEmptyFlag(
     Selector* self,
     MI_Boolean allowEmptySelector)
@@ -699,9 +726,22 @@ MI_Result Selector_Run(
                     /* If callback wants to continue getting events */
                     if (!more)
                     {
+                        Handler* temp = NULL;
+                        if(next != NULL){
+                            temp = next->next;
+                        }
+
                         /* Remove handler */
                         Selector_RemoveHandler(self, p);
 
+                        // the selector_removeHandler also removes the enginehandler from the list
+                        // engine handler could be the next value in the list which can cause use after free access to the memory
+                        // to avoid this advance to next->next
+
+                        if(MI_RESULT_OK != Selector_ContainsHandler(self, next)){
+                            next = temp;
+                        } 
+
                         /* Refresh current time stamp */
                         if (PAL_TRUE != PAL_Time(&currentTimeUsec))
                         {

diff --git a/Unix/sock/selector.h b/Unix/sock/selector.h
@@ -74,6 +74,10 @@ MI_Result Selector_ContainsHandler(
     Selector* self,
     Handler* handler);
 
+MI_Result Selector_ContainsHandlerId(
+    Selector* self,
+    MI_Uint64 handlerId);
+
 /* Runs socket processing loop;
     Parameters:
     self - selector

diff --git a/Unix/sock/sock.c b/Unix/sock/sock.c
@@ -33,6 +33,7 @@
 #endif
 
 #include <base/log.h>
+#include <base/paths.h>
 #include <pal/strings.h>
 #include <pal/format.h>
 
@@ -642,9 +643,19 @@ MI_Result Sock_CreateLocalListener(
         return MI_RESULT_FAILED;
     }
 
-    /* Change mode to allow non-root to connect to it (they need 'w' to connect) */
-
-    chmod(socketName, S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IWGRP | S_IXGRP | S_IROTH | S_IWOTH | S_IXOTH);
+    const char *engineSocketName = OMI_GetPath(ID_SOCKETFILE);
+    if(strcmp(engineSocketName,socketName) == 0)
+    {
+        if(chmod(socketName, S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IWGRP | S_IXGRP) != 0)
+        {
+            trace_LocalSocketFailed(socketName);
+            return MI_RESULT_FAILED;
+        } 
+    }
+    else
+    {
+        chmod(socketName, S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IWGRP | S_IXGRP | S_IROTH | S_IWOTH | S_IXOTH);
+    }
 
     /* Listen on this socket for connections */
     {

diff --git a/Unix/xml/xml.c b/Unix/xml/xml.c
@@ -1725,7 +1725,8 @@ int XML_ParseCharFault(const XML *self, const XML_Char *data, XML_Char *buffer,
 {
 #define PREFIX_SIZE 32
     XML_Char prefix[PREFIX_SIZE];
-    int i, j, k, l;
+    int i, j;
+    size_t k, l;
     MI_Boolean prefixFound = MI_FALSE;
 
     for (i=0; i<PREFIX_SIZE-1; ++i)