15001 Unable to set smb property max_protocol to 3.1.1 or empty

Reviewed-by: Jerry Jelinek <gjelinek@racktopsystems.com>
Reviewed by: Albert Lee <alee@racktopsystems.com>
Reviewed-by: Jim Johnson <jjohnson@racktopsystems.com>
Reviewed by: Joyce McIntosh <jmcintosh@racktopsystems.com>
Reviewed by: Matt Barden <mbarden@tintri.com>
Approved by: Dan McDonald <danmcd@mnx.io>

4 files changed, 108 insertions, 204 deletions

diff --git a/usr/src/lib/libshare/smb/libshare_smb.c b/usr/src/lib/libshare/smb/libshare_smb.c
index da14ea94c3..b926637def 100644
--- a/usr/src/lib/libshare/smb/libshare_smb.c
+++ b/usr/src/lib/libshare/smb/libshare_smb.c
@@ -890,12 +890,12 @@ struct smb_proto_option_defs {
 	    SMB_REFRESH_REFRESH },
 	{ SMB_CI_DISPOSITION, 0, MAX_VALUE_BUFLEN,
 	    disposition_validator, SMB_REFRESH_REFRESH },
+	{ SMB_CI_MIN_PROTOCOL, 0, MAX_VALUE_BUFLEN, protocol_validator,
+	    SMB_REFRESH_REFRESH },
 	{ SMB_CI_MAX_PROTOCOL, 0, MAX_VALUE_BUFLEN, protocol_validator,
 	    SMB_REFRESH_REFRESH },
 	{ SMB_CI_ENCRYPT, 0, MAX_VALUE_BUFLEN, require_validator,
 	    SMB_REFRESH_REFRESH },
-	{ SMB_CI_MIN_PROTOCOL, 0, MAX_VALUE_BUFLEN, protocol_validator,
-	    SMB_REFRESH_REFRESH },
 	{ SMB_CI_BYPASS_TRAVERSE_CHECKING, 0, 0, true_false_validator,
 	    SMB_REFRESH_REFRESH },
 	{ SMB_CI_OPLOCK_ENABLE, 0, 0, true_false_validator,
@@ -1490,27 +1490,6 @@ smb_enable_service(void)
 }
 
 /*
- * smb_validate_proto_prop(index, name, value)
- *
- * Verify that the property specified by name can take the new
- * value. This is a sanity check to prevent bad values getting into
- * the default files.
- */
-static int
-smb_validate_proto_prop(int index, char *name, char *value)
-{
-	if ((name == NULL) || (index < 0))
-		return (SA_BAD_VALUE);
-
-	if (smb_proto_options[index].validator == NULL)
-		return (SA_OK);
-
-	if (smb_proto_options[index].validator(index, value) == SA_OK)
-		return (SA_OK);
-	return (SA_BAD_VALUE);
-}
-
-/*
  * smb_set_proto_prop(prop)
  *
  * check that prop is valid.
@@ -1527,35 +1506,50 @@ smb_set_proto_prop(sa_property_t prop)
 
 	name = sa_get_property_attr(prop, "type");
 	value = sa_get_property_attr(prop, "value");
-	if (name != NULL && value != NULL) {
-		index = findprotoopt(name);
-		if (index >= 0) {
-			/* should test for valid value */
-			ret = smb_validate_proto_prop(index, name, value);
-			if (ret == SA_OK) {
-				opt = &smb_proto_options[index];
-
-				/* Save to SMF */
-				if (smb_config_set(opt->smb_index,
-				    value) != 0) {
-					ret = SA_BAD_VALUE;
-					goto out;
-				}
-				/*
-				 * Specialized refresh mechanisms can
-				 * be flagged in the proto_options and
-				 * processed here.
-				 */
-				if (opt->refresh & SMB_REFRESH_REFRESH)
-					(void) smf_refresh_instance(
-					    SMBD_DEFAULT_INSTANCE_FMRI);
-				else if (opt->refresh & SMB_REFRESH_RESTART)
-					(void) smf_restart_instance(
-					    SMBD_DEFAULT_INSTANCE_FMRI);
-			}
-		}
+	if (name == NULL || value == NULL) {
+		ret = SA_NO_SUCH_PROP;
+		goto out;
+	}
+
+	index = findprotoopt(name);
+	if (index < 0) {
+		ret = SA_NO_SUCH_PROP;
+		goto out;
+	}
+	opt = &smb_proto_options[index];
+
+	/*
+	 * When setting max_protocol or min_protocol,
+	 * allow"3.1.1" as an alias for "3.11".
+	 */
+	if (opt->smb_index == SMB_CI_MAX_PROTOCOL ||
+	    opt->smb_index == SMB_CI_MIN_PROTOCOL)
+		if (strcmp(value, "3.1.1") == 0)
+			strcpy(value, "3.11");
+
+	/* Test for valid value */
+	if (opt->validator != NULL &&
+	    (ret = opt->validator(index, value)) != SA_OK)
+		goto out;
+
+	/* Save to SMF */
+	if (smb_config_set(opt->smb_index, value) != 0) {
+		ret = SA_BAD_VALUE;
+		goto out;
 	}
 
