[PATCH] Core: fixed uninitialized memory access

[Maxim Dounin]

Hello!

On Tue, Sep 27, 2016 at 11:23:00PM +0200, Jan Seda wrote:

> Struct sockaddr was allocated without zeroing. As getsockname() does not
> necessarily fill all the bytes, this left potentially uninitialized
> memory, which caused me a weird failure in the on-the-fly upgrade
> mechanism, where the new master failed to pair the config file AF_UNIX
> sockets with the inherited ones. ngx_cmp_sockaddr() would return
> NGX_DECLINED because the inherited one would contain uninitialized
> garbage after the actual name. This in turn would cause nginx to try to
> bind() the unix socket again, and obviously fail, because the listening
> socket was bound already (and inherited).
> 
> Easy to reproduce on debian wheezy, debian jessie and CentOS 7 systems
> (all x86_64) with statically linked openssl 1.1.0. The attached patch
> seems to fix the problem.
> 
> Also harmonized socklen calculation for AF_UNIX sockets to the way
> ngx_parse_unix_domain_url() does it (sizeof(struct sockaddr_un)), so
> ngx_cmp_sockaddr() is now less dangerous.

Thanks for catching this.

Dropping the length returned by getsockname() doesn't look like 
a correct solution though.  Instead, teaching ngx_cmp_sockaddr() to 
compare sockaddrs with not-completely-filled sun_path - that is, 
respecting socklen - should be the right way to go.

Please try the following patch:

diff --git a/src/core/ngx_inet.c b/src/core/ngx_inet.c
--- a/src/core/ngx_inet.c
+++ b/src/core/ngx_inet.c
@@ -1364,6 +1364,7 @@ ngx_cmp_sockaddr(struct sockaddr *sa1, s
     struct sockaddr_in6  *sin61, *sin62;
 #endif
 #if (NGX_HAVE_UNIX_DOMAIN)
+    size_t                len;
     struct sockaddr_un   *saun1, *saun2;
 #endif
 
@@ -1393,15 +1394,45 @@ ngx_cmp_sockaddr(struct sockaddr *sa1, s
 #if (NGX_HAVE_UNIX_DOMAIN)
     case AF_UNIX:
 
-        /* TODO length */
-
         saun1 = (struct sockaddr_un *) sa1;
         saun2 = (struct sockaddr_un *) sa2;
 
-        if (ngx_memcmp(&saun1->sun_path, &saun2->sun_path,
-                       sizeof(saun1->sun_path))
-            != 0)
-        {
+        if (slen1 <= (socklen_t) offsetof(struct sockaddr_un, sun_path)) {
+            return NGX_DECLINED;
+        }
+
+        if (slen2 <= (socklen_t) offsetof(struct sockaddr_un, sun_path)) {
+            return NGX_DECLINED;
+        }
+
+        if (saun1->sun_path[0] == '\0' || saun2->sun_path[0] == '\0') {
+
+            /* an abstract socket address */
+
+            if (ngx_memn2cmp((u_char *) saun1->sun_path,
+                             (u_char *) saun2->sun_path,
+                             slen1 - offsetof(struct sockaddr_un, sun_path),
+                             slen2 - offsetof(struct sockaddr_un, sun_path))
+                != 0)
+            {
+                return NGX_DECLINED;
+            }
+
+            break;
+        }
+
+        if (slen1 < slen2) {
+            len = slen1 - offsetof(struct sockaddr_un, sun_path);
+
+        } else {
+            len = slen2 - offsetof(struct sockaddr_un, sun_path);
+        }
+
+        if (len > sizeof(saun1->sun_path)) {
+            len = sizeof(saun1->sun_path);
+        }
+
+        if (ngx_strncmp(saun1->sun_path, saun2->sun_path, len) != 0) {
             return NGX_DECLINED;
         }
 

-- 
Maxim Dounin
http://nginx.org/