[PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

[PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

[Shyam Prasad N]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

Hi,

This is a fix for the scenario of a krb5 user running a "sudo mount".
Even if the user has cred cache populated, when the mount is run using
sudo, uid switches to 0. So cred cache for the root user will be
searched for, unless cruid is specified explicitly. This fix checks
for cruid=$SUDO_UID as a fallback option, when the mount fails with
ENOKEY.

--
-Shyam

[-- Attachment #2: 0001-mount.cifs-use-SUDO_UID-env-variable-for-cruid.patch --]
[-- Type: application/octet-stream, Size: 5619 bytes --]

From 15ac08056ec3c7175da1b6d20a50cae855189258 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Wed, 16 Sep 2020 00:18:44 -0700
Subject: [PATCH] mount.cifs: use SUDO_UID env variable for cruid

In the current mount.cifs logic, when sudo is used for mount,
uid=0, so the mount command searches for cruid=0 unless explicitly
specified by the user. The user may already have cred cache populated
but mount.cifs would end up searching cred cache for uid=0.

mount.cifs can avoid this confusion by reading the cruid from SUDO_UID
environment variable. If it is set to non-zero, we can make cruid=$SUDO_UID.

However, to maintain backward compatibility, keeping this as a fallback option.
If mount fails with permission denied, then retry with this option.
To enable this fallback, I had to make a few minor changes in the flow.
---
 mount.cifs.c | 79 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 62 insertions(+), 17 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 4feb397..cf62c30 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -171,7 +171,11 @@
 
 #define NTFS_TIME_OFFSET ((unsigned long long)(369*365 + 89) * 24 * 3600 * 10000000)
 
-/* struct for holding parsed mount info for use by privileged process */
+/*
+* struct for holding parsed mount info for use by privileged process.
+* Please do not keep pointers in this struct.
+* That way, reinit of this struct is a simple memset.
+*/
 struct parsed_mount_info {
 	unsigned long flags;
 	char host[NI_MAXHOST + 1];
@@ -189,6 +193,7 @@ struct parsed_mount_info {
 	unsigned int verboseflag:1;
 	unsigned int nofail:1;
 	unsigned int got_domain:1;
+	uid_t sudo_uid;
 };
 
 static const char *thisprogram;
@@ -1199,6 +1204,10 @@ nocopy:
 		snprintf(out + out_len, word_len + 5, "uid=%s", txtbuf);
 		out_len = strlen(out);
 	}
+	if (parsed_info->sudo_uid) {
+		cruid = parsed_info->sudo_uid;
+		got_cruid = 1;
+	}
 	if (got_cruid) {
 		word_len = snprintf(txtbuf, sizeof(txtbuf), "%u", cruid);
 
@@ -2012,12 +2021,16 @@ int main(int argc, char **argv)
 	char *options = NULL;
 	char *orig_dev = NULL;
 	char *currentaddress, *nextaddress;
+	char *value = NULL;
+	char *ep = NULL;
 	int rc = 0;
 	int already_uppercased = 0;
 	int sloppy = 0;
 	size_t options_size = MAX_OPTIONS_LEN;
 	struct parsed_mount_info *parsed_info = NULL;
+	struct parsed_mount_info *reinit_parsed_info = NULL;
 	pid_t pid;
+	uid_t sudo_uid = 0;
 
 	rc = check_setuid();
 	if (rc)
@@ -2053,7 +2066,24 @@ int main(int argc, char **argv)
 		parsed_info = NULL;
 		fprintf(stderr, "Unable to allocate memory: %s\n",
 				strerror(errno));
-		return EX_SYSERR;
+		rc = EX_SYSERR;
+		goto mount_exit;
+	}
+
+	reinit_parsed_info = 
+		(struct parsed_mount_info *) malloc(sizeof(*reinit_parsed_info));
+	if (reinit_parsed_info == NULL) {
+		fprintf(stderr, "Unable to allocate memory: %s\n",
+				strerror(errno));
+		rc = EX_SYSERR;
+		goto mount_exit;
+	}
+
+	options = calloc(options_size, 1);
+	if (!options) {
+		fprintf(stderr, "Unable to allocate memory.\n");
+		rc = EX_SYSERR;
+		goto mount_exit;
 	}
 
 	/* add sharename in opts string as unc= parm */
@@ -2110,10 +2140,13 @@ int main(int argc, char **argv)
 	/* chdir into mountpoint as soon as possible */
 	rc = acquire_mountpoint(&mountpoint);
 	if (rc) {
-		free(orgoptions);
-		return rc;
+		goto mount_exit;
 	}
 
+	/* Before goto assemble_retry, reinitialize parsed_info with reinit_parsed_info */
+	memcpy(reinit_parsed_info, parsed_info,	sizeof(*reinit_parsed_info));
+
+assemble_retry:
 	/*
 	 * mount.cifs does privilege separation. Most of the code to handle
 	 * assembling the mount info is done in a child process that drops
@@ -2131,9 +2164,7 @@ int main(int argc, char **argv)
 		/* child */
 		rc = assemble_mountinfo(parsed_info, thisprogram, mountpoint,
 					orig_dev, orgoptions);
-		free(orgoptions);
-		free(mountpoint);
-		return rc;
+		goto mount_exit;
 	} else {
 		/* parent */
 		pid = wait(&rc);
@@ -2147,13 +2178,6 @@ int main(int argc, char **argv)
 			goto mount_exit;
 	}
 
-	options = calloc(options_size, 1);
-	if (!options) {
-		fprintf(stderr, "Unable to allocate memory.\n");
-		rc = EX_SYSERR;
-		goto mount_exit;
-	}
-
 	currentaddress = parsed_info->addrlist;
 	nextaddress = strchr(currentaddress, ',');
 	if (nextaddress)
@@ -2228,6 +2252,7 @@ mount_retry:
 				if (nextaddress)
 					*nextaddress++ = '\0';
 			}
+			memset(options, 0, sizeof(*options));
 			goto mount_retry;
 		case ENODEV:
 			fprintf(stderr,
@@ -2250,6 +2275,21 @@ mount_retry:
 				already_uppercased = 1;
 				goto mount_retry;
 			}
+			break;
+		case ENOKEY:
+			/* mount could have failed because cruid option was not passed when triggered with sudo */
+			value = getenv("SUDO_UID");
+			if (value && !parsed_info->sudo_uid) {
+				errno = 0;
+				sudo_uid = strtoul(value, &ep, 10);
+				if (errno == 0 && *ep == '\0' && sudo_uid) {
+					/* Reinitialize parsed_info and assemble options again with sudo_uid */
+					memcpy(parsed_info, reinit_parsed_info, sizeof(*parsed_info));
+					parsed_info->sudo_uid = sudo_uid;
+					goto assemble_retry;
+				}
+			}
+			break;
 		}
 		fprintf(stderr, "mount error(%d): %s\n", errno,
 			strerror(errno));