+	/*
+	 * Specialized refresh mechanisms can
+	 * be flagged in the proto_options and
+	 * processed here.
+	 */
+	if (opt->refresh & SMB_REFRESH_REFRESH)
+		(void) smf_refresh_instance(
+		    SMBD_DEFAULT_INSTANCE_FMRI);
+	else if (opt->refresh & SMB_REFRESH_RESTART)
+		(void) smf_restart_instance(
+		    SMBD_DEFAULT_INSTANCE_FMRI);
+
 out:
 	if (name != NULL)
 		sa_free_attr_string(name);
@@ -2366,21 +2360,69 @@ disposition_validator(int index, char *value)
 	return (SA_BAD_VALUE);
 }
 
-/*ARGSUSED*/
 static int
 protocol_validator(int index, char *value)
 {
+	struct smb_proto_option_defs *opt;
+	smb_cfg_val_t encrypt;
+	uint32_t max;
+	uint32_t min;
+	uint32_t val;
+
 	if (value == NULL)
 		return (SA_BAD_VALUE);
 
+	/* Allow setting back to empty (use defaults) */
 	if (*value == '\0')
 		return (SA_OK);
 
-	if (smb_config_check_protocol(value) == 0)
-		return (SA_OK);
+	val = smb_convert_version_str(value);
+	if (val == 0)
+		return (SA_BAD_VALUE);
 
-	return (SA_BAD_VALUE);
+	/*
+	 * We don't want people who care enough about protecting their data
+	 * by requiring encryption to accidentally expose their data by
+	 * lowering the max protocol, so prevent them from going below 3.0
+	 * if encryption is required.
+	 * Also, ensure that max_protocol >= min_protocol.
+	 */
+	opt = &smb_proto_options[index];
+	switch (opt->smb_index) {
 
+	case SMB_CI_MAX_PROTOCOL:
+
+		encrypt = smb_config_get_require(SMB_CI_ENCRYPT);
+		if (encrypt == SMB_CONFIG_REQUIRED && val < SMB_VERS_3_0) {
+			syslog(LOG_ERR, "Cannot set smbd/max_protocol below 3.0"
+			    " while smbd/encrypt == required.");
+			return (SA_VALUE_CONFLICT);
+		}
+		min = smb_config_get_min_protocol();
+		if (val < min) {
+			syslog(LOG_ERR, "Cannot set smbd/max_protocol to less"
+			    " than smbd/min_protocol.");
+			return (SA_VALUE_CONFLICT);
+		}
+		break;
+
+	case SMB_CI_MIN_PROTOCOL:
+
+		max = smb_config_get_max_protocol();
+		if (val > max) {
+			syslog(LOG_ERR, "Cannot set smbd/min_protocol to more"
+			    " than smbd/max_protocol.");
+			return (SA_VALUE_CONFLICT);
+		}
+		break;
+
+	default:
+		syslog(LOG_ERR, "Unexpected smb protocol validator index %d",
+		    opt->smb_index);
+		return (SA_BAD_VALUE);
+	}
+
+	return (SA_OK);
 }
 
 /*
diff --git a/usr/src/lib/smbsrv/libsmb/common/libsmb.h b/usr/src/lib/smbsrv/libsmb/common/libsmb.h
index 5b6a41cbf0..48336f2d73 100644
--- a/usr/src/lib/smbsrv/libsmb/common/libsmb.h
+++ b/usr/src/lib/smbsrv/libsmb/common/libsmb.h
@@ -220,9 +220,9 @@ extern void smb_config_get_version(smb_version_t *);
 uint32_t smb_config_get_execinfo(char *, char *, size_t);
 extern void smb_config_get_negtok(uchar_t *, uint32_t *);
 
-extern int smb_config_check_protocol(char *);
 extern uint32_t smb_config_get_max_protocol(void);
 extern uint32_t smb_config_get_min_protocol(void);
+extern uint32_t smb_convert_version_str(const char *);
 extern void smb_config_upgrade(void);
 extern uint16_t smb31_config_get_encrypt_cipher(void);
 
diff --git a/usr/src/lib/smbsrv/libsmb/common/mapfile-vers b/usr/src/lib/smbsrv/libsmb/common/mapfile-vers
index ddb0698522..9cb89eb509 100644
--- a/usr/src/lib/smbsrv/libsmb/common/mapfile-vers
+++ b/usr/src/lib/smbsrv/libsmb/common/mapfile-vers
@@ -98,13 +98,13 @@ SYMBOL_VERSION SUNWprivate {
 	smb_codepage_init;
 	smb_common_decode;
 	smb_common_encode;
-	smb_config_check_protocol;
 	smb_config_get;
 	smb_config_get_ads_enable;
 	smb_config_get_debug;
 	smb_config_get_execinfo;
 	smb_config_get_fg_flag;
 	smb_config_get_localsid;
+	smb_config_get_require;
 	smb_config_get_secmode;
 	smb_config_get_version;
 	smb_config_getbool;
@@ -124,6 +124,7 @@ SYMBOL_VERSION SUNWprivate {
 	smb_config_setnum;
 	smb_config_setstr;
 	smb_config_upgrade;
+	smb_convert_version_str;
 	smb_crc_gen;
 	smb_ctxbuf_init;
 	smb_ctxbuf_len;
diff --git a/usr/src/lib/smbsrv/libsmb/common/smb_cfg.c b/usr/src/lib/smbsrv/libsmb/common/smb_cfg.c
index 160629e2ed..88890f4f8c 100644
--- a/usr/src/lib/smbsrv/libsmb/common/smb_cfg.c
+++ b/usr/src/lib/smbsrv/libsmb/common/smb_cfg.c
@@ -28,6 +28,13 @@
  * CIFS configuration management library
  */
 
