cifs.upcall: make using ip address conditional on new option

Igor Mammedov pointed out that reverse resolving an IP address to get
the hostname portion of a principal could open a possible attack
vector. If an attacker were to gain control of DNS, then he could
redirect the mount to a server of his choosing, and fix the reverse
resolution to point to a hostname of his choosing (one where he has
the key for the corresponding cifs/ or host/ principal).

That said, we often trust DNS for other reasons and it can be useful
to do so. Make the code that allows trusting DNS to be enabled by
adding --trust-dns to the cifs.upcall invocation.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
(cherry picked from commit 6aa0f05509ec1b8578021051f83627f4ca296ef8)

2 files changed, 50 insertions, 27 deletions

diff --git a/docs-xml/manpages-3/cifs.upcall.8.xml b/docs-xml/manpages-3/cifs.upcall.8.xml
index 56a12adf126..a60dd9d461d 100644
--- a/docs-xml/manpages-3/cifs.upcall.8.xml
+++ b/docs-xml/manpages-3/cifs.upcall.8.xml
@@ -19,8 +19,8 @@
 <refsynopsisdiv>
         <cmdsynopsis>
                 <command>cifs.upcall</command>
-                <arg choice="opt">-c</arg>
-                <arg choice="opt">-v</arg>
+                <arg choice="opt">--trust-dns|-t</arg>
+                <arg choice="opt">--version|-v</arg>
                 <arg choice="req">keyid</arg>
         </cmdsynopsis>
 </refsynopsisdiv>
@@ -51,9 +51,14 @@ to be run that way.</para>
 		<listitem><para>This option is deprecated and is currently ignored.
 		</para></listitem>
 		</varlistentry>
-
 		<varlistentry>
-		<term>-v</term>
+		<term>--trust-dns|-t</term>
+		<listitem><para>With krb5 upcalls, the name used as the host portion of the service principal defaults to the hostname portion of the UNC. This option allows the upcall program to reverse resolve the network address of the server in order to get the hostname.</para>
+		<para>This is less secure than not trusting DNS. When using this option, it's possible that an attacker could get control of DNS and trick the client into mounting a different server altogether. It's preferable to instead add server principals to the KDC for every possible hostname, but this option exists for cases where that isn't possible. The default is to not trust reverse hostname lookups in this fashion.
+		</para></listitem>
+		</varlistentry>
+		<varlistentry>
+		<term>--version|-v</term>
 		<listitem><para>Print version number and exit.
 		</para></listitem>
 		</varlistentry>
@@ -85,7 +90,7 @@ to be run that way.</para>
 	<para>To make this program useful for CIFS, you'll need to set up entries for them in request-key.conf<manvolnum>5</manvolnum>. Here's an example of an entry for each key type:</para>
 <programlisting>
 #OPERATION  TYPE           D C PROGRAM ARG1 ARG2...
-#=========  =============  = = ==========================================
+#=========  =============  = = ================================
 create      cifs.spnego    * * /usr/local/sbin/cifs.upcall %k
 create      dns_resolver   * * /usr/local/sbin/cifs.upcall %k
 </programlisting>
