[PATCH] Fixing buffer over-read when accepting unix domain sockets

[Yichun Zhang (agentzh)]

Hello!

On Wed, Jul 10, 2013 at 6:18 AM, Maxim Dounin wrote:
>> -        if (ls->addr_ntop) {
>> +        if (ls->addr_ntop && socklen > sizeof(c->sockaddr->sa_family)) {
>>              c->addr_text.data = ngx_pnalloc(c->pool, ls->addr_text_max_len);
>>              if (c->addr_text.data == NULL) {
>>                  ngx_close_accepted_connection(c);
>
> The patch looks wrong - it doesn't initialize c->addr_text at all,
> while it's requested by a caller.
>

Thank you for the review!

How about this?

--- nginx-1.4.1/src/event/ngx_event_accept.c 2013-05-06 03:26:50.000000000 -0700
+++ nginx-1.4.1-patched/src/event/ngx_event_accept.c 2013-07-10
13:05:02.001249099 -0700
@@ -269,17 +269,28 @@ ngx_event_accept(ngx_event_t *ev)
 #endif

         if (ls->addr_ntop) {
-            c->addr_text.data = ngx_pnalloc(c->pool, ls->addr_text_max_len);
-            if (c->addr_text.data == NULL) {
-                ngx_close_accepted_connection(c);
-                return;
-            }
+            if (socklen > sizeof(c->sockaddr->sa_family)) {
+                c->addr_text.data = ngx_pnalloc(c->pool,
ls->addr_text_max_len);
+                if (c->addr_text.data == NULL) {
+                    ngx_close_accepted_connection(c);
+                    return;
+                }
+
+                c->addr_text.len = ngx_sock_ntop(c->sockaddr,
c->addr_text.data,
+                                                 ls->addr_text_max_len, 0);
+                if (c->addr_text.len == 0) {
+                    ngx_close_accepted_connection(c);
+                    return;
+                }
+
+            } else {
+                /*
+                 * Linux accept/accept4 syscalls, for example, do not return
+                 * address data upon unix domain sockets
+                 */

-            c->addr_text.len = ngx_sock_ntop(c->sockaddr, c->addr_text.data,
-                                             ls->addr_text_max_len, 0);
-            if (c->addr_text.len == 0) {
-                ngx_close_accepted_connection(c);
-                return;
+                c->addr_text.data = NULL;
+                c->addr_text.len = 0;
             }
         }

-agentzh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nginx-1.4.1-unix_socket_accept_over_read.patch
Type: application/octet-stream
Size: 1669 bytes
Desc: not available
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20130710/adbacda0/attachment.obj>