+/*
+ * Checking for things like unsupportable parameter combinations are
+ * the responsibility of callers of these functions.  Example include:
+ * trying to set min_protocol above max_protocol, or requiring encryption
+ * with an allowed protocol range that can't support it.
+ */
+
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -198,7 +205,7 @@ static int smb_config_set_idmap_preferred_dc(char *);
 static int smb_config_get_idmap_site_name(char *, int);
 static int smb_config_set_idmap_site_name(char *);
 
-static uint32_t
+uint32_t
 smb_convert_version_str(const char *version)
 {
 	uint32_t dialect = 0;
@@ -683,49 +690,7 @@ smb_config_setstr(smb_cfg_id_t id, char *value)
 		value = tmp;
 	}
 
-	/*
-	 * We don't want people who care enough about protecting their data
-	 * by requiring encryption to accidentally expose their data
-	 * by lowering the protocol, so prevent them from going below 3.0
-	 * if encryption is required.
-	 * Also, ensure that max_protocol >= min_protocol.
-	 */
-	if (id == SMB_CI_MAX_PROTOCOL) {
-		smb_cfg_val_t encrypt;
-		uint32_t min;
-		uint32_t val;
-
-		encrypt = smb_config_get_require(SMB_CI_ENCRYPT);
-		min = smb_config_get_min_protocol();
-		val = smb_convert_version_str(value);
-
-		if (encrypt == SMB_CONFIG_REQUIRED &&
-		    val < SMB_VERS_3_0) {
-			syslog(LOG_ERR, "Cannot set smbd/max_protocol below 3.0"
-			    " while smbd/encrypt == required.");
-			rc = SMBD_SMF_INVALID_ARG;
-		} else if (val < min) {
-			syslog(LOG_ERR, "Cannot set smbd/max_protocol to less"
-			    " than smbd/min_protocol.");
-			rc = SMBD_SMF_INVALID_ARG;
-		}
-	} else if (id == SMB_CI_MIN_PROTOCOL) {
-		uint32_t max;
-		uint32_t val;
-
-		max = smb_config_get_max_protocol();
-		val = smb_convert_version_str(value);
-
-		if (val > max) {
-			syslog(LOG_ERR, "Cannot set smbd/min_protocol to more"
-			    " than smbd/max_protocol.");
-			rc = SMBD_SMF_INVALID_ARG;
-		}
-	}
-
-	if (rc == SMBD_SMF_OK) {
-		rc = smb_smf_set_string_property(handle, cfg->sc_name, value);
-	}
+	rc = smb_smf_set_string_property(handle, cfg->sc_name, value);
 
 	free(tmp);
 	(void) smb_smf_end_transaction(handle);