@@ -2276,8 +2316,13 @@ mount_exit:
 		memset(parsed_info->password, 0, sizeof(parsed_info->password));
 		munmap(parsed_info, sizeof(*parsed_info));
 	}
-	free(options);
-	free(orgoptions);
-	free(mountpoint);
+	if (reinit_parsed_info)
+		free(reinit_parsed_info);
+	if (options)
+		free(options);
+	if (orgoptions)
+		free(orgoptions);
+	if (mountpoint)
+		free(mountpoint);
 	return rc;
 }
-- 
2.25.1

Re: [PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

[Aurélien Aptel]

Shyam Prasad N <nspmangalore@gmail.com> writes:
> This is a fix for the scenario of a krb5 user running a "sudo mount".
> Even if the user has cred cache populated, when the mount is run using
> sudo, uid switches to 0. So cred cache for the root user will be
> searched for, unless cruid is specified explicitly. This fix checks
> for cruid=$SUDO_UID as a fallback option, when the mount fails with
> ENOKEY.

The idea seems good.

> @@ -2053,7 +2066,24 @@ int main(int argc, char **argv)
>  		parsed_info = NULL;
>  		fprintf(stderr, "Unable to allocate memory: %s\n",
>  				strerror(errno));
> -		return EX_SYSERR;
> +		rc = EX_SYSERR;
> +		goto mount_exit;
> +	}
> +
> +	reinit_parsed_info = 
> +		(struct parsed_mount_info *) malloc(sizeof(*reinit_parsed_info));
> +	if (reinit_parsed_info == NULL) {
> +		fprintf(stderr, "Unable to allocate memory: %s\n",
> +				strerror(errno));
> +		rc = EX_SYSERR;
> +		goto mount_exit;
> +	}
> +
> +	options = calloc(options_size, 1);
> +	if (!options) {
> +		fprintf(stderr, "Unable to allocate memory.\n");
> +		rc = EX_SYSERR;
> +		goto mount_exit;

This function later forks, so if you allocate before the fork, you need
to free in parent and in the child.
> @@ -2228,6 +2252,7 @@ mount_retry:
>  				if (nextaddress)
>  					*nextaddress++ = '\0';
>  			}
> +			memset(options, 0, sizeof(*options));
>  			goto mount_retry;

Altho not wrong this is a bit misleading as options is a char*. If you
do a memset do it option_size, or do options[0] = 0;

>  		case ENODEV:
>  			fprintf(stderr,
> @@ -2250,6 +2275,21 @@ mount_retry:
>  				already_uppercased = 1;
>  				goto mount_retry;
>  			}

Need to reset options again before goto I guess. Maybe reset option
after the retry label?

>  	}
> -	free(options);
> -	free(orgoptions);
> -	free(mountpoint);
> +	if (reinit_parsed_info)
> +		free(reinit_parsed_info);
> +	if (options)
> +		free(options);
> +	if (orgoptions)
> +		free(orgoptions);
> +	if (mountpoint)
> +		free(mountpoint);
>  	return rc;

free(NULL) is defined to be a no-op, you don't need the checks.

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

[Shyam Prasad N]

Hi Aurélien,

Thanks for the review. Please read my responses inline below.

