From mboxrd@z Thu Jan  1 00:00:00 1970
From: Miloslav =?UTF-8?Q?Trma=C4=8D?= <mitr@redhat.com>
Subject: Re: [PATCH 1/2] audit: fix NUL handling in untrusted strings
Date: Thu, 11 Sep 2008 16:43:19 +0200
Message-ID: <1221144199.3033.47.camel@amilo>
References: <1221085418.2705.19.camel@amilo>
	<1221143113.2992.9.camel@localhost.localdomain>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Return-path: <linux-audit-bounces@redhat.com>
In-Reply-To: <1221143113.2992.9.camel@localhost.localdomain>
List-Unsubscribe: <https://www.redhat.com/mailman/listinfo/linux-audit>,
	<mailto:linux-audit-request@redhat.com?subject=unsubscribe>
List-Archive: <https://www.redhat.com/archives/linux-audit>
List-Post: <mailto:linux-audit@redhat.com>
List-Help: <mailto:linux-audit-request@redhat.com?subject=help>
List-Subscribe: <https://www.redhat.com/mailman/listinfo/linux-audit>,
	<mailto:linux-audit-request@redhat.com?subject=subscribe>
Sender: linux-audit-bounces@redhat.com
Errors-To: linux-audit-bounces@redhat.com
To: Eric Paris <eparis@redhat.com>
Cc: linux-audit <linux-audit@redhat.com>, viro@zeniv.linux.org.uk, linux-kernel <linux-kernel@vger.kernel.org>
List-Id: linux-audit@redhat.com

Thanks for the review.
Eric Paris p=C3=AD=C5=A1e v =C4=8Ct 11. 09. 2008 v 10:25 -0400:
> On Thu, 2008-09-11 at 00:23 +0200, Miloslav Trma=C4=8D wrote:
> > This patch modifies audit_log_n_untrustedstring() to only log the dat=
a
> > before the first NUL byte, if any.
>=20
> I'm going to have to say NAK on this patch.
>=20
> It's still not right looking at the other user,
> audit_log_single_execve_arg().  An execve arg with a NULL could loose
> the stuff after the NULL (not break the record like audit_tty) since th=
e
> execve uses %s rather than calling trusted string.
execve() arguments are NUL-terminated strings:
audit_log_single_execve_arg() starts with
	len_left =3D len =3D strnlen_user(p, MAX_ARG_STRLEN) - 1;
and the two cycles handle chunks in the first "len" bytes of "p".
audit_log_single_execve_arg() never touches any bytes after the first
NUL.

So, when audit_log_single_execve_arg() calls
audit_string_contains_control(buf, to_send), strlen(buf) =3D=3D to_send a=
nd
the patch does not change anything.

> How about we change the meaning of audit_string_contains_control()
> return values?  If it returns positive that is the number of bytes in a
> legitimate string up to the first null.  -1 means it is hex.
That is possible, although the return value convention is somewhat
complex.  Anyway, no change to the semantics of
audit_string_contains_control() is necessary for execve() argument
logging.
	Mirek