@@ -1268,15 +1233,6 @@ smb_config_get_min_protocol(void)
 	return (min);
 }
 
-int
-smb_config_check_protocol(char *value)
-{
-	if (smb_convert_version_str(value) != 0)
-		return (0);
-
-	return (-1);
-}
-
 /*
  * Only SMB 3.x supports encryption.
  * SMB 3.0.2 uses AES128-CCM only.
@@ -1323,100 +1279,6 @@ smb31_config_get_encrypt_cipher(void)
 	return (cipher);
 }
 
-/*
- * If smb2_enable is present and max_protocol is empty,
- * set max_protocol.  Delete smb2_enable.
- */
-static void
-upgrade_smb2_enable()
-{
-	smb_scfhandle_t *handle;
-	char *s2e_name = "smb2_enable";
-	char *s2e_sval;
-	uint8_t	s2e_bval;
-	char *maxp_name = "max_protocol";
-	char *maxp_sval;
-	char verstr[SMB_VERSTR_LEN];
-	int rc;
-
-	handle = smb_smf_scf_init(SMBD_FMRI_PREFIX);
-	if (handle == NULL)
-		return;
-	rc = smb_smf_create_service_pgroup(handle, SMBD_PG_NAME);
-	if (rc != SMBD_SMF_OK)
-		goto out;
-
-	/* Is there an "smb2_enable" property? */
-	rc = smb_smf_get_boolean_property(handle, s2e_name, &s2e_bval);
-	if (rc != SMBD_SMF_OK) {
-		syslog(LOG_DEBUG, "upgrade: smb2_enable not found");
-		goto out;
-	}
-
-	/*
-	 * We will try to delete the smb2_enable property, so we need
-	 * the transaction to start now, before we modify max_protocol
-	 */
-	if ((rc = smb_smf_start_transaction(handle)) != 0) {
-		syslog(LOG_DEBUG, "upgrade_smb2_enable: start trans (%d)", rc);
-		goto out;
-	}
-
-	/*
-	 * Old (smb2_enable) property exists.
-	 * Does the new one? (max_protocol)
-	 */
-	rc = smb_smf_get_string_property(handle, maxp_name,
-	    verstr, sizeof (verstr));
-	if (rc == SMBD_SMF_OK && !smb_config_check_protocol(verstr)) {
-		syslog(LOG_DEBUG, "upgrade: found %s = %s",
-		    maxp_name, verstr);
-		/* Leave existing max_protocol as we found it. */
-	} else {
-		/*
-		 * New property missing or invalid.
-		 * Upgrade from "smb2_enable".
-		 */
-		if (s2e_bval == 0) {
-			s2e_sval = "false";
-			maxp_sval = "1";
-		} else {
-			s2e_sval = "true";
-			maxp_sval = "2.1";
-		}
-		/*
-		 * Note: Need this in the same transaction as the
-		 * delete of smb2_enable below.
-		 */
-		rc = smb_smf_set_string_property(handle, maxp_name, maxp_sval);
-		if (rc != SMBD_SMF_OK) {
-			syslog(LOG_ERR, "failed to set smbd/%d (%d)",
-			    maxp_name, rc);
-			goto out;
-		}
-		syslog(LOG_INFO, "upgrade smbd/smb2_enable=%s "
-		    "converted to smbd/max_protocol=%s",
-		    s2e_sval, maxp_sval);
-	}
-
-	/*
-	 * Delete the old smb2_enable property.
-	 */
-	if ((rc = smb_smf_delete_property(handle, s2e_name)) != 0) {
-		syslog(LOG_DEBUG, "upgrade_smb2_enable: delete prop (%d)", rc);
-	} else if ((rc = smb_smf_end_transaction(handle)) != 0) {
-		syslog(LOG_DEBUG, "upgrade_smb2_enable: end trans (%d)", rc);
-	}
-	if (rc != 0) {
-		syslog(LOG_ERR, "failed to delete property smbd/%d (%d)",
-		    s2e_name, rc);
-	}
-
-out:
-	(void) smb_smf_end_transaction(handle);
-	smb_smf_scf_fini(handle);
-}
-
 
 /*
  * Run once at startup convert old SMF settings to current.
@@ -1424,7 +1286,6 @@ out:
 void
 smb_config_upgrade(void)
 {
-	upgrade_smb2_enable();
 }
 
 smb_cfg_val_t