| Message ID | 1456259040-13721-3-git-send-email-dcashman@android.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On 02/23/2016 12:24 PM, Daniel Cashman wrote: > From: dcashman <dcashman@android.com> > > getpidcon documentation does not specify that a pid of 0 refers to the > current process, and getcon exists specifically to provide this > functionality, and getpidcon(getpid()) would provide it as well. > Disallow pid values <= 0 that may lead to unintended behavior in > userspace object managers. > > Signed-off-by: Daniel Cashman <dcashman@android.com> > --- > libselinux/src/procattr.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c > index c20f003..eee4612 100644 > --- a/libselinux/src/procattr.c > +++ b/libselinux/src/procattr.c > @@ -306,11 +306,21 @@ static int setprocattrcon(const char * context, > #define getpidattr_def(fn, attr) \ > int get##fn##_raw(pid_t pid, char **c) \ > { \ > - return getprocattrcon_raw(c, pid, #attr); \ > + if (pid <= 0) { \ > + errno = EINVAL; \ > + return -1; \ > + } else { \ > + return getprocattrcon_raw(c, pid, #attr); \ > + } \ > } \ > int get##fn(pid_t pid, char **c) \ > { \ > - return getprocattrcon(c, pid, #attr); \ > + if (pid <= 0) { \ > + errno = EINVAL; \ > + return -1; \ > + } else { \ > + return getprocattrcon(c, pid, #attr); \ > + } \ > } > > all_selfattr_def(con, current) > I need to point out explicitly that this change would, of course, break the existing ABI and result in a change in behavior for applications relying on getpidcon(0,) calls to be the same as getcon(). Thanks, Dan
Thanks for proposing this patch Dan. As you said, the current API feels error prone, as it has two entirely different behaviors depending on whether the pid is zero or non-zero. Your patch corrects this error prone API and clearly separates out a query by PID vs a query of the process itself. This patch helps provide robustness against bugs similar to: https://code.google.com/p/google-security-research/issues/detail?id=727 https://code.google.com/p/android/issues/detail?id=200617 (note that the Android code in question was reviewed by Stephen, myself, and others within Google, and we all missed this particular bug) I would recommend the upstream SELinux community accept the patch, even at the potential expense of breaking compatibility with other apps. -- Nick On Tue, Feb 23, 2016 at 12:29 PM, Daniel Cashman <dcashman@android.com> wrote: > On 02/23/2016 12:24 PM, Daniel Cashman wrote: >> From: dcashman <dcashman@android.com> >> >> getpidcon documentation does not specify that a pid of 0 refers to the >> current process, and getcon exists specifically to provide this >> functionality, and getpidcon(getpid()) would provide it as well. >> Disallow pid values <= 0 that may lead to unintended behavior in >> userspace object managers. >> >> Signed-off-by: Daniel Cashman <dcashman@android.com> Acked-by: Nick Kralevich <nnk@google.com> >> --- >> libselinux/src/procattr.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c >> index c20f003..eee4612 100644 >> --- a/libselinux/src/procattr.c >> +++ b/libselinux/src/procattr.c >> @@ -306,11 +306,21 @@ static int setprocattrcon(const char * context, >> #define getpidattr_def(fn, attr) \ >> int get##fn##_raw(pid_t pid, char **c) \ >> { \ >> - return getprocattrcon_raw(c, pid, #attr); \ >> + if (pid <= 0) { \ >> + errno = EINVAL; \ >> + return -1; \ >> + } else { \ >> + return getprocattrcon_raw(c, pid, #attr); \ >> + } \ >> } \ >> int get##fn(pid_t pid, char **c) \ >> { \ >> - return getprocattrcon(c, pid, #attr); \ >> + if (pid <= 0) { \ >> + errno = EINVAL; \ >> + return -1; \ >> + } else { \ >> + return getprocattrcon(c, pid, #attr); \ >> + } \ >> } >> >> all_selfattr_def(con, current) >> > > I need to point out explicitly that this change would, of course, break > the existing ABI and result in a change in behavior for applications > relying on getpidcon(0,) calls to be the same as getcon(). > > Thanks, > Dan
On 02/23/2016 03:24 PM, Daniel Cashman wrote: > From: dcashman <dcashman@android.com> > > getpidcon documentation does not specify that a pid of 0 refers to the > current process, and getcon exists specifically to provide this > functionality, and getpidcon(getpid()) would provide it as well. > Disallow pid values <= 0 that may lead to unintended behavior in > userspace object managers. I'll try to see if there are any legitimate users of getpidcon with pid == 0. If anyone on the list knows of one, please speak up. > > Signed-off-by: Daniel Cashman <dcashman@android.com> > --- > libselinux/src/procattr.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c > index c20f003..eee4612 100644 > --- a/libselinux/src/procattr.c > +++ b/libselinux/src/procattr.c > @@ -306,11 +306,21 @@ static int setprocattrcon(const char * context, > #define getpidattr_def(fn, attr) \ > int get##fn##_raw(pid_t pid, char **c) \ > { \ > - return getprocattrcon_raw(c, pid, #attr); \ > + if (pid <= 0) { \ > + errno = EINVAL; \ > + return -1; \ > + } else { \ > + return getprocattrcon_raw(c, pid, #attr); \ > + } \ > } \ > int get##fn(pid_t pid, char **c) \ > { \ > - return getprocattrcon(c, pid, #attr); \ > + if (pid <= 0) { \ > + errno = EINVAL; \ > + return -1; \ > + } else { \ > + return getprocattrcon(c, pid, #attr); \ > + } \ > } > > all_selfattr_def(con, current) >
A quick Google search for "getpidcon(0" shows only the Android bug. https://www.google.com/webhp#q=%22getpidcon(0%22 -- Nick On Wed, Feb 24, 2016 at 6:49 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 02/23/2016 03:24 PM, Daniel Cashman wrote: > >> From: dcashman <dcashman@android.com> >> >> getpidcon documentation does not specify that a pid of 0 refers to the >> current process, and getcon exists specifically to provide this >> functionality, and getpidcon(getpid()) would provide it as well. >> Disallow pid values <= 0 that may lead to unintended behavior in >> userspace object managers. >> > > I'll try to see if there are any legitimate users of getpidcon with pid == > 0. If anyone on the list knows of one, please speak up. > > >> Signed-off-by: Daniel Cashman <dcashman@android.com> >> --- >> libselinux/src/procattr.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c >> index c20f003..eee4612 100644 >> --- a/libselinux/src/procattr.c >> +++ b/libselinux/src/procattr.c >> @@ -306,11 +306,21 @@ static int setprocattrcon(const char * context, >> #define getpidattr_def(fn, attr) \ >> int get##fn##_raw(pid_t pid, char **c) \ >> { \ >> - return getprocattrcon_raw(c, pid, #attr); \ >> + if (pid <= 0) { \ >> + errno = EINVAL; \ >> + return -1; \ >> + } else { \ >> + return getprocattrcon_raw(c, pid, #attr); \ >> + } \ >> } \ >> int get##fn(pid_t pid, char **c) \ >> { \ >> - return getprocattrcon(c, pid, #attr); \ >> + if (pid <= 0) { \ >> + errno = EINVAL; \ >> + return -1; \ >> + } else { \ >> + return getprocattrcon(c, pid, #attr); \ >> + } \ >> } >> >> all_selfattr_def(con, current) >> >> >
On 02/24/2016 10:25 AM, Nick Kralevich wrote: > A quick Google search for "getpidcon(0" shows only the Android bug. > > https://www.google.com/webhp#q=%22getpidcon(0%22 > > -- Nick Yes, I looked through searchcode.com results for getpidcon (not just with a hardcoded 0 but also looking for any cases where they assume that setting a variable pid == 0 degenerates to getcon behavior), and didn't see anything. I've also asked the Fedora SELinux maintainers if they know of anything that would break. > > On Wed, Feb 24, 2016 at 6:49 AM, Stephen Smalley <sds@tycho.nsa.gov > <mailto:sds@tycho.nsa.gov>> wrote: > > On 02/23/2016 03:24 PM, Daniel Cashman wrote: > > From: dcashman <dcashman@android.com <mailto:dcashman@android.com>> > > getpidcon documentation does not specify that a pid of 0 refers > to the > current process, and getcon exists specifically to provide this > functionality, and getpidcon(getpid()) would provide it as well. > Disallow pid values <= 0 that may lead to unintended behavior in > userspace object managers. > > > I'll try to see if there are any legitimate users of getpidcon with > pid == 0. If anyone on the list knows of one, please speak up. > > > Signed-off-by: Daniel Cashman <dcashman@android.com > <mailto:dcashman@android.com>> > --- > libselinux/src/procattr.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c > index c20f003..eee4612 100644 > --- a/libselinux/src/procattr.c > +++ b/libselinux/src/procattr.c > @@ -306,11 +306,21 @@ static int setprocattrcon(const char * > context, > #define getpidattr_def(fn, attr) \ > int get##fn##_raw(pid_t pid, char **c) \ > { \ > - return getprocattrcon_raw(c, pid, #attr); \ > + if (pid <= 0) { \ > + errno = EINVAL; \ > + return -1; \ > + } else { \ > + return getprocattrcon_raw(c, pid, #attr); \ > + } \ > } \ > int get##fn(pid_t pid, char **c) \ > { \ > - return getprocattrcon(c, pid, #attr); \ > + if (pid <= 0) { \ > + errno = EINVAL; \ > + return -1; \ > + } else { \ > + return getprocattrcon(c, pid, #attr); \ > + } \ > } > > all_selfattr_def(con, current) > > > > > > -- > Nick Kralevich | Android Security | nnk@google.com > <mailto:nnk@google.com> | 650.214.4037
On 02/24/2016 04:32 PM, Stephen Smalley wrote: > On 02/24/2016 10:25 AM, Nick Kralevich wrote: >> A quick Google search for "getpidcon(0" shows only the Android bug. >> >> https://www.google.com/webhp#q=%22getpidcon(0%22 >> >> -- Nick > > Yes, I looked through searchcode.com results for getpidcon (not just > with a hardcoded 0 but also looking for any cases where they assume that > setting a variable pid == 0 degenerates to getcon behavior), and didn't > see anything. > > I've also asked the Fedora SELinux maintainers if they know of anything > that would break. Thanks for heads up. I haven't found such case as well. Nevertheless I resent it to the Fedora development mailing list to make the change more visible. Thanks, Petr > >> >> On Wed, Feb 24, 2016 at 6:49 AM, Stephen Smalley <sds@tycho.nsa.gov >> <mailto:sds@tycho.nsa.gov>> wrote: >> >> On 02/23/2016 03:24 PM, Daniel Cashman wrote: >> >> From: dcashman <dcashman@android.com >> <mailto:dcashman@android.com>> >> >> getpidcon documentation does not specify that a pid of 0 refers >> to the >> current process, and getcon exists specifically to provide this >> functionality, and getpidcon(getpid()) would provide it as well. >> Disallow pid values <= 0 that may lead to unintended behavior in >> userspace object managers. >> >> >> I'll try to see if there are any legitimate users of getpidcon with >> pid == 0. If anyone on the list knows of one, please speak up. >> >> >> Signed-off-by: Daniel Cashman <dcashman@android.com >> <mailto:dcashman@android.com>> >> --- >> libselinux/src/procattr.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/libselinux/src/procattr.c >> b/libselinux/src/procattr.c >> index c20f003..eee4612 100644 >> --- a/libselinux/src/procattr.c >> +++ b/libselinux/src/procattr.c >> @@ -306,11 +306,21 @@ static int setprocattrcon(const char * >> context, >> #define getpidattr_def(fn, attr) \ >> int get##fn##_raw(pid_t pid, char **c) \ >> { \ >> - return getprocattrcon_raw(c, pid, #attr); \ >> + if (pid <= 0) { \ >> + errno = EINVAL; \ >> + return -1; \ >> + } else { \ >> + return getprocattrcon_raw(c, pid, >> #attr); \ >> + } \ >> } \ >> int get##fn(pid_t pid, char **c) \ >> { \ >> - return getprocattrcon(c, pid, #attr); \ >> + if (pid <= 0) { \ >> + errno = EINVAL; \ >> + return -1; \ >> + } else { \ >> + return getprocattrcon(c, pid, #attr); \ >> + } \ >> } >> >> all_selfattr_def(con, current) >> >> >> >> >> >> -- >> Nick Kralevich | Android Security | nnk@google.com >> <mailto:nnk@google.com> | 650.214.4037 > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to > Selinux-request@tycho.nsa.gov.
On 02/23/2016 04:22 PM, Nick Kralevich wrote: > Thanks for proposing this patch Dan. > > As you said, the current API feels error prone, as it has two entirely > different behaviors depending on whether the pid is zero or non-zero. > Your patch corrects this error prone API and clearly separates out a > query by PID vs a query of the process itself. > > This patch helps provide robustness against bugs similar to: > > https://code.google.com/p/google-security-research/issues/detail?id=727 > https://code.google.com/p/android/issues/detail?id=200617 > > (note that the Android code in question was reviewed by Stephen, > myself, and others within Google, and we all missed this particular > bug) > > I would recommend the upstream SELinux community accept the patch, > even at the potential expense of breaking compatibility with other > apps. As I haven't heard or seen of anything that would break with this change, I've merged this patch. You'll need to apply it separately to Android libselinux since that is still a fork.
diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c index c20f003..eee4612 100644 --- a/libselinux/src/procattr.c +++ b/libselinux/src/procattr.c @@ -306,11 +306,21 @@ static int setprocattrcon(const char * context, #define getpidattr_def(fn, attr) \ int get##fn##_raw(pid_t pid, char **c) \ { \ - return getprocattrcon_raw(c, pid, #attr); \ + if (pid <= 0) { \ + errno = EINVAL; \ + return -1; \ + } else { \ + return getprocattrcon_raw(c, pid, #attr); \ + } \ } \ int get##fn(pid_t pid, char **c) \ { \ - return getprocattrcon(c, pid, #attr); \ + if (pid <= 0) { \ + errno = EINVAL; \ + return -1; \ + } else { \ + return getprocattrcon(c, pid, #attr); \ + } \ } all_selfattr_def(con, current)