Discussion:
[PATCH ghak99 v1] audit: print empty EXECVE args
(too old to reply)
Richard Guy Briggs
2018-10-10 20:22:57 UTC
Permalink
Empty executable arguments were being skipped when printing out the list
of arguments in an EXECVE record, making it appear they were somehow
lost. Include empty arguments as an itemized empty string.

Reproducer:
autrace /bin/ls "" "/etc"
ausearch --start recent -m execve -i | grep EXECVE
type=EXECVE msg=audit(10/03/2018 13:04:03.208:1391) : argc=3 a0=/bin/ls a2=/etc

With fix:
type=EXECVE msg=audit(10/03/2018 21:51:38.290:194) : argc=3 a0=/bin/ls a1= a2=/etc
type=EXECVE msg=audit(1538617898.290:194): argc=3 a0="/bin/ls" a1="" a2="/etc"

Passes audit-testsuite
Based on: v4.19-rc2 (audit/next)
See: https://github.com/linux-audit/audit-kernel/issues/99
Signed-off-by: Richard Guy Briggs <***@redhat.com>
---
kernel/auditsc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index b2d1f04..1513873 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1107,7 +1107,7 @@ static void audit_log_execve_info(struct audit_context *context,
}

/* write as much as we can to the audit log */
- if (len_buf > 0) {
+ if (len_buf >= 0) {
/* NOTE: some magic numbers here - basically if we
* can't fit a reasonable amount of data into the
* existing audit buffer, flush it and start with
--
1.8.3.1
Paul Moore
2018-11-05 22:05:39 UTC
Permalink
Post by Richard Guy Briggs
Empty executable arguments were being skipped when printing out the list
of arguments in an EXECVE record, making it appear they were somehow
lost. Include empty arguments as an itemized empty string.
autrace /bin/ls "" "/etc"
ausearch --start recent -m execve -i | grep EXECVE
type=EXECVE msg=audit(10/03/2018 13:04:03.208:1391) : argc=3 a0=/bin/ls a2=/etc
type=EXECVE msg=audit(10/03/2018 21:51:38.290:194) : argc=3 a0=/bin/ls a1= a2=/etc
type=EXECVE msg=audit(1538617898.290:194): argc=3 a0="/bin/ls" a1="" a2="/etc"
Passes audit-testsuite
Based on: v4.19-rc2 (audit/next)
See: https://github.com/linux-audit/audit-kernel/issues/99
Merged into audit/next, but I did some cleanup on your metadata and I
want you to limit yourself to the more conventional metadata in the
future (e.g. Signed-off-by, Fixes, etc.).

The "Based on" information doesn't belong as metadata. In fact I
would suggest that you shouldn't need to explicitly state the tree
your patch(set) is based on, it should be based on either the current
audit/next tree at the time of your posting (preferable) or Linus
master tree. If you feel that you must provide the base of your
patch(set), either due to a wide cross-posting or some patch(set)
specific complexities, please do so in a cover letter.

I'm less upset about the GH issue reference as metadata, but since
we're talking about these things, I'd prefer if it was included in the
main patch description instead of metadata. Also a reminder that
linking the GH issue doesn't remove the need for you to adequately
describe the patch in the commit message. The git log needs to
standalone as a useful source of information. This particular patch
does a good job of that; this is just a reminder for others who are
following the mailing list.
--
paul moore
www.paul-moore.com
Paul Moore
2018-11-06 20:26:57 UTC
Permalink
Post by Paul Moore
Post by Richard Guy Briggs
Empty executable arguments were being skipped when printing out the list
of arguments in an EXECVE record, making it appear they were somehow
lost. Include empty arguments as an itemized empty string.
autrace /bin/ls "" "/etc"
ausearch --start recent -m execve -i | grep EXECVE
type=EXECVE msg=audit(10/03/2018 13:04:03.208:1391) : argc=3 a0=/bin/ls a2=/etc
type=EXECVE msg=audit(10/03/2018 21:51:38.290:194) : argc=3 a0=/bin/ls a1= a2=/etc
type=EXECVE msg=audit(1538617898.290:194): argc=3 a0="/bin/ls" a1="" a2="/etc"
Passes audit-testsuite
Based on: v4.19-rc2 (audit/next)
See: https://github.com/linux-audit/audit-kernel/issues/99
Merged into audit/next, but I did some cleanup on your metadata and I
want you to limit yourself to the more conventional metadata in the
future (e.g. Signed-off-by, Fixes, etc.).
The "Based on" information doesn't belong as metadata. In fact I
would suggest that you shouldn't need to explicitly state the tree
your patch(set) is based on, it should be based on either the current
audit/next tree at the time of your posting (preferable) or Linus
master tree. If you feel that you must provide the base of your
patch(set), either due to a wide cross-posting or some patch(set)
specific complexities, please do so in a cover letter.
I'm less upset about the GH issue reference as metadata, but since
we're talking about these things, I'd prefer if it was included in the
main patch description instead of metadata. Also a reminder that
linking the GH issue doesn't remove the need for you to adequately
describe the patch in the commit message. The git log needs to
standalone as a useful source of information. This particular patch
does a good job of that; this is just a reminder for others who are
following the mailing list.
Ok, thanks, sorry for the noise.
Would simply separating the metadata from the rest of the patch by a
blank line be sufficient?
No. Please don't do that either.
I didn't really consider "Based on" to be
metadata. I understand about the lack of need for "Based on". Is there
a better label for "See:" similar to "Reported-by:" such as
"Issue-tracker:"? Similarly, "Reproducer:" I don't consider metadata.
I consider "XXXX: YYYY" anywhere in the commit description to be metadata.
--
paul moore
www.paul-moore.com
Loading...