[Q] enumprinterdrivers 2|3 is broken

Jeremy Allison jra at samba.org
Wed Jul 2 21:22:33 GMT 2008


On Wed, Jul 02, 2008 at 02:59:10PM +0400, Alexander Bokovoy wrote:
> It appears that we are never protecting ourselves from \\ slashes in
> the incoming server name in spoolss requests. While doing
> enumprinterdrivers 2 and 3 levels we call
> rpc_server/srv_spoolss_nt.c:enumprinterdrivers_level2() which formats
> returned strings as "\\\\%s\%s" where first parameter is server name.
> Therefore, our response is sending \\\\ slashes.
> 
> In particular, this is different to what Windows does: it looks they
> simply normalize slashes everywhere.
> 
> This difference actually has a harm effect: Windows client thinks that
> a driver is changed on the server and reloads it from the Samba server
> on each opening of the printer properties. This is quite noticeable
> for large drivers as network consumption increases.
> 
> We could normalize name in
> rpc_parse/rpc_parse_spoolss.c:spoolss_io_q_enumprinterdrivers()
> (reffering to 3-0-stable source) or could strip/normalize in
> enumprinterdrivers_level2(). Not sure which way is better and safer.
> 
> This logical error exists in rpc code in 3-0 and upwards.

Alexander, can you test this patch for me please ?

Should fix all uses of \\[\\..]servername to be
a canonical \\servername.

Jeremy.
-------------- next part --------------
diff --git a/source/rpc_server/srv_spoolss_nt.c b/source/rpc_server/srv_spoolss_nt.c
index 5fff900..d1f9960 100644
--- a/source/rpc_server/srv_spoolss_nt.c
+++ b/source/rpc_server/srv_spoolss_nt.c
@@ -72,6 +72,18 @@ struct xcv_api_table {
 	WERROR(*fn) (NT_USER_TOKEN *token, RPC_BUFFER *in, RPC_BUFFER *out, uint32 *needed);
 };
 
+/********************************************************************
+ * Canonicalize servername.
+ ********************************************************************/
+
+static const char *canon_servername(const char *servername)
+{
+	const char *pservername = servername;
+	while (*pservername == '\\') {
+		pservername++;
+	}
+	return pservername;
+}
 
 /* translate between internal status numbers and NT status numbers */
 static int nt_printj_status(int v)
