Hello,
Am Dienstag, 9. Juli 2013 schrieb Kshitij Gupta:
> === added file 'apparmor/aa.py'
> --- apparmor/aa.py 1970-01-01 00:00:00 +0000
> +++ apparmor/aa.py 2013-07-06 13:27:06 +0000
> +# Setup logging incase of debugging is enabled
> +if os.getenv('LOGPROF_DEBUG', False):
> + import logging, atexit
> + DEBUGGING = True
> + logprof_debug = os.environ['LOGPROF_DEBUG']
> + logging.basicConfig(filename=logprof_debug, level=logging.DEBUG)
>
> ### taking the filename from an env variable? Not sure if this is a
> good idea security-wise...
>
> ##### The user (not a casual user ofcourse) would set an environment
> variable if debugging,
> ##### Any other you'd want to set debugging?
If someone can inject the environment variable into root's env (or
user's env before calling su), it might be possible to destroy various
files.
Maybe I'm too paranoid, but I'd use a fixed or tempfile-generated debug
log filename, and then just check for LOGPROF_DEBUG=1.
Or at least use the filename from LOGPROF_DEBUG as prefix for a
tempfile.
(@John: am I too paranoid?)
> +CONFDIR = '/etc/apparmor'
>
> ### It's just a path, but you should keep it at _one_ place
> (config.py). ### (besides that, CONFDIR is unused in aa.py)
>
> ##### Unused but only as of now! ;-)
Nevertheless, this should only be in config.py.
> +unimplemented_warning = False
> ##### Some variables are there just as placeholder for later to avoid
> corrupting the global namespace later
Ah, OK.
(As usual, avoid global variables whenever possible.)
> +aa = dict() # Profiles originally in sd, replace by aa
> +original_aa = dict()
>
> ### aa and original_aa aren't very self-explaining variable names -
> can you give them a better name?
>
> ##### They basically contain the list of profiles present version and
> the one at the time of loading
Nice, but the variable name should represent that. What about
profile_list and original_profile_list?
> +def check_for_LD_XXX(file):
> + """Returns True if specified program contains references to
> LD_PRELOAD or + LD_LIBRARY_PATH to give the PX/UX code better
> suggestions"""
>
> ### the x in Px/Ux is always lowercase
> ##### Fixed
Not yet (in r16) ;-)
> + found = False
> + if not os.path.isfile(file):
> + return False
> + size = os.stat(file).st_size
> + # Limit to checking files under 10k for the sake of speed
>
> ### hmm, performance vs. reliable results.
> ### what about a higher limit?
>
> ##### I'm open to suggestions for a higher value :-)
I'm not an expert for binary files - does someone have a recommendation
for a good size limit? 10k feels quite small nowadays...
> +def get_profile_filename(profile):
> + """Returns the full profile name"""
> + filename = profile
> + if filename.startswith('/'):
> + # Remove leading /
> + filename = filename[1:]
> + else:
> + filename = "profile_" + filename
> + filename.replace('/', '.')
>
> ### should we replace more chars?
> ### just imagine someone has "~/bin/bad $$ script ? for * stars"
> ### (ok, extreme example - but you want a profile for this for sure
> ;-) ###
> ### I'm not sure about backward compability if we change this
> (John?) ### but people could also have a profile for /bin/foo in
> /etc/apparmor.d/mybar ### so we can't really expect the /bin/foo
> profile in bin.foo
John, what's your opinion on this? Personally, I'd replace everything
except a-zA-Z0-9_- with _ to be on the safe side. (Of course only when
creating new profiles. When modifying profiles, keep the old filename.)
> ##### It would be interesting to have custom profile names,
> storing/using them. ##### Tell me where you'll store them and I'll
> fetch them ;-)
Fetching profiles basically means checking /etc/apparmor.d/* - the
filenames can be good hints, but people can rename the profile files as
they wish.
> +def complain(path):
> + """Sets the profile to complain mode if it exists"""
> + filename, name = name_to_prof_filename(path)
>
> ### see above - you can't rely on having the proifle for /bin/foo in
> bin.foo ### (IIRC there's even an open bugreport about this)
>
> ##### Look above
>
> +def enforce(path):
> + """Sets the profile to complain mode if it exists"""
> + filename, name = name_to_prof_filename(path)
>
> ### same here
>
> ##### Look above
>
> +def head(file):
> + """Returns the first/head line of the file"""
> + first = ''
> + if os.path.isfile(file):
> + with open_file_read(file) as f_in:
> + first = f_in.readline().rstrip()
> + return first
>
> ### should head() raise an error if file doesn't exist?
> ### (or does open_file_read() do this already?)
>
> ##### os.path.isfile() checks that and returns an empty line, maybe a
> warning could be logged. :)
I'd even expect an error.
> +def get_output(params):
> + """Returns the return code output by running the program with the
> args given in the list"""
> + except subprocess.CalledProcessError as e:
> + output = e.output
> + output = output.decode('utf-8').split('\n')
>
> ### what happens if someone is not using utf-8?
>
> ##### The program's output needs to be utf-8 encoded else they get
> some weird stuff, on encoding look below.
This means you have to set the encoding somewhere ;-)
Or set LANG=C and you should have pure ASCII ;-)
> + ret = e.returncode
> + else:
> + ret = 0
> + output = output.decode('utf-8').split('\n')
> + # Remove the extra empty string caused due to \n if present
> + if len(output) > 1:
> + output.pop()
> + return (ret, output)
>
> +def get_reqs(file):
> + """Returns a list of paths from ldd output"""
> + pattern1 = re.compile('^\s*\S+ => (\/\S+)')
> + pattern2 = re.compile('^\s*(\/\S+)')
> + reqs = []
> + ret, ldd_out = get_output(ldd, file)
>
> ### ldd is "None" - you should fill it before using it ;-)
> ### also, I'm not sure if ldd needs to be a global variable
>
> ##### The setter method is yet to come. ;-)
> ##### Will fix it when refractoring the module later as I planned. :-)
OK.
> === added file 'apparmor/common.py'
> --- apparmor/common.py 1970-01-01 00:00:00 +0000
> +++ apparmor/common.py 2013-07-03 23:34:04 +0000
>
>
> +def cmd_pipe(command1, command2):
> + '''Try to pipe command1 into command2.'''
> + try:
> + sp1 = subprocess.Popen(command1, stdout=subprocess.PIPE)
> + sp2 = subprocess.Popen(command2, stdin=sp1.stdout)
> + except OSError as ex:
> + return [127, str(ex)]
> +
> + if sys.version_info[0] >= 3:
> + out = sp2.communicate()[0].decode('ascii', 'ignore')
>
> ### is "ascii" correct here, or should it be the system locale?
>
> ##### From aa-enforce, havent used it yet. However, utf-8 is default
> for Python3, so maybe we can use it?
If we use it everywhere, then I'd say yes.
> +def open_file_read(path):
> + '''Open specified file read-only'''
> + try:
> + orig = codecs.open(path, 'r', "UTF-8")
>
> # once more - is hardcoding utf-8 a good idea?
>
> ##### Again from aa-enforce, and we can discuss using utf-8 or system
> locale.
Sounds like something we should discuss in the meeting later ;-)
Regards,
Christian Boltz
--
"Guten Tag, ich möchte gerne einen Tisch reservieren."
"Gerne, auf welchen Namen denn?"
"31337 /-/ /\ X0R!"
[Jens Benecke in suse-linux zum Thema Realnames]
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
