From: hanseld <ha...@us...> - 2008-04-16 08:53:30
|
Update of /cvsroot/oprofile/oprofile In directory sc8-pr-cvs3.sourceforge.net:/tmp/cvs-serv12698 Modified Files: Tag: JIT_SUPPORT TODO.jit Log Message: Removed items that are solved now Index: TODO.jit =================================================================== RCS file: /cvsroot/oprofile/oprofile/Attic/TODO.jit,v retrieving revision 1.1.2.25 retrieving revision 1.1.2.26 diff -u -p -d -r1.1.2.25 -r1.1.2.26 --- TODO.jit 11 Feb 2008 09:39:55 -0000 1.1.2.25 +++ TODO.jit 16 Apr 2008 08:53:31 -0000 1.1.2.26 @@ -7,82 +7,6 @@ o To fix before merge section must be em to fix before merge to the second section if the fix needs a libiopagent API change or a jitdump file format change. ------- -security questions ------------------- -o opjitconv run as root, that's bad, we should drop root capacity during the - conversion, create the elf file in ...oprofile/jitdump dir then let daemon - do the copy? Or just audit the code to ensure it can run as root? Take - care the input data are potentially malicious since the jitdump dir is - world writable. -phe - -I think what we need to do is carefully audit and comment opjitconv AND -drop root. In particular we need to clearly separate out the "input -parsing" bits from the rest of the code. - -I can see this working something like this: - -First, as root: - - - for each file in jitdump/, move it into a private 'opjit' dir for - use by the user 'opjit'. We verify no directory components, no - following of symlinks etc, and possibly a maximum file size. - grant ownership of the file to user opjit (now no normal user can - see it), and create an output file for 'opjit' too. - - now we drop privs and do the conversion. We know that at least the - input and output directories are secure. "All" we have to do is be - carefuly parsing the input - -*Wherever* possible, code dealing with insecure input should be clearly -documented. See how Perl does 'tainting' - would be good to have the -equivalent in comments. - -Existing issues with the code I noticed: - -<movement> we should have clear comments in opjitconv/ describin what's under control of the user -<hanseld> where the code would have (direct) contact to external content. right? -<movement> yes -<movement> for example, file names are under the user's control. -<movement> we seem to test for ".." already though -<movement> (but do we correctly not follow symlinks?) -<hanseld> good point. -<hanseld> i will look for that issue (plus according things). and comments will be added too. -<movement> 288 printf("opjitconv: dumpfile path is corrupt.\n"); -<movement> 289 fflush(stdout); -<movement> 290 rc = OP_JIT_CONV_FAIL; -<movement> e.g. what is consequence of that -<movement> shouldn't invoke any real failure (or even leave a log message - obvious DoS then) -<movement> I'd like to see process_jit_dumpfile decomposed a bit more -<movement> so it's obvious that the "dump file name -> anon name" part has to deal with insecure input and give secure output -<hanseld> sorry. i don't understand. for the code you have mentioned i cannot see any problem. if the path is corrupt an error message will be put out and opjitconv would exit with fail return code. -<movement> where does that error message go? -<hanseld> to stdout -<movement> which is where? -<hanseld> should be oprofiled.log -<hanseld> hmm -<movement> see the problem now? :) -<movement> I wish I could think of a better way to minimise uid==0 code here. -<movement> we need an upper limit on the incoming dump file size -<hanseld> but who should decide where we should cut? -<movement> well there's no good answer to that. see what's in the wild and pick something higher than that. -<movement> hmm even then... user can just create as many as they like -<movement> ugghh. -<hanseld> hmm right. -<movement> up to their quota though. -<hanseld> if there is any quota ... :-| -<hanseld> hmmm... -<hanseld> bad thing -<movement> well that's fine. -<movement> plenty of stuff assumes user quota -<movement> your jitdump directory creation is broken -<movement> it needs to be like /tmp, not 777 -<movement> in particular sticky bit -<hanseld> the point is that for the case a user will create a bad dump file he has to put content in that file that is already good enough to be parsed. the parsing routines have several checks for correct records and so on. - - -To fix or to answer before merge: - -o audit the conversion code, opjitconv must return != 0 only on real failures To fix after merge: o We need a more dynamic structure to handle entries_address_ascending and |