nfs-utils: mount: Fixed collision between commas in options

PATCH In review Steve Dickson 3 participants 6 messages Feb 26, 2007 – Mar 1, 2007

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Steve Dickson

author

2007-02-26 11:18 UTC #0 lore raw

PATCH 11/11 nfs-utils: mount: Fixed collision between commas in options

commit 96a3ceb3d35bf5edcb9446aded8375d3b98b4f5b

Author: Cory Olmo <[email protected]>
Date:   Sat Feb 24 16:20:40 2007 -0500

This patch avoid the collision between commas in security contexts and the delimiter betweeen mount options.

Signed-off-by Karel Zak <[email protected]>

Signed-off-by Cory Olmo <[email protected]>

utils/mount/mount.c +19 -7

index b3d3696..f22747b 100644
@@ -285,18 +285,30 @@ static void parse_opts (const char *options, int *flags, char **extra_opts)
 {
 	if (options != NULL) {
 		char *opts = xstrdup(options);
-		char *opt;
-		int len = strlen(opts) + 20;
-
+		char *opt, *p;
+		int len = strlen(opts) + 256;
+		int open_quote = 0;
+ 
 		*extra_opts = xmalloc(len);
 		**extra_opts = '\0';
 
-		for (opt = strtok(opts, ","); opt; opt = strtok(NULL, ","))
-			parse_opt(opt, flags, *extra_opts, len);
-
+		for (p=opts, opt=NULL; p && *p; p++) {
+			if (!opt)
+				opt = p;		/* begin of the option item */
+			if (*p == '"') 
+				open_quote ^= 1;	/* reverse the status */
+			if (open_quote)
+				continue;		/* still in quoted block */
+			if (*p == ',')
+				*p = '\0';		/* terminate the option item */
+			/* end of option item or last item */
+			if (*p == '\0' || *(p+1) == '\0') {
+				parse_opt(opt, flags, *extra_opts, len);
+				opt = NULL;
+			}
+		} 
 		free(opts);
 	}
-
 }
 
 /*

utils/mount/nfsmount.c +30 -4

index 3ca9b80..08a8ac1 100644
@@ -548,15 +548,31 @@ parse_options(char *old_opts, struct nfs_mount_data *data,
 	struct pmap *mnt_pmap = &mnt_server->pmap;
 	struct pmap *nfs_pmap = &nfs_server->pmap;
 	int len;
-	char *opt, *opteq;
+	char *opt, *opteq, *p, *opt_b;
 	char *mounthost = NULL;
 	char cbuf[128];
+	int open_quote = 0;
 
 	data->flags = 0;
 	*bg = 0;
 
 	len = strlen(new_opts);
-	for (opt = strtok(old_opts, ","); opt; opt = strtok(NULL, ",")) {
+	for (p=old_opts, opt_b=NULL; p && *p; p++) {
+		if (!opt_b)
+			opt_b = p;		/* begin of the option item */
+		if (*p == '"') 
+			open_quote ^= 1;	/* reverse the status */
+		if (open_quote)
+			continue;		/* still in quoted block */
+		if (*p == ',')
+			*p = '\0';		/* terminate the option item */
+		if (*p == '\0' || *(p+1) == '\0') {
+			opt = opt_b;		/* opt is useful now */
+			opt_b = NULL;
+		}
+		else
+			continue;		/* still somewhere in the option item */
+
 		if (strlen(opt) >= sizeof(cbuf))
 			goto bad_parameter;
 		if ((opteq = strchr(opt, '=')) && isdigit(opteq[1])) {

@@ -671,13 +687,23 @@ parse_options(char *old_opts, struct nfs_mount_data *data,
 						   strcspn(opteq+1," \t\n\r,"));
 			 else if (!strcmp(opt, "context")) {
  				char *context = opteq + 1;
+				int ctxlen = strlen(context);
  				
- 				if (strlen(context) > NFS_MAX_CONTEXT_LEN) {
+ 				if (ctxlen > NFS_MAX_CONTEXT_LEN) {
  					printf(_("context parameter exceeds limit of %d\n"),
  						 NFS_MAX_CONTEXT_LEN);
 					goto bad_parameter;
  				}
- 				strncpy(data->context, context, NFS_MAX_CONTEXT_LEN);
+				/* The context string is in the format of
+				 * "system_u:object_r:...".  We only want
+				 * the context str between the quotes.
+				 */
+				if (*context == '"')
+					strncpy(data->context, context+1, 
+							ctxlen-2);
+				else
+					strncpy(data->context, context, 
+							NFS_MAX_CONTEXT_LEN);
  			} else if (!sloppy)
 				goto bad_parameter;
 			sprintf(cbuf, "%s=%s,", opt, opteq+1);

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Neil Brown

2007-02-27 06:37 UTC #1 lore raw

Quoted 15 lines · On Monday February 26, [email protected] wrote:

commit 96a3ceb3d35bf5edcb9446aded8375d3b98b4f5b
Author: Cory Olmo <[email protected]>
Date:   Sat Feb 24 16:20:40 2007 -0500
This patch avoid the collision between commas in security contexts and the delimiter betweeen mount options.
Signed-off-by Karel Zak <[email protected]>
Signed-off-by Cory Olmo <[email protected]>
utils/mount/mount.c +0 -0
index b3d3696..f22747b 100644

utils/mount/mount.c static void parse_opts (const char *options, int *flags, char **extra_opts) · line 289

Quoted 8 lines

utils/mount/mount.c +2 -3

@@ -285,18 +285,30 @@ static void parse_opts (const char *options, int *flags, char **extra_opts)
 {
 	if (options != NULL) {
 		char *opts = xstrdup(options);
-		char *opt;
-		int len = strlen(opts) + 20;
-
+		char *opt, *p;
+		int len = strlen(opts) + 256;

This is a worry. If 20 isn't big enough, why do you thing 256 will be? and where do we check that the buffer doesn't overflow (in this setuid program).
What are we making space for here?

Would strlen(opts)*2 or *3 be sure to be sufficient?

NeilBrown

Karel Zak

2007-02-27 09:34 UTC #2 lore raw

Quoted 11 lines · Neil Brown

Quoted 10 lines · On Monday February 26, [email protected] wrote:
commit 96a3ceb3d35bf5edcb9446aded8375d3b98b4f5b
Author: Cory Olmo <[email protected]>
Date:   Sat Feb 24 16:20:40 2007 -0500
This patch avoid the collision between commas in security contexts and the delimiter betweeen mount options.
Signed-off-by Karel Zak <[email protected]>
Signed-off-by Cory Olmo <[email protected]>

Quoted 15 lines

utils/mount/mount.c static void parse_opts (const char *options, int *flags, char **extra_opts) · line 289
Quoted 13 lines
utils/mount/mount.c +2 -3
index b3d3696..f22747b 100644
@@ -285,18 +285,30 @@ static void parse_opts (const char *options, int *flags, char **extra_opts)
 {
 	if (options != NULL) {
 		char *opts = xstrdup(options);
-		char *opt;
-		int len = strlen(opts) + 20;
-
+		char *opt, *p;
+		int len = strlen(opts) + 256;
This is a worry. If 20 isn't big enough, why do you thing 256 will

The 20 was big enough for conversion from usernames to UIDs, but it's not enough for selinux context names. I didn't found any definition for maximal length of selinux context name, so we need to select any range and assume that it's enough (or completely rewrite this part of the mount command and allocate all dynamically).

Quoted 2 lines

be? and where do we check that the buffer doesn't overflow (in this setuid program).

Please, read the code. We check the buffer size everywhere. I don't see there any place where the buffer can overflow.

Quoted 1 line

Would strlen(opts)*2 or *3 be sure to be sufficient?

not sure, I don't think that for selinux is there any correlation between normal and raw context name. But 256 is probably overkill for non-selinux version -- it'd be better use:

  if (options != NULL) {
      char *opts = xstrdup(options);
      char *opt;
#ifdef HAVE_LIBSELINUX
      int len = strlen(opts) + 256;
#else
      int len = strlen(opts) + 20;

#endif

... but I don't think it's so important that we have to add next #ifdef to code.

Karel

Signature · 10 lines

 Karel Zak  <[email protected]>

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Karel Zak

2007-02-27 11:45 UTC #3 lore raw

Quoted 28 lines · Karel Zak

Quoted 11 lines · Neil Brown
Quoted 10 lines · On Monday February 26, [email protected] wrote:
commit 96a3ceb3d35bf5edcb9446aded8375d3b98b4f5b
Author: Cory Olmo <[email protected]>
Date:   Sat Feb 24 16:20:40 2007 -0500
This patch avoid the collision between commas in security contexts and the delimiter betweeen mount options.
Signed-off-by Karel Zak <[email protected]>
Signed-off-by Cory Olmo <[email protected]>
Quoted 15 lines
utils/mount/mount.c static void parse_opts (const char *options, int *flags, char **extra_opts) · line 289
Quoted 13 lines
utils/mount/mount.c +2 -3
index b3d3696..f22747b 100644
@@ -285,18 +285,30 @@ static void parse_opts (const char *options, int *flags, char **extra_opts)
 {
 	if (options != NULL) {
 		char *opts = xstrdup(options);
-		char *opt;
-		int len = strlen(opts) + 20;
-
+		char *opt, *p;
+		int len = strlen(opts) + 256;
This is a worry. If 20 isn't big enough, why do you thing 256 will

Quoted 5 lines

The 20 was big enough for conversion from usernames to UIDs, but it's not enough for selinux context names. I didn't found any definition for maximal length of selinux context name, so we need to select any range and assume that it's enough (or completely rewrite this part of the mount command and allocate all dynamically).

Well, it seems like non-invasive change to use realloc() for extra_opts string. I'll prepare a patch with this change in next weeks.

Karel

Signature · 10 lines

 Karel Zak  <[email protected]>

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Neil Brown

2007-02-28 23:35 UTC #4 lore raw

In reply to Karel Zak

Quoted 34 lines · On Tuesday February 27, [email protected] wrote:

Quoted 11 lines · Neil Brown
Quoted 10 lines · On Monday February 26, [email protected] wrote:
commit 96a3ceb3d35bf5edcb9446aded8375d3b98b4f5b
Author: Cory Olmo <[email protected]>
Date:   Sat Feb 24 16:20:40 2007 -0500
This patch avoid the collision between commas in security contexts and the delimiter betweeen mount options.
Signed-off-by Karel Zak <[email protected]>
Signed-off-by Cory Olmo <[email protected]>
Quoted 15 lines
utils/mount/mount.c static void parse_opts (const char *options, int *flags, char **extra_opts) · line 289
Quoted 13 lines
utils/mount/mount.c +2 -3
index b3d3696..f22747b 100644
@@ -285,18 +285,30 @@ static void parse_opts (const char *options, int *flags, char **extra_opts)
 {
 	if (options != NULL) {
 		char *opts = xstrdup(options);
-		char *opt;
-		int len = strlen(opts) + 20;
-
+		char *opt, *p;
+		int len = strlen(opts) + 256;
This is a worry. If 20 isn't big enough, why do you thing 256 will
The 20 was big enough for conversion from usernames to UIDs, but it's not enough for selinux context names. I didn't found any definition for maximal length of selinux context name, so we need to select any range and assume that it's enough (or completely rewrite this part of the mount command and allocate all dynamically).

Quoted 5 lines

Quoted 2 lines
be? and where do we check that the buffer doesn't overflow (in this setuid program).
Please, read the code. We check the buffer size everywhere. I don't see there any place where the buffer can overflow.

OK, so I did read the code a bit more carefully, and now I'm even more confused and concerned.

Firstly, I didn't notice that 'len' was being passed into parse_opt. However the way it is being passed makes it totally useless. parse_opt is called in a loop from parse_opts and each time it reduces 'len' by the amount that is added to 'extra_opts'.
However 'len' is passed by value, not by reference, so each time around it starts at the same value and so is not effective in making sure that extra_opts doesn't overflow it's buffer.

But further, I cannot see how extra_opts ever needs to be large than 'options'. All that is ever appended to it are sections that have been removed from 'options'. So why was the +20 there in the first place.

You mention "conversion from usernames to UIDs", but I cannot find that anywhere. Could you please help me understand exactly why "extra_opts" needs to be larger than "options" ??

Thanks,
NeilBrown

Karel Zak

2007-03-01 09:03 UTC #5 lore raw

Quoted 44 lines · Neil Brown

Quoted 34 lines · On Tuesday February 27, [email protected] wrote:
Quoted 11 lines · Neil Brown
Quoted 10 lines · On Monday February 26, [email protected] wrote:
commit 96a3ceb3d35bf5edcb9446aded8375d3b98b4f5b
Author: Cory Olmo <[email protected]>
Date:   Sat Feb 24 16:20:40 2007 -0500
This patch avoid the collision between commas in security contexts and the delimiter betweeen mount options.
Signed-off-by Karel Zak <[email protected]>
Signed-off-by Cory Olmo <[email protected]>
Quoted 15 lines
utils/mount/mount.c static void parse_opts (const char *options, int *flags, char **extra_opts) · line 289
Quoted 13 lines
utils/mount/mount.c +2 -3
index b3d3696..f22747b 100644
@@ -285,18 +285,30 @@ static void parse_opts (const char *options, int *flags, char **extra_opts)
 {
 	if (options != NULL) {
 		char *opts = xstrdup(options);
-		char *opt;
-		int len = strlen(opts) + 20;
-
+		char *opt, *p;
+		int len = strlen(opts) + 256;
This is a worry. If 20 isn't big enough, why do you thing 256 will
The 20 was big enough for conversion from usernames to UIDs, but it's not enough for selinux context names. I didn't found any definition for maximal length of selinux context name, so we need to select any range and assume that it's enough (or completely rewrite this part of the mount command and allocate all dynamically).
Quoted 5 lines
Quoted 2 lines
be? and where do we check that the buffer doesn't overflow (in this setuid program).
Please, read the code. We check the buffer size everywhere. I don't see there any place where the buffer can overflow.
OK, so I did read the code a bit more carefully, and now I'm even more confused and concerned.

Quoted 7 lines

Firstly, I didn't notice that 'len' was being passed into parse_opt. However the way it is being passed makes it totally useless. parse_opt is called in a loop from parse_opts and each time it reduces 'len' by the amount that is added to 'extra_opts'.
However 'len' is passed by value, not by reference, so each time around it starts at the same value and so is not effective in making sure that extra_opts doesn't overflow it's buffer.

See parse_opt(), there is:

    len -= strlen(extra_opts);

it sets the "len" to correct length.

Quoted 4 lines

But further, I cannot see how extra_opts ever needs to be large than 'options'. All that is ever appended to it are sections that have been removed from 'options'. So why was the +20 there in the first place.

Hmm... I think the reason is copy & past from the mount command. The mount.nfs is simplificated version of the original mount. In the mount command is conversion to UID/GID and selinux contexts transformation. The mount.nfs doesn't do these conversions (in my previous mail I've read the mount and not mount.nfs. Sorry.).

Someone has copyed the code with +20 and I've enlarged to +256 like in original mount.

I'll resubmit the patch next week. Thanks for your review.

Karel

Signature · 10 lines

 Karel Zak  <[email protected]>

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs