Add initial set of tests for audit(4) subsystem

[aniketp_iitk.ac.in]

Summary

The introduced changes test the proper audit of system calls corresponding to file-create (fc) audit class. Each system call is tested twice, once for failure mode and the other for success. These tests lay the guidelines for further addition of tests covering wide range of functionalities of a specific system call.

A total of 12 file-create system calls were tested:

• mkdir(2), mkdirat(2)
• mknod(2), mknodat(2) : (requires root privileges)
• mkfifo(2), mkfifoat(2)
• link(2), linkat(2)
• symlink(2), symlinkat(2)
• rename(2), renameat(2)

Individual test-cases are independent and follow Kyua guidelines. As recommended by @asomers, it is ensured that every test-case starts auditd(8) in case it is not already running and closes it in the cleanup section. A general observation, which follows from this approach is that the time taken to complete the tests in case auditd(8) is already running is way less than the situation otherwise.

A particular observation:

1. auditd(8) already running:

▶ /usr/bin/time kyua test
file-create:link_failure  ->  passed  [0.021s]
file-create:link_success  ->  passed  [0.012s]
.........
file-create:symlinkat_failure  ->  passed  [0.014s]
file-create:symlinkat_success  ->  passed  [0.013s]

24/24 passed (0 failed)
        0.42 real         0.13 user         0.27 sys

2. auditd(8) not running initially

▶ /usr/bin/time kyua test
file-create:link_failure  ->  passed  [0.043s]
file-create:link_success  ->  passed  [0.047s]
 ......
file-create:symlinkat_failure  ->  passed  [0.053s]
file-create:symlinkat_success  ->  passed  [0.046s]

24/24 passed (0 failed)
       26.35 real         0.53 user         0.94 sys

Please note the difference in time.

Test Plan

• Execute make from test/sys/audit.
• Execute kyua test from test/sys/audit. All testcases should succeed.

[asomers]

tests/sys/audit/file-create.c

74

If every single test case needs to run as root, then you can simply put this line in your Makefile, and leave out all of the "require.user" lines from your test program's source:

TEST_METADATA.file-create+= required_user="root"

[aniketp_iitk.ac.in]

Set the test-case metadata "required_user" as root in Makefile:

TEST_METADATA.file-create+= required_user="root"

and remove atf_tc_set_md_var(tc, "require.user", "root"); from the head of individual test-case since each one of them require root user privileges

[asomers]

I think one of your recent changes has broken the tests. When I run them, I no longer see anything come out of auditpipe(4) at all. Does it still work for you?

tests/sys/audit/utils.c

72 ↗

