allow-recursion could incorrectly inherit from the default allow-query

diff --git a/CHANGES b/CHANGES
index 2a03c7b511f8dcb07bd99a8f94bd914de39f8f87..5019454462d52ab7c1223a485709d4ff3f945193 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -22,7 +22,12 @@
 4961.  [protocol]      Remove support for ECC-GOST (GOST R 34.11-94).
                        [GL #295]
 
-4960.  [placeholder]
+4960.  [security]      When recursion is enabled, but the "allow-recursion"
+                       and "allow-query-cache" ACLs are not specified,
+                       they should be limited to local networks,
+                       but were inadvertently set to match the default
+                       "allow-query", thus allowing remote queries.
+                       (CVE-2018-5738) [GL #309]
 
 4959.  [func]          NSID logging (enabled by the "request-nsid" option)
                        now has its own "nsid" category, instead of using the

diff --git a/bin/named/server.c b/bin/named/server.c
index 1f827a8700c612d257f3031a8994deaa7bec5007..4121d1f9ed6b229b21bf0a543e7db8c914c83a93 100644 (file)
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -3725,10 +3725,6 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist,
        CHECKM(named_config_getport(config, &port), "port");
        dns_view_setdstport(view, port);
 
-       CHECK(configure_view_acl(vconfig, config, named_g_config,
-                                "allow-query", NULL, actx,
-                                named_g_mctx, &view->queryacl));
-
        /*
         * Make the list of response policy zone names for a view that
         * is used for real lookups and so cares about hints.
@@ -4697,21 +4693,35 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist,
                                 "allow-query-cache-on", NULL, actx,
                                 named_g_mctx, &view->cacheonacl));
        /*
-        * Set "allow-query-cache", "allow-recursion", and
-        * "allow-recursion-on" acls if configured in named.conf.
-        * (Ignore the global defaults for now, because these ACLs
-        * can inherit from each other when only some of them set at
-        * the options/view level.)
+        * Set the "allow-query", "allow-query-cache", "allow-recursion",
+        * and "allow-recursion-on" ACLs if configured in named.conf, but
+        * NOT from the global defaults. This is done by leaving the third
+        * argument to configure_view_acl() NULL.
+        *
+        * We ignore the global defaults here because these ACLs
+        * can inherit from each other.  If any are still unset after
+        * applying the inheritance rules, we'll look up the defaults at
+        * that time.
         */
-       CHECK(configure_view_acl(vconfig, config, NULL, "allow-query-cache",
-                                NULL, actx, named_g_mctx, &view->cacheacl));
+
+       /* named.conf only */
+       CHECK(configure_view_acl(vconfig, config, NULL,
+                                "allow-query", NULL, actx,
+                                named_g_mctx, &view->queryacl));
+
+       /* named.conf only */
+       CHECK(configure_view_acl(vconfig, config, NULL,
+                                "allow-query-cache", NULL, actx,
+                                named_g_mctx, &view->cacheacl));
 
        if (strcmp(view->name, "_bind") != 0 &&
            view->rdclass != dns_rdataclass_chaos)
        {
+               /* named.conf only */
                CHECK(configure_view_acl(vconfig, config, NULL,
                                         "allow-recursion", NULL, actx,
                                         named_g_mctx, &view->recursionacl));
+               /* named.conf only */
                CHECK(configure_view_acl(vconfig, config, NULL,
                                         "allow-recursion-on", NULL, actx,
                                         named_g_mctx, &view->recursiononacl));
@@ -4749,18 +4759,21 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist,
                 * the global config.
                 */
                if (view->recursionacl == NULL) {
+                       /* global default only */
                        CHECK(configure_view_acl(NULL, NULL, named_g_config,
                                                 "allow-recursion", NULL,
                                                 actx, named_g_mctx,
                                                 &view->recursionacl));
                }
                if (view->recursiononacl == NULL) {
+                       /* global default only */
                        CHECK(configure_view_acl(NULL, NULL, named_g_config,
                                                 "allow-recursion-on", NULL,
                                                 actx, named_g_mctx,
                                                 &view->recursiononacl));
                }
                if (view->cacheacl == NULL) {
+                       /* global default only */
                        CHECK(configure_view_acl(NULL, NULL, named_g_config,
                                                 "allow-query-cache", NULL,
                                                 actx, named_g_mctx,
@@ -4774,6 +4787,14 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist,
                CHECK(dns_acl_none(mctx, &view->cacheacl));
        }
 
+       if (view->queryacl == NULL) {
+               /* global default only */
+               CHECK(configure_view_acl(NULL, NULL, named_g_config,
+                                        "allow-query", NULL,
+                                        actx, named_g_mctx,
+                                        &view->queryacl));
+       }
+
        /*
         * Ignore case when compressing responses to the specified
         * clients. This causes case not always to be preserved,

diff --git a/doc/arm/notes.xml b/doc/arm/notes.xml
index 1c7085fda4ea5364fab62960c10c0a3b19f32e4e..36c056e93acf90f90adab14e04845bc278b46157 100644 (file)
--- a/doc/arm/notes.xml
+++ b/doc/arm/notes.xml
@@ -65,7 +65,11 @@
     <itemizedlist>
       <listitem>
        <para>
-         None.
+         When recursion is enabled but the <command>allow-recursion</command>
+         and <command>allow-query-cache</command> ACLs are not specified, they
+         should be limited to local networks, but they were inadvertently set
+         to match the default <command>allow-query</command>, thus allowing
+         remote queries. This flaw is disclosed in CVE-2018-5738. [GL #309]
        </para>
       </listitem>
     </itemizedlist>
@@ -89,7 +93,7 @@
          information about root key rollover status can be gathered.
          To disable this feature, add
          <command>root-key-sentinel no;</command> to
-         <filename>named.conf</filename>.
+         <filename>named.conf</filename>. [GL #37]
        </para>
       </listitem>
       <listitem>