* [PATCH 0/2] Return error on invalid pids for procattr funcs.
@ 2016-02-23 20:23 Daniel Cashman
2016-02-23 20:23 ` [PATCH 1/2] libselinux: procattr: return error on invalid pid_t input Daniel Cashman
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Cashman @ 2016-02-23 20:23 UTC (permalink / raw)
To: selinux; +Cc: nnk, jeffv, sds, dcashman
From: dcashman <dcashman@android.com>
The getprocattrcon functions currently do not do any validation of the supplied
pid argument. This argument is nonsensical for negative pid values and is also
not specified for an input value of zero. This is an error-prone convention and
should be changed. This is evidenced by issues such as the following android
bug:
https://code.google.com/p/android/issues/detail?id=200617
dcashman (2):
libselinux: procattr: return error on invalid pid_t input.
libselinux: procattr: return einval for <= 0 pid args.
libselinux/src/procattr.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] libselinux: procattr: return error on invalid pid_t input.
2016-02-23 20:23 [PATCH 0/2] Return error on invalid pids for procattr funcs Daniel Cashman
@ 2016-02-23 20:23 ` Daniel Cashman
2016-02-23 20:24 ` [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args Daniel Cashman
2016-02-24 14:21 ` [PATCH 1/2] libselinux: procattr: return error on invalid pid_t input Stephen Smalley
0 siblings, 2 replies; 11+ messages in thread
From: Daniel Cashman @ 2016-02-23 20:23 UTC (permalink / raw)
To: selinux; +Cc: nnk, jeffv, sds, dcashman
From: dcashman <dcashman@android.com>
Signed-off-by: Daniel Cashman <dcashman@android.com>
---
libselinux/src/procattr.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
index 527a0a5..c20f003 100644
--- a/libselinux/src/procattr.c
+++ b/libselinux/src/procattr.c
@@ -70,9 +70,9 @@ static int openattr(pid_t pid, const char *attr, int flags)
char *path;
pid_t tid;
- if (pid > 0)
+ if (pid > 0) {
rc = asprintf(&path, "/proc/%d/attr/%s", pid, attr);
- else {
+ } else if (pid == 0) {
rc = asprintf(&path, "/proc/thread-self/attr/%s", attr);
if (rc < 0)
return -1;
@@ -82,6 +82,9 @@ static int openattr(pid_t pid, const char *attr, int flags)
free(path);
tid = gettid();
rc = asprintf(&path, "/proc/self/task/%d/attr/%s", tid, attr);
+ } else {
+ errno = EINVAL;
+ return -1;
}
if (rc < 0)
return -1;
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args.
2016-02-23 20:23 ` [PATCH 1/2] libselinux: procattr: return error on invalid pid_t input Daniel Cashman
@ 2016-02-23 20:24 ` Daniel Cashman
2016-02-23 20:29 ` Daniel Cashman
2016-02-24 14:49 ` getpidcon with pid == 0 (Was: Re: [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args.) Stephen Smalley
2016-02-24 14:21 ` [PATCH 1/2] libselinux: procattr: return error on invalid pid_t input Stephen Smalley
1 sibling, 2 replies; 11+ messages in thread
From: Daniel Cashman @ 2016-02-23 20:24 UTC (permalink / raw)
To: selinux; +Cc: nnk, jeffv, sds, dcashman
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)
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args.
2016-02-23 20:24 ` [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args Daniel Cashman
@ 2016-02-23 20:29 ` Daniel Cashman
2016-02-23 21:22 ` Nick Kralevich
2016-02-24 14:49 ` getpidcon with pid == 0 (Was: Re: [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args.) Stephen Smalley
1 sibling, 1 reply; 11+ messages in thread
From: Daniel Cashman @ 2016-02-23 20:29 UTC (permalink / raw)
To: selinux; +Cc: nnk, jeffv, sds
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args.
2016-02-23 20:29 ` Daniel Cashman
@ 2016-02-23 21:22 ` Nick Kralevich
2016-02-29 17:38 ` Stephen Smalley
0 siblings, 1 reply; 11+ messages in thread
From: Nick Kralevich @ 2016-02-23 21:22 UTC (permalink / raw)
To: Daniel Cashman; +Cc: SELinux, Jeffrey Vander Stoep, Stephen Smalley
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
--
Nick Kralevich | Android Security | nnk@google.com | 650.214.4037
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] libselinux: procattr: return error on invalid pid_t input.
2016-02-23 20:23 ` [PATCH 1/2] libselinux: procattr: return error on invalid pid_t input Daniel Cashman
2016-02-23 20:24 ` [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args Daniel Cashman
@ 2016-02-24 14:21 ` Stephen Smalley
1 sibling, 0 replies; 11+ messages in thread
From: Stephen Smalley @ 2016-02-24 14:21 UTC (permalink / raw)
To: Daniel Cashman, selinux
On 02/23/2016 03:23 PM, Daniel Cashman wrote:
> From: dcashman <dcashman@android.com>
>
> Signed-off-by: Daniel Cashman <dcashman@android.com>
Thanks, applied.
> ---
> libselinux/src/procattr.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
> index 527a0a5..c20f003 100644
> --- a/libselinux/src/procattr.c
> +++ b/libselinux/src/procattr.c
> @@ -70,9 +70,9 @@ static int openattr(pid_t pid, const char *attr, int flags)
> char *path;
> pid_t tid;
>
> - if (pid > 0)
> + if (pid > 0) {
> rc = asprintf(&path, "/proc/%d/attr/%s", pid, attr);
> - else {
> + } else if (pid == 0) {
> rc = asprintf(&path, "/proc/thread-self/attr/%s", attr);
> if (rc < 0)
> return -1;
> @@ -82,6 +82,9 @@ static int openattr(pid_t pid, const char *attr, int flags)
> free(path);
> tid = gettid();
> rc = asprintf(&path, "/proc/self/task/%d/attr/%s", tid, attr);
> + } else {
> + errno = EINVAL;
> + return -1;
> }
> if (rc < 0)
> return -1;
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* getpidcon with pid == 0 (Was: Re: [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args.)
2016-02-23 20:24 ` [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args Daniel Cashman
2016-02-23 20:29 ` Daniel Cashman
@ 2016-02-24 14:49 ` Stephen Smalley
2016-02-24 15:25 ` Nick Kralevich
1 sibling, 1 reply; 11+ messages in thread
From: Stephen Smalley @ 2016-02-24 14:49 UTC (permalink / raw)
To: Daniel Cashman, selinux
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)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: getpidcon with pid == 0 (Was: Re: [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args.)
2016-02-24 14:49 ` getpidcon with pid == 0 (Was: Re: [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args.) Stephen Smalley
@ 2016-02-24 15:25 ` Nick Kralevich
2016-02-24 15:32 ` Stephen Smalley
0 siblings, 1 reply; 11+ messages in thread
From: Nick Kralevich @ 2016-02-24 15:25 UTC (permalink / raw)
To: Stephen Smalley; +Cc: Daniel Cashman, SELinux, Jeffrey Vander Stoep
[-- Attachment #1: Type: text/plain, Size: 2101 bytes --]
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)
>>
>>
>
--
Nick Kralevich | Android Security | nnk@google.com | 650.214.4037
[-- Attachment #2: Type: text/html, Size: 3296 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: getpidcon with pid == 0 (Was: Re: [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args.)
2016-02-24 15:25 ` Nick Kralevich
@ 2016-02-24 15:32 ` Stephen Smalley
2016-02-24 16:47 ` Petr Lautrbach
0 siblings, 1 reply; 11+ messages in thread
From: Stephen Smalley @ 2016-02-24 15:32 UTC (permalink / raw)
To: Nick Kralevich; +Cc: Daniel Cashman, SELinux, Jeffrey Vander Stoep
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: getpidcon with pid == 0 (Was: Re: [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args.)
2016-02-24 15:32 ` Stephen Smalley
@ 2016-02-24 16:47 ` Petr Lautrbach
0 siblings, 0 replies; 11+ messages in thread
From: Petr Lautrbach @ 2016-02-24 16:47 UTC (permalink / raw)
To: Stephen Smalley, Nick Kralevich; +Cc: SELinux
[-- Attachment #1: Type: text/plain, Size: 3574 bytes --]
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.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args.
2016-02-23 21:22 ` Nick Kralevich
@ 2016-02-29 17:38 ` Stephen Smalley
0 siblings, 0 replies; 11+ messages in thread
From: Stephen Smalley @ 2016-02-29 17:38 UTC (permalink / raw)
To: Nick Kralevich, Daniel Cashman; +Cc: SELinux, Jeffrey Vander Stoep
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-02-29 17:38 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-23 20:23 [PATCH 0/2] Return error on invalid pids for procattr funcs Daniel Cashman
2016-02-23 20:23 ` [PATCH 1/2] libselinux: procattr: return error on invalid pid_t input Daniel Cashman
2016-02-23 20:24 ` [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args Daniel Cashman
2016-02-23 20:29 ` Daniel Cashman
2016-02-23 21:22 ` Nick Kralevich
2016-02-29 17:38 ` Stephen Smalley
2016-02-24 14:49 ` getpidcon with pid == 0 (Was: Re: [PATCH 2/2] libselinux: procattr: return einval for <= 0 pid args.) Stephen Smalley
2016-02-24 15:25 ` Nick Kralevich
2016-02-24 15:32 ` Stephen Smalley
2016-02-24 16:47 ` Petr Lautrbach
2016-02-24 14:21 ` [PATCH 1/2] libselinux: procattr: return error on invalid pid_t input Stephen Smalley
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.