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 20:10:12 +0200 Message-ID: <1221156612.17533.14.camel@amilo> References: <1221085418.2705.19.camel@amilo> <1221143113.2992.9.camel@localhost.localdomain> <48C955C8.2000602@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <48C955C8.2000602@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 13:30 -0400: > Special processing with regards to the presence or absence of a null > byte is one example of prohibited interpretation. This is UNIX, "string" means "NUL-terminated string" (in fact the presence of a NUL byte is the only way to reasonably detect binary data).=20 You're far more likely to encounter a fixed-length field with an optional terminating NUL (like the old-style, 16-byte directory entries) than an ASCII-compatible string that intentionally contains a NUL byte. TTY input auditing was the only place where it makes a difference, all other code was passing a string that was at least as long as the specified size to audit_log_n_untrustedstring(). > It seems to me the problem is with audit_string_contains_control(): >=20 > int audit_string_contains_control(const char *string, size_t len) > { > const unsigned char *p; > for (p =3D string; p < (const unsigned char *)string + len && *p; p > ++) { > if (*p =3D=3D '"' || *p < 0x21 || *p > 0x7e) > return 1; > } > return 0; > } >=20 > The problem is that it is passed a counted octet sequence but in some > circumstances ignores the count. This occurs when *p =3D=3D 0, the test > for NULL should be removed. If that test is removed it will return the > flag which indicates the string must be encoded differently to be > conformant with the protocol. Yes, that's possible - but then audit_log_n_untrustedstring() would be more accurately called audit_log_n_ascii_like_binary_data(). Anyway, Eric/Al - if you prefer this solution, I can prepare an alternative patch. > As a side note I'm concerned there may be places in the user audit > code which treat string data as null terminated (at least that is my > recollection). Yes, auditd adds a NUL terminator to the audit record, and then treats it as a regular NUL-terminated string; if the audit record contains an embedded NUL byte, the rest of the record is discarded by auditd.=20 Mirek