incorrect handling of bind option on wildcard listen addresses

[Roman Odaisky]

Given the configuration

listen 10.2.2.2:80;
...
listen *:80;

or alternatively,

listen [2001:db8::42]:80;
...
listen [::]:80;

or any other configuration where a wildcard listen directive is present along with non-wildcard entries for the same port, nginx may fail to perform bind() correctly only once, depending on the order in which it encounters the directives. This is due to faulty sorting in ngx_http_optimize_servers according to ngx_http_cmp_conf_addrs criterion, which is not guaranteed to place the wildcard entry/entries at the end of the list as expected by ngx_http_init_listening. Eventually the problem causes EADDRINUSE upon startup due to multiple bind() calls. The following simple patch fixes the problem, presuming the wildcard field always equals 0 or 1.

src/http/ngx_http.c

@@ -1608 +1608 @@
     first = (ngx_http_conf_addr_t *) one;
     second = (ngx_http_conf_addr_t *) two;
 
-    if (first->opt.wildcard) {
+    if (first->opt.wildcard != second->opt.wildcard) {
         /* a wildcard address must be the last resort, shift it to the end */
-        return 1;
+        return (int)first->opt.wildcard - (int)second->opt.wildcard;
     }
 
     if (first->opt.bind && !second->opt.bind) {

--
HTH
Roman Odaisky

[Ruslan Ermilov]

I can't reproduce this. Is this actually exploitable?

ngx_sort() does an "insertion sort", and even though the comparator may erroneously report that "first == *" where "*" is a wildcard address, "*" will eventually be checked via the "first" parameter, and moved appropriately.

[Roman Odaisky]

Most definitely. I encountered this in an actual setup. Minimal example to reproduce:

server {
    listen 127.0.0.1:58080;
}

server {
    listen 58080 sndbuf=42;
}

Swap the entries in the configuration file and everything works correctly. Otherwise, the comparator is invoked exactly once, sees the bind option on the right side, fails to see the wildcard option on the right side, and erroneously returns -1, causing swapping of the entries.

The problem is triggered when addresses in listen directives differ and when socket options are present. I would guess this is rare with IPv4, but will be commonplace with IPv6 (due to the ipv6only=on socket option and multitude of available addresses).

The usefulness of IP-based virtual hosts is obvious, DNS flexibility.

[Maxim Dounin]

Ok, thanks, the problem is clear. It's in fact due to bind option (in your above config - implicitly set by "sndbuf" parameter) was not ignored for wildcard addresses, and caused separate bind on wildcard address. I would suggest the following patch, which looks more logical for me:

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1342179498 -14400
# Node ID c2f751648432a57fe0e9c88ea1ca313f5fa5c8d8
# Parent  5f74b296240eceebefa97009e9f1e1f62cf4e80a
Ignore bind option on wildcard addresses (ticket #187).

Without this patch config

    server { listen 127.0.0.1:58080; }
    server { listen 58080 sndbuf=42; }

resulted in two binds, while the opposite order

    server { listen 58080 sndbuf=42; }
    server { listen 127.0.0.1:58080; }

in only wildcard one.  This happened due to the "bind" option set by
the "sndbuf" parameter, which caused wrong order after sorting with
ngx_http_cmp_conf_addrs().

diff --git a/src/http/ngx_http.c b/src/http/ngx_http.c
--- a/src/http/ngx_http.c
+++ b/src/http/ngx_http.c
@@ -1618,7 +1618,7 @@ ngx_http_cmp_conf_addrs(const void *one,
         return -1;
     }
 
-    if (!first->opt.bind && second->opt.bind) {
+    if (!first->opt.bind && second->opt.bind && !second->opt.wildcard) {
         /* shift explicit bind()ed addresses to the start */
         return 1;
     }

[Roman Odaisky]

This makes the comparator fail the strict antisymmetry property (cmp(a, b) = -1 <=> cmp(b, a) = 1), which has the potential to break the sorting in subtle ways (do you think it was easy to discover the problem in the first place?).

My patch, the only function of which is restoring antisymmetry on the wildcard comparison, causes the entries to be sorted as the code originally intended:

Non-wildcard, bind
Non-wildcard, non-bind
Wildcard, bind
Wildcard, non-bind

(in other words, ORDER BY wildcard ASC, bind DESC), which, if I understand the code correctly, is right. If another ordering is required, then the proper procedure is to specify the ordering, then implement the comparator to satisfy strict weak ordering requirements (the comparator got this right with the bind option but not with the wildcard option).

So I suggest either the original patch, or to handle the wildcard option like bind:

src/http/ngx_http.c

@@ -1608 +1608 @@
     first = (ngx_http_conf_addr_t *) one;
     second = (ngx_http_conf_addr_t *) two;
 
-    if (first->opt.wildcard) {
+    if (first->opt.wildcard && !second->opt.wildcard) {
         /* a wildcard address must be the last resort, shift it to the end */
         return 1;
     }
 
+    if (!first->opt.wildcard && second->opt.wildcard) {
+        return -1;
+    }
+
     if (first->opt.bind && !second->opt.bind) {
         /* shift explicit bind()ed addresses to the start */
         return -1;

[Ruslan Ermilov]

In 4756/nginx:

(The changeset message doesn't reference this ticket)

[Maxim Dounin]

In 4793/nginx:

(The changeset message doesn't reference this ticket)

[Ruslan Ermilov]

In 4818/nginx:

(The changeset message doesn't reference this ticket)

[Maxim Dounin]

In 4854/nginx:

(The changeset message doesn't reference this ticket)