On Wed, Sep 16, 2020 at 4:26 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
> > This is a fix for the scenario of a krb5 user running a "sudo mount".
> > Even if the user has cred cache populated, when the mount is run using
> > sudo, uid switches to 0. So cred cache for the root user will be
> > searched for, unless cruid is specified explicitly. This fix checks
> > for cruid=$SUDO_UID as a fallback option, when the mount fails with
> > ENOKEY.
>
> The idea seems good.
This was Pavel's suggestion. :)

>
> > @@ -2053,7 +2066,24 @@ int main(int argc, char **argv)
> >               parsed_info = NULL;
> >               fprintf(stderr, "Unable to allocate memory: %s\n",
> >                               strerror(errno));
> > -             return EX_SYSERR;
> > +             rc = EX_SYSERR;
> > +             goto mount_exit;
> > +     }
> > +
> > +     reinit_parsed_info =
> > +             (struct parsed_mount_info *) malloc(sizeof(*reinit_parsed_info));
> > +     if (reinit_parsed_info == NULL) {
> > +             fprintf(stderr, "Unable to allocate memory: %s\n",
> > +                             strerror(errno));
> > +             rc = EX_SYSERR;
> > +             goto mount_exit;
> > +     }
> > +
> > +     options = calloc(options_size, 1);
> > +     if (!options) {
> > +             fprintf(stderr, "Unable to allocate memory.\n");
> > +             rc = EX_SYSERR;
> > +             goto mount_exit;
>
> This function later forks, so if you allocate before the fork, you need
> to free in parent and in the child.

Good point.
I think I'm doing it right though. I allocate all that I need at the beginning.
And the function always terminates in mount_exit, both for parent and
child. That is where the allocations are freed.
I know the child will have the options buffer unnecessarily allocated,
but isn't the code flow simpler this way?
Please let me know if you see an issue there.

> > @@ -2228,6 +2252,7 @@ mount_retry:
> >                               if (nextaddress)
> >                                       *nextaddress++ = '\0';
> >                       }
> > +                     memset(options, 0, sizeof(*options));
> >                       goto mount_retry;
>
> Altho not wrong this is a bit misleading as options is a char*. If you
> do a memset do it option_size, or do options[0] = 0;

Got it. Will do.

>
> >               case ENODEV:
> >                       fprintf(stderr,
> > @@ -2250,6 +2275,21 @@ mount_retry:
> >                               already_uppercased = 1;
> >                               goto mount_retry;
> >                       }
>
> Need to reset options again before goto I guess. Maybe reset option
> after the retry label?

Good catch. This code existed before my changes, and I had noted this
bug. But forgot it during my changes. :)
I was actually confused if I should reset after the label, or before the goto.
After the label is an added overhead in the "happy" code path, so went
with this. But it does reduce the chances of missing out a reset.
For now I'll reset options before each goto mount_retry. Please let me
know if you feel the other approach is better.

>
> >       }
> > -     free(options);
> > -     free(orgoptions);
> > -     free(mountpoint);
> > +     if (reinit_parsed_info)
> > +             free(reinit_parsed_info);
> > +     if (options)
> > +             free(options);
> > +     if (orgoptions)
> > +             free(orgoptions);
> > +     if (mountpoint)
> > +             free(mountpoint);
> >       return rc;
>
> free(NULL) is defined to be a no-op, you don't need the checks.
Sure. Will do.

>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)



-- 
-Shyam

Re: [PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

[Aurélien Aptel]

Shyam Prasad N <nspmangalore@gmail.com> writes:
>> This function later forks, so if you allocate before the fork, you need
>> to free in parent and in the child.
>
> Good point.
> I think I'm doing it right though. I allocate all that I need at the beginning.
> And the function always terminates in mount_exit, both for parent and
> child. That is where the allocations are freed.

Ah yes ok

> I know the child will have the options buffer unnecessarily allocated,
> but isn't the code flow simpler this way?
> Please let me know if you see an issue there.

No it's fine I think

> Good catch. This code existed before my changes, and I had noted this
> bug. But forgot it during my changes. :)
> I was actually confused if I should reset after the label, or before the goto.
> After the label is an added overhead in the "happy" code path, so went
> with this. But it does reduce the chances of missing out a reset.
> For now I'll reset options before each goto mount_retry. Please let me
> know if you feel the other approach is better.

I think we can agree that mount.cifs is not performance critical code
but that it should be safe so I think reset after the label is
better. (To be honnest the whole function could use some refactoring and
be split up probably, but that can be a patch for later on)

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

[Shyam Prasad N]

[-- Attachment #1: Type: text/plain, Size: 1908 bytes --]

Attaching the patch with the review comments incorporated.
Tested a few positive and negative test cases. Works as expected.

Regards,
Shyam

On Thu, Sep 17, 2020 at 2:27 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
> >> This function later forks, so if you allocate before the fork, you need
> >> to free in parent and in the child.
> >
> > Good point.
> > I think I'm doing it right though. I allocate all that I need at the beginning.
> > And the function always terminates in mount_exit, both for parent and
> > child. That is where the allocations are freed.
>
> Ah yes ok
>
> > I know the child will have the options buffer unnecessarily allocated,
> > but isn't the code flow simpler this way?
> > Please let me know if you see an issue there.
>
> No it's fine I think
>
> > Good catch. This code existed before my changes, and I had noted this
> > bug. But forgot it during my changes. :)
> > I was actually confused if I should reset after the label, or before the goto.
> > After the label is an added overhead in the "happy" code path, so went
> > with this. But it does reduce the chances of missing out a reset.
> > For now I'll reset options before each goto mount_retry. Please let me
> > know if you feel the other approach is better.
>
> I think we can agree that mount.cifs is not performance critical code
> but that it should be safe so I think reset after the label is
> better. (To be honnest the whole function could use some refactoring and
> be split up probably, but that can be a patch for later on)
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)



