On 15/01/12, Tetsuo Handa wrote:
> Thank you for comments.
>
> Richard Guy Briggs wrote:
> > Steve already mentioned any user-influenced fields need to be escaped,
> > so I'd recommend audit_log_untrustedstring() as being much simpler from
> > your perspective and much better tested and understood from audit
> > maintainer's perspective. At least use the existing 'o' printf format
> > specifier instead of inventing your own. I do acknowledge that the
> > resulting output from your function is easier to read in its raw format
> > passed from the kernel, however, it makes your code harder to maintain.
>
> I'm not sure whether I should use audit_log_untrustedstring().
>
> This record contains multiple user-influenced comm names. If I use
> audit_log_untrustedstring(), I would need to split this record into
> multiple records like history[0]='...' history[1]='...' history[2]='...'
> in order to avoid matching delimiters (i.e. ';', '=' and '>') used in
> this record. This would also change from "char *" in "struct task_struct"
> to array of struct { "comm name", "pid", "stamp" } in "struct task_struct".
> I don't know whether such change makes easier to maintain than now.
This will end up producing a varying number of fields. The userspace
tools are looking for a constant number of fields per record type.
> > As for the date-stamping bits, they seem to be the majority of the code
> > in audit_update_history(). I'd just emit a number and punt that to
> > userspace for decoding. Alternatively, I'd use an existing service in
> > the kernel to do that date formatting, or at least call a new function
> > to format that date string should a suitable one not already exist, so
> > you can remove that complexity from audit_update_history().
>
> Since I don't know existing functions for date formatting, I split it as
> a new function. If it is acceptable, I'd like to make that function public
> and replace tomoyo_convert_time() in security/tomoyo/util.c with that
> function.
That is an improvement, but would still like to see existing functions
used or punt to userspace.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 072566d..2edeba2 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1344,6 +1354,17 @@ out:
> audit_log_end(ab);
> }
>
> +static void audit_log_history(struct audit_context *context)
> +{
> + struct audit_buffer *ab;
> +
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_PROCHISTORY);
> + if (!ab)
> + return; /* audit_panic or being filtered */
> + audit_log_format(ab, "history='%s'", current->comm_history);
This is where I would seperate them to:
audit_log_format(ab, "history=");
audit_log_untrustedstring(ab, current->comm_history);
Making sure, of course, that there are no NULLs printed from any of the
comm fields (which should be impossible due to your "if (!c) break;").
> + audit_log_end(ab);
> +}
> +
> static void audit_log_exit(struct audit_context *context, struct task_struct
> *tsk)
> {
...
> +void audit_update_history(void)
> + *cp++ = '\\';
> + *cp++ = (c >> 6) + '0';
> + *cp++ = ((c >> 3) & 7) + '0';
> + *cp++ = (c & 7) + '0';
Is there a reason you are not using the printf 'o' octal converter?
> + /* Append timestamp. */
> + {
> + struct yyyymmdd_hhmmss stamp;
> +
> + tt_get_time(&stamp);
> + cp += snprintf(cp, buf - cp + sizeof(buf) - 1,
> + ";start=%04u%02u%02u%02u%02u%02u", stamp.year,
> + stamp.month, stamp.day, stamp.hour, stamp.min,
> + stamp.sec);
> + }
Why not just return a string? Better yet, punt to userspace.
- RGB
--
Richard Guy Briggs <rbri...@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