(On Diff #42215)

Misindentation

124 ↗

(On Diff #42215)

misindentation

129 ↗

(On Diff #42215)

This line has spaces instead of tabs. There are a few others that do, too. You must use tabs throughout.

174 ↗

(On Diff #42215)

Misindentation

[aniketp_iitk.ac.in]

Few corrections in the formatting of source program. The current changes did not seem to affect the result for me.

file-create:link_failure  ->  passed  [0.023s]
file-create:link_success  ->  passed  [0.022s]
file-create:linkat_failure  ->  passed  [0.023s]
file-create:linkat_success  ->  passed  [0.023s]
file-create:mkdir_failure  ->  passed  [0.024s]
file-create:mkdir_success  ->  passed  [0.026s]
file-create:mkdirat_failure  ->  passed  [0.026s]
file-create:mkdirat_success  ->  passed  [0.023s]
file-create:mkfifo_failure  ->  passed  [0.023s]
file-create:mkfifo_success  ->  passed  [0.028s]
file-create:mkfifoat_failure  ->  passed  [0.027s]
file-create:mkfifoat_success  ->  passed  [0.023s]
file-create:mknod_failure  ->  passed  [0.022s]
file-create:mknod_success  ->  passed  [0.022s]
file-create:mknodat_failure  ->  passed  [0.023s]
file-create:mknodat_success  ->  passed  [0.023s]
file-create:rename_failure  ->  passed  [0.022s]
file-create:rename_success  ->  passed  [0.023s]
file-create:renameat_failure  ->  passed  [0.023s]
file-create:renameat_success  ->  passed  [0.025s]
file-create:symlink_failure  ->  passed  [0.024s]
file-create:symlink_success  ->  passed  [0.023s]
file-create:symlinkat_failure  ->  passed  [0.025s]
file-create:symlinkat_success  ->  passed  [0.022s]

Results file id is usr_src_tests_sys_audit.20180507-064439-219093
Results saved to /root/.kyua/store/results.usr_src_tests_sys_audit.20180507-064439-219093.db

24/24 passed (0 failed)

[aniketp_iitk.ac.in]

Update: Add the local preselection flag for non-attributable events
AUDITPIPE_SET_PRESELECT_NAFLAGS

Additional: Indentation fixes

[asomers]

tests/sys/audit/utils.c

99 ↗

(On Diff #42330)

Standard style(9) says that single-line if statements don't get curly braces. There are exceptions where the braces are needed for clariy, like if the else statement is multiline, or if there's a long comment above the single line. But those exceptions mostly don't apply in this file.

104 ↗

(On Diff #42330)

This change seems to have fixed the intermittency problems.

[aniketp_iitk.ac.in]

Remove the braces for single line if statements according to style(9) from functions:

• FILE *setup
• static void set_preselect_mode

[asomers]

tests/sys/audit/utils.c

165 ↗

(On Diff #42340)

spaces -> tabs

179 ↗

(On Diff #42340)

spaces -> tabs

[aniketp_iitk.ac.in]

Replace spaces with tabs at a couple of places

[asomers]

tests/sys/audit/utils.c

111 ↗

(On Diff #42341)

Why did you add all the curly braces back in?

[aniketp_iitk.ac.in]

Revert the accidental addition of curly braces from single line if statements.

[aniketp_iitk.ac.in]

Remove the redundant line SRCS.file-create+= file-create.c from Makefile.

[asomers]

tests/sys/audit/utils.c

124 ↗

(On Diff #42342)

This line is failing because "audit startup" is not necessarily the first event to come out of the audit pipe. You should loop here, just as you do in check_audit

[aniketp_iitk.ac.in]

• Fix the "audit startup" issue by looping within the "check_audit_startup" function to eliminate any unwanted events that might have been logged before audit startup.
• Define two wrapper functions check_audit_startup and check_audit around a static function check_auditpipe.

[asomers]

tests/sys/audit/utils.c

125 ↗

(On Diff #42374)

Probably don't need the am_failure line.

136 ↗

(On Diff #42374)

You should choose a more descriptive name for the flag variable.

146 ↗

(On Diff #42374)

style(9) actually says that for(;;) is preferred over while (true)

156 ↗

(On Diff #42374)

A more idiomatic way to do this would be to use an endless loop and a break statement.

198 ↗

(On Diff #42374)

It would probably make sense just to move the cleanup steps into check_audit, and dispense with the flag for check_auditpipe

214 ↗

(On Diff #42374)

set_preselect_mode must be called before starting auditd. Otherwise there's a race.

[aniketp_iitk.ac.in]

Set of corrections in utils.c :

• Remove am_failure flag for "ad" audit class
• Remove extraneous boolean flag
• Shift set_preselect_mode to before audit startup to avoid race condition
• Additional changes in code sematics

[aniketp_iitk.ac.in]

• Shift set_preselect_mode call post to "audit startup" as the tests failed when auditd(8) was already running.

[aniketp_iitk.ac.in]

Fix for the intermittent issue "failed: incomplete audit record". Also, clean any outstanding auditpipe buffer after audit startup is emitted from auditpipe

[asomers]

tests/sys/audit/utils.c

72 ↗

(On Diff #42426)

This change looks wrong to me. Can you explain it?

[aniketp_iitk.ac.in]

tests/sys/audit/utils.c

72 ↗

(On Diff #42426)

In a case when reclen - bytes < token.len we would not have the entire audit token which looked as if it caused the "incomplete audit record" issue.
Also, why subtract bytes from reclen if there is a chance we might miss some records? Letting it be reclen seems to ensure that complete tokens are read.

111 ↗

(On Diff #42341)

Oh, apologies. I somehow edited different file than the one present in tests/sys/audit

[asomers]

tests/sys/audit/utils.c

72 ↗

(On Diff #42426)

au_fetch_tok's man page says that reclen is supposed to be the length of the supplied buffer. When bytes > 0, the buffer you're actually passing will be smaller than reclen.

[aniketp_iitk.ac.in]

tests/sys/audit/utils.c

72 ↗

(On Diff #42426)

I switched back to reclen - bytes from reclen and the intermittent issue seems to recur after a while. My take is, subtracting bytes from the original reclen causes the len parameter to eventually become less than token.len in some test-cases, which causes "incomplete audit record" issue

Also, since I'm comparing against a regex which includes return,success/failure which is essentially at the end of the audit record, this might explain why some time the tests timeout. (As this part of the regex would be absent from buffer)

[asomers]

tests/sys/audit/utils.c

72 ↗

(On Diff #42426)

No, the token must be completely contained within the buffer (where else would it be?). And the third argument to au_fetch_tok *must* be the length of the buffer. It's never ok to overrun a buffer.

[aniketp_iitk.ac.in]

Revert back to using reclen-bytes as the third argument of au_fetch_tok instead of reclen to avoid buffer overrun.

[aniketp_iitk.ac.in]

Add ATF_REQUIRE check on au_read_rec(3) function. Also, some formatting changes.

[aniketp_iitk.ac.in]

Remove if condition check from au_fetch_tok as it is possible that final audit tokens are not complete and are not required as well. There is no need to fail the tests in such scenario.

[aniketp_iitk.ac.in]

• Remove ATF_REQUIRE condition from au_read_rec since reading extraneous corrupted records from auditpipe might not be successful always
• Call set_preselect_mode twice, once for audit startup and another for syscall audit
• print warning statement in case "au_fetch_tok" returns incomplete audit record

[asomers]

tests/sys/audit/utils.c

159 ↗

(On Diff #42484)

This error message isn't quite right. Auditpipe may have returned something, just not what we were looking for. Also, the error message does not indicate whether we we missed the audit record for the syscall under test, or whether we failed to observe auditd startup. Could you please make that more clear? It looks to me like most if not all remaining test failures are due to missing auditd startup.

[aniketp_iitk.ac.in]

Update the atf_tc_fail message for ppoll(2) timeout in check_auditpipe function to show the audit regex which caused the tests to fail (timeout).

[aniketp_iitk.ac.in]

After a long period of debugging the incomplete audit record issue, which was fixed by D15381, this update introduces the following changes

• Remove the code which sets the preselection audit mask as "ad" for checking "audit startup" in the auditpipe, as essentially the "audit startup" record shows up irrespective of the audit mask setting

• Add a few ioctl calls to set the QLIMIT as QLIMIT_MAX

[asomers]

tests/sys/audit/file-create.c

32

Why include this header?

tests/sys/audit/utils.c

73 ↗

(On Diff #43101)

You should also print errno here. You can do it like this:

perror("au_read_rec");
atf_tc_fail("Incomplete audit record")

220 ↗

(On Diff #43101)

This will probably work in the default case. However, if somebody has enabled too many classes in /etc/security/audit_control, then the audit queue might overflow before getting the audit startup message. To prevent that, you can set the audit class before starting auditd.

[aniketp_iitk.ac.in]

• Include a perror("au_read_rec") for a better insight into incomplete audit record failure (If any)
• Remove redundant header sys/syscall.h
• Introduce a "no" audit class specifically for audit startup.

[aniketp_iitk.ac.in]

Improvements in some comments of set_preselect_mode function

[aniketp_iitk.ac.in]

Correct the placement of

	/* Set local preselection audit_class as "no" for audit startup */
	set_preselect_mode(fd[0].fd, &nomask);

[asomers]

tests/sys/audit/utils.c

216 ↗

(On Diff #43109)

Don't dereference an uninitialized variable.

[aniketp_iitk.ac.in]

Initialize nomask before dereferencing it

[asomers]

tests/sys/audit/Makefile

16

Why do you need this line?

tests/sys/audit/file-create.c

57

If you're going to take the approach of having one file per audit class, then you may as well make the audit class a constant.

tests/sys/audit/utils.c

65 ↗

(On Diff #43110)

You should check for failure of both fmemopen and au_read_rec.

78 ↗

(On Diff #43110)

80 characters per line

181 ↗

(On Diff #43110)

s/an unknown event/too many file descriptors/

190 ↗

(On Diff #43110)

80 characters per line

232 ↗

(On Diff #43110)

This line would be a little bit faster if you move the check from sh to C by using atf_utils_file_exists.

[aniketp_iitk.ac.in]

Changes as suggested in the above inline comments.