-- 
-Shyam

[-- Attachment #2: 0001-mount.cifs-use-SUDO_UID-env-variable-for-cruid.patch --]
[-- Type: application/octet-stream, Size: 5462 bytes --]

From b0d9ff1490ca2973cbe8701e9c1d65ded0366b8c Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Wed, 16 Sep 2020 00:18:44 -0700
Subject: [PATCH] mount.cifs: use SUDO_UID env variable for cruid

In the current mount.cifs logic, when sudo is used for mount,
uid=0, so the mount command searches for cruid=0 unless explicitly
specified by the user. The user may already have cred cache populated
but mount.cifs would end up searching cred cache for uid=0.

mount.cifs can avoid this confusion by reading the cruid from SUDO_UID
environment variable. If it is set to non-zero, we can make cruid=$SUDO_UID.

However, to maintain backward compatibility, keeping this as a fallback option.
If mount fails with permission denied, then retry with this option.
To enable this fallback, I had to make a few minor changes in the flow.
---
 mount.cifs.c | 69 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 14 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 4feb397..6922d24 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -171,7 +171,11 @@
 
 #define NTFS_TIME_OFFSET ((unsigned long long)(369*365 + 89) * 24 * 3600 * 10000000)
 
-/* struct for holding parsed mount info for use by privileged process */
+/*
+* struct for holding parsed mount info for use by privileged process.
+* Please do not keep pointers in this struct.
+* That way, reinit of this struct is a simple memset.
+*/
 struct parsed_mount_info {
 	unsigned long flags;
 	char host[NI_MAXHOST + 1];
@@ -189,6 +193,7 @@ struct parsed_mount_info {
 	unsigned int verboseflag:1;
 	unsigned int nofail:1;
 	unsigned int got_domain:1;
+	uid_t sudo_uid;
 };
 
 static const char *thisprogram;
@@ -1199,6 +1204,10 @@ nocopy:
 		snprintf(out + out_len, word_len + 5, "uid=%s", txtbuf);
 		out_len = strlen(out);
 	}
+	if (parsed_info->sudo_uid) {
+		cruid = parsed_info->sudo_uid;
+		got_cruid = 1;
+	}
 	if (got_cruid) {
 		word_len = snprintf(txtbuf, sizeof(txtbuf), "%u", cruid);
 
@@ -2012,12 +2021,16 @@ int main(int argc, char **argv)
 	char *options = NULL;
 	char *orig_dev = NULL;
 	char *currentaddress, *nextaddress;
+	char *value = NULL;
+	char *ep = NULL;
 	int rc = 0;
 	int already_uppercased = 0;
 	int sloppy = 0;
 	size_t options_size = MAX_OPTIONS_LEN;
 	struct parsed_mount_info *parsed_info = NULL;
+	struct parsed_mount_info *reinit_parsed_info = NULL;
 	pid_t pid;
+	uid_t sudo_uid = 0;
 
 	rc = check_setuid();
 	if (rc)
@@ -2053,7 +2066,24 @@ int main(int argc, char **argv)
 		parsed_info = NULL;
 		fprintf(stderr, "Unable to allocate memory: %s\n",
 				strerror(errno));
-		return EX_SYSERR;
+		rc = EX_SYSERR;
+		goto mount_exit;
+	}
+
+	reinit_parsed_info = 
+		(struct parsed_mount_info *) malloc(sizeof(*reinit_parsed_info));
+	if (reinit_parsed_info == NULL) {
+		fprintf(stderr, "Unable to allocate memory: %s\n",
+				strerror(errno));
+		rc = EX_SYSERR;
+		goto mount_exit;
+	}
+
+	options = calloc(options_size, 1);
+	if (!options) {
+		fprintf(stderr, "Unable to allocate memory.\n");
+		rc = EX_SYSERR;
+		goto mount_exit;
 	}
 
 	/* add sharename in opts string as unc= parm */
@@ -2110,10 +2140,13 @@ int main(int argc, char **argv)
 	/* chdir into mountpoint as soon as possible */
 	rc = acquire_mountpoint(&mountpoint);
 	if (rc) {
-		free(orgoptions);
-		return rc;
+		goto mount_exit;
 	}
 
+	/* Before goto assemble_retry, reinitialize parsed_info with reinit_parsed_info */
+	memcpy(reinit_parsed_info, parsed_info,	sizeof(*reinit_parsed_info));
+
+assemble_retry:
 	/*
 	 * mount.cifs does privilege separation. Most of the code to handle
 	 * assembling the mount info is done in a child process that drops
@@ -2131,9 +2164,7 @@ int main(int argc, char **argv)
 		/* child */
 		rc = assemble_mountinfo(parsed_info, thisprogram, mountpoint,
 					orig_dev, orgoptions);
-		free(orgoptions);
-		free(mountpoint);
-		return rc;
+		goto mount_exit;
 	} else {
 		/* parent */
 		pid = wait(&rc);
@@ -2147,19 +2178,13 @@ int main(int argc, char **argv)
 			goto mount_exit;
 	}
 
