On 18/4/24 01:28, Andreas Hasenack wrote:
Hi,
in Ubuntu we are building most packages with -D_FORTIFY_SOURCE=3
nowadays, and we just got a bug report that 5.1.9 was crashing with a
buffer overflow warning. When rebuilt with -D_FORTIFY_SOURCE=2, it
does not crash.
Here is a small reproducer using a loop device. This is on kernel 6.8.0:
/etc/auto.master:
/- file,sun:/etc/auto.mp strictexpire
"strictexpire" is what triggers the crash.
/etc/auto.mp:
/mp defaults :/dev/loop0
# automount -f -d3
Starting automounter version 5.1.9, master map /etc/auto.master
using kernel protocol version 5.05
lookup_nss_read_master: reading master file /etc/auto.master
do_init: parse(sun): init gathered global options: (null)
lookup_read_master: lookup(file): read entry /-
master_do_mount: mounting /-
reading file map /etc/auto.mp
do_init: parse(sun): init gathered global options: (null)
*** buffer overflow detected ***: terminated
Aborted (core dumped)
gdb show this being in the snprintf call in lib/mounts.c when
",strictexpire" is being added to the autofs mount options string:
#9 0x00007ffff7dbaab4 in snprintf (__fmt=0x7ffff7dca232 "%s", __n=93,
__s=0x7fffec002c1c "") at
/usr/include/x86_64-linux-gnu/bits/stdio2.h:54
No locals.
#10 make_options_string (path=0x5555555b7d50 "/-", pipefd=6,
type=type@entry=0x7ffff7dca02b "direct", flags=2560) at
/usr/src/autofs-5.1.9-1ubuntu3/lib/mounts.c:764
kver_major = <optimized out>
kver_minor = 5
options = 0x7fffec002bf0
"fd=6,pgrp=22935,minproto=5,maxproto=5,direct"
max_len = 93
len = 44
new = <optimized out>
__FUNCTION__ = "make_options_string"
lib/mounts.c:760
/* maybe add ",strictexpire" */
if (flags & MOUNT_FLAG_STRICTEXPIRE) {
new = snprintf(options + len,
max_len, "%s", ",strictexpire");
I don't think this is actually overflowing options in this particular
case, but the max_len argument doesn't seem right, as that was the
original max size for options.
Ha, I had report of this just the other day and I missed that obvious
stupid mistake so thanks
for reporting it.
I chose to change the snprintf() to strcat() because it clearly wasn't
going to overflow as
the calculated maximum size was sure to be larger than the contents.
fyi, what I used was this but I'll update the description to include the
max_len usage mistake
you have pointed out (and push the change to the repo. in case you
prefer to use it in Ubuntu):
autofs-5.1.9 - fix crash in make_options_string()
From: Ian Kent <raven@xxxxxxxxxx>
glibc claims there's a memory overflow when make_options_string() tries
to use the pointer <malloc()ed options string> + <current offset> with
snprintf().
The maximum options string length is calculated earlier in the function
and analysis shows there's no overflow possible.
To fix this use strcat(3) instead of snprintf(), in this case it's
probably less overhead anyway. While we are at it drop the useless error
checks because we know it won't overflow.
Signed-off-by: Ian Kent <raven@xxxxxxxxxx>
---
lib/mounts.c | 35 +++++++++--------------------------
1 file changed, 9 insertions(+), 26 deletions(-)
diff --git a/lib/mounts.c b/lib/mounts.c
index 05f18dbcf..7680c59c1 100644
--- a/lib/mounts.c
+++ b/lib/mounts.c
@@ -695,10 +695,11 @@ static int cacl_max_options_len(unsigned int flags)
unsigned int kver_minor = get_kver_minor();
int max_len;
- /* %d and %u are maximum lenght of 10 and mount type is maximum
- * length of 9 (e. ",indirect").
+ /* %d and %u are maximum length of 10 and mount type is maximum
+ * length of 9 (ie. ",indirect").
* The base temaplate is "fd=%d,pgrp=%u,minproto=5,maxproto=%d"
- * plus the length of mount type plus 1 for the NULL.
+ * plus the length of mount type plus 1 for the NULL (and an
+ * additional 10 characters for good measure!).
*/
max_len = 79 + 1;
@@ -728,7 +729,7 @@ char *make_options_string(char *path, int pipefd,
unsigned int kver_major = get_kver_major();
unsigned int kver_minor = get_kver_minor();
char *options;
- int max_len, len, new;
+ int max_len, len;
max_len = cacl_max_options_len(flags);
@@ -751,21 +752,13 @@ char *make_options_string(char *path, int pipefd,
if (len < 0)
goto error_out;
- if (len >= max_len)
- goto truncated;
-
if (kver_major < 5 || (kver_major == 5 && kver_minor < 4))
goto out;
/* maybe add ",strictexpire" */
if (flags & MOUNT_FLAG_STRICTEXPIRE) {
- new = snprintf(options + len,
- max_len, "%s", ",strictexpire");
- if (new < 0)
- goto error_out;
- len += new;
- if (len >= max_len)
- goto truncated;
+ strcat(options, ",strictexpire");
+ len += 13;
}
if (kver_major == 5 && kver_minor < 5)
@@ -773,23 +766,13 @@ char *make_options_string(char *path, int pipefd,
/* maybe add ",ignore" */
if (flags & MOUNT_FLAG_IGNORE) {
- new = snprintf(options + len,
- max_len, "%s", ",ignore");
- if (new < 0)
- goto error_out;
- len += new;
- if (len >= max_len)
- goto truncated;
+ strcat(options, ",ignore");
+ len += 7;
}
out:
options[len] = '\0';
return options;
-truncated:
- logerr("buffer to small for options - truncated");
- len = max_len -1;
- goto out;
-
error_out:
logerr("error constructing mount options string for %s", path);
free(options);
