| Message ID | 1452019093-4556-1-git-send-email-jann@thejh.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 5, 2016 at 10:38 AM, Jann Horn <jann@thejh.net> wrote: > Let %h and %e print empty values as "!", "." as "!" and > ".." as "!.". > > This prevents hostnames and comm values that are empty or > consist of one or two dots from changing the directory > level at which the corefile will be stored. > > It seems very unlikely that this caused security issues > anywhere, so I'm not requesting a stable backport. > > Signed-off-by: Jann Horn <jann@thejh.net> Acked-by: Kees Cook <keescook@chromium.org> -Kees > --- > fs/coredump.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/fs/coredump.c b/fs/coredump.c > index dfc87c5..689577b 100644 > --- a/fs/coredump.c > +++ b/fs/coredump.c > @@ -120,6 +120,26 @@ int cn_esc_printf(struct core_name *cn, const char *fmt, ...) > ret = cn_vprintf(cn, fmt, arg); > va_end(arg); > > + if (ret == 0) { > + /* > + * Ensure that this coredump name component can't cause the > + * resulting corefile path to contain a ".." or "." component. > + */ > + if ((cn->used - cur == 1 && cn->corename[cur] == '.') || > + (cn->used - cur == 2 && cn->corename[cur] == '.' > + && cn->corename[cur+1] == '.')) > + cn->corename[cur] = '!'; > + > + /* > + * Empty names are fishy and could be used to create a "//" in a > + * corefile name, causing the coredump to happen one directory > + * level too high. Enforce that all components of the core > + * pattern are at least one character long. > + */ > + if (cn->used == cur) > + ret = cn_printf(cn, "!"); > + } > + > for (; cur < cn->used; ++cur) { > if (cn->corename[cur] == '/') > cn->corename[cur] = '!'; > -- > 2.1.4 >
On Tue, 5 Jan 2016 19:38:13 +0100 Jann Horn <jann@thejh.net> wrote: > Let %h and %e print empty values as "!", "." as "!" and > ".." as "!.". How can an empty name cause the core file name to contain a "//"? And how can a "//" "cause the coredump to happen one directory level too high"? That's what ".." does. And a "//" in a pathname is the same as a "/"? And what's wrong with a plain old "."? That doesn't change the directory level. Confused. > This prevents hostnames and comm values that are empty or > consist of one or two dots from changing the directory > level at which the corefile will be stored. > > It seems very unlikely that this caused security issues > anywhere, so I'm not requesting a stable backport. > > ... > > --- a/fs/coredump.c > +++ b/fs/coredump.c > @@ -120,6 +120,26 @@ int cn_esc_printf(struct core_name *cn, const char *fmt, ...) > ret = cn_vprintf(cn, fmt, arg); > va_end(arg); > > + if (ret == 0) { > + /* > + * Ensure that this coredump name component can't cause the > + * resulting corefile path to contain a ".." or "." component. > + */ > + if ((cn->used - cur == 1 && cn->corename[cur] == '.') || > + (cn->used - cur == 2 && cn->corename[cur] == '.' > + && cn->corename[cur+1] == '.')) > + cn->corename[cur] = '!'; > + > + /* > + * Empty names are fishy and could be used to create a "//" in a > + * corefile name, causing the coredump to happen one directory > + * level too high. Enforce that all components of the core > + * pattern are at least one character long. > + */ > + if (cn->used == cur) > + ret = cn_printf(cn, "!"); > + } > + > for (; cur < cn->used; ++cur) { > if (cn->corename[cur] == '/') -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 05, 2016 at 01:52:25PM -0800, Andrew Morton wrote: > On Tue, 5 Jan 2016 19:38:13 +0100 Jann Horn <jann@thejh.net> wrote: > > > Let %h and %e print empty values as "!", "." as "!" and > > ".." as "!.". > > How can an empty name cause the core file name to contain a "//"? > > And how can a "//" "cause the coredump to happen one directory level > too high"? That's what ".." does. And a "//" in a pathname is the > same as a "/"? > > And what's wrong with a plain old "."? That doesn't change the > directory level. > > Confused. Consider the case where someone decides to sort coredumps by hostname with a core pattern like "/cores/%h/core.%e.%p.%t" or so. In this case, hostnames "" and "." would cause the coredump to land directly in /cores, which is not what the intent behind the core pattern is, and ".." would cause the coredump to land in /. Yeah, there probably aren't many people who do that, but I still don't want this edgecase to be kind of broken.
diff --git a/fs/coredump.c b/fs/coredump.c index dfc87c5..689577b 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -120,6 +120,26 @@ int cn_esc_printf(struct core_name *cn, const char *fmt, ...) ret = cn_vprintf(cn, fmt, arg); va_end(arg); + if (ret == 0) { + /* + * Ensure that this coredump name component can't cause the + * resulting corefile path to contain a ".." or "." component. + */ + if ((cn->used - cur == 1 && cn->corename[cur] == '.') || + (cn->used - cur == 2 && cn->corename[cur] == '.' + && cn->corename[cur+1] == '.')) + cn->corename[cur] = '!'; + + /* + * Empty names are fishy and could be used to create a "//" in a + * corefile name, causing the coredump to happen one directory + * level too high. Enforce that all components of the core + * pattern are at least one character long. + */ + if (cn->used == cur) + ret = cn_printf(cn, "!"); + } + for (; cur < cn->used; ++cur) { if (cn->corename[cur] == '/') cn->corename[cur] = '!';
Let %h and %e print empty values as "!", "." as "!" and ".." as "!.". This prevents hostnames and comm values that are empty or consist of one or two dots from changing the directory level at which the corefile will be stored. It seems very unlikely that this caused security issues anywhere, so I'm not requesting a stable backport. Signed-off-by: Jann Horn <jann@thejh.net> --- fs/coredump.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)