-	options = calloc(options_size, 1);
-	if (!options) {
-		fprintf(stderr, "Unable to allocate memory.\n");
-		rc = EX_SYSERR;
-		goto mount_exit;
-	}
-
 	currentaddress = parsed_info->addrlist;
 	nextaddress = strchr(currentaddress, ',');
 	if (nextaddress)
 		*nextaddress++ = '\0';
 
 mount_retry:
+	options[0] = '\0';
 	if (!currentaddress) {
 		fprintf(stderr, "Unable to find suitable address.\n");
 		rc = parsed_info->nofail ? 0 : EX_FAIL;
@@ -2250,6 +2275,21 @@ mount_retry:
 				already_uppercased = 1;
 				goto mount_retry;
 			}
+			break;
+		case ENOKEY:
+			/* mount could have failed because cruid option was not passed when triggered with sudo */
+			value = getenv("SUDO_UID");
+			if (value && !parsed_info->sudo_uid) {
+				errno = 0;
+				sudo_uid = strtoul(value, &ep, 10);
+				if (errno == 0 && *ep == '\0' && sudo_uid) {
+					/* Reinitialize parsed_info and assemble options again with sudo_uid */
+					memcpy(parsed_info, reinit_parsed_info, sizeof(*parsed_info));
+					parsed_info->sudo_uid = sudo_uid;
+					goto assemble_retry;
+				}
+			}
+			break;
 		}
 		fprintf(stderr, "mount error(%d): %s\n", errno,
 			strerror(errno));
@@ -2276,6 +2316,7 @@ mount_exit:
 		memset(parsed_info->password, 0, sizeof(parsed_info->password));
 		munmap(parsed_info, sizeof(*parsed_info));
 	}
+	free(reinit_parsed_info);
 	free(options);
 	free(orgoptions);
 	free(mountpoint);
-- 
2.25.1

Re: [PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

[Aurélien Aptel]

Shyam Prasad N <nspmangalore@gmail.com> writes:
> Attaching the patch with the review comments incorporated.
> Tested a few positive and negative test cases. Works as expected.

Great, I only have one small comment:
> +	reinit_parsed_info = 
> +		(struct parsed_mount_info *) malloc(sizeof(*reinit_parsed_info));

Don't cast malloc return value (void* gets implicitely promoted to
any pointer type).

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

[Shyam Prasad N]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

Thanks Aurélien.
Here's the updated patch.

Regards,
Shyam

On Thu, Sep 17, 2020 at 3:43 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
> > Attaching the patch with the review comments incorporated.
> > Tested a few positive and negative test cases. Works as expected.
>
> Great, I only have one small comment:
> > +     reinit_parsed_info =
> > +             (struct parsed_mount_info *) malloc(sizeof(*reinit_parsed_info));
>
> Don't cast malloc return value (void* gets implicitely promoted to
> any pointer type).
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)



-- 
-Shyam

