[apparmor] [patch] aamode.py - fix LOG_MODE_RE

[Peter Maloney]

Howdy,

Oh if that's the way it is supposed to be (each part of the regex has to
be the whole string), using a set instead of a regex there is also very
easy, and should be faster just like the other regex to set change.


Patch attached.

Peter



PS / FYI. this patch is from openSUSE apparmor-utils 2.9.0-3.1 rather
than bzr, which has a duplicated line that is just the same as the
previous but is a comment... So if the patch looks weird, you know why.

def validate_log_mode(mode):
    if LOG_MODE_RE.search(mode):
    #if LOG_MODE_RE.search(mode):
        return True
    else:
        return False


On 2014-12-01 21:09, Christian Boltz wrote:
> Hello,
>
> Am Montag, 1. Dezember 2014 schrieb Steve Beattie:
>> On Sat, Nov 29, 2014 at 08:10:38PM +0100, Christian Boltz wrote:
>>> LOG_MODE_RE (used in validate_log_mode() in aamode.py) just checked
>>> if the given parameter contains one of the possible matches. This
>>> resulted in "invalid" [1] being a valid log mode (from audit.log
>>> requested_mask or denied_mask) because it contains 'a', which is a
>>> valid file mode.
>>>
>>> This patch wraps the regex into   ^(...)*$   to make sure the full
>>> string contains only allowed file modes.
>>>
>>> (The question is if the log (!) really contains things like Pix -
>>> but if not, that's worth another patch ;-)
>>>
>>> The patch also adds some tests for validate_log_mode().
>>>
>>> (Just in case you wonder about the order - I first noticed the regex
>>> bug, then wrote the tests.)
>>> +    def test_validate_log_mode_5(self):
>>> +        self.assertTrue(validate_log_mode(''))
>> Is this the correct result? I don't think the old version would have
>> returned True in that situation, and it seems fishy to do so. I think
>> you want '+' instead of '*' (Kleene star).
> Indeed, that's the only sanity check that worked in the old code ;-)
> Good catch!
>
> Here's the updated patch with two changes:
> - change the regex to use "+" instead of "*" (which means an empty 
>   string will return False)
> - rename test_validate_log_mode_5() to test_validate_log_mode_invalid_4() 
>   and expect False for ''
>
>
> [ aamode-validate_log_mode-v2.diff ]
>
> === modified file 'utils/apparmor/aamode.py'
> --- utils/apparmor/aamode.py    2014-12-01 19:56:31 +0000
> +++ utils/apparmor/aamode.py    2014-12-01 20:05:41 +0000
> @@ -68,7 +68,7 @@
>               'N': AA_EXEC_NT
>               }
>  
> -LOG_MODE_RE = re.compile('(r|w|l|m|k|a|x|ix|ux|px|pux|cx|nx|pix|cix|Ux|Px|PUx|Cx|Nx|Pix|Cix)')
> +LOG_MODE_RE = re.compile('^(r|w|l|m|k|a|x|ix|ux|px|pux|cx|nx|pix|cix|Ux|Px|PUx|Cx|Nx|Pix|Cix)+$')
>  MODE_MAP_SET = {"r", "w", "l", "m", "k", "a", "x", "i", "u", "p", "c", "n", "I", "U", "P", "C", "N"}
>  
>  def str_to_mode(string):
>
> === modified file 'utils/test/test-aamode.py'
> --- utils/test/test-aamode.py   2014-11-29 08:15:17 +0000
> +++ utils/test/test-aamode.py   2014-12-01 20:06:12 +0000
> @@ -11,7 +11,7 @@
>  
>  import unittest
>  
> -from apparmor.aamode import split_log_mode, sub_str_to_mode
> +from apparmor.aamode import split_log_mode, sub_str_to_mode, validate_log_mode
>  from apparmor.common import AppArmorBug
>  
>  class AamodeTest_split_log_mode(unittest.TestCase):
> @@ -51,6 +51,24 @@
>      def test_sub_str_to_mode_dupes(self):
>          self.assertEqual(sub_str_to_mode('rwrwrw'), {'r', 'w'})
>  
> +class AamodeTest_validate_log_mode(unittest.TestCase):
> +    def test_validate_log_mode_1(self):
> +        self.assertTrue(validate_log_mode('a'))
> +    def test_validate_log_mode_2(self):
> +        self.assertTrue(validate_log_mode('rw'))
> +    def test_validate_log_mode_3(self):
> +        self.assertTrue(validate_log_mode('Pixrw'))
> +    def test_validate_log_mode_4(self):
> +        self.assertTrue(validate_log_mode('rrrr'))
> +
> +    def test_validate_log_mode_invalid_1(self):
> +        self.assertFalse(validate_log_mode('c'))  # 'c' (create) must be converted to 'a' before calling validate_log_mode()
> +    def test_validate_log_mode_invalid_2(self):
> +        self.assertFalse(validate_log_mode('R'))  # only lowercase 'r' is valid
> +    def test_validate_log_mode_invalid_3(self):
> +        self.assertFalse(validate_log_mode('foo'))
> +    def test_validate_log_mode_invalid_4(self):
> +        self.assertFalse(validate_log_mode(''))
>  
>  if __name__ == '__main__':
>      unittest.main(verbosity=2)
>
>
>
> Regards,
>
> Christian Boltz


-- 

--------------------------------------------
Peter Maloney
Brockmann Consult
Max-Planck-Str. 2
21502 Geesthacht
Germany
Tel: +49 4152 889 300
Fax: +49 4152 889 333
E-mail: peter.maloney at brockmann-consult.de
Internet: http://www.brockmann-consult.de
--------------------------------------------

-------------- next part --------------
A non-text attachment was scrubbed...
Name: p4-aamode-log-mode-set.patch
Type: text/x-patch
Size: 1026 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141202/2db9841d/attachment.bin>