Opened 2 years ago

Closed 2 years ago

Last modified 22 months ago

#187 closed defect (fixed)

incorrect handling of bind option on wildcard listen addresses

Reported by: Roman Odaisky Owned by: somebody
Priority: minor Milestone:
Component: nginx-core Version: 1.3.x
Keywords: Cc:
Sensitive:
uname -a: N/A
nginx -V: nginx/1.2.1 — fix tested; nginx/1.3.3 — code unchanged, issue presumed to persist.

Description

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.

--- a/src/http/ngx_http.c 2012-07-11 22:17:51.000000000 +0000
+++ b/src/http/ngx_http.c 2012-07-11 22:18:15.000000000 +0000
@@ -1608,9 +1608,9 @@
     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

Change History (9)

comment:1 Changed 2 years ago by 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.

comment:2 Changed 2 years ago by 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.

comment:3 Changed 2 years ago by 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;
     }

comment:4 Changed 2 years ago by Maxim Dounin

  • Priority changed from major to minor
  • Status changed from new to accepted
  • Summary changed from [patch] Failure to detect wildcard listening to incorrect handling of bind option on wildcard listen addresses

comment:5 Changed 2 years ago by 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:

--- a/src/http/ngx_http.c 2012-04-03 07:37:31.000000000 +0000
+++ b/src/http/ngx_http.c 2012-07-13 12:58:05.000000000 +0000
@@ -1608,11 +1608,15 @@
     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;

comment:6 Changed 2 years ago by Ruslan Ermilov

  • Resolution set to fixed
  • Status changed from accepted to closed

In 4756/nginx:

(The changeset message doesn't reference this ticket)

comment:7 Changed 2 years ago by Maxim Dounin

In 4793/nginx:

(The changeset message doesn't reference this ticket)

comment:8 Changed 2 years ago by Ruslan Ermilov

In 4818/nginx:

(The changeset message doesn't reference this ticket)

comment:9 Changed 22 months ago by Maxim Dounin

In 4854/nginx:

(The changeset message doesn't reference this ticket)

Note: See TracTickets for help on using tickets.