[PATCH] fsck: use xasprintf to avoid buffer overruns with an insane fs type

[PATCH] fsck: use xasprintf to avoid buffer overruns with an insane fs type

[Theodore Ts'o]

This prevents a crash when running the command:

fsck -t AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA /dev/sda

Reported-by: Hornseth_Brenan@bah.com
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 disk-utils/fsck.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/disk-utils/fsck.c b/disk-utils/fsck.c
index 58fd8ac59..8a07bc272 100644
--- a/disk-utils/fsck.c
+++ b/disk-utils/fsck.c
@@ -544,20 +544,20 @@ static char *find_fsck(const char *type)
 {
 	char *s;
 	const char *tpl;
-	static char prog[256];
+	static char *prog = NULL;
 	char *p = xstrdup(fsck_path);
 
 	/* Are we looking for a program or just a type? */
 	tpl = (strncmp(type, "fsck.", 5) ? "%s/fsck.%s" : "%s/%s");
 
 	for(s = strtok(p, ":"); s; s = strtok(NULL, ":")) {
-		sprintf(prog, tpl, s, type);
+		xasprintf(&prog, tpl, s, type);
 		if (access(prog, X_OK) == 0)
 			break;
+		free(prog); prog = NULL;
 	}
 	free(p);
-
-	return(s ? prog : NULL);
+	return(prog);
 }
 
 static int progress_active(void)
@@ -885,7 +885,7 @@ static int wait_many(int flags)
  */
 static int fsck_device(struct libmnt_fs *fs, int interactive)
 {
-	char progname[80], *progpath;
+	char *progname, *progpath;
 	const char *type;
 	int retval;
 
@@ -902,9 +902,10 @@ static int fsck_device(struct libmnt_fs *fs, int interactive)
 	else
 		type = DEFAULT_FSTYPE;
 
-	sprintf(progname, "fsck.%s", type);
+	xasprintf(&progname, "fsck.%s", type);
 	progpath = find_fsck(progname);
 	if (progpath == NULL) {
+		free(progname);
 		if (fs_check_required(type)) {
 			retval = ENOENT;
 			goto err;
@@ -914,6 +915,8 @@ static int fsck_device(struct libmnt_fs *fs, int interactive)
 
 	num_running++;
 	retval = execute(progname, progpath, type, fs, interactive);
+	free(progname);
+	free(progpath);
 	if (retval) {
 		num_running--;
 		goto err;
-- 
2.16.1.72.g5be1f00a9a

Re: [PATCH] fsck: use xasprintf to avoid buffer overruns with an insane fs type

[Karel Zak]

On Thu, Feb 15, 2018 at 03:05:08PM -0500, Theodore Ts'o wrote:
>  disk-utils/fsck.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)

Applied, thanks!

    Karel


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

Re: [PATCH] fsck: use xasprintf to avoid buffer overruns with an insane fs type

[Peter Cordes]

On Thu, Feb 15, 2018 at 03:05:08PM -0500, Theodore Ts'o wrote:
> This prevents a crash when running the command:
> 
> fsck -t AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA /dev/sda
> 
> Reported-by: Hornseth_Brenan@bah.com
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  disk-utils/fsck.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/disk-utils/fsck.c b/disk-utils/fsck.c
> index 58fd8ac59..8a07bc272 100644
> --- a/disk-utils/fsck.c
> +++ b/disk-utils/fsck.c
> @@ -544,20 +544,20 @@ static char *find_fsck(const char *type)
>  {
>  	char *s;
>  	const char *tpl;
> -	static char prog[256];
> +	static char *prog = NULL;

 You're allocating / freeing every time it's used, so it shouldn't be
static anymore.

It might be easier to just use snprintf to truncate long strings,
instead of introducing dynamic allocation which requires explicit
freeing.  OTOH xasprintf makes it re-entrant / thread-safe, at the
cost of forcing the caller to care about memory management.  (And at
the cost of efficiency: prog is allocated / freed inside the loop.)

>  	char *p = xstrdup(fsck_path);
>  
>  	/* Are we looking for a program or just a type? */
>  	tpl = (strncmp(type, "fsck.", 5) ? "%s/fsck.%s" : "%s/%s");
>  
>  	for(s = strtok(p, ":"); s; s = strtok(NULL, ":")) {
> -		sprintf(prog, tpl, s, type);
> +		xasprintf(&prog, tpl, s, type);
>  		if (access(prog, X_OK) == 0)
>  			break;
> +		free(prog); prog = NULL;
>  	}
>  	free(p);
> -
> -	return(s ? prog : NULL);
> +	return(prog);
>  }


-- 
#define X(x,y) x##y
Peter Cordes ;  e-mail: X(peter@cor , des.ca)

"The gods confound the man who first found out how to distinguish the hours!
 Confound him, too, who in this place set up a sundial, to cut and hack
 my day so wretchedly into small pieces!" -- Plautus, 200 BC

Re: [PATCH] fsck: use xasprintf to avoid buffer overruns with an insane fs type

[Theodore Ts'o]

On Fri, Feb 16, 2018 at 11:55:05AM -0400, Peter Cordes wrote:
> > -	static char prog[256];
> > +	static char *prog = NULL;
> 
>  You're allocating / freeing every time it's used, so it shouldn't be
> static anymore.

Good point.  Karels has already accepted the patch, but if he hasn't
pushed it out, maybe he can ammend it.

> It might be easier to just use snprintf to truncate long strings,
> instead of introducing dynamic allocation which requires explicit
> freeing.  OTOH xasprintf makes it re-entrant / thread-safe, at the
> cost of forcing the caller to care about memory management.  (And at
> the cost of efficiency: prog is allocated / freed inside the loop.)

That's certainly an alternative, and in fact that's how I fixed it in
the old version of fsck still shipping in e2fsprogs (there are a few
places that still use it).

My main reason for using it there is that in e2fsprogs we don't assume
the use of xasprintf and family (it's a GNU extension, and e2fsprogs
tries to support legacy platforms such as MacOS and Windows :-).

However, given that fsck does use xasprintf already (and so there is
no portability advantag) and it's a lot easier to prove (or at least
satisfy to oneself) that an attacker who is trying to play tricks
can't do something unexpected when using xasprintf versus snprintf, I
went with asprintf in my patch to util-linux.

Cheers,

					- Ted

Re: [PATCH] fsck: use xasprintf to avoid buffer overruns with an insane fs type

[Karel Zak]

On Fri, Feb 16, 2018 at 11:55:05AM -0400, Peter Cordes wrote:
> On Thu, Feb 15, 2018 at 03:05:08PM -0500, Theodore Ts'o wrote:
> > This prevents a crash when running the command:
> > 
> > fsck -t AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA /dev/sda
> > 
> > Reported-by: Hornseth_Brenan@bah.com
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > ---
> >  disk-utils/fsck.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/disk-utils/fsck.c b/disk-utils/fsck.c
> > index 58fd8ac59..8a07bc272 100644
> > --- a/disk-utils/fsck.c
> > +++ b/disk-utils/fsck.c
> > @@ -544,20 +544,20 @@ static char *find_fsck(const char *type)
> >  {
> >  	char *s;
> >  	const char *tpl;
> > -	static char prog[256];
> > +	static char *prog = NULL;
> 
>  You're allocating / freeing every time it's used, so it shouldn't be
> static anymore.

Ah, I miss the "static". Thanks.

> It might be easier to just use snprintf to truncate long strings,
> instead of introducing dynamic allocation which requires explicit
> freeing.  OTOH xasprintf makes it re-entrant / thread-safe, at the
> cost of forcing the caller to care about memory management.  (And at
> the cost of efficiency: prog is allocated / freed inside the loop.)

Well, I don't think dynamic allocation so big issue in this case, but
I'll try to improve it on Monday to make the code more elegant.

Maybe all we need is to check -t argument and reject non-senses
already in main() ;-)

    Karel

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

Re: [PATCH] fsck: use xasprintf to avoid buffer overruns with an insane fs type

[Peter Cordes]

On Fri, Feb 16, 2018 at 07:08:20PM +0100, Karel Zak wrote:
> On Fri, Feb 16, 2018 at 11:55:05AM -0400, Peter Cordes wrote:
> > On Thu, Feb 15, 2018 at 03:05:08PM -0500, Theodore Ts'o wrote:
> > > This prevents a crash when running the command:
> > > 
> > > fsck -t AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA /dev/sda
> > > 
> > > Reported-by: Hornseth_Brenan@bah.com
> > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > ---
> > >  disk-utils/fsck.c | 15 +++++++++------
> > >  1 file changed, 9 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/disk-utils/fsck.c b/disk-utils/fsck.c
> > > index 58fd8ac59..8a07bc272 100644
> > > --- a/disk-utils/fsck.c
> > > +++ b/disk-utils/fsck.c
> > > @@ -544,20 +544,20 @@ static char *find_fsck(const char *type)
> > >  {
> > >  	char *s;
> > >  	const char *tpl;
> > > -	static char prog[256];
> > > +	static char *prog = NULL;
> > 
> >  You're allocating / freeing every time it's used, so it shouldn't be
> > static anymore.
> 
> Ah, I miss the "static". Thanks.
> 
> > It might be easier to just use snprintf to truncate long strings,
> > instead of introducing dynamic allocation which requires explicit
> > freeing.  OTOH xasprintf makes it re-entrant / thread-safe, at the
> > cost of forcing the caller to care about memory management.  (And at
> > the cost of efficiency: prog is allocated / freed inside the loop.)
> 
> Well, I don't think dynamic allocation so big issue in this case, but
> I'll try to improve it on Monday to make the code more elegant.
> 
> Maybe all we need is to check -t argument and reject non-senses
> already in main() ;-)

Ted makes a good point that xasprintf makes it easier to reason about
correctness, and that's probably the most important consideration for
FSCK.  This code hardly needs to be efficient, and malloc/free aren't
terrible especially in a single-threaded program.

OTOH, I was worried for a while about a possible memory leak until I
figured out that leaving the loop without freeing was intentional, and
it was returning that memory to the caller.  (Of course it does;
that's the whole point of the function; but still,  if() break; before
free() looked worrying.)

---

If your proposed check in main() is based on length, then static buffer
size should probably also be set in main, otherwise you have magic
constants in two places that have to match.

Maybe have main() pass in a pointer + size for find_fsck to write
into (using snprintf)?

-- 
#define X(x,y) x##y
Peter Cordes ;  e-mail: X(peter@cor , des.ca)

"The gods confound the man who first found out how to distinguish the hours!
 Confound him, too, who in this place set up a sundial, to cut and hack
 my day so wretchedly into small pieces!" -- Plautus, 200 BC

Re: [PATCH] fsck: use xasprintf to avoid buffer overruns with an insane fs type

[Theodore Ts'o]

On Fri, Feb 16, 2018 at 07:08:20PM +0100, Karel Zak wrote:
> 
> Maybe all we need is to check -t argument and reject non-senses
> already in main() ;-)

I also thought about rejecting any file system type larger than, say,
16 characters.  Then I looked at the kernel code, and in theory
there's nothing stopping someone from creating an out-of-tree kernel
file system with a file system type of, say, for example:

	supercalifragilisticexpialidocious

:-)

					- Ted