From 9e0346b707862c5f61331d99db4f3d78f223e5c7 Mon Sep 17 00:00:00 2001
From: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
Date: Sat, 25 Mar 2017 15:36:06 -0500
Subject: [PATCH] Implement sensible default EDNS(0) padding policy.

At NDSS 2017's DNS privacy workshop, I presented an empirical study of
DNS padding policies:

https://www.internetsociety.org/events/ndss-symposium/ndss-symposium-2017/dns-privacy-workshop-2017-programme#session3

The slide deck is here:
https://dns.cmrg.net/ndss2017-dprive-empirical-DNS-traffic-size.pdf

The resulting recommendation from the research is that a simple
padding policy is relatively cheap and still protective of metadata
when DNS traffic is encrypted:

 * queries should be padded to a multiple of 128 octets
 * responses should be padded to a multiple of 468 octets

Since future research could propose even better policies, and future
DNS traffic characteristics might evolve, I've implemented this
recommendation as a new function in libknot:
knot_edns_default_padding_size()

This changeset also modifies kdig to use this padding policy by
default when doing queries over TLS, and defines +padding (with no
argument) as a kdig option that forces the use of the default padding
policy.

With this changeset, any libknot user who wants to use "a sensible DNS
padding policy" can just rely on the library; this means that if a
better padding policy is determined in the future, it can be
distributed to all users by upgrading libknot.
---
 doc/man/kdig.1in             |  8 ++++++--
 doc/man_kdig.rst             |  8 ++++++--
 src/libknot/packet/pkt.h     |  3 ---
 src/libknot/rrtype/opt.c     | 17 +++++++++++++++++
 src/libknot/rrtype/opt.h     | 14 ++++++++++++++
 src/utils/kdig/kdig_exec.c   | 14 ++++++++++++--
 src/utils/kdig/kdig_params.c | 30 +++++++++++++++++-------------
 src/utils/kdig/kdig_params.h |  2 +-
 8 files changed, 73 insertions(+), 23 deletions(-)

diff --git a/doc/man/kdig.1in b/doc/man/kdig.1in
index 5973db126f..1cb4fb7ed0 100644
--- a/doc/man/kdig.1in
+++ b/doc/man/kdig.1in
@@ -222,8 +222,12 @@ Request the nameserver identifier (NSID).
 \fB+\fP[\fBno\fP]\fBbufsize\fP=\fIB\fP
 Set EDNS buffer size in bytes (default is 512 bytes).
 .TP
-\fB+\fP[\fBno\fP]\fBpadding\fP=\fIB\fP
-Set EDNS(0) padding option data length (default is no).
+\fB+\fP[\fBno\fP]\fBpadding\fP[=\fIB\fP]
+Use EDNS(0) padding option to pad queries, optionally to a specific
+size. The default is to pad queries with a sensible amount when using
++tls, and not to pad at all when queries are sent without TLS.  With
+no argument (i.e., just +padding) pad every query with a sensible
+amount regardless of the use of TLS. With +nopadding, never pad.
 .TP
 \fB+\fP[\fBno\fP]\fBalignment\fP[=\fIB\fP]
 Align the query to B\-byte\-block message using the EDNS(0) padding option
diff --git a/doc/man_kdig.rst b/doc/man_kdig.rst
index 84c00f6fca..b5d4147760 100644
--- a/doc/man_kdig.rst
+++ b/doc/man_kdig.rst
@@ -199,8 +199,12 @@ Options
 **+**\ [\ **no**\ ]\ **bufsize**\ =\ *B*
   Set EDNS buffer size in bytes (default is 512 bytes).
 
-**+**\ [\ **no**\ ]\ **padding**\ =\ *B*
-  Set EDNS(0) padding option data length (default is no).
+**+**\ [\ **no**\ ]\ **padding**\[\ =\ *B*\]
+  Use EDNS(0) padding option to pad queries, optionally to a specific
+  size. The default is to pad queries with a sensible amount when using
+  +tls, and not to pad at all when queries are sent without TLS.  With
+  no argument (i.e., just +padding) pad every query with a sensible
+  amount regardless of the use of TLS. With +nopadding, never pad.
 
 **+**\ [\ **no**\ ]\ **alignment**\[\ =\ *B*\]
   Align the query to B\-byte-block message using the EDNS(0) padding option
diff --git a/src/libknot/packet/pkt.h b/src/libknot/packet/pkt.h
index adc552ade3..5df3c1eab5 100644
--- a/src/libknot/packet/pkt.h
+++ b/src/libknot/packet/pkt.h
@@ -38,9 +38,6 @@
 /* Number of packet sections (ANSWER, AUTHORITY, ADDITIONAL). */
 #define KNOT_PKT_SECTIONS 3
 
