[PATCH] libmount: Fix access to uninitialised value in mnt_optstr_locate_option

[PATCH] libmount: Fix access to uninitialised value in mnt_optstr_locate_option

[soeren]

From: Sören Tempel <soeren@soeren-tempel.net>

Consider the following libmount example program:

	#include <libmount.h>

	int
	main(void)
	{
		mnt_match_options("", "+");
		return 0;
	}

Compiling this program and executing it with valgrind(1) will yield
the following warning regarding a conditional jump depending on an
uninitialised value:

	Conditional jump or move depends on uninitialised value(s)
	   at 0x48AA61B: strlen (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
	   by 0x48C6154: ??? (in /lib/libmount.so.1.1.0)
	   by 0x48C65A0: mnt_optstr_get_option (in /lib/libmount.so.1.1.0)
	   by 0x48C7B85: mnt_match_options (in /lib/libmount.so.1.1.0)
	   by 0x1091C1: main (util-linux-test.c:6)

This is because if name == "+" then we advance to the null byte
in name due to the following code in mnt_match_options():

	if (*name == '+')
		name++, namesz--;

This will cause the `xstrncpy(buf, name, namesz + 1)` invocation in
mnt_match_options() to copy nothing to the destination buffer. The
buffer (buf) is therefore passed uninitialized as the name argument
to mnt_optstr_get_option(). When mnt_optstr_locate_option() (which
is called by mnt_optstr_get_option) attempts to determine the
length of the name argument using strlen(3) then everything blows
up because the name argument is not initialized.

This patch fixes this issue by initializing the buf argument in
mnt_match_options() with NULL before calling xstrncpy thereby
ensuring that buf is /always/ initialized even if xstrncpy
returns without copying any data to the destination buffer
due to the following early return in xstrncpy:

	size_t len = src ? strlen(src) : 0;
	if (!len)
		return;

This issue has been discovered using KLEE <https://klee.github.io/>.

Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
---
 libmount/src/optstr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libmount/src/optstr.c b/libmount/src/optstr.c
index a8b56e212..ae3efc78b 100644
--- a/libmount/src/optstr.c
+++ b/libmount/src/optstr.c
@@ -853,6 +853,7 @@ int mnt_match_options(const char *optstr, const char *pattern)
 		else if ((no = (startswith(name, "no") != NULL)))
 			name += 2, namesz -= 2;
 
+		buf = NULL; /* ensure buf is initialized even if name == "" */
 		xstrncpy(buf, name, namesz + 1);
 
 		rc = mnt_optstr_get_option(optstr, buf, &val, &sz);

[PATCH v2] libmount: Fix access to uninitialised value in mnt_optstr_locate_option

[soeren]

From: Sören Tempel <soeren@soeren-tempel.net>

Consider the following libmount example program:

	#include <libmount.h>

	int
	main(void)
	{
		mnt_match_options("", "+");
		return 0;
	}

Compiling this program and executing it with valgrind(1) will yield
the following warning regarding a conditional jump depending on an
uninitialised value:

	Conditional jump or move depends on uninitialised value(s)
	   at 0x48AA61B: strlen (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
	   by 0x48C6154: ??? (in /lib/libmount.so.1.1.0)
	   by 0x48C65A0: mnt_optstr_get_option (in /lib/libmount.so.1.1.0)
	   by 0x48C7B85: mnt_match_options (in /lib/libmount.so.1.1.0)
	   by 0x1091C1: main (util-linux-test.c:6)

This is because if name == "+" then we advance to the null byte
in name due to the following code in mnt_match_options():

	if (*name == '+')
		name++, namesz--;

This will cause the `xstrncpy(buf, name, namesz + 1)` invocation in
mnt_match_options() to copy nothing to the destination buffer. The
buffer (buf) is therefore passed uninitialized as the name argument
to mnt_optstr_get_option(). When mnt_optstr_locate_option() (which
is called by mnt_optstr_get_option) attempts to determine the
length of the name argument using strlen(3) then everything blows
up because the name argument is not initialized.

This patch fixes this issue by initializing the buf argument in
mnt_match_options() with NULL before calling xstrncpy thereby
ensuring that buf is /always/ initialized even if xstrncpy
returns without copying any data to the destination buffer
due to the following early return in xstrncpy:

	size_t len = src ? strlen(src) : 0;
	if (!len)
		return;

This issue has been discovered using KLEE <https://klee.github.io/>.

Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
---
Changes since v1: Obviously, we shouldn't set buf to NULL
unconditionally. Instead, duplicate the check for the xstrncpy early
return in mnt_match_options and thereby only set buf to NULL if xstrncpy
didn't copy data to it. While at it, also free buf. This is a bit hacky,
ideally xstrncpy should be modified in a way that it never leaves the
first argument uninitialized I suppose?

 libmount/src/optstr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libmount/src/optstr.c b/libmount/src/optstr.c
index a8b56e212..719e16ab3 100644
--- a/libmount/src/optstr.c
+++ b/libmount/src/optstr.c
@@ -854,6 +854,10 @@ int mnt_match_options(const char *optstr, const char *pattern)
 			name += 2, namesz -= 2;
 
 		xstrncpy(buf, name, namesz + 1);
+		if (namesz == 0) { /* if xstrncpy didn't copy anything */
+			free(buf);
+			buf = NULL;
+		}
 
 		rc = mnt_optstr_get_option(optstr, buf, &val, &sz);
 

Re: [PATCH v2] libmount: Fix access to uninitialised value in mnt_optstr_locate_option

[Sören Tempel]

soeren@soeren-tempel.net wrote:
>  		xstrncpy(buf, name, namesz + 1);
> +		if (namesz == 0) { /* if xstrncpy didn't copy anything */
> +			free(buf);
> +			buf = NULL;
> +		}

This solution also has the issue that it handles the case incorrectly
where xstrncpy actually calls strlen(3). I wanted to avoid that but
maybe the best solution is to just change the xstrncpy API in a way that
it indicates whether it has written anything through an int return
value?

Sorry for all the noise. Next time, I will just write a bug report...

Greetings,
Sören

Re: [PATCH v2] libmount: Fix access to uninitialised value in mnt_optstr_locate_option

[Karel Zak]

On Sat, Feb 25, 2023 at 02:40:52PM +0100, Sören Tempel wrote:
> This solution also has the issue that it handles the case incorrectly
> where xstrncpy actually calls strlen(3). I wanted to avoid that but
> maybe the best solution is to just change the xstrncpy API in a way that
> it indicates whether it has written anything through an int return
> value?

The best will be to avoid all the buffer, xstrndup(), and         
mnt_optstr_get_option() using if the options string or the pattern is   
empty.  It's unnecessary because we know that mnt_optstr_get_option()
will return nothing.

I have committed
https://github.com/util-linux/util-linux/commit/06ee5267516761721ebfbdfa313980cef8e54c66

> Sorry for all the noise. Next time, I will just write a bug report...

That's no problem. We should be friendly to people who invest time to
report bugs and write code ;-)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH v2] libmount: Fix access to uninitialised value in mnt_optstr_locate_option

