Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/pid/$entry
On Tue, May 27, 2014 at 11:38:54AM -0700, Kees Cook wrote: On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni tix...@opendz.org wrote: Add the deny or allow flags, so we can perform proper permission checks and set the result accordingly. These flags are needed in case we have to cache the result of permission checks that are done during -open() time. Later during -read(), we can decide to allow or deny the read(). The pid entries that need these flags are: /proc/pid/stat /proc/pid/wchan /proc/pid/maps (will be handled in next patches). These files are world readable, userspace depend on that. To prevent ASLR leaks and to avoid breaking userspace, we follow this scheme: a) Perform permission checks during -open() b) Cache the result of a) and return success c) Recheck the cached result during -read() d) If cached == PID_ENTRY_DENY: then we replace the sensitive fields with zeros, userspace won't break and sensitive fields are protected. These flags are internal to /proc/pid/* Since this complex area of behavior has seen a lot of changes, I think I'd really like to see some tests in tools/testsing/selftests/ somewhere that actually codify what the expected behaviors should be. Ok, sounds good! We have a lot of corner cases, a lot of userspace behaviors to retain, and given how fragile this area has been, I'd love to avoid seeing regressions. It seems like we need to test file permissions, open/read permissions, contents, etc, under many different cases (priv, unpriv, passing between priv/unpriv and unpriv/priv, ptrace checks, etc). Yes, nice. If we could do a make run_tests in a selftests subdirectory, it'd be much easier to a) validate these fixes, and b) avoid regressions. Ok! Since I'm working on this on my free time and when time permits, please give me some days! I'll try to handle the cases I've discussed here. Now Kees, some of these files are still world readable and affected: smaps, maps ... I know, it's a matter of suid binary on your distro, and every one can exploit it. So what to do: make the tests public or write the tests and fix these entries then at last make the tests public ? Where should I send the tests ? Thank you! -Kees -- Kees Cook Chrome OS Security -- Djalal Harouni http://opendz.org -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/pid/$entry
On Wed, May 28, 2014 at 4:42 AM, Djalal Harouni tix...@opendz.org wrote: On Tue, May 27, 2014 at 11:38:54AM -0700, Kees Cook wrote: On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni tix...@opendz.org wrote: Add the deny or allow flags, so we can perform proper permission checks and set the result accordingly. These flags are needed in case we have to cache the result of permission checks that are done during -open() time. Later during -read(), we can decide to allow or deny the read(). The pid entries that need these flags are: /proc/pid/stat /proc/pid/wchan /proc/pid/maps (will be handled in next patches). These files are world readable, userspace depend on that. To prevent ASLR leaks and to avoid breaking userspace, we follow this scheme: a) Perform permission checks during -open() b) Cache the result of a) and return success c) Recheck the cached result during -read() d) If cached == PID_ENTRY_DENY: then we replace the sensitive fields with zeros, userspace won't break and sensitive fields are protected. These flags are internal to /proc/pid/* Since this complex area of behavior has seen a lot of changes, I think I'd really like to see some tests in tools/testsing/selftests/ somewhere that actually codify what the expected behaviors should be. Ok, sounds good! We have a lot of corner cases, a lot of userspace behaviors to retain, and given how fragile this area has been, I'd love to avoid seeing regressions. It seems like we need to test file permissions, open/read permissions, contents, etc, under many different cases (priv, unpriv, passing between priv/unpriv and unpriv/priv, ptrace checks, etc). Yes, nice. If we could do a make run_tests in a selftests subdirectory, it'd be much easier to a) validate these fixes, and b) avoid regressions. Ok! Since I'm working on this on my free time and when time permits, please give me some days! I'll try to handle the cases I've discussed here. Now Kees, some of these files are still world readable and affected: smaps, maps ... I know, it's a matter of suid binary on your distro, and every one can exploit it. So what to do: make the tests public or write the tests and fix these entries then at last make the tests public ? I expect these tests to be public -- there is nothing secret about how things are currently vulnerable. I think the priv vs unpriv tests can be emulated (without root setuid) using a prctl(PR_SET_DUMPABLE, 0) call which should give you similar protections. Where should I send the tests ? They should be part of the patch series, and live in the tools/testing/selftests/ tree of the kernel. There are plenty of examples in there. If you have the tests as the first set of patches, then you can show which tests start passing with each additional fix. I would break the tests up into what is expected to work now that all pass, and then add all the cases that are currently a problem that will all fail. Then as more of the fixes land from your series, more of those tests will pass until everything is passing. -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/pid/$entry
On Wed, May 28, 2014 at 09:59:54AM -0700, Kees Cook wrote: On Wed, May 28, 2014 at 4:42 AM, Djalal Harouni tix...@opendz.org wrote: On Tue, May 27, 2014 at 11:38:54AM -0700, Kees Cook wrote: On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni tix...@opendz.org wrote: Add the deny or allow flags, so we can perform proper permission checks and set the result accordingly. These flags are needed in case we have to cache the result of permission checks that are done during -open() time. Later during -read(), we can decide to allow or deny the read(). The pid entries that need these flags are: /proc/pid/stat /proc/pid/wchan /proc/pid/maps (will be handled in next patches). These files are world readable, userspace depend on that. To prevent ASLR leaks and to avoid breaking userspace, we follow this scheme: a) Perform permission checks during -open() b) Cache the result of a) and return success c) Recheck the cached result during -read() d) If cached == PID_ENTRY_DENY: then we replace the sensitive fields with zeros, userspace won't break and sensitive fields are protected. These flags are internal to /proc/pid/* Since this complex area of behavior has seen a lot of changes, I think I'd really like to see some tests in tools/testsing/selftests/ somewhere that actually codify what the expected behaviors should be. Ok, sounds good! We have a lot of corner cases, a lot of userspace behaviors to retain, and given how fragile this area has been, I'd love to avoid seeing regressions. It seems like we need to test file permissions, open/read permissions, contents, etc, under many different cases (priv, unpriv, passing between priv/unpriv and unpriv/priv, ptrace checks, etc). Yes, nice. If we could do a make run_tests in a selftests subdirectory, it'd be much easier to a) validate these fixes, and b) avoid regressions. Ok! Since I'm working on this on my free time and when time permits, please give me some days! I'll try to handle the cases I've discussed here. Now Kees, some of these files are still world readable and affected: smaps, maps ... I know, it's a matter of suid binary on your distro, and every one can exploit it. So what to do: make the tests public or write the tests and fix these entries then at last make the tests public ? I expect these tests to be public -- there is nothing secret about how things are currently vulnerable. I think the priv vs unpriv tests can be emulated (without root setuid) using a prctl(PR_SET_DUMPABLE, 0) call which should give you similar protections. Ok, thanks for the hint! Where should I send the tests ? They should be part of the patch series, and live in the tools/testing/selftests/ tree of the kernel. There are plenty of examples in there. If you have the tests as the first set of patches, then you can show which tests start passing with each additional fix. I would break the tests up into what is expected to work now that all pass, and then add all the cases that are currently a problem that will all fail. Then as more of the fixes land from your series, more of those tests will pass until everything is passing. Ok, will follow and do that. Thank you Kees! -- Kees Cook Chrome OS Security -- Djalal Harouni http://opendz.org -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/pid/$entry
On Mon, May 26, 2014 at 12:17:48PM -0700, Andy Lutomirski wrote: On Mon, May 26, 2014 at 12:13 PM, Djalal Harouni tix...@opendz.org wrote: On Mon, May 26, 2014 at 11:06:40AM -0700, Andy Lutomirski wrote: On Mon, May 26, 2014 at 10:21 AM, Djalal Harouni tix...@opendz.org wrote: I would like to keep it enum, enum is type-safe and I want to follow the semantics of /proc/pid/stat and others: It's not type-safe the way you're doing it, though. Can you please shed some light Andy, thank you in advance! You're casting these things back and forth. If you were storing enum values in an enum-typed variable, great, but you're not. Ok I see your point! Yes, but AFAIK in this case it's perfectly safe! We don't use large values, the one used here can be represented as an int, and later we cast them to unsigned long to make sure that we are using the correct value and the correct size of a pointer. GCC is able to warn in this case of any appropriate cast: -Wpointer-to-int-cast ... And from Documentation/CodingStyle: Enums are preferred when defining several related constants. Perhaps I'm missing something! --Andy -- Djalal Harouni http://opendz.org -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/pid/$entry
On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni tix...@opendz.org wrote: Add the deny or allow flags, so we can perform proper permission checks and set the result accordingly. These flags are needed in case we have to cache the result of permission checks that are done during -open() time. Later during -read(), we can decide to allow or deny the read(). The pid entries that need these flags are: /proc/pid/stat /proc/pid/wchan /proc/pid/maps (will be handled in next patches). These files are world readable, userspace depend on that. To prevent ASLR leaks and to avoid breaking userspace, we follow this scheme: a) Perform permission checks during -open() b) Cache the result of a) and return success c) Recheck the cached result during -read() d) If cached == PID_ENTRY_DENY: then we replace the sensitive fields with zeros, userspace won't break and sensitive fields are protected. These flags are internal to /proc/pid/* Since this complex area of behavior has seen a lot of changes, I think I'd really like to see some tests in tools/testsing/selftests/ somewhere that actually codify what the expected behaviors should be. We have a lot of corner cases, a lot of userspace behaviors to retain, and given how fragile this area has been, I'd love to avoid seeing regressions. It seems like we need to test file permissions, open/read permissions, contents, etc, under many different cases (priv, unpriv, passing between priv/unpriv and unpriv/priv, ptrace checks, etc). If we could do a make run_tests in a selftests subdirectory, it'd be much easier to a) validate these fixes, and b) avoid regressions. -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/9] procfs: use flags to deny or allow access to /proc/pid/$entry
Add the deny or allow flags, so we can perform proper permission checks
and set the result accordingly. These flags are needed in case we have
to cache the result of permission checks that are done during -open()
time. Later during -read(), we can decide to allow or deny the read().
The pid entries that need these flags are:
/proc/pid/stat
/proc/pid/wchan
/proc/pid/maps (will be handled in next patches).
These files are world readable, userspace depend on that. To prevent
ASLR leaks and to avoid breaking userspace, we follow this scheme:
a) Perform permission checks during -open()
b) Cache the result of a) and return success
c) Recheck the cached result during -read()
d) If cached == PID_ENTRY_DENY:
then we replace the sensitive fields with zeros, userspace won't
break and sensitive fields are protected.
These flags are internal to /proc/pid/*
Signed-off-by: Djalal Harouni tix...@opendz.org
---
fs/proc/internal.h | 9 +
1 file changed, 9 insertions(+)
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 3ab6d14..e696284 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -19,6 +19,15 @@ struct ctl_table_header;
struct mempolicy;
/*
+ * Flags used to deny or allow current to access /proc/pid/$entry
+ * after proper permission checks.
+ */
+enum {
+ PID_ENTRY_DENY = 0,/* Deny access */
+ PID_ENTRY_ALLOW = 1,/* Allow access */
+};
+
+/*
* This is not completely implemented yet. The idea is to
* create an in-memory tree (like the actual /proc filesystem
* tree) of these proc_dir_entries, so that we can dynamically
--
1.7.11.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/pid/$entry
On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni tix...@opendz.org wrote:
Add the deny or allow flags, so we can perform proper permission checks
and set the result accordingly. These flags are needed in case we have
to cache the result of permission checks that are done during -open()
time. Later during -read(), we can decide to allow or deny the read().
The pid entries that need these flags are:
/proc/pid/stat
/proc/pid/wchan
/proc/pid/maps (will be handled in next patches).
These files are world readable, userspace depend on that. To prevent
ASLR leaks and to avoid breaking userspace, we follow this scheme:
a) Perform permission checks during -open()
b) Cache the result of a) and return success
c) Recheck the cached result during -read()
Why is (c) needed?
/*
+ * Flags used to deny or allow current to access /proc/pid/$entry
+ * after proper permission checks.
+ */
+enum {
+ PID_ENTRY_DENY = 0,/* Deny access */
+ PID_ENTRY_ALLOW = 1,/* Allow access */
+};
I think this would be less alarming if this were:
#define PID_ENTRY_DENY ((void *)1UL)
#define PID_ENTRY_ALLOW ((void *)2UL)
Also, I don't like DENY and ALLOW. It's not denying and allowing.
How about PID_ENTRY_OPENER_MAY_PTRACE and
PID_ENTRY_OPENER_MAY_NOT_PTRACE?
--Andy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/pid/$entry
On Mon, May 26, 2014 at 09:57:16AM -0700, Andy Lutomirski wrote:
On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni tix...@opendz.org wrote:
Add the deny or allow flags, so we can perform proper permission checks
and set the result accordingly. These flags are needed in case we have
to cache the result of permission checks that are done during -open()
time. Later during -read(), we can decide to allow or deny the read().
The pid entries that need these flags are:
/proc/pid/stat
/proc/pid/wchan
/proc/pid/maps (will be handled in next patches).
These files are world readable, userspace depend on that. To prevent
ASLR leaks and to avoid breaking userspace, we follow this scheme:
a) Perform permission checks during -open()
b) Cache the result of a) and return success
c) Recheck the cached result during -read()
Why is (c) needed?
In order to not break these entries, some of them are world readable.
So we perform the re-check that *single* cached integer, in order to
allow access for the non-sensitive, and block or pad with zeros the
sensitive.
/*
+ * Flags used to deny or allow current to access /proc/pid/$entry
+ * after proper permission checks.
+ */
+enum {
+ PID_ENTRY_DENY = 0,/* Deny access */
+ PID_ENTRY_ALLOW = 1,/* Allow access */
+};
I think this would be less alarming if this were:
#define PID_ENTRY_DENY ((void *)1UL)
#define PID_ENTRY_ALLOW ((void *)2UL)
Hmm,
I would like to keep it enum, enum is type-safe and I want to follow the
semantics of /proc/pid/stat and others:
check the patches and you will see that by making the variable 1 or 0 it
follows what's currently done, and IMHO 0 or 1 is more intuitive in this
case!
Also, I don't like DENY and ALLOW. It's not denying and allowing.
How about PID_ENTRY_OPENER_MAY_PTRACE and
PID_ENTRY_OPENER_MAY_NOT_PTRACE?
Hm, Ok I'll perhaps change this! will see what other thinks!
Thank you!
--Andy
--
Djalal Harouni
http://opendz.org
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/pid/$entry
On Mon, May 26, 2014 at 10:21 AM, Djalal Harouni tix...@opendz.org wrote:
On Mon, May 26, 2014 at 09:57:16AM -0700, Andy Lutomirski wrote:
On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni tix...@opendz.org wrote:
Add the deny or allow flags, so we can perform proper permission checks
and set the result accordingly. These flags are needed in case we have
to cache the result of permission checks that are done during -open()
time. Later during -read(), we can decide to allow or deny the read().
The pid entries that need these flags are:
/proc/pid/stat
/proc/pid/wchan
/proc/pid/maps (will be handled in next patches).
These files are world readable, userspace depend on that. To prevent
ASLR leaks and to avoid breaking userspace, we follow this scheme:
a) Perform permission checks during -open()
b) Cache the result of a) and return success
c) Recheck the cached result during -read()
Why is (c) needed?
In order to not break these entries, some of them are world readable.
So we perform the re-check that *single* cached integer, in order to
allow access for the non-sensitive, and block or pad with zeros the
sensitive.
What I mean is: why not just not re-check? Is it to paper over the
lack of revoke.
/*
+ * Flags used to deny or allow current to access /proc/pid/$entry
+ * after proper permission checks.
+ */
+enum {
+ PID_ENTRY_DENY = 0,/* Deny access */
+ PID_ENTRY_ALLOW = 1,/* Allow access */
+};
I think this would be less alarming if this were:
#define PID_ENTRY_DENY ((void *)1UL)
#define PID_ENTRY_ALLOW ((void *)2UL)
Hmm,
I would like to keep it enum, enum is type-safe and I want to follow the
semantics of /proc/pid/stat and others:
It's not type-safe the way you're doing it, though.
--Andy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/pid/$entry
On Mon, May 26, 2014 at 11:06:40AM -0700, Andy Lutomirski wrote:
On Mon, May 26, 2014 at 10:21 AM, Djalal Harouni tix...@opendz.org wrote:
On Mon, May 26, 2014 at 09:57:16AM -0700, Andy Lutomirski wrote:
On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni tix...@opendz.org wrote:
Add the deny or allow flags, so we can perform proper permission checks
and set the result accordingly. These flags are needed in case we have
to cache the result of permission checks that are done during -open()
time. Later during -read(), we can decide to allow or deny the read().
The pid entries that need these flags are:
/proc/pid/stat
/proc/pid/wchan
/proc/pid/maps (will be handled in next patches).
These files are world readable, userspace depend on that. To prevent
ASLR leaks and to avoid breaking userspace, we follow this scheme:
a) Perform permission checks during -open()
b) Cache the result of a) and return success
c) Recheck the cached result during -read()
Why is (c) needed?
In order to not break these entries, some of them are world readable.
So we perform the re-check that *single* cached integer, in order to
allow access for the non-sensitive, and block or pad with zeros the
sensitive.
What I mean is: why not just not re-check? Is it to paper over the
lack of revoke.
Ahh ok, you mean *re-check* the cached permission during -read() since
this is necessary, and do *not* re-check ptrace capabilities during
-read()!
Indeed, this is precisely due to the lack of revoke! if we do not
re-check ptrace capabilities during -read() we may offer this scenario
to attackers:
open(/proc/$process_I_can_ptrace/*, O_RDONLY)
and make the $process_I_can_ptrace exec a suid binary, this will pass
the cached permission of -open() and let users to read the
/proc/suid-exec/* entries.
In this case a process like cat which we find in all systems can be
used to disclose sensitive data.
In the other hand if we continue to do the ptrace capability check
during read() then attackers need to find a *suid* binary that reads
from specified input in order to bypass that ptrace check during
-read() instead of using a normal program. This is a big difference!
So in the mean time, Yes we must let the re-check ptrace capability
during -read() to reduce the attack surface. Later if there is a
revoke(), then you can remove that ptrace check and just check the cached
permission during -read(), revoke will handle it, in case of an exec!
/*
+ * Flags used to deny or allow current to access /proc/pid/$entry
+ * after proper permission checks.
+ */
+enum {
+ PID_ENTRY_DENY = 0,/* Deny access */
+ PID_ENTRY_ALLOW = 1,/* Allow access */
+};
I think this would be less alarming if this were:
#define PID_ENTRY_DENY ((void *)1UL)
#define PID_ENTRY_ALLOW ((void *)2UL)
Hmm,
I would like to keep it enum, enum is type-safe and I want to follow the
semantics of /proc/pid/stat and others:
It's not type-safe the way you're doing it, though.
Can you please shed some light Andy, thank you in advance!
--Andy
--
To unsubscribe from this list: send the line unsubscribe linux-fsdevel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Djalal Harouni
http://opendz.org
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/pid/$entry
On Mon, May 26, 2014 at 12:13 PM, Djalal Harouni tix...@opendz.org wrote: On Mon, May 26, 2014 at 11:06:40AM -0700, Andy Lutomirski wrote: On Mon, May 26, 2014 at 10:21 AM, Djalal Harouni tix...@opendz.org wrote: I would like to keep it enum, enum is type-safe and I want to follow the semantics of /proc/pid/stat and others: It's not type-safe the way you're doing it, though. Can you please shed some light Andy, thank you in advance! You're casting these things back and forth. If you were storing enum values in an enum-typed variable, great, but you're not. --Andy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
