[PATCH v2 2/2] libmount: handle arbitrary line length for mounts

[PATCH v2 2/2] libmount: handle arbitrary line length for mounts

[Kees Cook]

The kernel's maximum path length is PATH_MAX (4096). The use of BUFSIZ
(8192) would seem sufficient for reading mountinfo files, but it's
not. Paths may contain escaped characters (requiring 4x as many bytes
to read), and filesystem options are of unknown length. To avoid mounts
being either intentionally or unintentionally hidden from libmount and
its users, we must accept arbitrary length lines when parsing.

Long valid entries are currently ignored, with warnings like this:
mount: /proc/self/mountinfo: parse error: ignore entry at line 11.
mount: /proc/self/mountinfo: parse error: ignore entry at line 12.

Instead of using a malloc on every line parsed from mount files, do it
once per mount file context, growing it as needed. The general case will
never grow it.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2:
- use getline, duh. thanks vapier!
---
 libmount/src/mountP.h    |  2 ++
 libmount/src/tab.c       |  1 +
 libmount/src/tab_parse.c | 19 +++++++++----------
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/libmount/src/mountP.h b/libmount/src/mountP.h
index 660e0ad..cd02785 100644
--- a/libmount/src/mountP.h
+++ b/libmount/src/mountP.h
@@ -212,6 +212,8 @@ struct libmnt_table {
 	int		comms;		/* enable/disable comment parsing */
 	char		*comm_intro;	/* First comment in file */
 	char		*comm_tail;	/* Last comment in file */
+	char		*buf;		/* Memory for reading lines */
+	size_t		buf_size;	/* Size of line-reading buffer */
 
 	struct libmnt_cache *cache;		/* canonicalized paths/tags cache */
 
diff --git a/libmount/src/tab.c b/libmount/src/tab.c
index 0df8d49..5e3069d 100644
--- a/libmount/src/tab.c
+++ b/libmount/src/tab.c
@@ -166,6 +166,7 @@ void mnt_free_table(struct libmnt_table *tb)
 	mnt_unref_cache(tb->cache);
 	free(tb->comm_intro);
 	free(tb->comm_tail);
+	free(tb->buf);
 	free(tb);
 }
 
