linux-kernel - Re: [PATCH 0/2] vsprintf: ignore %n again

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130916155504.GC13318@ZenIV.linux.org.uk>
Date:	Mon, 16 Sep 2013 16:55:04 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Kees Cook <keescook@...omium.org>
Cc:	linux-kernel@...r.kernel.org, joe@...ches.com,
	George Spelvin <linux@...izon.com>, dan.carpenter@...cle.com,
	Jan Beulich <JBeulich@...e.com>,
	KOSAKI Motohiro <kosaki.motohiro@...il.com>,
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
	akpm@...ux-foundation.org
Subject: Re: [PATCH 0/2] vsprintf: ignore %n again

On Mon, Sep 16, 2013 at 12:43:55AM -0700, Kees Cook wrote:
> Whether seq_printf should return void or error, %n still needs to be removed.
> As such, instead of changing the seq_file structure and adding instructions
> to all callers of seq_printf, just examine seq->count for the callers that
> care about how many characters were put into the buffer, as suggested by
> George Spelvin. First patch removes all %n usage in favor of checking
> seq->count before/after. Second patch makes %n ignore its argument.

This is completely pointless.  *ANY* untrusted format string kernel-side
is pretty much it.  Blocking %n is not "defense in depth", it's security
theater.  Again, if attacker can feed an arbitrary format string to
vsnprintf(), it's over - you've lost.  It's not just about information
leaks vs. ability to store a value of attacker's choosing at the address
of attacker's choosing as it was in userland.  Kernel-side an ability to
trigger read from an arbitrary address is much nastier than information
leak risk; consider iomem, for starters.

What we ought to do is prevention of _that_.  AFAICS, we have reasonably
few call chains that might transmit format string; most of the calls
are with plain and simple string literal.  I wonder if could get away
with reasonable amount of annotations to catch such crap...

Consider, e.g. introducing __vsnprint(), with vsnprintf(s, n, fmt, ...)
expanding to __vsnprintf(1, s, n, fmt, ...) if fmt is a string literal
and __vsnprintf(0, s, n, fmt, ...) otherwise.  Now,
int __sprintf(int safe, char *buf, const char *fmt, ...)
{
        va_list args;
        int i;

        va_start(args, fmt);
        i = __vsnprintf(safe, buf, INT_MAX, fmt, args);
        va_end(args);

        return i;
}
and #define for sprintf (expanding it to either __sprintf(1, ...)
or __sprintf(0, ...)).  That plus similar for snprintf and seq_printf
will already take care of most of the call chains leading to __vsnprintf() -
relatively few calls with have 0 passed to it.  Add WARN_ON(!safe) to
__vsnprintf and we probably won't drown in warnings.  Now, we can start
adding things like that to remaining call chains *and* do things like
replacing
                        snd_iprintf(buffer, fields[i].format,
                                *get_dummy_ll_ptr(dummy, fields[i].offset));
with
			/* fields[i].format is known to be a valid format */
                        __snd_iprintf(1, buffer, fields[i].format,
                                *get_dummy_ll_ptr(dummy, fields[i].offset));
to deal with the places where the origin of format string is provably safe,
but not a string literal (actually, s/1/__FORMAT_IS_SAFE/, to make it
greppable).

Comments?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/