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

Date:	Thu, 11 Sep 2008 20:10:12 +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 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). 

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():
> 
> int audit_string_contains_control(const char *string, size_t len)
> {
>     const unsigned char *p;
>     for (p = string; p < (const unsigned char *)string + len && *p; p
> ++) {
>         if (*p == '"' || *p < 0x21 || *p > 0x7e)
>             return 1;
>     }
>     return 0;
> }
> 
> The problem is that it is passed a counted octet sequence but in some
> circumstances ignores the count. This occurs when *p == 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. 
	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/