[-- Attachment #2: 0001-mount.cifs-use-SUDO_UID-env-variable-for-cruid.patch --]
[-- Type: application/octet-stream, Size: 5429 bytes --]

From 1c478a92f98df3ca211ab08f7fdb6dee7679dc99 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Wed, 16 Sep 2020 00:18:44 -0700
Subject: [PATCH] mount.cifs: use SUDO_UID env variable for cruid

In the current mount.cifs logic, when sudo is used for mount,
uid=0, so the mount command searches for cruid=0 unless explicitly
specified by the user. The user may already have cred cache populated
but mount.cifs would end up searching cred cache for uid=0.

mount.cifs can avoid this confusion by reading the cruid from SUDO_UID
environment variable. If it is set to non-zero, we can make cruid=$SUDO_UID.

However, to maintain backward compatibility, keeping this as a fallback option.
If mount fails with permission denied, then retry with this option.
To enable this fallback, I had to make a few minor changes in the flow.
---
 mount.cifs.c | 68 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 14 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 4feb397..2710cca 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -171,7 +171,11 @@
 
 #define NTFS_TIME_OFFSET ((unsigned long long)(369*365 + 89) * 24 * 3600 * 10000000)
 
-/* struct for holding parsed mount info for use by privileged process */
+/*
+* struct for holding parsed mount info for use by privileged process.
+* Please do not keep pointers in this struct.
+* That way, reinit of this struct is a simple memset.
+*/
 struct parsed_mount_info {
 	unsigned long flags;
 	char host[NI_MAXHOST + 1];
@@ -189,6 +193,7 @@ struct parsed_mount_info {
 	unsigned int verboseflag:1;
 	unsigned int nofail:1;
 	unsigned int got_domain:1;
+	uid_t sudo_uid;
 };
 
 static const char *thisprogram;
@@ -1199,6 +1204,10 @@ nocopy:
 		snprintf(out + out_len, word_len + 5, "uid=%s", txtbuf);
 		out_len = strlen(out);
 	}
+	if (parsed_info->sudo_uid) {
+		cruid = parsed_info->sudo_uid;
+		got_cruid = 1;
+	}
 	if (got_cruid) {
 		word_len = snprintf(txtbuf, sizeof(txtbuf), "%u", cruid);
 
@@ -2012,12 +2021,16 @@ int main(int argc, char **argv)
 	char *options = NULL;
 	char *orig_dev = NULL;
 	char *currentaddress, *nextaddress;
+	char *value = NULL;
+	char *ep = NULL;
 	int rc = 0;
 	int already_uppercased = 0;
 	int sloppy = 0;
 	size_t options_size = MAX_OPTIONS_LEN;
 	struct parsed_mount_info *parsed_info = NULL;
+	struct parsed_mount_info *reinit_parsed_info = NULL;
 	pid_t pid;
+	uid_t sudo_uid = 0;
 
 	rc = check_setuid();
 	if (rc)
@@ -2053,7 +2066,23 @@ int main(int argc, char **argv)
 		parsed_info = NULL;
 		fprintf(stderr, "Unable to allocate memory: %s\n",
 				strerror(errno));
-		return EX_SYSERR;
+		rc = EX_SYSERR;
+		goto mount_exit;
+	}
+
+	reinit_parsed_info = malloc(sizeof(*reinit_parsed_info));
+	if (reinit_parsed_info == NULL) {
+		fprintf(stderr, "Unable to allocate memory: %s\n",
+				strerror(errno));
+		rc = EX_SYSERR;
+		goto mount_exit;
+	}
+
+	options = calloc(options_size, 1);
+	if (!options) {
+		fprintf(stderr, "Unable to allocate memory.\n");
+		rc = EX_SYSERR;
+		goto mount_exit;
 	}
 
 	/* add sharename in opts string as unc= parm */
@@ -2110,10 +2139,13 @@ int main(int argc, char **argv)
 	/* chdir into mountpoint as soon as possible */
 	rc = acquire_mountpoint(&mountpoint);
 	if (rc) {
-		free(orgoptions);
-		return rc;
+		goto mount_exit;
 	}
 
+	/* Before goto assemble_retry, reinitialize parsed_info with reinit_parsed_info */
+	memcpy(reinit_parsed_info, parsed_info,	sizeof(*reinit_parsed_info));
+
+assemble_retry:
 	/*
 	 * mount.cifs does privilege separation. Most of the code to handle
 	 * assembling the mount info is done in a child process that drops
@@ -2131,9 +2163,7 @@ int main(int argc, char **argv)
 		/* child */
 		rc = assemble_mountinfo(parsed_info, thisprogram, mountpoint,
 					orig_dev, orgoptions);
-		free(orgoptions);
-		free(mountpoint);
-		return rc;
+		goto mount_exit;
 	} else {
 		/* parent */
 		pid = wait(&rc);
@@ -2147,19 +2177,13 @@ int main(int argc, char **argv)
 			goto mount_exit;
 	}
 
-	options = calloc(options_size, 1);
-	if (!options) {
-		fprintf(stderr, "Unable to allocate memory.\n");
-		rc = EX_SYSERR;
-		goto mount_exit;
-	}
-
 	currentaddress = parsed_info->addrlist;
 	nextaddress = strchr(currentaddress, ',');
 	if (nextaddress)
 		*nextaddress++ = '\0';
 
 mount_retry:
+	options[0] = '\0';
 	if (!currentaddress) {
 		fprintf(stderr, "Unable to find suitable address.\n");
 		rc = parsed_info->nofail ? 0 : EX_FAIL;
@@ -2250,6 +2274,21 @@ mount_retry:
 				already_uppercased = 1;
 				goto mount_retry;
 			}
+			break;
+		case ENOKEY:
+			/* mount could have failed because cruid option was not passed when triggered with sudo */
+			value = getenv("SUDO_UID");
+			if (value && !parsed_info->sudo_uid) {
+				errno = 0;
+				sudo_uid = strtoul(value, &ep, 10);
+				if (errno == 0 && *ep == '\0' && sudo_uid) {
+					/* Reinitialize parsed_info and assemble options again with sudo_uid */
+					memcpy(parsed_info, reinit_parsed_info, sizeof(*parsed_info));
+					parsed_info->sudo_uid = sudo_uid;
+					goto assemble_retry;
+				}
+			}
+			break;
 		}
 		fprintf(stderr, "mount error(%d): %s\n", errno,
 			strerror(errno));
@@ -2276,6 +2315,7 @@ mount_exit:
 		memset(parsed_info->password, 0, sizeof(parsed_info->password));
 		munmap(parsed_info, sizeof(*parsed_info));
 	}
+	free(reinit_parsed_info);
 	free(options);
 	free(orgoptions);
 	free(mountpoint);
-- 
2.25.1