-/* Forward decls */
-struct knot_pkt;
-
 /*!
  * \brief Packet flags.
  */
diff --git a/src/libknot/rrtype/opt.c b/src/libknot/rrtype/opt.c
index e7d1fd737a..096a934539 100644
--- a/src/libknot/rrtype/opt.c
+++ b/src/libknot/rrtype/opt.c
@@ -25,6 +25,7 @@
 #include "libknot/consts.h"
 #include "libknot/descriptor.h"
 #include "libknot/rrtype/opt.h"
+#include "libknot/packet/pkt.h"
 #include "contrib/wire.h"
 #include "contrib/wire_ctx.h"
 
@@ -46,6 +47,9 @@ enum knot_edns_private_consts {
 	EDNS_OFFSET_CLIENT_SUBNET_DST_MASK = 3,
 	/*! \brief Byte offset of the address field in option data. */
 	EDNS_OFFSET_CLIENT_SUBNET_ADDR     = 4,
+
+	EDNS_DEFAULT_QUERY_ALIGNMENT_SIZE    = 128,
+	EDNS_DEFAULT_RESPONSE_ALIGNMENT_SIZE = 468,
 };
 
 _public_
@@ -507,6 +511,19 @@ bool knot_edns_check_record(knot_rrset_t *opt_rr)
 	return wire.error == KNOT_EOK;
 }
 
