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

[Miloslav TrmaÄ]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/