RFE: add test for audit lost counter reset #41

rgbriggs · 2017-02-15T21:27:52Z

See:
linux-audit/audit-kernel#3
https://github.com/linux-audit/audit-kernel/wiki/RFE-Reset-the-Lost-Record-Counter

Signed-off-by: Richard Guy Briggs rgb@redhat.com

See: linux-audit/audit-kernel#3 https://github.com/linux-audit/audit-kernel/wiki/RFE-Reset-the-Lost-Record-Counter Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

pcmoore · 2017-03-03T23:44:50Z

tests/lost_reset/test

+system("auditctl -d exit,always -S all -F pid=$ping_pid >/dev/null");
+
+# Restart the daemon to collect messages in the log
+system("auditd -n &");


Any reason why we don't restart auditd with systemctl so that we put the system back the way it was intended?

systemctl doesn't exist on rhel6, but now that I think of it, this feature isn't supported in rhel6.
Instead, we could use "service auditd start" since that redirects to systemd.

pcmoore · 2017-03-03T23:45:47Z

tests/lost_reset/test

+###
+# cleanup
+system("auditctl -D >& /dev/null");
+system("auditctl -b 320 >/dev/null 2>&1");


Another reason why we should restart auditd using whatever the system's service manager (almost surely systemd/systemctl these days).

True for fedora, rhel7, Debian 8.

Fixed, amendment pushed.

Update: rgbriggs@970f6d6

pcmoore

Please see comments inline.

Use the standard "service" call to stop and start the audit daemon rather than manually (or with systemctl directly since systemd isn't available on older systems and systemctl can't be used directly from the command line for auditd.service). Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

pcmoore · 2017-03-09T22:52:58Z

The backlog issue on test cleanup remains (see my earlier comments).

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

pcmoore · 2017-04-18T21:28:02Z

This test fails for me on my test system:

# ./test 
1..6
# Running under perl version 5.024001 for linux
# Current time local: Tue Apr 18 17:25:37 2017
# Current time GMT:   Tue Apr 18 21:25:37 2017
# Using Test.pm version 1.28_01
Stopping logging: [  OK  ]
ok 1
ok 2
ok 3
ok 4
ok 5
not ok 6
# Failed test 6 in ./test at line 79
#  ./test line 79 is: ok($result_lost == $lost); # Do the two lost values agree?
Stopping logging: [  OK  ]
Redirecting start to /bin/systemctl start auditd.service

Test system information:

# uname -r
4.11.0-0.rc7.git0.1.1.secnext.fc27.x86_64
# rpm -q audit
audit-2.7.5-1.fc27.x86_64

rgbriggs · 2017-04-19T07:06:50Z

Works fine on my Rawhide and RHEL7 VMs. I need more information. If I can't put that information in the test and I can't reproduce it, I need your help.

pcmoore · 2017-04-24T15:17:11Z

Works fine on my Rawhide and RHEL7 VMs. I need more information. If I can't put that information in the test and I can't reproduce it, I need your help.

The failing test is the ok($result_lost == $lost) check, on my Rawhide system $loss reports 0 and $report_lost reports 2. Looking a bit closer, I think something odd is happening when auditctl reports the lost counter, for example:

# uname -r
4.11.0-0.rc7.git0.1.1.secnext.fc27.x86_64
# rpm -q audit
audit-2.7.6-1.fc27.x86_64
# auditctl -s
enabled 1
failure 1
pid 1409
rate_limit 0
backlog_limit 64
lost 0
backlog 0
backlog_wait_time 60000
loginuid_immutable 0 unlocked
# auditctl --reset-lost
lost: 2

It is worth mentioning that this is on an idle system that isn't generating any audit records (and no indication of lost records in the kernel's ring buffer). It is easily repeatable for me.

rgbriggs · 2017-04-25T06:33:24Z

Ok, all my test VMs have a boot parameter of "audit=1" to force auditting on always, so when that is not set, stopping the daemon would have the kernel ignore those messages and not increase the lost count significantly. Either that, or the flood ping command failed. Testing without "audit=1" I get the same result you did, so I'll need to think of another way to trigger this without needing a change to the kernel boot parameters and rebooting.

pcmoore · 2017-04-25T15:21:56Z

Testing with "audit=1" on the kernel command line is good, but it is equally important (perhaps more so) to test the default configuration that many distributions use, e.g. "audit=0".

rgbriggs · 2017-04-25T16:28:27Z

On 2017-04-25 08:21, Paul Moore wrote: Testing with "audit=1" on the kernel command line is good, but it is equally important (perhaps more so) to test the default configuration that many distributions use, e.g. "audit=0".

Yes, I fully agree. Maybe the tests should be iterated through both settings.

#41 (comment)

- RGB

…

-- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635