+_public_
+int knot_edns_default_padding_size(const knot_pkt_t *pkt,
+                                   const knot_rrset_t *opt_rr)
+{
+	if (knot_wire_get_qr(pkt->wire)) {
+		return knot_edns_alignment_size(pkt->size, knot_rrset_size(opt_rr),
+		                                EDNS_DEFAULT_RESPONSE_ALIGNMENT_SIZE);
+	} else {
+		return knot_edns_alignment_size(pkt->size, knot_rrset_size(opt_rr),
+		                                EDNS_DEFAULT_QUERY_ALIGNMENT_SIZE);
+	}
+}
+
 /*----------------------------------------------------------------------------*/
 
 /*!
diff --git a/src/libknot/rrtype/opt.h b/src/libknot/rrtype/opt.h
index 023d3a69dd..f7661c3763 100644
--- a/src/libknot/rrtype/opt.h
+++ b/src/libknot/rrtype/opt.h
@@ -29,6 +29,9 @@
 #include "libknot/consts.h"
 #include "libknot/rrset.h"
 
+/* Forward decls */
+typedef struct knot_pkt knot_pkt_t;
+
 /*! \brief Constants related to EDNS. */
 enum knot_edns_const {
 	/*! \brief Supported EDNS version. */
@@ -354,6 +357,17 @@ bool knot_edns_has_nsid(const knot_rrset_t *opt_rr);
  */
 bool knot_edns_check_record(knot_rrset_t *opt_rr);
 
+/*!
+ * \brief Computes a reasonable Padding data length for a given packet and opt RR.
+ *
+ * \param pkt     DNS Packet prepared and otherwise ready to go, no OPT yet added.
+ * \param opt_rr  OPT RR, not yet including padding.
+ *
+ * \return Required padding length or -1 if padding not required.
+ */
+int knot_edns_default_padding_size(const knot_pkt_t *pkt,
+                                   const knot_rrset_t *opt_rr);
+
 /*!
  * \brief Computes additional Padding data length for required packet alignment.
  *
diff --git a/src/utils/kdig/kdig_exec.c b/src/utils/kdig/kdig_exec.c
index f5483b59db..422c31b903 100644
--- a/src/utils/kdig/kdig_exec.c
+++ b/src/utils/kdig/kdig_exec.c
@@ -268,10 +268,12 @@ static int add_query_edns(knot_pkt_t *packet, const query_t *query, uint16_t max
 
 	/* Append EDNS Padding. */
 	int padding = query->padding;
-	if (query->alignment > 0) {
+	if (padding != -3 && query->alignment > 0) {
 		padding = knot_edns_alignment_size(packet->size,
 		                                   knot_rrset_size(&opt_rr),
 		                                   query->alignment);
+	} else if (query->padding == -2 || (query->padding == -1 && query->tls.enable)) {
+		padding = knot_edns_default_padding_size(packet, &opt_rr);
 	}
 	if (padding > -1) {
 		uint8_t zeros[padding];
@@ -294,11 +296,19 @@ static int add_query_edns(knot_pkt_t *packet, const query_t *query, uint16_t max
 	return ret;
 }
 
+static bool do_padding(const query_t *query)
+{
+	return (query->padding != -3) &&                       // Disabled padding.
+	       (query->padding > -1 || query->alignment > 0 || // Explicit padding.
+	        query->padding == -2 ||                        // Default padding.
+	        (query->padding == -1 && query->tls.enable));  // TLS automatic.
+}
+
 static bool use_edns(const query_t *query)
 {
 	return query->edns > -1 || query->udp_size > -1 || query->nsid ||
 	       query->flags.do_flag || query->subnet != NULL ||
-	       query->padding > -1 || query->alignment > 0;
+	       do_padding(query);
 }
 
 static knot_pkt_t *create_query_packet(const query_t *query)
diff --git a/src/utils/kdig/kdig_params.c b/src/utils/kdig/kdig_params.c
index 00d981246c..1d354572eb 100644
--- a/src/utils/kdig/kdig_params.c
+++ b/src/utils/kdig/kdig_params.c
@@ -711,22 +711,26 @@ static int opt_padding(const char *arg, void *query)
 {
 	query_t *q = query;
 
-	uint16_t num;
-	if (str_to_u16(arg, &num) != KNOT_EOK) {
-		ERR("invalid +padding=%s\n", arg);
-		return KNOT_EINVAL;
-	}
-
-	q->padding = num;
+	if (arg == NULL) {
+		q->padding = -2;
+		return KNOT_EOK;
+	} else {
+		uint16_t num = 0;
+		if (str_to_u16(arg, &num) != KNOT_EOK) {
+			ERR("invalid +padding=%s\n", arg);
+			return KNOT_EINVAL;
+		}
 
-	return KNOT_EOK;
+		q->padding = num;
+		return KNOT_EOK;
+	}
 }
 
 static int opt_nopadding(const char *arg, void *query)
 {
 	query_t *q = query;
 
-	q->padding = -1;
+	q->padding = -3;
 
 	return KNOT_EOK;
 }
@@ -1016,7 +1020,7 @@ static const param_t kdig_opts2[] = {
 	{ "bufsize",        ARG_REQUIRED, opt_bufsize },
 	{ "nobufsize",      ARG_NONE,     opt_nobufsize },
 
-	{ "padding",        ARG_REQUIRED, opt_padding },
+	{ "padding",        ARG_OPTIONAL, opt_padding },
 	{ "nopadding",      ARG_NONE,     opt_nopadding },
 
 	{ "alignment",      ARG_OPTIONAL, opt_alignment },
@@ -1615,8 +1619,8 @@ static void print_help(void)
 	       "       +[no]tls-hostname=STR Use TLS with remote server hostname.\n"
 	       "       +[no]nsid             Request NSID.\n"
 	       "       +[no]bufsize=B        Set EDNS buffer size.\n"
-	       "       +[no]padding=N        Padding block size EDNS(0) padding.\n"
-	       "       +[no]alignment[=N]    Set packet alignment with EDNS(0) padding.\n"
+	       "       +[no]padding[=N]      Pad with EDNS(0) (default or specify size).\n"
+	       "       +[no]alignment[=N]    Pad with EDNS(0) to blocksize (%u or specify size).\n"
 	       "       +[no]subnet=SUBN      Set EDNS(0) client subnet addr/prefix.\n"
 	       "       +[no]edns[=N]         Use EDNS(=version).\n"
 	       "       +[no]time=T           Set wait for reply interval in seconds.\n"
@@ -1625,7 +1629,7 @@ static void print_help(void)
 	       "\n"
 	       "       -h, --help            Print the program help.\n"
 	       "       -V, --version         Print the program version.\n",
-	       PROGRAM_NAME);
+	       PROGRAM_NAME, DEFAULT_ALIGNMENT_SIZE);
 }
 
 static int parse_opt1(const char *opt, const char *value, kdig_params_t *params,
diff --git a/src/utils/kdig/kdig_params.h b/src/utils/kdig/kdig_params.h
index 2b19f94c6e..5a35fcc9bf 100644
--- a/src/utils/kdig/kdig_params.h
+++ b/src/utils/kdig/kdig_params.h
@@ -120,7 +120,7 @@ struct query {
 	knot_tsig_key_t tsig_key;
 	/*!< EDNS client subnet. */
 	knot_edns_client_subnet_t *subnet;
-	/*!< EDNS0 padding (16unsigned + -1 uninitialized). */
+	/*!< EDNS0 padding (16unsigned + -1 ~ uninitialized, -2 ~ default, -3 ~ none). */
 	int32_t		padding;
 	/*!< Query alignment with EDNS0 padding (0 ~ uninitialized). */
 	uint16_t	alignment;
-- 
GitLab