Re: [PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

[Aurélien Aptel]

Thanks Shyam, looks good to me :)

Shyam Prasad N <nspmangalore@gmail.com> writes:
> Here's the updated patch.

Reviewed-by: Aurelien Aptel <aaptel@suse.com>

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

[Pavel Shilovsky]

Merged. Thanks!
--
Best regards,
Pavel Shilovsky

пн, 21 сент. 2020 г. в 01:19, Aurélien Aptel <aaptel@suse.com>:
>
>
> Thanks Shyam, looks good to me :)
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
> > Here's the updated patch.
>
> Reviewed-by: Aurelien Aptel <aaptel@suse.com>
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

[Shyam Prasad N]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

Hi Pavel,

Please consider the attached patch instead of the one I submitted earlier.
Contains the fix to the bug identified by Aurelien.

Regards,
Shyam

On Tue, Nov 10, 2020 at 5:13 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> Merged. Thanks!
> --
> Best regards,
> Pavel Shilovsky
>
> пн, 21 сент. 2020 г. в 01:19, Aurélien Aptel <aaptel@suse.com>:
> >
> >
> > Thanks Shyam, looks good to me :)
> >
> > Shyam Prasad N <nspmangalore@gmail.com> writes:
> > > Here's the updated patch.
> >
> > Reviewed-by: Aurelien Aptel <aaptel@suse.com>
> >
> > Cheers,
> > --
> > Aurélien Aptel / SUSE Labs Samba Team
> > GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)



-- 
-Shyam

[-- Attachment #2: 0001-mount.cifs-use-SUDO_UID-env-variable-for-cruid.patch --]
[-- Type: application/octet-stream, Size: 6174 bytes --]

From 4933a08f59256bc9fde341d437f85780b1271849 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Wed, 16 Sep 2020 00:18:44 -0700
Subject: [PATCH] mount.cifs: use SUDO_UID env variable for cruid

In the current mount.cifs logic, when sudo is used for mount,
uid=0, so the mount command searches for cruid=0 unless explicitly
specified by the user. The user may already have cred cache populated
but mount.cifs would end up searching cred cache for uid=0.

mount.cifs can avoid this confusion by reading the cruid from SUDO_UID
environment variable. If it is set to non-zero, we can make cruid=$SUDO_UID.

However, to maintain backward compatibility, keeping this as a fallback option.
If mount fails with ENOKEY, then retry with this option.
To enable this fallback, I had to make a few minor changes in the flow.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
Reviewed-by: Aurelien Aptel <aaptel@suse.com>

---
 mount.cifs.c | 83 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 16 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 4feb397..c0fb0e4 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -171,7 +171,11 @@
 
 #define NTFS_TIME_OFFSET ((unsigned long long)(369*365 + 89) * 24 * 3600 * 10000000)
 
-/* struct for holding parsed mount info for use by privileged process */
+/*
+* struct for holding parsed mount info for use by privileged process.
+* Please do not keep pointers in this struct.
+* That way, reinit of this struct is a simple memset.
+*/
 struct parsed_mount_info {
 	unsigned long flags;
 	char host[NI_MAXHOST + 1];
@@ -189,6 +193,8 @@ struct parsed_mount_info {
 	unsigned int verboseflag:1;
 	unsigned int nofail:1;
 	unsigned int got_domain:1;
+	unsigned int is_krb5:1;
+	uid_t sudo_uid;
 };
 
 static const char *thisprogram;
@@ -891,9 +897,12 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
 
 		case OPT_SEC:
 			if (value) {
-				if (!strncmp(value, "none", 4) ||
-				    !strncmp(value, "krb5", 4))
+				if (!strncmp(value, "none", 4))
+					parsed_info->got_password = 1;
+				if (!strncmp(value, "krb5", 4)) {
+					parsed_info->is_krb5 = 1;
 					parsed_info->got_password = 1;
+				}
 			}
 			break;
 
@@ -1199,6 +1208,10 @@ nocopy:
 		snprintf(out + out_len, word_len + 5, "uid=%s", txtbuf);
 		out_len = strlen(out);
 	}
+	if (parsed_info->is_krb5 && parsed_info->sudo_uid) {
+		cruid = parsed_info->sudo_uid;
+		got_cruid = 1;
+	}
 	if (got_cruid) {
 		word_len = snprintf(txtbuf, sizeof(txtbuf), "%u", cruid);
 
@@ -2012,12 +2025,17 @@ int main(int argc, char **argv)
 	char *options = NULL;
 	char *orig_dev = NULL;
 	char *currentaddress, *nextaddress;
+	char *value = NULL;
+	char *ep = NULL;
 	int rc = 0;
 	int already_uppercased = 0;
 	int sloppy = 0;
+	int fallback_sudo_uid = 0;
 	size_t options_size = MAX_OPTIONS_LEN;
 	struct parsed_mount_info *parsed_info = NULL;
+	struct parsed_mount_info *reinit_parsed_info = NULL;
 	pid_t pid;
+	uid_t sudo_uid = 0;
 
 	rc = check_setuid();
 	if (rc)
@@ -2053,7 +2071,23 @@ int main(int argc, char **argv)
 		parsed_info = NULL;
 		fprintf(stderr, "Unable to allocate memory: %s\n",
 				strerror(errno));
-		return EX_SYSERR;
+		rc = EX_SYSERR;
+		goto mount_exit;
+	}
+
+	reinit_parsed_info = malloc(sizeof(*reinit_parsed_info));
+	if (reinit_parsed_info == NULL) {
+		fprintf(stderr, "Unable to allocate memory: %s\n",
+				strerror(errno));
+		rc = EX_SYSERR;
+		goto mount_exit;
+	}
+
+	options = calloc(options_size, 1);
+	if (!options) {
+		fprintf(stderr, "Unable to allocate memory.\n");
+		rc = EX_SYSERR;
+		goto mount_exit;
 	}
 
 	/* add sharename in opts string as unc= parm */
@@ -2110,10 +2144,13 @@ int main(int argc, char **argv)
 	/* chdir into mountpoint as soon as possible */
 	rc = acquire_mountpoint(&mountpoint);
 	if (rc) {
-		free(orgoptions);
-		return rc;
+		goto mount_exit;
 	}
 
+	/* Before goto assemble_retry, reinitialize parsed_info with reinit_parsed_info */
+	memcpy(reinit_parsed_info, parsed_info,	sizeof(*reinit_parsed_info));
+
+assemble_retry:
 	/*
 	 * mount.cifs does privilege separation. Most of the code to handle
 	 * assembling the mount info is done in a child process that drops
@@ -2131,9 +2168,7 @@ int main(int argc, char **argv)
 		/* child */
 		rc = assemble_mountinfo(parsed_info, thisprogram, mountpoint,
 					orig_dev, orgoptions);
-		free(orgoptions);
-		free(mountpoint);
-		return rc;
+		goto mount_child_exit;
 	} else {
 		/* parent */
 		pid = wait(&rc);
@@ -2147,19 +2182,13 @@ int main(int argc, char **argv)
 			goto mount_exit;
 	}
 
