From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miloslav =?UTF-8?Q?Trma=C4=8D?= Subject: Re: [PATCH 1/2] audit: fix NUL handling in untrusted strings Date: Thu, 11 Sep 2008 22:03:06 +0200 Message-ID: <1221163386.17533.52.camel@amilo> References: <1221085418.2705.19.camel@amilo> <1221143113.2992.9.camel@localhost.localdomain> <48C955C8.2000602@redhat.com> <1221156612.17533.14.camel@amilo> <48C96D90.70608@redhat.com> <1221161260.17533.30.camel@amilo> <48C975D5.3020603@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <48C975D5.3020603@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: John Dennis Cc: linux-audit , viro@zeniv.linux.org.uk, linux-kernel List-Id: linux-audit@redhat.com John Dennis p=C3=AD=C5=A1e v =C4=8Ct 11. 09. 2008 v 15:47 -0400: > Miloslav Trma=C4=8D wrote:=20 > > If the interface says "NUL-terminated string", any bytes after that a= re > > not "actual data". > Yes, that's correct. However, the function in question, > audit_log_n_untrustedstring() is not an interface accepting a null > terminated string, it accepts a count. These options are not exclusive - see strnlen(). audit_log_n_untrustedstring(), as patched, follows that model exactly. > > Yes, that's why it was wrong to use audit_*string() for TTY input dat= a. > > And the 2/2 patch fixes it - at the source of the problem, not in an > > unrelated function that was incorrectly used. > > =20 > This is true, but it's only part of the problem, the string functions > still need to be robust, even used inappropriately. They need to do what they promise in the first place, only then can they possibly do something useful with invalid input. AUDIT_USER_TTY messages, like all messages sent from user-space, are sent with a trailing NUL. The contents of the message are (more or less) trusted, it is perfectly reasonable to stop at first NUL. audit_log_n_untrustedstring(), as patched, does exactly what is needed - the length parameter is used only to make unrelated kernel data does not appear in the audit record.=20 Now, given a function called fn(), either behavior (terminating at NUL, or treating NUL as a control character) is valid, and AUDIT_USER_TTY handling can be written for both behaviors of fn(). But given the function is called ...untrustedstring, and "string" in C implies NUL-terminated, I think it is more consistent to stop on NUL. Mirek