Linux-CIFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] mount.cifs: Fix double-free issue when mounting with setuid root
@ 2019-09-05 18:49 Paulo Alcantara (SUSE)
  2019-09-09 12:46 ` Aurélien Aptel
  0 siblings, 1 reply; 3+ messages in thread
From: Paulo Alcantara (SUSE) @ 2019-09-05 18:49 UTC (permalink / raw)
  To: linux-cifs, samba-technical, piastryyy; +Cc: aaptel, Paulo Alcantara (SUSE)

It can be easily reproduced with the following:

  # chmod +s `which mount.cifs`
  # echo "//localhost/share /mnt cifs \
    users,username=foo,password=XXXX" >> /etc/fstab
  # su - foo
  $ mount /mnt
  free(): double free detected in tcache 2
  Child process terminated abnormally.

The problem was that check_fstab() already freed orgoptions pointer
and then we freed it again in main() function.

Fixes: bf7f48f4c7dc ("mount.cifs.c: fix memory leaks in main func")
Signed-off-by: Paulo Alcantara (SUSE) <paulo@paulo.ac>
---
 mount.cifs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 7748d54aa814..2116fc803311 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -247,7 +247,6 @@ check_fstab(const char *progname, const char *mountpoint, const char *devname,
 	 * set of options. We don't want to trust what the user
 	 * gave us, so just take whatever is in /etc/fstab.
 	 */
-	free(*options);
 	*options = strdup(mnt->mnt_opts);
 	return 0;
 }
@@ -1762,6 +1761,7 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
 		   const char *orig_dev, char *orgoptions)
 {
 	int rc;
+	char *newopts = NULL;
 
 	rc = drop_capabilities(0);
 	if (rc)
@@ -1773,10 +1773,11 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
 
 	if (getuid()) {
 		rc = check_fstab(thisprogram, mountpoint, orig_dev,
-				 &orgoptions);
+				 &newopts);
 		if (rc)
 			goto assemble_exit;
 
+		orgoptions = newopts;
 		/* enable any default user mount flags */
 		parsed_info->flags |= CIFS_SETUID_FLAGS;
 	}
@@ -1880,6 +1881,7 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
 	}
 
 assemble_exit:
+	free(newopts);
 	return rc;
 }
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] mount.cifs: Fix double-free issue when mounting with setuid root
  2019-09-05 18:49 [PATCH] mount.cifs: Fix double-free issue when mounting with setuid root Paulo Alcantara (SUSE)
@ 2019-09-09 12:46 ` Aurélien Aptel
  2019-10-04  0:22   ` Pavel Shilovsky
  0 siblings, 1 reply; 3+ messages in thread
From: Aurélien Aptel @ 2019-09-09 12:46 UTC (permalink / raw)
  To: Paulo Alcantara \(SUSE\), piastryyy, samba-technical, linux-cifs

" Paulo Alcantara (SUSE) " <paulo@paulo.ac> writes:
> It can be easily reproduced with the following:
>
>   # chmod +s `which mount.cifs`
>   # echo "//localhost/share /mnt cifs \
>     users,username=foo,password=XXXX" >> /etc/fstab
>   # su - foo
>   $ mount /mnt
>   free(): double free detected in tcache 2
>   Child process terminated abnormally.
>
> The problem was that check_fstab() already freed orgoptions pointer
> and then we freed it again in main() function.
>
> Fixes: bf7f48f4c7dc ("mount.cifs.c: fix memory leaks in main func")
> Signed-off-by: Paulo Alcantara (SUSE) <paulo@paulo.ac>

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

I've compiled next branch with ASAN and can confirm the double-free and
the fix works

Compiling with ASAN
-------------------

$ CFLAGS=-fsanitize=address \
  LDFLAGS=-fsanitize=address \
  ac_cv_func_malloc_0_nonnull=yes \
  ./configure
$ make clean && make -j4  

Next branch
-----------

$ mount /mnt; echo $?
=================================================================
==29883==ERROR: AddressSanitizer: attempting double-free on 0x607000000020 in thread T0:
    #0 0x7f69d480a1b8 in __interceptor_free (/usr/lib64/libasan.so.4+0xdc1b8)
    #1 0x559381795f33 in main (/sbin/mount.cifs+0xef33)
    #2 0x7f69d4394f89 in __libc_start_main (/lib64/libc.so.6+0x20f89)
    #3 0x55938178e079 in _start (/sbin/mount.cifs+0x7079)

0x607000000020 is located 0 bytes inside of 68-byte region [0x607000000020,0x607000000064)
freed by thread T0 here:
    #0 0x7f69d480a1b8 in __interceptor_free (/usr/lib64/libasan.so.4+0xdc1b8)
    #1 0x55938178e372 in check_fstab (/sbin/mount.cifs+0x7372)
    #2 0x559381794661 in assemble_mountinfo (/sbin/mount.cifs+0xd661)
    #3 0x559381795eef in main (/sbin/mount.cifs+0xeeef)
    #4 0x7f69d4394f89 in __libc_start_main (/lib64/libc.so.6+0x20f89)

previously allocated by thread T0 here:
    #0 0x7f69d480a510 in malloc (/usr/lib64/libasan.so.4+0xdc510)
    #1 0x7f69d43fc2a9 in __GI___strndup (/lib64/libc.so.6+0x882a9)

SUMMARY: AddressSanitizer: double-free (/usr/lib64/libasan.so.4+0xdc1b8) in __interceptor_free
==29883==ABORTING
1

With fix
--------

$ mount /mnt; echo $?
0

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)

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] mount.cifs: Fix double-free issue when mounting with setuid root
  2019-09-09 12:46 ` Aurélien Aptel
