linux-kernel - Re: [PATCH 1/2] audit: fix NUL handling in untrusted strings

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>] [day] [month] [year] [list]

Message-Id: <1221163386.17533.52.camel@amilo>
Date:	Thu, 11 Sep 2008 22:03:06 +0200
From:	Miloslav Trmač <mitr@...hat.com>
To:	John Dennis <jdennis@...hat.com>
Cc:	Eric Paris <eparis@...hat.com>,
	linux-audit <linux-audit@...hat.com>, viro@...iv.linux.org.uk,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] audit: fix NUL handling in untrusted strings

John Dennis píše v Čt 11. 09. 2008 v 15:47 -0400:
> Miloslav Trmač wrote: 
> > If the interface says "NUL-terminated string", any bytes after that are
> > 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 data.
> > And the 2/2 patch fixes it - at the source of the problem, not in an
> > unrelated function that was incorrectly used.
> >   
> 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. 

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

--
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/