A patch for chroot checker

Clang Frontend
1

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.

chroot.patch (11.3 KB)

2

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.

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.

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.

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

Cheers,
Ted

3

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@apple.com>

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?

4

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

Sorry, but how to check this?

If you use emacs, set indent-tabs-mode to nil.

5

Hi, zhongxing

Thanks.

If i already used some tabs, how to find it and fix it?

2010/9/15 Zhongxing Xu <xuzhongxing@gmail.com>

6

In Emacs:
C-x h
M-x untabify

From the command line (assuming a *nix shell):

$ sed -e 's/'"$(printf '\011')"'/ /g' filename -o tmp.out
$ mv tmp.out filename

You can tell Emacs to not insert tabs by setting indent-tabs-mode to nil in your startup file. The usual places for Emacs documentation and hints should have some samples of how to do this for only .cpp/c/h/etc files and keep using tabs for things like Makefiles. You may also find it useful to set Emacs up to highlight tab characters for you (EmacsWiki: show-wspace.el).

Regards,
  Jim

7

hi, Jim

Thank you very much. It’s helpful.

2010/9/15 Jim Grosbach <grosbach@apple.com>

8

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

These states are stored directly in the GDM and operated by the ChrootChecker. Is this sufficient for checking this?

9

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@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?

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.

10

2010/9/17 章磊 <ioripolo@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@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?

11

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

2010/9/18 Zhongxing Xu <xuzhongxing@gmail.com>

12

2010/9/19 章磊 <ioripolo@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?

13

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@gmail.com>

14

Hi clang,

Sorry for the late reply. In this patch i re-implement the chrootchecker.

Main changes are:

  1. Remove the SymbolEnv, use enums to represent chroot() jail state.

  2. Directly set the enum value in the GDM.

  3. Add some comments.
    Is there any advice about this patch?

chroot.patch (7.45 KB)

15

Hi Lei,

I committed the patch with minor modification: int does not always has the same width as void*. intptr_t is used. The patch looks good for now, though I’m not quite sure about the rationale behind the check. Since the checker does not affect other parts of the analyzer, we could improve it later.