@@ -455,13 +467,12 @@ static bool set_printer_hnd_name(Printer_entry *Printer, char *handlename)
 
 	aprinter = handlename;
 	if ( *handlename == '\\' ) {
-		servername = handlename + 2;
-		if ( (aprinter = strchr_m( handlename+2, '\\' )) != NULL ) {
+		servername = canon_servername(handlename);
+		if ( (aprinter = strchr_m( servername, '\\' )) != NULL ) {
 			*aprinter = '\0';
 			aprinter++;
 		}
-	}
-	else {
+	} else {
 		servername = "";
 	}
 
@@ -4661,20 +4672,16 @@ static WERROR enumprinters_level1( uint32 flags, fstring name,
  * handle enumeration of printers at level 2
  ********************************************************************/
 
-static WERROR enumprinters_level2( uint32 flags, fstring servername,
+static WERROR enumprinters_level2( uint32 flags, const char *servername,
 			         RPC_BUFFER *buffer, uint32 offered,
 			         uint32 *needed, uint32 *returned)
 {
-	char *s = servername;
-
 	if (flags & PRINTER_ENUM_LOCAL) {
 			return enum_all_printers_info_2(buffer, offered, needed, returned);
 	}
 
 	if (flags & PRINTER_ENUM_NAME) {
-		if ((servername[0] == '\\') && (servername[1] == '\\'))
-			s = servername + 2;
-		if (is_myname_or_ipaddr(s))
+		if (is_myname_or_ipaddr(canon_servername(servername)))
 			return enum_all_printers_info_2(buffer, offered, needed, returned);
 		else
 			return WERR_INVALID_NAME;
@@ -4690,7 +4697,7 @@ static WERROR enumprinters_level2( uint32 flags, fstring servername,
  * handle enumeration of printers at level 5
  ********************************************************************/
 
-static WERROR enumprinters_level5( uint32 flags, fstring servername,
+static WERROR enumprinters_level5( uint32 flags, const char *servername,
 			         RPC_BUFFER *buffer, uint32 offered,
 			         uint32 *needed, uint32 *returned)
 {
@@ -5110,7 +5117,7 @@ WERROR _spoolss_getprinter(pipes_struct *p, SPOOL_Q_GETPRINTER *q_u, SPOOL_R_GET
  * fill a DRIVER_INFO_1 struct
  ********************************************************************/
 
-static void fill_printer_driver_info_1(DRIVER_INFO_1 *info, NT_PRINTER_DRIVER_INFO_LEVEL driver, fstring servername, fstring architecture)
+static void fill_printer_driver_info_1(DRIVER_INFO_1 *info, NT_PRINTER_DRIVER_INFO_LEVEL driver, const char *servername, fstring architecture)
 {
 	init_unistr( &info->name, driver.info_3->name);
 }
@@ -5119,7 +5126,7 @@ static void fill_printer_driver_info_1(DRIVER_INFO_1 *info, NT_PRINTER_DRIVER_IN
  * construct_printer_driver_info_1
  ********************************************************************/
 
-static WERROR construct_printer_driver_info_1(DRIVER_INFO_1 *info, int snum, fstring servername, fstring architecture, uint32 version)
+static WERROR construct_printer_driver_info_1(DRIVER_INFO_1 *info, int snum, const char *servername, fstring architecture, uint32 version)
 {
 	NT_PRINTER_INFO_LEVEL *printer = NULL;
 	NT_PRINTER_DRIVER_INFO_LEVEL driver;
@@ -5146,21 +5153,21 @@ static WERROR construct_printer_driver_info_1(DRIVER_INFO_1 *info, int snum, fst
  * fill a printer_info_2 struct
  ********************************************************************/
 
-static void fill_printer_driver_info_2(DRIVER_INFO_2 *info, NT_PRINTER_DRIVER_INFO_LEVEL driver, fstring servername)
+static void fill_printer_driver_info_2(DRIVER_INFO_2 *info, NT_PRINTER_DRIVER_INFO_LEVEL driver, const char *servername)
 {
 	TALLOC_CTX *ctx = talloc_tos();
 	char *temp = NULL;
+	const char *cservername = canon_servername(servername);
 
 	info->version=driver.info_3->cversion;
 
 	init_unistr( &info->name, driver.info_3->name );
 	init_unistr( &info->architecture, driver.info_3->environment );
 
-
 	if (strlen(driver.info_3->driverpath)) {
 		temp = talloc_asprintf(ctx,
 				"\\\\%s%s",
-				servername,
+				cservername,
 				driver.info_3->driverpath);
 		init_unistr( &info->driverpath, temp );
 	} else {
@@ -5171,7 +5178,7 @@ static void fill_printer_driver_info_2(DRIVER_INFO_2 *info, NT_PRINTER_DRIVER_IN
 	if (strlen(driver.info_3->datafile)) {
 		temp = talloc_asprintf(ctx,
 				"\\\\%s%s",
-				servername,
+				cservername,
 				driver.info_3->datafile);
 		init_unistr( &info->datafile, temp );
 	} else
@@ -5181,7 +5188,7 @@ static void fill_printer_driver_info_2(DRIVER_INFO_2 *info, NT_PRINTER_DRIVER_IN
 	if (strlen(driver.info_3->configfile)) {
 		temp = talloc_asprintf(ctx,
 				"\\\\%s%s",
-				servername,
+				cservername,
 				driver.info_3->configfile);
 		init_unistr( &info->configfile, temp );
 	} else
@@ -5193,7 +5200,7 @@ static void fill_printer_driver_info_2(DRIVER_INFO_2 *info, NT_PRINTER_DRIVER_IN
  * fill a printer_info_2 struct
  ********************************************************************/
 
-static WERROR construct_printer_driver_info_2(DRIVER_INFO_2 *info, int snum, fstring servername, fstring architecture, uint32 version)
+static WERROR construct_printer_driver_info_2(DRIVER_INFO_2 *info, int snum, const char *servername, fstring architecture, uint32 version)
 {
 	NT_PRINTER_INFO_LEVEL *printer = NULL;
 	NT_PRINTER_DRIVER_INFO_LEVEL driver;
@@ -5249,7 +5256,7 @@ static uint32 init_unistr_array(uint16 **uni_array, fstring *char_array, const c
 		if ( servername ) {
 			line = talloc_asprintf(ctx,
 					"\\\\%s%s",
-					servername,
+					canon_servername(servername),
 					v);
 		} else {
 			line = talloc_strdup(ctx, v);
@@ -5294,10 +5301,11 @@ static uint32 init_unistr_array(uint16 **uni_array, fstring *char_array, const c
  * fill a printer_info_3 struct
  ********************************************************************/
 
-static void fill_printer_driver_info_3(DRIVER_INFO_3 *info, NT_PRINTER_DRIVER_INFO_LEVEL driver, fstring servername)
+static void fill_printer_driver_info_3(DRIVER_INFO_3 *info, NT_PRINTER_DRIVER_INFO_LEVEL driver, const char *servername)
 {
 	char *temp = NULL;
 	TALLOC_CTX *ctx = talloc_tos();
+	const char *cservername = canon_servername(servername);
 
 	ZERO_STRUCTP(info);
 
@@ -5309,7 +5317,7 @@ static void fill_printer_driver_info_3(DRIVER_INFO_3 *info, NT_PRINTER_DRIVER_IN
 	if (strlen(driver.info_3->driverpath)) {
 		temp = talloc_asprintf(ctx,
 				"\\\\%s%s",
-				servername,
+				cservername,
 				driver.info_3->driverpath);
 		init_unistr( &info->driverpath, temp );
 	} else
@@ -5319,7 +5327,7 @@ static void fill_printer_driver_info_3(DRIVER_INFO_3 *info, NT_PRINTER_DRIVER_IN
 	if (strlen(driver.info_3->datafile)) {
 		temp = talloc_asprintf(ctx,
 				"\\\\%s%s",
-				servername,
+				cservername,
 				driver.info_3->datafile);
 		init_unistr( &info->datafile, temp );
 	} else
@@ -5329,7 +5337,7 @@ static void fill_printer_driver_info_3(DRIVER_INFO_3 *info, NT_PRINTER_DRIVER_IN
 	if (strlen(driver.info_3->configfile)) {
 		temp = talloc_asprintf(ctx,
 				"\\\\%s%s",
-				servername,
+				cservername,
 				driver.info_3->configfile);
 		init_unistr( &info->configfile, temp );
 	} else
@@ -5339,7 +5347,7 @@ static void fill_printer_driver_info_3(DRIVER_INFO_3 *info, NT_PRINTER_DRIVER_IN
 	if (strlen(driver.info_3->helpfile)) {
 		temp = talloc_asprintf(ctx,
 				"\\\\%s%s",
-				servername,
+				cservername,
 				driver.info_3->helpfile);
 		init_unistr( &info->helpfile, temp );
 	} else
@@ -5350,7 +5358,7 @@ static void fill_printer_driver_info_3(DRIVER_INFO_3 *info, NT_PRINTER_DRIVER_IN
 	init_unistr( &info->defaultdatatype, driver.info_3->defaultdatatype );
 
 	info->dependentfiles=NULL;
-	init_unistr_array(&info->dependentfiles, driver.info_3->dependentfiles, servername);
+	init_unistr_array(&info->dependentfiles, driver.info_3->dependentfiles, cservername);
 }
 
 /********************************************************************
@@ -5358,7 +5366,7 @@ static void fill_printer_driver_info_3(DRIVER_INFO_3 *info, NT_PRINTER_DRIVER_IN
  * fill a printer_info_3 struct
  ********************************************************************/
 
-static WERROR construct_printer_driver_info_3(DRIVER_INFO_3 *info, int snum, fstring servername, fstring architecture, uint32 version)
+static WERROR construct_printer_driver_info_3(DRIVER_INFO_3 *info, int snum, const char *servername, fstring architecture, uint32 version)
 {
 	NT_PRINTER_INFO_LEVEL *printer = NULL;
 	NT_PRINTER_DRIVER_INFO_LEVEL driver;
@@ -5417,11 +5425,12 @@ static WERROR construct_printer_driver_info_3(DRIVER_INFO_3 *info, int snum, fst
  * fill a printer_info_6 struct - we know that driver is really level 3. This sucks. JRA.
  ********************************************************************/
 
-static void fill_printer_driver_info_6(DRIVER_INFO_6 *info, NT_PRINTER_DRIVER_INFO_LEVEL driver, fstring servername)
+static void fill_printer_driver_info_6(DRIVER_INFO_6 *info, NT_PRINTER_DRIVER_INFO_LEVEL driver, const char *servername)
 {
 	char *temp = NULL;
 	fstring nullstr;
 	TALLOC_CTX *ctx = talloc_tos();
+	const char *cservername = canon_servername(servername);
 
 	ZERO_STRUCTP(info);
 	memset(&nullstr, '\0', sizeof(fstring));
@@ -5434,7 +5443,7 @@ static void fill_printer_driver_info_6(DRIVER_INFO_6 *info, NT_PRINTER_DRIVER_IN
 	if (strlen(driver.info_3->driverpath)) {
 		temp = talloc_asprintf(ctx,
 				"\\\\%s%s",
-				servername,
+				cservername,
 				driver.info_3->driverpath);
 		init_unistr( &info->driverpath, temp );
 	} else
@@ -5444,7 +5453,7 @@ static void fill_printer_driver_info_6(DRIVER_INFO_6 *info, NT_PRINTER_DRIVER_IN
 	if (strlen(driver.info_3->datafile)) {
 		temp = talloc_asprintf(ctx,
 				"\\\\%s%s",
-				servername,
+				cservername,
 				driver.info_3->datafile);
 		init_unistr( &info->datafile, temp );
 	} else
@@ -5454,7 +5463,7 @@ static void fill_printer_driver_info_6(DRIVER_INFO_6 *info, NT_PRINTER_DRIVER_IN
 	if (strlen(driver.info_3->configfile)) {
 		temp = talloc_asprintf(ctx,
 				"\\\\%s%s",
-				servername,
+				cservername,
 				driver.info_3->configfile);
 		init_unistr( &info->configfile, temp );
 	} else
@@ -5464,7 +5473,7 @@ static void fill_printer_driver_info_6(DRIVER_INFO_6 *info, NT_PRINTER_DRIVER_IN
 	if (strlen(driver.info_3->helpfile)) {
 		temp = talloc_asprintf(ctx,
 				"\\\\%s%s",
-				servername,
+				cservername,
 				driver.info_3->helpfile);
 		init_unistr( &info->helpfile, temp );
 	} else
@@ -5498,7 +5507,7 @@ static void fill_printer_driver_info_6(DRIVER_INFO_6 *info, NT_PRINTER_DRIVER_IN
  ********************************************************************/
 
 static WERROR construct_printer_driver_info_6(DRIVER_INFO_6 *info, int snum,
-              fstring servername, fstring architecture, uint32 version)
+              const char *servername, fstring architecture, uint32 version)
 {
 	NT_PRINTER_INFO_LEVEL 		*printer = NULL;
 	NT_PRINTER_DRIVER_INFO_LEVEL 	driver;
@@ -5565,7 +5574,7 @@ static void free_printer_driver_info_6(DRIVER_INFO_6 *info)
 /****************************************************************************
 ****************************************************************************/
 
-static WERROR getprinterdriver2_level1(fstring servername, fstring architecture, uint32 version, int snum, RPC_BUFFER *buffer, uint32 offered, uint32 *needed)
+static WERROR getprinterdriver2_level1(const char *servername, fstring architecture, uint32 version, int snum, RPC_BUFFER *buffer, uint32 offered, uint32 *needed)
 {
 	DRIVER_INFO_1 *info=NULL;
 	WERROR result;
@@ -5603,7 +5612,7 @@ out:
 /****************************************************************************
 ****************************************************************************/
 
-static WERROR getprinterdriver2_level2(fstring servername, fstring architecture, uint32 version, int snum, RPC_BUFFER *buffer, uint32 offered, uint32 *needed)
+static WERROR getprinterdriver2_level2(const char *servername, fstring architecture, uint32 version, int snum, RPC_BUFFER *buffer, uint32 offered, uint32 *needed)
 {
 	DRIVER_INFO_2 *info=NULL;
 	WERROR result;
@@ -5641,7 +5650,7 @@ out:
 /****************************************************************************
 ****************************************************************************/
 
-static WERROR getprinterdriver2_level3(fstring servername, fstring architecture, uint32 version, int snum, RPC_BUFFER *buffer, uint32 offered, uint32 *needed)
+static WERROR getprinterdriver2_level3(const char *servername, fstring architecture, uint32 version, int snum, RPC_BUFFER *buffer, uint32 offered, uint32 *needed)
 {
 	DRIVER_INFO_3 info;
 	WERROR result;
@@ -5677,7 +5686,7 @@ out:
 /****************************************************************************
 ****************************************************************************/
 
-static WERROR getprinterdriver2_level6(fstring servername, fstring architecture, uint32 version, int snum, RPC_BUFFER *buffer, uint32 offered, uint32 *needed)
+static WERROR getprinterdriver2_level6(const char *servername, fstring architecture, uint32 version, int snum, RPC_BUFFER *buffer, uint32 offered, uint32 *needed)
 {
 	DRIVER_INFO_6 info;
 	WERROR result;
@@ -6910,7 +6919,7 @@ WERROR _spoolss_setjob(pipes_struct *p, SPOOL_Q_SETJOB *q_u, SPOOL_R_SETJOB *r_u
  Enumerates all printer drivers at level 1.
 ****************************************************************************/
 
-static WERROR enumprinterdrivers_level1(fstring servername, fstring architecture, RPC_BUFFER *buffer, uint32 offered, uint32 *needed, uint32 *returned)
+static WERROR enumprinterdrivers_level1(const char *servername, fstring architecture, RPC_BUFFER *buffer, uint32 offered, uint32 *needed, uint32 *returned)
 {
 	int i;
 	int ndrivers;
@@ -6994,7 +7003,7 @@ out:
  Enumerates all printer drivers at level 2.
 ****************************************************************************/
 
-static WERROR enumprinterdrivers_level2(fstring servername, fstring architecture, RPC_BUFFER *buffer, uint32 offered, uint32 *needed, uint32 *returned)
+static WERROR enumprinterdrivers_level2(const char *servername, fstring architecture, RPC_BUFFER *buffer, uint32 offered, uint32 *needed, uint32 *returned)
 {
 	int i;
 	int ndrivers;
@@ -7079,7 +7088,7 @@ out:
  Enumerates all printer drivers at level 3.
 ****************************************************************************/
 
-static WERROR enumprinterdrivers_level3(fstring servername, fstring architecture, RPC_BUFFER *buffer, uint32 offered, uint32 *needed, uint32 *returned)
+static WERROR enumprinterdrivers_level3(const char *servername, fstring architecture, RPC_BUFFER *buffer, uint32 offered, uint32 *needed, uint32 *returned)
 {
 	int i;
 	int ndrivers;
@@ -7175,7 +7184,7 @@ WERROR _spoolss_enumprinterdrivers( pipes_struct *p, SPOOL_Q_ENUMPRINTERDRIVERS
 	uint32 offered = q_u->offered;
 	uint32 *needed = &r_u->needed;
 	uint32 *returned = &r_u->returned;
-
+	const char *cservername;
 	fstring servername;
 	fstring architecture;
 
@@ -7196,16 +7205,18 @@ WERROR _spoolss_enumprinterdrivers( pipes_struct *p, SPOOL_Q_ENUMPRINTERDRIVERS
 	unistr2_to_ascii(architecture, &q_u->environment, sizeof(architecture));
 	unistr2_to_ascii(servername, &q_u->name, sizeof(servername));
 
-	if ( !is_myname_or_ipaddr( servername ) )
+	cservername = canon_servername(servername);
+
+	if (!is_myname_or_ipaddr(cservername))
 		return WERR_UNKNOWN_PRINTER_DRIVER;
 
 	switch (level) {
 	case 1:
-		return enumprinterdrivers_level1(servername, architecture, buffer, offered, needed, returned);
+		return enumprinterdrivers_level1(cservername, architecture, buffer, offered, needed, returned);
 	case 2:
-		return enumprinterdrivers_level2(servername, architecture, buffer, offered, needed, returned);
+		return enumprinterdrivers_level2(cservername, architecture, buffer, offered, needed, returned);
 	case 3:
-		return enumprinterdrivers_level3(servername, architecture, buffer, offered, needed, returned);
+		return enumprinterdrivers_level3(cservername, architecture, buffer, offered, needed, returned);
 	default:
 		return WERR_UNKNOWN_LEVEL;
 	}
@@ -7992,7 +8003,7 @@ static WERROR getprinterdriverdir_level_1(UNISTR2 *name, UNISTR2 *uni_environmen
 	char *path = NULL;
 	char *long_archi = NULL;
 	char *servername = NULL;
-	char *pservername = NULL;
+	const char *pservername = NULL;
 	const char *short_archi;
 	DRIVER_DIRECTORY_1 *info=NULL;
 	WERROR result = WERR_OK;
@@ -8010,12 +8021,9 @@ static WERROR getprinterdriverdir_level_1(UNISTR2 *name, UNISTR2 *uni_environmen
 	/* check for beginning double '\'s and that the server
 	   long enough */
 
-	pservername = servername;
-	if ( *pservername == '\\' && strlen(servername)>2 ) {
-		pservername += 2;
-	}
+	pservername = canon_servername(servername);
 
-	if ( !is_myname_or_ipaddr( pservername ) )
+	if ( !is_myname_or_ipaddr(pservername))
 		return WERR_INVALID_PARAM;
 
 	if (!(short_archi = get_short_archi(long_archi)))


More information about the samba-technical mailing list