[cfe-dev] A patch for chroot checker

[章磊]

Hi Ted,

Thank you very much for your reply, next time i will add more comments to
explain my intention.

Comments inline below.

2010/9/15 Ted Kremenek <kremenek at apple.com>

>
> On Sep 14, 2010, at 1:09 AM, 章磊 wrote:
>
> > hi, clang
> >
> > This patch try to check improper use of chroot.
> >
> > In order to implement this checker, i add a subclass (SymbolEnv) of
> SymbolData to represent some environment variables. Now it contains only one
> kind of environment variables(JailKind).Then adds several states to the Jail
> Symbol.
> >
> > This is an experimental checker, and i don't know it is the right way to
> do this stuff.
> >
> > I'll appreciate it if there are any advice about this patch.
> >
>
>
> Hi Lei,
>
> Thanks for the patch.  One thing that would really help me evaluate it are
> some comments in the checker that describe what it is trying to do.  Those
> comments could include code examples that show examples of good and bad
> behavior.
>
>
What i am trying to do is to check CWE-243: Failure to Change Working
Directory in chroot Jail. Because The chroot() function call does not change
the process's current working directory, so in order to properly invoke
chroo() one should call chdir("/") after a call to chroot() before other
function that may break the chroot Jail.

Coverity Prevent says "To fix this defect, immediately call chdir("/") after
a call to chroot()". IMO this may generate many false positives, but i can't
figure out a better way to do this. So this checker's behavior is similar to
what Coverity does:

   1. After the call to chroot(), there shouldn't be any function call
   before calling chdir("/");
   2. but chroot() and chdir() are OK.


I think the point I'm most uncertain about is the addition of the SymbolEnv
> class.  Typically we haven't needed to add new kinds of symbols for specific
> checkers, so if you could explain it's intended usage that would help me
> assess whether it is useful or whether something else is more appropriate
> for your checker.
>
>
I didn't mean to add new kinds of symbols for specific checkers, it's just
because i don't know which Symbol is proper to represent the chroot jail
(may be some other process's environment variables). I think it's process
related, and different from normal variables.


> It looks to me that you are tracking some kind of type state with the
> JailState class (please include a comment above this class that indicates
> what it represents), but I'm not certain what value the SymbolEnv symbol
> serves other than to hang some global typestate in the GDM.  I don't think
> SymbolEnv is necessary since the following line will always return the same
> symbol:
>
>  const SymbolEnv* Sym = SymMgr.getEnvironmentSymbol(SymbolEnv::JailKind);
>
> This means that the ImmutableMap in the GDM will always have at most one
> entry.  It would be much more efficient to store the enum value directly in
> the GDM if it doesn't need to be associated with any symbol.  Also, the
> notion of 'JailKind' seems specific to this checker, and shouldn't be in
> SymbolManager.h.
>

Because i think it's process related, i want SymbolEnv to have only one
object with JailKind. So i only profiles
 EnvKind, and make both getEnvironMentSymbol() to return the same symbol.
If i should store the enum value directly in the GDM, how?


> Minor nit: please don't use tabs (they appear in a few places in your
> patch).
>

Sorry, but how to check this?


>
> Cheers,
> Ted




-- 
Best regards!

Lei Zhang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100915/1f1fd9e8/attachment.html>