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

[Shyam Prasad N <nspmangalore@gmail.com>]

[-- 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