[Sören Tempel]

Hey Karel,

Thanks for your patch, the changes look good to me!

I have one small follow-up question though. You added the following
sentence to the documentation for mnt_match_options():

> The alone "no" is error and all matching ends with False.

I believe this to be a good change. However, If my understanding of
this documentation comment is correct I would have expected

	mnt_match_options("bla", "no,,")

to return False as well but to my surprise it actually returns True. I
understand that this is an edge case but is that intended? Maybe the ","
should be treated as an terminating character for the "no" as well (i.e.
in addition to the null byte)?

diff --git a/libmount/src/optstr.c b/libmount/src/optstr.c
index 4fbbb0859..a0056c767 100644
--- a/libmount/src/optstr.c
+++ b/libmount/src/optstr.c
@@ -865,7 +865,7 @@ int mnt_match_options(const char *optstr, const char *pattern)
 			name++, namesz--;
 		else if ((no = (startswith(name, "no") != NULL))) {
 			name += 2, namesz -= 2;
-			if (!*name) {
+			if (!*name || *name == ',') {
 				match = 0;
 				break;	/* alone "no" keyword is error */
 			}

With this patch applied it also returns False for the "no,," pattern.

Greetings,
Sören

Karel Zak <kzak@redhat.com> wrote:
> On Sat, Feb 25, 2023 at 02:40:52PM +0100, Sören Tempel wrote:
> > This solution also has the issue that it handles the case incorrectly
> > where xstrncpy actually calls strlen(3). I wanted to avoid that but
> > maybe the best solution is to just change the xstrncpy API in a way that
> > it indicates whether it has written anything through an int return
> > value?
> 
> The best will be to avoid all the buffer, xstrndup(), and         
> mnt_optstr_get_option() using if the options string or the pattern is   
> empty.  It's unnecessary because we know that mnt_optstr_get_option()
> will return nothing.
> 
> I have committed
> https://github.com/util-linux/util-linux/commit/06ee5267516761721ebfbdfa313980cef8e54c66
> 
> > Sorry for all the noise. Next time, I will just write a bug report...
> 
> That's no problem. We should be friendly to people who invest time to
> report bugs and write code ;-)
> 
>     Karel

Re: [PATCH v2] libmount: Fix access to uninitialised value in mnt_optstr_locate_option

[Karel Zak]

On Mon, Feb 27, 2023 at 08:00:55PM +0100, Sören Tempel wrote:
> Thanks for your patch, the changes look good to me!

Thanks.

> I have one small follow-up question though. You added the following
> sentence to the documentation for mnt_match_options():
> 
> > The alone "no" is error and all matching ends with False.
> 
> I believe this to be a good change. However, If my understanding of
> this documentation comment is correct I would have expected
> 
> 	mnt_match_options("bla", "no,,")

Good point.

> 
> to return False as well but to my surprise it actually returns True. I
> understand that this is an edge case but is that intended? Maybe the ","
> should be treated as an terminating character for the "no" as well (i.e.
> in addition to the null byte)?
> 
> diff --git a/libmount/src/optstr.c b/libmount/src/optstr.c
> index 4fbbb0859..a0056c767 100644
> --- a/libmount/src/optstr.c
> +++ b/libmount/src/optstr.c
> @@ -865,7 +865,7 @@ int mnt_match_options(const char *optstr, const char *pattern)
>  			name++, namesz--;
>  		else if ((no = (startswith(name, "no") != NULL))) {
>  			name += 2, namesz -= 2;
> -			if (!*name) {
> +			if (!*name || *name == ',') {
>  				match = 0;
>  				break;	/* alone "no" keyword is error */
>  			}
> 
> With this patch applied it also returns False for the "no,," pattern.

 Applied, thanks!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com