diff --git a/source/client/cifs.upcall.c b/source/client/cifs.upcall.c
index 1a35854cb5d..a04fca23942 100644
--- a/source/client/cifs.upcall.c
+++ b/source/client/cifs.upcall.c
@@ -154,9 +154,9 @@ handle_krb5_mech(const char *oid, const char *principal, DATA_BLOB *secblob,
 #define DKD_HAVE_IP		0x8
 #define DKD_HAVE_UID		0x10
 #define DKD_HAVE_PID		0x20
-#define DKD_MUSTHAVE_SET (DKD_HAVE_IP|DKD_HAVE_VERSION|DKD_HAVE_SEC)
+#define DKD_MUSTHAVE_SET (DKD_HAVE_HOSTNAME|DKD_HAVE_VERSION|DKD_HAVE_SEC)
 
-static struct decoded_args {
+struct decoded_args {
 	int		ver;
 	char		*hostname;
 	char		*ip;
@@ -354,11 +354,12 @@ ip_to_fqdn(const char *addrstr, char *host, size_t hostlen)
 static void
 usage(void)
 {
-	syslog(LOG_INFO, "Usage: %s [-v] key_serial", prog);
-	fprintf(stderr, "Usage: %s [-v] key_serial\n", prog);
+	syslog(LOG_INFO, "Usage: %s [-t] [-v] key_serial", prog);
+	fprintf(stderr, "Usage: %s [-t] [-v] key_serial\n", prog);
 }
 
 const struct option long_options[] = {
+	{ "trust-dns",	0, NULL, 't' },
 	{ "version",	0, NULL, 'v' },
 	{ NULL,		0, NULL, 0 }
 };
@@ -372,19 +373,24 @@ int main(const int argc, char *const argv[])
 	size_t datalen;
 	unsigned int have;
 	long rc = 1;
-	int c;
-	char *buf, *princ, *ccname = NULL;
-	char hostbuf[NI_MAXHOST];
+	int c, try_dns = 0;
+	char *buf, *princ = NULL, *ccname = NULL;
+	char hostbuf[NI_MAXHOST], *host;
 	struct decoded_args arg = { };
 	const char *oid;
 
+	hostbuf[0] = '\0';
+
 	openlog(prog, 0, LOG_DAEMON);
 
-	while ((c = getopt_long(argc, argv, "cv", long_options, NULL)) != -1) {
+	while ((c = getopt_long(argc, argv, "ctv", long_options, NULL)) != -1) {
 		switch (c) {
 		case 'c':
 			/* legacy option -- skip it */
 			break;
+		case 't':
+			try_dns++;
+			break;
 		case 'v':
 			printf("version: %s\n", CIFSSPNEGO_VERSION);
 			goto out;
@@ -452,21 +458,18 @@ int main(const int argc, char *const argv[])
 	if (have & DKD_HAVE_PID)
 		ccname = get_krb5_ccname(arg.pid);
 
-	if (have & DKD_HAVE_IP) {
-		rc = ip_to_fqdn(arg.ip, hostbuf, sizeof(hostbuf));
-		if (rc)
-			goto out;
-	}
+	host = arg.hostname;
 
 	// do mech specific authorization
 	switch (arg.sec) {
 	case MS_KRB5:
 	case KRB5:
+retry_new_hostname:
 		/* for "cifs/" service name + terminating 0 */
-		datalen = strnlen(hostbuf, sizeof(hostbuf)) + 5 + 1;
+		datalen = strlen(host) + 5 + 1;
 		princ = SMB_XMALLOC_ARRAY(char, datalen);
 		if (!princ) {
-			rc = 1;
+			rc = -ENOMEM;
 			break;
 		}
 
@@ -480,21 +483,35 @@ int main(const int argc, char *const argv[])
 		 * getting a host/ principal if that doesn't work.
 		 */
 		strlcpy(princ, "cifs/", datalen);
-		strlcpy(princ + 5, hostbuf, datalen - 5);
+		strlcpy(princ + 5, host, datalen - 5);
 		rc = handle_krb5_mech(oid, princ, &secblob, &sess_key, ccname);
-		if (rc) {
-			memcpy(princ, "host/", 5);
-			rc = handle_krb5_mech(oid, princ, &secblob, &sess_key,
-						ccname);
-		}
+		if (!rc)
+			break;
+
+		memcpy(princ, "host/", 5);
+		rc = handle_krb5_mech(oid, princ, &secblob, &sess_key, ccname);
+		if (!rc)
+			break;
+
+		if (!try_dns || !(have & DKD_HAVE_IP))
+			break;
+
+		rc = ip_to_fqdn(arg.ip, hostbuf, sizeof(hostbuf));
+		if (rc)
+			break;
+
 		SAFE_FREE(princ);
-		break;
+		try_dns = 0;
+		host = hostbuf;
+		goto retry_new_hostname;
 	default:
 		syslog(LOG_ERR, "sectype: %d is not implemented", arg.sec);
 		rc = 1;
 		break;
 	}
 
+	SAFE_FREE(princ);
+
 	if (rc)
 		goto out;
 
@@ -536,6 +553,7 @@ out:
 	data_blob_free(&sess_key);
 	SAFE_FREE(ccname);
 	SAFE_FREE(arg.hostname);
+	SAFE_FREE(arg.ip);
 	SAFE_FREE(keydata);
 	return rc;
 }