-	options = calloc(options_size, 1);
-	if (!options) {
-		fprintf(stderr, "Unable to allocate memory.\n");
-		rc = EX_SYSERR;
-		goto mount_exit;
-	}
-
 	currentaddress = parsed_info->addrlist;
 	nextaddress = strchr(currentaddress, ',');
 	if (nextaddress)
 		*nextaddress++ = '\0';
 
 mount_retry:
+	options[0] = '\0';
 	if (!currentaddress) {
 		fprintf(stderr, "Unable to find suitable address.\n");
 		rc = parsed_info->nofail ? 0 : EX_FAIL;
@@ -2250,6 +2279,24 @@ mount_retry:
 				already_uppercased = 1;
 				goto mount_retry;
 			}
+			break;
+		case ENOKEY:
+			if (!fallback_sudo_uid && parsed_info->is_krb5) {
+				/* mount could have failed because cruid option was not passed when triggered with sudo */
+				value = getenv("SUDO_UID");
+				if (value) {
+					errno = 0;
+					sudo_uid = strtoul(value, &ep, 10);
+					if (errno == 0 && *ep == '\0' && sudo_uid) {
+						/* Reinitialize parsed_info and assemble options again with sudo_uid */
+						memcpy(parsed_info, reinit_parsed_info, sizeof(*parsed_info));
+						parsed_info->sudo_uid = sudo_uid;
+						fallback_sudo_uid = 1;
+						goto assemble_retry;
+					}
+				}
+			}
+			break;
 		}
 		fprintf(stderr, "mount error(%d): %s\n", errno,
 			strerror(errno));
@@ -2276,6 +2323,10 @@ mount_exit:
 		memset(parsed_info->password, 0, sizeof(parsed_info->password));
 		munmap(parsed_info, sizeof(*parsed_info));
 	}
+
+mount_child_exit:
+	/* Objects to be freed both in main process and child */
+	free(reinit_parsed_info);
 	free(options);
 	free(orgoptions);
 	free(mountpoint);
-- 
2.25.1

Re: [PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

[Pavel Shilovsky]

Merged. Thanks!
--
Best regards,
Pavel Shilovsky

пт, 27 нояб. 2020 г. в 02:25, Shyam Prasad N <nspmangalore@gmail.com>:
>
> Hi Pavel,
>
> Please consider the attached patch instead of the one I submitted earlier.
> Contains the fix to the bug identified by Aurelien.
>
> Regards,
> Shyam
>
> On Tue, Nov 10, 2020 at 5:13 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > Merged. Thanks!
> > --
> > Best regards,
> > Pavel Shilovsky
> >
> > пн, 21 сент. 2020 г. в 01:19, Aurélien Aptel <aaptel@suse.com>:
> > >
> > >
> > > Thanks Shyam, looks good to me :)
> > >
> > > Shyam Prasad N <nspmangalore@gmail.com> writes:
> > > > Here's the updated patch.
> > >
> > > Reviewed-by: Aurelien Aptel <aaptel@suse.com>
> > >
> > > Cheers,
> > > --
> > > Aurélien Aptel / SUSE Labs Samba Team
> > > GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> > > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> > > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
>
>
>
> --
> -Shyam