Since the default configuration is to not have "audit=1" on the kernel boot command line, set buffers to lowest setting and wait time to lowest setting, loop on starting and stopping the daemon while overflowing the queue to provoke a lost message. Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

rgbriggs · 2017-05-07T16:54:06Z

Ok, I was able to pass the test by deleting the "audit=" kernel boot parameter, setting backlog_limit (-b) to 1, backlog_wait_time to 1(ms), enable (-e) to 1, starting and stopping the daemon multiple times and using a floodping to generate messages. Update pushed to my pull request.

pcmoore · 2017-07-03T14:44:12Z

tests/lost_reset/test

+	system("auditctl -b 1 >/dev/null 2>&1");
+	system("auditctl --backlog_wait_time 1 >/dev/null 2>&1");
+	system("auditctl -e 1 >/dev/null 2>&1");
+


I didn't think this needed to be said, but please don't add a tab/indent to blank lines.

pcmoore · 2017-07-03T14:44:32Z

tests/lost_reset/test

+	sleep 1;
+	kill 'TERM', $ping_pid;
+	system("auditctl -d exit,always -S all -F pid=$ping_pid >/dev/null 2>&1");
+


See above comment about indents on blank lines.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

pcmoore · 2017-07-11T21:45:45Z

@rgbriggs Okay, this looks reasonable. Let's do two final things so we can get this merged:

Add the test to tests/Makefile so it runs by default (that is your desire, yes?)
Wrap your additional debug output with ATS_DEBUG, see 29aa532 and RFE: create a debug flag/option so we have a consistent way to toggle debugging output #54

Limit debug output based on presence/value of ATS_DEBUG flag. See linux-audit#41 See linux-audit#54 Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

Enable the lost_reset test. See: linux-audit#41 Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

pcmoore · 2017-07-12T15:26:35Z

It would appear there is a problem with lost_reset/test on line 7:

BEGIN { plan tests => 5 + $iterations }

... if I run the test directly from the lost_reset directory things work as expected:

# cd tests/lost_reset
# ./test
1..5
# Running under perl version 5.026000 for linux
# Current time local: Wed Jul 12 11:19:31 2017
# Current time GMT:   Wed Jul 12 15:19:31 2017
# Using Test.pm version 1.30
ok 1
ok 2
ok 3
ok 4
ok 5
ok 6
ok 7
ok 8
ok 9
ok 10
ok 11
ok 12
ok 13
ok 14
ok 15
Stopping logging: [  OK  ]
Redirecting start to /bin/systemctl start auditd.service

... however if I run it using make test from the top level directory things fall apart:

# make test
make -C tests test
{snip}
Running as   user    root
        with context unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
        on   system  Fedora

exec_execve/test ......... ok   
exec_name/test ........... ok   
file_create/test ......... ok   
file_delete/test ......... ok   
file_rename/test ......... ok   
filter_exclude/test ...... ok     
filter_sessionid/test .... ok   
login_tty/test ........... ok   
lost_reset/test .......... Use of uninitialized value $iterations in addition (+) at lost_reset/test line 7.
lost_reset/test .......... All 5 subtests passed 
netfilter_pkt/test ....... ok     
syscalls_file/test ....... ok   
syscall_module/test ...... ok   
syscall_socketcall/test .. ok   
user_msg/test ............ ok   

Test Summary Report
-------------------
lost_reset/test        (Wstat: 0 Tests: 15 Failed: 10)
  Failed tests:  6-15
  Parse errors: Bad plan.  You planned 5 tests but ran 15.
Files=14, Tests=88, 29 wallclock secs ( 0.06 usr  0.01 sys +  0.88 cusr  0.50 csys =  1.45 CPU)
Result: FAIL
Failed 1/14 test programs. 10/88 subtests failed.
make[1]: *** [Makefile:39: test] Error 255
make[1]: Leaving directory '/root/sources/audit-testsuite/tests'
make: *** [Makefile:10: test] Error 2

I'm guessing you never tested this by running make test, or is this WORKSFORME?

rgbriggs · 2017-07-12T17:10:27Z

Only ran that isolated test. This is bizarre. When run isolated, $iterations is visible inside the "BEGIN..." on line 7, but it is not visible when run as part of the suite. What could cause this? I even tried changing the variable name to avoid other name collisions, but same result. I may have to hard-code it.

pcmoore · 2017-07-12T18:25:11Z

Hard code it. No need to make this more complicated.

Also a lesson to be learned that tests should be verified both by running the full suite and the isolated test(s).

rgbriggs · 2017-07-14T14:30:13Z

Yes, really. I don't like magic numbers buried deep in the code. This gives it a name and a way to track the usage throughout the file. Otherwise I'll track down why the difference between running one test and running the suite...

pcmoore · 2017-07-14T15:56:54Z

Change it.