diff --git a/libmount/src/tab_parse.c b/libmount/src/tab_parse.c
index ca8a618..1af8e6a 100644
--- a/libmount/src/tab_parse.c
+++ b/libmount/src/tab_parse.c
@@ -424,7 +424,6 @@ static int mnt_table_parse_next(struct libmnt_table *tb, FILE *f,
 				struct libmnt_fs *fs,
 				const char *filename, int *nlines)
 {
-	char buf[BUFSIZ];
 	char *s;
 	int rc;
 
@@ -435,17 +434,17 @@ static int mnt_table_parse_next(struct libmnt_table *tb, FILE *f,
 	/* read the next non-blank non-comment line */
 next_line:
 	do {
-		if (fgets(buf, sizeof(buf), f) == NULL)
+		if (getline(&tb->buf, &tb->buf_size, f) < 0)
 			return -EINVAL;
 		++*nlines;
-		s = strchr (buf, '\n');
+		s = strchr(tb->buf, '\n');
 		if (!s) {
 			/* Missing final newline?  Otherwise an extremely */
 			/* long line - assume file was corrupted */
 			if (feof(f)) {
 				DBG(TAB, ul_debugobj(tb,
 					"%s: no final newline",	filename));
-				s = strchr (buf, '\0');
+				s = strchr(tb->buf, '\0');
 			} else {
 				DBG(TAB, ul_debugobj(tb,
 					"%s:%d: missing newline at line",
@@ -457,12 +456,12 @@ next_line:
 		/* comments parser */
 		if (tb->comms
 		    && (tb->fmt == MNT_FMT_GUESS || tb->fmt == MNT_FMT_FSTAB)
-		    && is_comment_line(buf)) {
+		    && is_comment_line(tb->buf)) {
 			do {
-				rc = append_comment(tb, fs, buf, feof(f));
+				rc = append_comment(tb, fs, tb->buf, feof(f));
 				if (!rc)
-					rc = next_comment_line(buf,
-							sizeof(buf),
+					rc = next_comment_line(tb->buf,
+							tb->buf_size,
 							f, &s, nlines);
 			} while (rc == 0);
 
@@ -474,9 +473,9 @@ next_line:
 		}
 
 		*s = '\0';
-		if (--s >= buf && *s == '\r')
+		if (--s >= tb->buf && *s == '\r')
 			*s = '\0';
-		s = (char *) skip_blank(buf);
+		s = (char *) skip_blank(tb->buf);
 	} while (*s == '\0' || *s == '#');
 
 	if (tb->fmt == MNT_FMT_GUESS) {
-- 
1.9.1


-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 2/2] libmount: handle arbitrary line length for mounts

[Karel Zak]

On Wed, Aug 26, 2015 at 03:49:23PM -0700, Kees Cook wrote:
> @@ -212,6 +212,8 @@ struct libmnt_table {
>  	int		comms;		/* enable/disable comment parsing */
>  	char		*comm_intro;	/* First comment in file */
>  	char		*comm_tail;	/* Last comment in file */
> +	char		*buf;		/* Memory for reading lines */
> +	size_t		buf_size;	/* Size of line-reading buffer */

It's overkill to add the buffer to the struct, it's necessary 
only in mnt_table_parse_next().

    Karel

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

Re: [PATCH v2 2/2] libmount: handle arbitrary line length for mounts

[Kees Cook]

On Wed, Aug 26, 2015 at 4:34 PM, Karel Zak <kzak@redhat.com> wrote:
> On Wed, Aug 26, 2015 at 03:49:23PM -0700, Kees Cook wrote:
>> @@ -212,6 +212,8 @@ struct libmnt_table {
>>       int             comms;          /* enable/disable comment parsing */
>>       char            *comm_intro;    /* First comment in file */
>>       char            *comm_tail;     /* Last comment in file */
>> +     char            *buf;           /* Memory for reading lines */
>> +     size_t          buf_size;       /* Size of line-reading buffer */
>
> It's overkill to add the buffer to the struct, it's necessary
> only in mnt_table_parse_next().

It seemed really inefficient to go from a stack buffer to a
malloc-per-call of mnt_table_parse_next(). I'm fine to write it that
way, though. What do you think is best here?

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 2/2] libmount: handle arbitrary line length for mounts

[Mike Frysinger]

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

On 26 Aug 2015 15:49, Kees Cook wrote:
> --- a/libmount/src/mountP.h
> +++ b/libmount/src/mountP.h
> @@ -212,6 +212,8 @@ struct libmnt_table {
>  	int		comms;		/* enable/disable comment parsing */
>  	char		*comm_intro;	/* First comment in file */
>  	char		*comm_tail;	/* Last comment in file */
> +	char		*buf;		/* Memory for reading lines */
> +	size_t		buf_size;	/* Size of line-reading buffer */

just to be clear, this structure is zero-allocated right ?  getline
needs the first call to have buf==NULL in order for everything to
fall into place.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/2] libmount: handle arbitrary line length for mounts

[Kees Cook]

On Wed, Aug 26, 2015 at 5:16 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 26 Aug 2015 15:49, Kees Cook wrote:
>> --- a/libmount/src/mountP.h
>> +++ b/libmount/src/mountP.h
>> @@ -212,6 +212,8 @@ struct libmnt_table {
>>       int             comms;          /* enable/disable comment parsing */
>>       char            *comm_intro;    /* First comment in file */
>>       char            *comm_tail;     /* Last comment in file */
>> +     char            *buf;           /* Memory for reading lines */
>> +     size_t          buf_size;       /* Size of line-reading buffer */
>
> just to be clear, this structure is zero-allocated right ?  getline
> needs the first call to have buf==NULL in order for everything to
> fall into place.

It is, yes. See mnt_new_table()'s calloc usage.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 2/2] libmount: handle arbitrary line length for mounts

[Karel Zak]

On Wed, Aug 26, 2015 at 04:44:04PM -0700, Kees Cook wrote:
> On Wed, Aug 26, 2015 at 4:34 PM, Karel Zak <kzak@redhat.com> wrote:
> > On Wed, Aug 26, 2015 at 03:49:23PM -0700, Kees Cook wrote:
> >> @@ -212,6 +212,8 @@ struct libmnt_table {
> >>       int             comms;          /* enable/disable comment parsing */
> >>       char            *comm_intro;    /* First comment in file */
> >>       char            *comm_tail;     /* Last comment in file */
> >> +     char            *buf;           /* Memory for reading lines */
> >> +     size_t          buf_size;       /* Size of line-reading buffer */
> >
> > It's overkill to add the buffer to the struct, it's necessary
> > only in mnt_table_parse_next().
> 
> It seemed really inefficient to go from a stack buffer to a
> malloc-per-call of mnt_table_parse_next(). I'm fine to write it that
> way, though. What do you think is best here?

I'll try to prepare a patch with new struct libmnt_parser where we can
store another parser specific things like current line number, file
name, etc.

    Karel

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

Re: [PATCH v2 2/2] libmount: handle arbitrary line length for mounts

[Karel Zak]

On Wed, Aug 26, 2015 at 03:49:23PM -0700, Kees Cook wrote:
>  libmount/src/mountP.h    |  2 ++
>  libmount/src/tab.c       |  1 +
>  libmount/src/tab_parse.c | 19 +++++++++----------
>  3 files changed, 12 insertions(+), 10 deletions(-)

I have applied a different version of the patch. See below. Thanks!

It would be possible to use the new libmnt_parser on more places in
the code, but we are in -rc2, so I don't want to be so invasive now.

    Karel


>From d92b8c3ba138f07a7432b367feb599141ce76121 Mon Sep 17 00:00:00 2001
From: Karel Zak <kzak@redhat.com>
Date: Thu, 27 Aug 2015 10:49:39 +0200
Subject: [PATCH] libmount: handle arbitrary line length for mounts

Based on patch from Kees Cook, he wrote:
> The kernel's maximum path length is PATH_MAX (4096). The use of BUFSIZ
> (8192) would seem sufficient for reading mountinfo files, but it's
> not. Paths may contain escaped characters (requiring 4x as many bytes
> to read), and filesystem options are of unknown length. To avoid
> mounts being either intentionally or unintentionally hidden from
> libmount and its users, we must accept arbitrary length lines when
> parsing.
>
> Long valid entries are currently ignored, with warnings like this:
> mount: /proc/self/mountinfo: parse error: ignore entry at line 11.
> mount: /proc/self/mountinfo: parse error: ignore entry at line 12.
>
> Instead of using a malloc on every line parsed from mount files, do it
> once per mount file context, growing it as needed. The general case
> will never grow it.

I have moved the parser stuff to the new struct libmnt_parser, maybe
we can move more things (e.g. libmnt_table->fmt) to this struct later.

Reported-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Karel Zak <kzak@redhat.com>
---
 libmount/src/tab_parse.c | 81 +++++++++++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 32 deletions(-)

diff --git a/libmount/src/tab_parse.c b/libmount/src/tab_parse.c
index ca8a618..2e55500 100644
--- a/libmount/src/tab_parse.c
+++ b/libmount/src/tab_parse.c
@@ -22,6 +22,22 @@
 #include "pathnames.h"
 #include "strutils.h"
 
+struct libmnt_parser {
+	FILE	*f;		/* fstab, mtab, swaps or mountinfo ... */
+	char	*filename;	/* file name or NULL */
+	char	*buf;		/* buffer (the current line content) */
+	size_t	bufsiz;		/* size of the buffer */
+	size_t	line;		/* current line */
+};
+
+static void parser_cleanup(struct libmnt_parser *pa)
+{
+	if (!pa)
+		return;
+	free(pa->buf);
+	memset(pa, 0, sizeof(*pa));
+}
+
 static int next_number(char **s, int *num)
 {
 	char *end = NULL;
@@ -378,16 +394,15 @@ static int is_terminated_by_blank(const char *str)
  *         1 if the line is not a comment
  *        <0 on error
  */
-static int next_comment_line(char *buf, size_t bufsz,
-			     FILE *f, char **last, int *nlines)
+static int next_comment_line(struct libmnt_parser *pa, char **last)
 {
-	if (fgets(buf, bufsz, f) == NULL)
-		return feof(f) ? 1 : -EINVAL;
+	if (getline(&pa->buf, &pa->bufsiz, pa->f) < 0)
+		return feof(pa->f) ? 1 : -errno;
 
-	++*nlines;
-	*last = strchr(buf, '\n');
+	pa->line++;
+	*last = strchr(pa->buf, '\n');
 
-	return is_comment_line(buf) ? 0 : 1;
+	return is_comment_line(pa->buf) ? 0 : 1;
 }
 
 static int append_comment(struct libmnt_table *tb,
@@ -420,36 +435,35 @@ static int append_comment(struct libmnt_table *tb,
 /*
  * Read and parse the next line from {fs,m}tab or mountinfo
  */
-static int mnt_table_parse_next(struct libmnt_table *tb, FILE *f,
-				struct libmnt_fs *fs,
-				const char *filename, int *nlines)
+static int mnt_table_parse_next(struct libmnt_parser *pa,
+				struct libmnt_table *tb,
+				struct libmnt_fs *fs)
 {
-	char buf[BUFSIZ];
 	char *s;
 	int rc;
 
 	assert(tb);
-	assert(f);
+	assert(pa);
 	assert(fs);
 
 	/* read the next non-blank non-comment line */
 next_line:
 	do {
-		if (fgets(buf, sizeof(buf), f) == NULL)
+		if (getline(&pa->buf, &pa->bufsiz, pa->f) < 0)
 			return -EINVAL;
-		++*nlines;
-		s = strchr (buf, '\n');
+		pa->line++;
+		s = strchr(pa->buf, '\n');
 		if (!s) {
 			/* Missing final newline?  Otherwise an extremely */
 			/* long line - assume file was corrupted */
-			if (feof(f)) {
+			if (feof(pa->f)) {
 				DBG(TAB, ul_debugobj(tb,
-					"%s: no final newline",	filename));
-				s = strchr (buf, '\0');
+					"%s: no final newline",	pa->filename));
+				s = strchr(pa->buf, '\0');
 			} else {
 				DBG(TAB, ul_debugobj(tb,
-					"%s:%d: missing newline at line",
-					filename, *nlines));
+					"%s:%zu: missing newline at line",
+					pa->filename, pa->line));
 				goto err;
 			}
 		}
@@ -457,16 +471,14 @@ next_line:
 		/* comments parser */
 		if (tb->comms
 		    && (tb->fmt == MNT_FMT_GUESS || tb->fmt == MNT_FMT_FSTAB)
-		    && is_comment_line(buf)) {
+		    && is_comment_line(pa->buf)) {
 			do {
-				rc = append_comment(tb, fs, buf, feof(f));
+				rc = append_comment(tb, fs, pa->buf, feof(pa->f));
 				if (!rc)
-					rc = next_comment_line(buf,
-							sizeof(buf),
-							f, &s, nlines);
+					rc = next_comment_line(pa, &s);
 			} while (rc == 0);
 
-			if (rc == 1 && feof(f))
+			if (rc == 1 && feof(pa->f))
 				rc = append_comment(tb, fs, NULL, 1);
 			if (rc < 0)
 				return rc;
@@ -474,9 +486,9 @@ next_line:
 		}
 
 		*s = '\0';
-		if (--s >= buf && *s == '\r')
+		if (--s >= pa->buf && *s == '\r')
 			*s = '\0';
-		s = (char *) skip_blank(buf);
+		s = (char *) skip_blank(pa->buf);
 	} while (*s == '\0' || *s == '#');
 
 	if (tb->fmt == MNT_FMT_GUESS) {
@@ -508,7 +520,7 @@ next_line:
 	if (rc == 0)
 		return 0;
 err:
-	DBG(TAB, ul_debugobj(tb, "%s:%d: %s parse error", filename, *nlines,
+	DBG(TAB, ul_debugobj(tb, "%s:%zu: %s parse error", pa->filename, pa->line,
 				tb->fmt == MNT_FMT_MOUNTINFO ? "mountinfo" :
 				tb->fmt == MNT_FMT_SWAPS ? "swaps" :
 				tb->fmt == MNT_FMT_FSTAB ? "tab" : "utab"));
@@ -516,7 +528,7 @@ err:
 	/* by default all errors are recoverable, otherwise behavior depends on
 	 * the errcb() function. See mnt_table_set_parser_errcb().
 	 */
-	return tb->errcb ? tb->errcb(tb, filename, *nlines) : 1;
+	return tb->errcb ? tb->errcb(tb, pa->filename, pa->line) : 1;
 }
 
 static pid_t path_to_tid(const char *filename)
@@ -595,11 +607,11 @@ static int kernel_fs_postparse(struct libmnt_table *tb,
  */
 int mnt_table_parse_stream(struct libmnt_table *tb, FILE *f, const char *filename)
 {
-	int nlines = 0;
 	int rc = -1;
 	int flags = 0;
 	pid_t tid = -1;
 	struct libmnt_fs *fs = NULL;
+	struct libmnt_parser pa = { .line = 0 };
 
 	assert(tb);
 	assert(f);
@@ -609,6 +621,9 @@ int mnt_table_parse_stream(struct libmnt_table *tb, FILE *f, const char *filenam
 				filename, mnt_table_get_nents(tb),
 				tb->fltrcb ? "yes" : "not"));
 
+	pa.filename = filename;
+	pa.f = f;
+
 	/* necessary for /proc/mounts only, the /proc/self/mountinfo
 	 * parser sets the flag properly
 	 */
@@ -622,7 +637,7 @@ int mnt_table_parse_stream(struct libmnt_table *tb, FILE *f, const char *filenam
 				goto err;
 		}
 
-		rc = mnt_table_parse_next(tb, f, fs, filename, &nlines);
+		rc = mnt_table_parse_next(&pa, tb, fs);
 
 		if (!rc && tb->fltrcb && tb->fltrcb(fs, tb->fltrcb_data))
 			rc = 1;	/* filtered out by callback... */
@@ -653,9 +668,11 @@ int mnt_table_parse_stream(struct libmnt_table *tb, FILE *f, const char *filenam
 
 	DBG(TAB, ul_debugobj(tb, "%s: stop parsing (%d entries)",
 				filename, mnt_table_get_nents(tb)));
+	parser_cleanup(&pa);
 	return 0;
 err:
 	DBG(TAB, ul_debugobj(tb, "%s: parse error (rc=%d)", filename, rc));
+	parser_cleanup(&pa);
 	return rc;
 }
 
-- 
2.4.3



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

Re: [PATCH v2 2/2] libmount: handle arbitrary line length for mounts

[Kees Cook]

On Thu, Aug 27, 2015 at 2:26 AM, Karel Zak <kzak@redhat.com> wrote:
> On Wed, Aug 26, 2015 at 03:49:23PM -0700, Kees Cook wrote:
>>  libmount/src/mountP.h    |  2 ++
>>  libmount/src/tab.c       |  1 +
>>  libmount/src/tab_parse.c | 19 +++++++++----------
>>  3 files changed, 12 insertions(+), 10 deletions(-)
>
> I have applied a different version of the patch. See below. Thanks!
>
> It would be possible to use the new libmnt_parser on more places in
> the code, but we are in -rc2, so I don't want to be so invasive now.

This looks great, thanks!

-Kees

-- 
Kees Cook
Chrome OS Security