[cfe-dev] A patch for chroot checker

[章磊]

Yes. I think it's what Coverity did, and this may cause many false
positives.

I got a example like this:

Event chroot_call: Called chroot: "chroot(&"/home/polo/chroot/")"
At conditional (1): "chroot(&"/home/polo/chroot/") != 0" taking false path
7          if (chroot("/home/polo/chroot/") != 0) {
8            perror("chroot");
9            return 0;
10         }
At conditional (2): "chdir(&"../../") != 0" taking true path
11         if (chdir("../../") != 0) {
Event chroot: Called function "perror" after chroot() but before calling
chdir("/")
12           perror("chdir");
13           return 0;
14         }
15         if (chdir("/") != 0) {
16           perror("chdir");
17           return 0;
18         }
19
20         char *arrays[]={"bash", NULL};
21         execvp("bash", arrays);

2010/9/19 Zhongxing Xu <xuzhongxing at gmail.com>

>
>
> 2010/9/19 章磊 <ioripolo at gmail.com>
>
>> Hi Zhongxing,
>>
>> Here are several simple examples( assume all function can successfully
>> execute ):
>>
>>    - workding directory: /home/polo/test/chr
>>
>> chroot("/var/local/ftp");  // root changed.
>> chdir("../../");                   // working directory changed to
>> "/home/polo/", it's out of the jail.
>> chdir("/");                        // enter the jail. working directory
>> changed to "/var/local/ftp".
>> foo();                               // call any other function, ok
>>
>>    - workding directory: /home/polo/test/chr
>>
>> chroot("/var/local/ftp");  // root changed.
>> chdir("/");                        // enter the jail. working directory
>> changed to "/var/local/ftp".
>> chdir("../../");                   // working directory is still
>> "/var/local/ftp", can't escape from the jail.
>> foo();                               // call any other function, ok
>>
>>    - workding directory: /home/polo/test/chr
>>
>> chroot("/var/local/ftp");  // root changed.
>> chdir("../../");                   // working directory changed to
>> "/home/polo/", it's out of the jail.
>> foo();                               // call any other function, may
>> access files outside of the jail.
>>
>> Above is my understanding of chroot and chdir. So IMO the full state
>> transition is something like:
>>
>>
>> NO_CHROOT ---chroot(foo)--> ROOT_CHANGED ---------------chdir(/) -->
>> JAIL_ENTERED
>>
>> |                                                                      |
>>
>> --chdir('..') --> ROOT_CHANGED             --chdir('..')-->JAIL_ENTERED
>>
>> |                                                                      |
>>                                                                    --foo()
>> -->JAIL_BROKEN or bug                --foo()-->JAIL_ENTERED
>>
>>
>>
>
> This FSM looks consistent with your examples. So when we have a call with
> the state being ROOT_CHANGED then should we emit a warning, isn't it?
>
>
>> 2010/9/18 Zhongxing Xu <xuzhongxing at gmail.com>
>>
>>
>>>
>>> 2010/9/17 章磊 <ioripolo at gmail.com>
>>>
>>> Hi Zhongxing,
>>>>
>>>> I think "use enums to represent the type state" it's ok for now, but i
>>>> am not sure it meets the needs in future if we need more precise analysis.
>>>>
>>>> More comments inline below.
>>>>
>>>> 2010/9/16 Zhongxing Xu <xuzhongxing at gmail.com>
>>>>
>>>> Hi Lei,
>>>>>
>>>>> Instead of introducing new symbols, how about use enums to represent
>>>>> the type state?
>>>>>
>>>>> For example, we could use the following states:
>>>>>
>>>>> NO_CHROOT, ROOT_CHANGED, JAIL_ENTERED, JAIL_BROKEN
>>>>>
>>>>> NO_CHROOT ---chroot(foo)--> ROOT_CHANGED ---chdir(/)--> JAIL_ENTERED
>>>>>                                                                    |
>>>>>
>>>>> ------chdir('..')--> JAIL_BROKEN
>>>>>
>>>>
>>>> IMO, it's something like this:
>>>>
>>>>
>>>> NO_CHROOT ---chroot(foo)--> ROOT_CHANGED ---chdir(/) --> JAIL_ENTERED
>>>>
>>>>                                                                    |
>>>>
>>>> ------chdir('..') --> ROOT_CHANGED
>>>>                                                                    |
>>>>
>>>> ------foo() -->JAIL_BROKEN
>>>>
>>>> What you think?
>>>>
>>>
>>> Sorry, I'm not sure about this. Do you have any references that explain
>>> what you are checking for?
>>>
>>>
>>>>
>>>>
>>>>> These states are stored directly in the GDM and operated by the
>>>>> ChrootChecker. Is this sufficient for checking this?
>>>>>
>>>>
>>>> OK, I'll do this later.
>>>>
>>>>
>>>>>
>>>>> On Tue, Sep 14, 2010 at 4:09 PM, 章磊 <ioripolo at gmail.com> 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.
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Best regards!
>>>>>>
>>>>>> Lei Zhang
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-dev mailing list
>>>>>> cfe-dev at cs.uiuc.edu
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Best regards!
>>>>
>>>> Lei Zhang
>>>>
>>>
>>>
>>
>>
>> --
>> Best regards!
>>
>> Lei Zhang
>>
>
>


-- 
Best regards!

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