pcmoore · 2017-07-14T16:38:15Z

Change it.

It just occurred to me that some additional commentary is probably warranted given the fact that we are even discussing this.

I agree, like most sane people, that magic numbers are generally bad, however, not every constant in every bit of source code is a "magic number". In your current patch(set) the "$iterations" variable is only used in one location ... a rather conventional if-conditional, this makes the value rather obvious, especially given the size of the file and the rather simple nature of the test.

Yes, it is annoying that we can't seem to use a variable in the test's BEGIN {...} stanza, if we could I would agree to keeping the $iterations variable, but we can't. You've placed a comment after the BEGIN statement which seems to implying that the 5 + 10 is linked to the $iterations variable, IMHO the following is just as meaningful, if not more so:

BEGIN { plan tests => 15 } # five fixed tests + the main for loop below

... it also has the benefit of not looking stupid.

rgbriggs · 2017-07-14T18:55:09Z

On 2017-07-14 16:38, Paul Moore wrote: > Change it. It just occurred to me that some additional commentary is probably warranted given the fact that we are even discussing this.

I thought that was obvious...

I agree, like most sane people, that magic numbers are generally bad, however, not every constant in every bit of source code is a "magic number". In your current patch(set) the "$iterations" variable is only used in one location ... [a rather conventional if-conditional](https://github.com/linux-audit/audit-testsuite/blob/9f0745ebd17971ffd5a6324d0c099908f7ed4f02/tests/lost_reset/test#L28), this makes the value rather obvious, especially given the size of the file and the rather simple nature of the test.

Putting those kinds of numbers near the top well identified helps. I failed to describe how it was used either in a self-describing variable or in a comment.

Yes, it is annoying that we can't seem to use a variable in the test's `BEGIN {...}` stanza, if we could I would agree to keeping the $iterations variable, but we can't. You've placed a comment after the `BEGIN` statement which seems to implying that the `5 + 10` is linked to the $iterations variable, IMHO the following is just as meaningful, if not more so: BEGIN { plan tests => 15 } # five fixed tests + the main for loop below

My code comment was incomplete and misleading, assisting in the confusion. Leaving as it as I had it but changing the comment to "# 5 fixed tests plus $iterations" I think is far clearer and much quicker to find with basic text search tools. I tried escaping it to fix it and made it worse.

... it also has the benefit of not looking stupid.

I strongly disagree on this last statement, but I guess that is this "judgement" and "taste" people keep talking about.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #41 (comment)

- RGB

…

-- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635

pcmoore · 2017-07-14T21:59:48Z

It isn't clear to me from your last response if you are planning another revision, but just to be clear on my expectations: I'm waiting on another revision to get rid of the $iterations variable and the "5 + 10" nonsense.

Hard code iterations to work around variable scoping issues. When run alone, no problem. When run in suite, it claims iterations is uninitialized. Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

rgbriggs · 2017-07-24T10:07:03Z

Cleaned up iterations comment: rgbriggs@027d515
Resolved merge conflict: rgbriggs@278d7ee

pcmoore · 2017-08-01T13:45:07Z

In the future please do not merge the upstream branches back into your forked branch, it makes it very messy when I pull from you. If you need to resync with upstream, rebase your development branch.

pcmoore · 2017-08-01T15:52:03Z

Merged via d549ded.

I also merged a commit to fix a number of style/formatting diffs, see 1807b2f. As this PR originally predated the 'make check-syntax' style checker I didn't require the code to satisfy all those style checks, but in the future having a clean 'make check-syntax' run will be necessary for merging.

Since restarting the daemon does not reset these two values unless they are explicitly set in the config file, reset them manually based on the kernel defaults at the time of committing this test. While it would be better to use the actual kernel defaults, those are not available to this test. See: linux-audit/audit-kernel#3 linux-audit#56 linux-audit#41 Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

Since restarting the daemon does not reset these two values unless they are explicitly set in the config file, reset them manually based on the auditctl status values at the start of the test. While it would be better to use the actual kernel defaults, those are not available to this test. See: linux-audit/audit-kernel#3 linux-audit#56 linux-audit#41 Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

tests: add test for audit lost counter reset

7784fa7

See: linux-audit/audit-kernel#3 https://github.com/linux-audit/audit-kernel/wiki/RFE-Reset-the-Lost-Record-Counter Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

pcmoore changed the title ~~tests: add test for audit lost counter reset~~ RFE: add test for audit lost counter reset Feb 17, 2017

pcmoore added enhancement pending/review priority/low labels Feb 17, 2017

pcmoore self-requested a review February 17, 2017 21:19

The-Mule approved these changes Feb 22, 2017

View reviewed changes

pcmoore force-pushed the master branch from 983d190 to 77f9054 Compare March 3, 2017 23:23

pcmoore reviewed Mar 3, 2017

View reviewed changes

pcmoore added pending/info pending/revision and removed pending/review labels Mar 3, 2017

pcmoore reviewed Mar 3, 2017

View reviewed changes

pcmoore removed the pending/info label Mar 9, 2017

lost_reset: restart auditd after test to return to defaults

101fb3a

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

pcmoore added pending/review pending/info and removed pending/revision labels Apr 18, 2017

pcmoore assigned rgbriggs Apr 25, 2017

pcmoore reviewed Jul 3, 2017

View reviewed changes

fixup! lost_reset: indent loop and remove output noise

d76449c

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

pcmoore added pending/review and removed pending/revision labels Jul 10, 2017

pcmoore added pending/revision and removed pending/review labels Jul 11, 2017

rgbriggs added 2 commits July 12, 2017 10:01

tests:lost_reset: use new debug flag

007e5cb

Limit debug output based on presence/value of ATS_DEBUG flag. See linux-audit#41 See linux-audit#54 Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

tests:Makefile: enable lost_reset test

f9d8bcc

Enable the lost_reset test. See: linux-audit#41 Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

tests:lost_reset: hard code iterations

027d515

Hard code iterations to work around variable scoping issues. When run alone, no problem. When run in suite, it claims iterations is uninitialized. Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

rgbriggs force-pushed the ghak3-audit-lost-reset branch from 9f0745e to 027d515 Compare July 24, 2017 06:39

Merge branch 'master' into ghak3-audit-lost-reset

278d7ee

pcmoore added pending/review and removed pending/revision labels Aug 1, 2017

pcmoore closed this Aug 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFE: add test for audit lost counter reset #41

RFE: add test for audit lost counter reset #41

rgbriggs commented Feb 15, 2017

pcmoore Mar 3, 2017

rgbriggs Mar 5, 2017 •

edited

Loading

pcmoore Mar 3, 2017

rgbriggs Mar 5, 2017

rgbriggs Mar 5, 2017

rgbriggs Mar 6, 2017

pcmoore left a comment •

edited

Loading

pcmoore commented Mar 9, 2017

pcmoore commented Apr 18, 2017

rgbriggs commented Apr 19, 2017

pcmoore commented Apr 24, 2017

rgbriggs commented Apr 25, 2017

pcmoore commented Apr 25, 2017

rgbriggs commented Apr 25, 2017 via email

rgbriggs commented May 7, 2017

pcmoore Jul 3, 2017

pcmoore Jul 3, 2017

pcmoore commented Jul 11, 2017

pcmoore commented Jul 12, 2017

rgbriggs commented Jul 12, 2017

pcmoore commented Jul 12, 2017

rgbriggs commented Jul 14, 2017

pcmoore commented Jul 14, 2017

pcmoore commented Jul 14, 2017

rgbriggs commented Jul 14, 2017 via email

pcmoore commented Jul 14, 2017

rgbriggs commented Jul 24, 2017

pcmoore commented Aug 1, 2017

pcmoore commented Aug 1, 2017

RFE: add test for audit lost counter reset #41

RFE: add test for audit lost counter reset #41

Conversation

rgbriggs commented Feb 15, 2017

pcmoore Mar 3, 2017

Choose a reason for hiding this comment

rgbriggs Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

pcmoore Mar 3, 2017

Choose a reason for hiding this comment

rgbriggs Mar 5, 2017

Choose a reason for hiding this comment

rgbriggs Mar 5, 2017

Choose a reason for hiding this comment

rgbriggs Mar 6, 2017

Choose a reason for hiding this comment

pcmoore left a comment • edited Loading

Choose a reason for hiding this comment

pcmoore commented Mar 9, 2017

pcmoore commented Apr 18, 2017

rgbriggs commented Apr 19, 2017

pcmoore commented Apr 24, 2017

rgbriggs commented Apr 25, 2017

pcmoore commented Apr 25, 2017

rgbriggs commented Apr 25, 2017 via email

rgbriggs commented May 7, 2017

pcmoore Jul 3, 2017

Choose a reason for hiding this comment

pcmoore Jul 3, 2017

Choose a reason for hiding this comment

pcmoore commented Jul 11, 2017

pcmoore commented Jul 12, 2017

rgbriggs commented Jul 12, 2017

pcmoore commented Jul 12, 2017

rgbriggs commented Jul 14, 2017

pcmoore commented Jul 14, 2017

pcmoore commented Jul 14, 2017

rgbriggs commented Jul 14, 2017 via email

pcmoore commented Jul 14, 2017

rgbriggs commented Jul 24, 2017

pcmoore commented Aug 1, 2017

pcmoore commented Aug 1, 2017

rgbriggs Mar 5, 2017 •

edited

Loading

pcmoore left a comment •

edited

Loading