@ 2019-10-04  0:22   ` Pavel Shilovsky
  0 siblings, 0 replies; 3+ messages in thread
From: Pavel Shilovsky @ 2019-10-04  0:22 UTC (permalink / raw)
  To: Aurélien Aptel; +Cc: Paulo Alcantara (SUSE), samba-technical, linux-cifs

пн, 9 сент. 2019 г. в 05:46, Aurélien Aptel <aaptel@suse.com>:
>
> " Paulo Alcantara (SUSE) " <paulo@paulo.ac> writes:
> > It can be easily reproduced with the following:
> >
> >   # chmod +s `which mount.cifs`
> >   # echo "//localhost/share /mnt cifs \
> >     users,username=foo,password=XXXX" >> /etc/fstab
> >   # su - foo
> >   $ mount /mnt
> >   free(): double free detected in tcache 2
> >   Child process terminated abnormally.
> >
> > The problem was that check_fstab() already freed orgoptions pointer
> > and then we freed it again in main() function.
> >
> > Fixes: bf7f48f4c7dc ("mount.cifs.c: fix memory leaks in main func")
> > Signed-off-by: Paulo Alcantara (SUSE) <paulo@paulo.ac>
>
> Reviewed-by: Aurelien Aptel <aaptel@suse.com>
>
> I've compiled next branch with ASAN and can confirm the double-free and
> the fix works
>
> Compiling with ASAN
> -------------------
>
> $ CFLAGS=-fsanitize=address \
>   LDFLAGS=-fsanitize=address \
>   ac_cv_func_malloc_0_nonnull=yes \
>   ./configure
> $ make clean && make -j4
>
> Next branch
> -----------
>
> $ mount /mnt; echo $?
> =================================================================
> ==29883==ERROR: AddressSanitizer: attempting double-free on 0x607000000020 in thread T0:
>     #0 0x7f69d480a1b8 in __interceptor_free (/usr/lib64/libasan.so.4+0xdc1b8)
>     #1 0x559381795f33 in main (/sbin/mount.cifs+0xef33)
>     #2 0x7f69d4394f89 in __libc_start_main (/lib64/libc.so.6+0x20f89)
>     #3 0x55938178e079 in _start (/sbin/mount.cifs+0x7079)
>
> 0x607000000020 is located 0 bytes inside of 68-byte region [0x607000000020,0x607000000064)
> freed by thread T0 here:
>     #0 0x7f69d480a1b8 in __interceptor_free (/usr/lib64/libasan.so.4+0xdc1b8)
>     #1 0x55938178e372 in check_fstab (/sbin/mount.cifs+0x7372)
>     #2 0x559381794661 in assemble_mountinfo (/sbin/mount.cifs+0xd661)
>     #3 0x559381795eef in main (/sbin/mount.cifs+0xeeef)
>     #4 0x7f69d4394f89 in __libc_start_main (/lib64/libc.so.6+0x20f89)
>
> previously allocated by thread T0 here:
>     #0 0x7f69d480a510 in malloc (/usr/lib64/libasan.so.4+0xdc510)
>     #1 0x7f69d43fc2a9 in __GI___strndup (/lib64/libc.so.6+0x882a9)
>
> SUMMARY: AddressSanitizer: double-free (/usr/lib64/libasan.so.4+0xdc1b8) in __interceptor_free
> ==29883==ABORTING
> 1
>
> With fix
> --------
>
> $ mount /mnt; echo $?
> 0
>
> 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)

Merged, thanks.
--
Best regards,
Pavel Shilovsky

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-05 18:49 [PATCH] mount.cifs: Fix double-free issue when mounting with setuid root Paulo Alcantara (SUSE)
2019-09-09 12:46 ` Aurélien Aptel
2019-10-04  0:22   ` Pavel Shilovsky

Linux-CIFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cifs/0 linux-cifs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cifs linux-cifs/ https://lore.kernel.org/linux-cifs \
		linux-cifs@vger.kernel.org linux-cifs@archiver.kernel.org
	public-inbox-index linux-cifs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cifs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox