linux-kernel - Re: [PATCH 3/3] 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] [day] [month] [year] [list]

Message-ID: <CAGXu5jKmmH=7fVEP+W0iWM+oafcNyCrvmOwgKNNSzhacPTWD9w@mail.gmail.com>
Date:	Tue, 8 Oct 2013 14:12:56 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
	Joe Perches <joe@...ches.com>
Subject: Re: [PATCH 3/3] vsprintf: ignore %n again

On Tue, Oct 8, 2013 at 1:28 PM, Andrew Morton <akpm@...ux-foundation.org> wrote:
> On Mon,  7 Oct 2013 19:56:51 -0700 Kees Cook <keescook@...omium.org> wrote:
>
>> This ignores %n in printf again, as was originally documented. Implementing
>> %n poses a greater security risk than utility, so it should stay ignored.
>> To help anyone attempting to use %n, a warning will be emitted if it is
>> encountered.
>>
>> Based on earlier patch by Joe Perches.
>
> Well this sucks.  Nowhere in this patchset are we told what is the
> alleged security risk with %n.  There's even a runtime warning telling
> people not to use it, but we've provided no way for them to find out
> *why*.
>
> Please send along suitable changelog text so I can fix this up.

Perhaps add these two paragraphs to the end of the "vsprintf: ignore
%n again" commit:

Because %n was designed to write to pointers on the stack, it has been
frequently used as an attack vector when bugs are found that leak
user-controlled strings into functions that ultimately process format
strings. While this class of bug can still be turned into an
information leak, removing %n eliminates the common method of
elevating such a bug into an arbitrary kernel memory writing
primitive, significantly reducing the danger of this class of bug.

For seq_file users that need to know the length of a written string
for padding, please see seq_setwidth() and seq_pad() instead.

> A new checkpatch rule might be appropriate?

I can look into that -- I worry it won't be very effective since
checkpatch lacks the knowledge of which functions are taking format
strings, and looking for just %n may lead to some false positives.
Maybe I can look for the common case of %n" (at the end of a string
literal).

> Two of these patches were acked-by:you.  But you sent the patches, so I
> changed these to Signed-off-by:, as per
> Documentation/SubmittingPatches, section 12.

Ah! Yes, thanks for fixing that.

-Kees

-- 
Kees Cook
Chrome OS Security
--
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/