A patch for chroot checker

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

A patch for chroot checker

Lei Zhang
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
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

chroot.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Ted Kremenek

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.

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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Lei Zhang
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 <[hidden email]>

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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Zhongxing Xu
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.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Lei Zhang
Hi, zhongxing

Thanks.

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


2010/9/15 Zhongxing Xu <[hidden email]>
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.



--
Best regards!

Lei Zhang

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Jim Grosbach
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 (http://www.emacswiki.org/cgi-bin/wiki/show-wspace.el).

Regards,
  Jim


On Sep 15, 2010, at 1:09 AM, 章磊 wrote:

> Hi, zhongxing
>
> Thanks.
>
> If i already used some tabs, how to find it and fix it?
>
>
> 2010/9/15 Zhongxing Xu <[hidden email]>
> 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.
>
>
>
> --
> Best regards!
>
> Lei Zhang
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Lei Zhang
hi, Jim

Thank you very much. It's helpful.

2010/9/15 Jim Grosbach <[hidden email]>
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 (http://www.emacswiki.org/cgi-bin/wiki/show-wspace.el).

Regards,
 Jim


On Sep 15, 2010, at 1:09 AM, 章磊 wrote:

> Hi, zhongxing
>
> Thanks.
>
> If i already used some tabs, how to find it and fix it?
>
>
> 2010/9/15 Zhongxing Xu <[hidden email]>
> 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.
>
>
>
> --
> Best regards!
>
> Lei Zhang
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev




--
Best regards!

Lei Zhang

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Zhongxing Xu
In reply to this post by Lei Zhang
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?

On Tue, Sep 14, 2010 at 4:09 PM, 章磊 <[hidden email]> 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
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Lei Zhang
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 <[hidden email]>
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.
 

On Tue, Sep 14, 2010 at 4:09 PM, 章磊 <[hidden email]> 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
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev





--
Best regards!

Lei Zhang

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Zhongxing Xu


2010/9/17 章磊 <[hidden email]>
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 <[hidden email]>

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, 章磊 <[hidden email]> 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
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev





--
Best regards!

Lei Zhang


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Lei Zhang
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 <[hidden email]>


2010/9/17 章磊 <[hidden email]>

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 <[hidden email]>

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, 章磊 <[hidden email]> 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
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev





--
Best regards!

Lei Zhang




--
Best regards!

Lei Zhang

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Zhongxing Xu


2010/9/19 章磊 <[hidden email]>
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 <[hidden email]>



2010/9/17 章磊 <[hidden email]>

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 <[hidden email]>

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, 章磊 <[hidden email]> 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
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev





--
Best regards!

Lei Zhang




--
Best regards!

Lei Zhang


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Lei Zhang
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 <[hidden email]>


2010/9/19 章磊 <[hidden email]>
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 <[hidden email]>



2010/9/17 章磊 <[hidden email]>

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 <[hidden email]>

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, 章磊 <[hidden email]> 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
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev





--
Best regards!

Lei Zhang




--
Best regards!

Lei Zhang




--
Best regards!

Lei Zhang

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Lei Zhang
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?


--
Best regards!

Lei Zhang

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

chroot.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: A patch for chroot checker

Zhongxing Xu
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.

On Fri, Oct 8, 2010 at 4:58 PM, 章磊 <[hidden email]> wrote:
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?


--
Best regards!

Lei Zhang


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev