| Age | Commit message (Collapse) | Author |
|---|
|
BugLink: http://bugs.launchpad.net/bugs/587888
Signed-off-by: Andy Whitcroft <apw@canonical.com>
Signed-off-by: Leann Ogasawara <leann.ogasawara@canonical.com>
|
|
Hardlinks can be abused in a similar fashion to symlinks above, but they
are not limited to world-writable directories. If /etc and /home are on
the same partition, a regular user can create a hardlink to /etc/shadow in
their home directory. While it retains the original owner and permissions,
it is possible for privileged programs that are otherwise symlink-safe
to mistakenly access the file through its hardlink. Additionally, a very
minor untraceable quota-bypassing local denial of service is possible by
an attacker exhausting disk space by filling a world-writable directory
with hardlinks.
The solution is to not allow the creation of hardlinks to files that a
given user would be unable to read or write originally, or are otherwise
sensitive.
Some links to the history of its discussion:
1997 Dec, Yuri Kuzmenko http://lkml.org/lkml/1997/12/29/20
2002 Apr, Chris Wright http://lkml.org/lkml/2002/4/13/99
Past objections and rebuttals could be summarized as:
- Violates POSIX.
- POSIX didn't consider this situation, and it's not useful to follow
a broken specification at the cost of security. Also, please reference
where POSIX says this.
- Might break atd, courier, and other unknown applications that use this
feature.
- These applications are easy to spot and can be tested and
fixed. Applications that are vulnerable to hardlink attacks by not
having the change aren't.
- Applications should correctly drop privileges before attempting to
access user files.
- True, but applications are not perfect, and new software is written
all the time that makes these mistakes; blocking this flaw at the
kernel is a single solution to the entire class of vulnerability.
This patch is based on the patch in grsecurity, which is similar to the
patch in Openwall. I have added a sysctl to toggle the behavior back
to the old handling via /proc/sys/fs/weak-nonaccess-hardlinks, as well as
a ratelimited deprecation warning.
Signed-off-by: Kees Cook <kees.cook@canonical.com>
Acked-by: Tim Gardner <tim.gardner@canonical.com>
Signed-off-by: Leann Ogasawara <leann.ogasawara@canonical.com>
|
|
A long-standing class of security issues is the symlink-based
time-of-check-time-of-use race, most commonly seen in world-writable
directories like /tmp. The common method of exploitation of this flaw
is to cross privilege boundaries when following a given symlink (i.e. a
root process follows a symlink belonging to another user).
The solution is to not permit symlinks to be followed when users do not
match, but only in a world-writable sticky directory (with an additional
improvement that the directory owner's symlinks can always be followed,
regardless who is following them).
Some pointers to the history of earlier discussion that I could find:
1996 Aug, Zygo Blaxell
http://marc.info/?l=bugtraq&m=87602167419830&w=2
1996 Oct, Andrew Tridgell
http://lkml.indiana.edu/hypermail/linux/kernel/9610.2/0086.html
1997 Dec, Albert D Cahalan
http://lkml.org/lkml/1997/12/16/4
2005 Feb, Lorenzo Hernández García-Hierro
http://lkml.indiana.edu/hypermail/linux/kernel/0502.0/1896.html
Past objections and rebuttals could be summarized as:
- Violates POSIX.
- POSIX didn't consider this situation, and it's not useful to follow
a broken specification at the cost of security. Also, please reference
where POSIX says this.
- Might break unknown applications that use this feature.
- Applications that break because of the change are easy to spot and
fix. Applications that are vulnerable to symlink ToCToU by not having
the change aren't.
- Applications should just use mkstemp() or O_CREATE|O_EXCL.
- True, but applications are not perfect, and new software is written
all the time that makes these mistakes; blocking this flaw at the
kernel is a single solution to the entire class of vulnerability.
This patch is based on the patch in grsecurity, which is similar to the
patch in Openwall. I have added a sysctl to toggle the behavior back
to the old handling via /proc/sys/fs/weak-sticky-symlinks, as well as
a ratelimited deprecation warning.
Signed-off-by: Kees Cook <kees.cook@canonical.com>
Acked-by: Tim Gardner <tim.gardner@canonical.com>
Signed-off-by: Leann Ogasawara <leann.ogasawara@canonical.com>
|
|
AppArmor falls back to allocating dfas with vmalloc when memory becomes
fragmented. However dfa life cycle is often dependent on credentials
which can be freed during interrupt context. This can in cause the dfa
to be freed during interrupt context, as well but vfree can not be
used in interrupt context.
So for dfas that are allocated with vmalloc delay freeing them to
a later time by placing them on the kernel shared workqueue.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: Andy Whitcroft <andy.whitcroft@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
OriginalAuthor: John Johansen <john.johansen@canonical.com>
OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
commit: e033dc48e794368f00fa70c2f17ee6e37165069e
BugLink: http://bugs.launchpad.net/bugs/562047
The global sid type was not properly updated when the sid was transitioned
from a u16:u16 pair of global and user sid to a single u32 sid. This
causes the sid to wrap, this won't cause problems for mediation, but could
conceivably cause problems for an extremely long lived learning session
where profile are frequently replaced.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: Andy Whitcroft <andy.whitcroft@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
OriginalAuthor: John Johansen <john.johansen@canonical.com>
OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
commit: 5b2ed5984571ca59570240f505dc90810bb56842
BugLink: http://bugs.launchpad.net/bugs/549428
If a file has been opened under an old version of a profile (one that
has been replaced) it is labeled with the original profile and the
labeling is used to avoid performing revalidation (name lookup + permission
check) on every file access.
Replacement changes the profile pointer so that the labeling check fails
and revalidation must be performed. This can cause a performance regression
that is noticable on files that are accessed frequently.
Make sure to get the newest version of the cached file profile before
comparing to current confinement profile.
Also, the permissions that were granted on open were not being stored
in the file->cxt forcing a revalidation because the check to avoid
revalidation also checks that the requested permissions are a subset of
cached granted permissions.
So Ensure that the granted permissions are stored.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: Andy Whitcroft <andy.whitcroft@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
OriginalAuthor: John Johansen <john.johansen@canonical.com>
OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
commit: 6857acf643ba19eddfc29125fc011a3ce48fe87b
BugLink: http://bugs.launchpad.net/bugs/529288
vmalloc doesn't guarentee that the tlbs of all cpus will be flushed
when it completes. Instead the tlbs gets flushed lazily, however for
AppArmor this is a problem as the dfa becomes live to all cpus as
soon as the profile replacedby value is written (this is even before
locking of the lists are removed).
It is possible for another cpu to be in a state where it has an old
tlb mapping for the vmalloc address (this will be caused by putting
a reference on an old profile while replacing to the current),
so that it references to the wrong memory location when doing dfa
lookups.
Replacement is not a common operation so make sure all memory
aliases are removed before the dfa goes live.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: Andy Whitcroft <andy.whitcroft@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
OriginalAuthor: John Johansen <john.johansen@canonical.com>
OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
commit: b96b66094d397cdf887abb77850e075c2d99b91b
BugLink: http://bugs.launchpad.net/bugs/367499
AppArmor uses lazy reference counting on chains of reference to reduce
refcounting overhead. When loading a replacement profile races with
a task replacing the cred's current reference, it is possible that
cxt->profile is the reference at the head of the replacement chain,
dropping this reference can actually trigger the loss of the last
profile in the chain if there are no more references to it.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: Andy Whitcroft <andy.whitcroft@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
OriginalAuthor: John Johansen <john.johansen@canonical.com>
OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
commit: 8d3ffc7c845dc1277b39572016fbf3265702f4d4
BugLink: http://bugs.launchpad.net/bugs/562056
The default behavior for AppArmor used to be to mediate deleted files.
This can now be controlled on a per profile basis but the field is
not defaulting to the correct value when path_flags is not specified.
This is causing regressions in profiles expecting deleted files to
be mediated by path instead of delegated.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: Andy Whitcroft <andy.whitcroft@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
OriginalAuthor: John Johansen <john.johansen@canonical.com>
OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
commit: bc826999a295c82ce54cfec4f8fa580eaa2cd6e5
BugLink: http://bugs.launchpad.net/bugs/562060
The debug warning of scrubbing the environment variables is used by
userspace to detect when exec and permission failures may be caused
by environment variable scrubbing like when firefox execs java
as in bug http://bugs.launchpad.net/bugs/484148. Having it mispelled
makes it difficult to grep for in the logs.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: Andy Whitcroft <andy.whitcroft@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
OriginalAuthor: John Johansen <john.johansen@canonical.com>
OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
commit: ea35fcdcec2316aa57f2d9d5c9ed9002d90b9607
BugLink: http://bugs.launchpad.net/bugs/367499
A reference for a profile is not obtained from aa_newest_version(cxt->profile)
this would normally be valid as cxt->profile will hold a reference keeping
the profile valid, however cxt->profiles reference is replaced with the
new_profile reference
aa_put_profile(cxt->profile);
/* transfer new profile reference will be released when cxt is freed */
cxt->profile = new_profile;
before we are done referencing profile in
aa_audit_file(profile,
so obtain a reference to profile for the duration of the function.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: Andy Whitcroft <andy.whitcroft@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
OriginalAuthor: John Johansen <john.johansen@canonical.com>
OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
commit: af11e86b3b91f0aaf5155e73d0d3f196124b25e2
BugLink: http://bugs.launchpad.net/bugs/562063
The error case for ptrace permission on exec missed putting the new_profile,
fix this ommission and consolidate the error cases to a single point.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: Andy Whitcroft <andy.whitcroft@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
If the call to aa_unpack in aa_interface_replace_profiles fails, it jumps
to the end of the function which performs a put_namespace on the unassigned
ns variable.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: Andy Whitcroft <andy.whitcroft@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
fails.
Profile verification should not be run if profile unpack fails, as this
will cause an oops trying to dereference invalid data.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Andy Whitcroft <apw@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
Buglink: http://launchpad.net/bugs/551844
AppArmor getprocattr was returning the wrong size for name for unconfined
tasks. It returned the size of memory allocated - 1 (\0 is omitted)
instead of the size of the string. In the case of unconfined tasks the mode
string is not output so the return size needs to be adjusted appropriately.
Signed-off-by: Kees Cook <kees.cook@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Andy Whitcroft <apw@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
policy load
Buglink: http://launchpad.net/bugs/458299
Policy load can allocate larger than page chunks of memory, but it will
attempt to use kmalloc first, and then only fall back to vmalloc if
it needs to. Currently the NO_WARN flag is missing from kmalloc so
it dumps a warning message, when it fails. As this isn't a real
failure lets quiet the warnings.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Andy Whitcroft <apw@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
Buglink: http://launchpad.net/bugs/539437
null_profile's currently have a list ref so they should not be using the
PFLAG_NO_LIST_REF flag, which prevent them from having their references
put correctly resulting in a leak.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Andy Whitcroft <apw@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
Upstream AppArmor has removed files for AppArmor 2.4 userspace compatibility,
reintroduce them.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
Update AppArmor to match upstream drop as of 2010-03-04, at the commit below
commit 167beacc4542efb33baf42570fc4b3be76246357
Author: John Johansen <john.johansen@canonical.com>
Date: Thu Mar 4 08:04:45 2010 -0800
AppArmor: Drop CONFIG_SECURITY_APPARMOR_COMPAT_24
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
Include linux/kref.h to get struct kref for non-x86 architectures.
Fixes compile failures on arm.
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
BugLink: http://bugs.launchpad.net/bugs/496110
BugLink: http://bugs.launchpad.net/bugs/507069
BugLink: http://bugs.launchpad.net/bugs/439560
Update AppArmor to match upstream drop as of 2010-02-18, at the commit below
commit f3bc097f3e3f8a31866bbf4bb850530037ae37d8
Author: John Johansen <john.johansen@canonical.com>
Date: Thu Feb 18 00:14:01 2010 -0800
AppArmor: Don't force cross namespace ptrace restrictions
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
Update AppArmor to match upstream drop as at 2010-01-06, at the commit below:
commit a4159210b7d2edfa92e5e3f9425f4ae6eba326f6
Author: John Johansen <john.johansen@canonical.com>
Date: Wed Jan 6 01:16:18 2010 -0800
AppArmor: Update copyright for 2010
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
Include linux/err.h to get ERR_PTR for non-x86 architectures. Fixes
compile failure.
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
Update AppArmor to mainline drop 2009-10-08.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
|
|
No functional changes.
keyctl_session_to_parent() is the only user of signal->count which needs
the correct value. Change it to use thread_group_empty() instead, this
must be strictly equivalent under tasklist, and imho looks better.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: David Howells <dhowells@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Acked-by: Roland McGrath <roland@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
|
|
call_usermodehelper_keys() uses call_usermodehelper_setkeys() to change
subprocess_info->cred in advance. Now that we have info->init() we can
change this code to set tgcred->session_keyring in context of execing
kernel thread.
Note: since currently call_usermodehelper_keys() is never called with
UMH_NO_WAIT, call_usermodehelper_keys()->key_get() and umh_keys_cleanup()
are not really needed, we could rely on install_session_keyring_to_cred()
which does key_get() on success.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Acked-by: David Howells <dhowells@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
|
|
SHRT_MAX and SHRT_MIN
- C99 knows about USHRT_MAX/SHRT_MAX/SHRT_MIN, not
USHORT_MAX/SHORT_MAX/SHORT_MIN.
- Make SHRT_MIN of type s16, not int, for consistency.
[akpm@linux-foundation.org: fix drivers/dma/timb_dma.c]
[akpm@linux-foundation.org: fix security/keys/keyring.c]
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
|
|
... kill their private list, while we are at it
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
|
|
Of the three uses of kref_set in the kernel:
One really should be kref_put as the code is letting go of a
reference,
Two really should be kref_init because the kref is being
initialised.
This suggests that making kref_set available encourages bad code.
So fix the three uses and remove kref_set completely.
Signed-off-by: NeilBrown <neilb@suse.de>
Acked-by: Mimi Zohar <zohar@us.ibm.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
|
|
We were using the wrong variable here so the error codes weren't being returned
properly. The original code returns -ENOKEY.
Signed-off-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
register_security() became __init function.
So do verify() and security_fixup_ops().
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
This patch adds pathname grouping support, which is useful for grouping
pathnames that cannot be represented using /\{dir\}/ pattern.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
The ACPI dependency moved to the TPM, where it belongs. Although
IMA per-se does not require access to the bios measurement log,
verifying the IMA boot aggregate does, which requires ACPI.
This patch prereq's 'TPM: ACPI/PNP dependency removal'
http://lkml.org/lkml/2010/5/4/378.
Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Reported-by: Jean-Christophe Dubois <jcd@tribudubois.net>
Acked-by: Serge Hallyn <serue@us.ibm.com>
Tested-by: Serge Hallyn <serue@us.ibm.com>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
Use kstrdup when the goal of an allocation is copy a string into the
allocated region.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression from,to;
expression flag,E1,E2;
statement S;
@@
- to = kmalloc(strlen(from) + 1,flag);
+ to = kstrdup(from, flag);
... when != \(from = E1 \| to = E1 \)
if (to==NULL || ...) S
... when != \(from = E2 \| to = E2 \)
- strcpy(to, from);
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
Acked-by: Eric Paris <eparis@redhat.com>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
Use stack memory for pending entry to reduce kmalloc() which will be kfree()d.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
This reverts commit a674fa46c79ffa37995bd1c8e4daa2b3be5a95ae.
Previous revert was a prereq.
Signed-off-by: James Morris <jmorris@namei.org>
|
|
Do preallocation for __key_link() so that the various callers in request_key.c
can deal with any errors from this source before attempting to construct a key.
This allows them to assume that the actual linkage step is guaranteed to be
successful.
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
Conflicts:
security/keys/keyring.c
Resolved conflict with whitespace fix in find_keyring_by_name()
Signed-off-by: James Morris <jmorris@namei.org>
|
|
Some of TOMOYO's functions may sleep after mutex_lock(). If OOM-killer selected
a process which is waiting at mutex_lock(), the to-be-killed process can't be
killed. Thus, replace mutex_lock() with mutex_lock_interruptible() so that the
to-be-killed process can immediately return from TOMOYO's functions.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
Errors from construct_alloc_key() shouldn't just be ignored in the way they are
by construct_key_and_link(). The only error that can be ignored so is
EINPROGRESS as that is used to indicate that we've found a key and don't need
to construct one.
We don't, however, handle ENOMEM, EDQUOT or EACCES to indicate allocation
failures of one sort or another.
Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
keyring_serialise_link_sem is only needed for keyring->keyring links as it's
used to prevent cycle detection from being avoided by parallel keyring
additions.
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
|
|
In Ubuntu, security_path_*() hooks are exported to Unionfs. Thus, prepare for
being called from inside VFS functions because I'm not sure whether it is safe
to use GFP_KERNEL or not.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
call_sbin_request_key() creates a keyring and then attempts to insert a link to
the authorisation key into that keyring, but does so without holding a write
lock on the keyring semaphore.
It will normally get away with this because it hasn't told anyone that the
keyring exists yet. The new keyring, however, has had its serial number
published, which means it can be accessed directly by that handle.
This was found by a previous patch that adds RCU lockdep checks to the code
that reads the keyring payload pointer, which includes a check that the keyring
semaphore is actually locked.
Without this patch, the following command:
keyctl request2 user b a @s
will provoke the following lockdep warning is displayed in dmesg:
===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
security/keys/keyring.c:727 invoked rcu_dereference_check() without protection!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 0
2 locks held by keyctl/2076:
#0: (key_types_sem){.+.+.+}, at: [<ffffffff811a5b29>] key_type_lookup+0x1c/0x71
#1: (keyring_serialise_link_sem){+.+.+.}, at: [<ffffffff811a6d1e>] __key_link+0x4d/0x3c5
stack backtrace:
Pid: 2076, comm: keyctl Not tainted 2.6.34-rc6-cachefs #54
Call Trace:
[<ffffffff81051fdc>] lockdep_rcu_dereference+0xaa/0xb2
[<ffffffff811a6d1e>] ? __key_link+0x4d/0x3c5
[<ffffffff811a6e6f>] __key_link+0x19e/0x3c5
[<ffffffff811a5952>] ? __key_instantiate_and_link+0xb1/0xdc
[<ffffffff811a59bf>] ? key_instantiate_and_link+0x42/0x5f
[<ffffffff811aa0dc>] call_sbin_request_key+0xe7/0x33b
[<ffffffff8139376a>] ? mutex_unlock+0x9/0xb
[<ffffffff811a5952>] ? __key_instantiate_and_link+0xb1/0xdc
[<ffffffff811a59bf>] ? key_instantiate_and_link+0x42/0x5f
[<ffffffff811aa6fa>] ? request_key_auth_new+0x1c2/0x23c
[<ffffffff810aaf15>] ? cache_alloc_debugcheck_after+0x108/0x173
[<ffffffff811a9d00>] ? request_key_and_link+0x146/0x300
[<ffffffff810ac568>] ? kmem_cache_alloc+0xe1/0x118
[<ffffffff811a9e45>] request_key_and_link+0x28b/0x300
[<ffffffff811a89ac>] sys_request_key+0xf7/0x14a
[<ffffffff81052c0b>] ? trace_hardirqs_on_caller+0x10c/0x130
[<ffffffff81394fb9>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
The keyring key type code should use RCU dereference wrappers, even when it
holds the keyring's key semaphore.
Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
find_keyring_by_name() can gain access to a keyring that has had its reference
count reduced to zero, and is thus ready to be freed. This then allows the
dead keyring to be brought back into use whilst it is being destroyed.
The following timeline illustrates the process:
|(cleaner) (user)
|
| free_user(user) sys_keyctl()
| | |
| key_put(user->session_keyring) keyctl_get_keyring_ID()
| || //=> keyring->usage = 0 |
| |schedule_work(&key_cleanup_task) lookup_user_key()
| || |
| kmem_cache_free(,user) |
| . |[KEY_SPEC_USER_KEYRING]
| . install_user_keyrings()
| . ||
| key_cleanup() [<= worker_thread()] ||
| | ||
| [spin_lock(&key_serial_lock)] |[mutex_lock(&key_user_keyr..mutex)]
| | ||
| atomic_read() == 0 ||
| |{ rb_ease(&key->serial_node,) } ||
| | ||
| [spin_unlock(&key_serial_lock)] |find_keyring_by_name()
| | |||
| keyring_destroy(keyring) ||[read_lock(&keyring_name_lock)]
| || |||
| |[write_lock(&keyring_name_lock)] ||atomic_inc(&keyring->usage)
| |. ||| *** GET freeing keyring ***
| |. ||[read_unlock(&keyring_name_lock)]
| || ||
| |list_del() |[mutex_unlock(&key_user_k..mutex)]
| || |
| |[write_unlock(&keyring_name_lock)] ** INVALID keyring is returned **
| | .
| kmem_cache_free(,keyring) .
| .
| atomic_dec(&keyring->usage)
v *** DESTROYED ***
TIME
If CONFIG_SLUB_DEBUG=y then we may see the following message generated:
=============================================================================
BUG key_jar: Poison overwritten
-----------------------------------------------------------------------------
INFO: 0xffff880197a7e200-0xffff880197a7e200. First byte 0x6a instead of 0x6b
INFO: Allocated in key_alloc+0x10b/0x35f age=25 cpu=1 pid=5086
INFO: Freed in key_cleanup+0xd0/0xd5 age=12 cpu=1 pid=10
INFO: Slab 0xffffea000592cb90 objects=16 used=2 fp=0xffff880197a7e200 flags=0x200000000000c3
INFO: Object 0xffff880197a7e200 @offset=512 fp=0xffff880197a7e300
Bytes b4 0xffff880197a7e1f0: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZZZZZZZZZ
Object 0xffff880197a7e200: 6a 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b jkkkkkkkkkkkkkkk
Alternatively, we may see a system panic happen, such as:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000001
IP: [<ffffffff810e61a3>] kmem_cache_alloc+0x5b/0xe9
PGD 6b2b4067 PUD 6a80d067 PMD 0
Oops: 0000 [#1] SMP
last sysfs file: /sys/kernel/kexec_crash_loaded
CPU 1
...
Pid: 31245, comm: su Not tainted 2.6.34-rc5-nofixed-nodebug #2 D2089/PRIMERGY
RIP: 0010:[<ffffffff810e61a3>] [<ffffffff810e61a3>] kmem_cache_alloc+0x5b/0xe9
RSP: 0018:ffff88006af3bd98 EFLAGS: 00010002
RAX: 0000000000000000 RBX: 0000000000000001 RCX: ffff88007d19900b
RDX: 0000000100000000 RSI: 00000000000080d0 RDI: ffffffff81828430
RBP: ffffffff81828430 R08: ffff88000a293750 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000100000 R12: 00000000000080d0
R13: 00000000000080d0 R14: 0000000000000296 R15: ffffffff810f20ce
FS: 00007f97116bc700(0000) GS:ffff88000a280000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000001 CR3: 000000006a91c000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process su (pid: 31245, threadinfo ffff88006af3a000, task ffff8800374414c0)
Stack:
0000000512e0958e 0000000000008000 ffff880037f8d180 0000000000000001
0000000000000000 0000000000008001 ffff88007d199000 ffffffff810f20ce
0000000000008000 ffff88006af3be48 0000000000000024 ffffffff810face3
Call Trace:
[<ffffffff810f20ce>] ? get_empty_filp+0x70/0x12f
[<ffffffff810face3>] ? do_filp_open+0x145/0x590
[<ffffffff810ce208>] ? tlb_finish_mmu+0x2a/0x33
[<ffffffff810ce43c>] ? unmap_region+0xd3/0xe2
[<ffffffff810e4393>] ? virt_to_head_page+0x9/0x2d
[<ffffffff81103916>] ? alloc_fd+0x69/0x10e
[<ffffffff810ef4ed>] ? do_sys_open+0x56/0xfc
[<ffffffff81008a02>] ? system_call_fastpath+0x16/0x1b
Code: 0f 1f 44 00 00 49 89 c6 fa 66 0f 1f 44 00 00 65 4c 8b 04 25 60 e8 00 00 48 8b 45 00 49 01 c0 49 8b 18 48 85 db 74 0d 48 63 45 18 <48> 8b 04 03 49 89 00 eb 14 4c 89 f9 83 ca ff 44 89 e6 48 89 ef
RIP [<ffffffff810e61a3>] kmem_cache_alloc+0x5b/0xe9
This problem is that find_keyring_by_name does not confirm that the keyring is
valid before accepting it.
Skipping keyrings that have been reduced to a zero count seems the way to go.
To this end, use atomic_inc_not_zero() to increment the usage count and skip
the candidate keyring if that returns false.
The following script _may_ cause the bug to happen, but there's no guarantee
as the window of opportunity is small:
#!/bin/sh
LOOP=100000
USER=dummy_user
/bin/su -c "exit;" $USER || { /usr/sbin/adduser -m $USER; add=1; }
for ((i=0; i<LOOP; i++))
do
/bin/su -c "echo '$i' > /dev/null" $USER
done
(( add == 1 )) && /usr/sbin/userdel -r $USER
exit
Note that the nominated user must not be in use.
An alternative way of testing this may be:
for ((i=0; i<100000; i++))
do
keyctl session foo /bin/true || break
done >&/dev/null
as that uses a keyring named "foo" rather than relying on the user and
user-session named keyrings.
Reported-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
key_gc_keyring() needs to either hold the RCU read lock or hold the keyring
semaphore if it's going to scan the keyring's list. Given that it only needs
to read the key list, and it's doing so under a spinlock, the RCU read lock is
the thing to use.
Furthermore, the RCU check added in e7b0a61b7929632d36cf052d9e2820ef0a9c1bfe is
incorrect as holding the spinlock on key_serial_lock is not grounds for
assuming a keyring's pointer list can be read safely. Instead, a simple
rcu_dereference() inside of the previously mentioned RCU read lock is what we
want.
Reported-by: Serge E. Hallyn <serue@us.ibm.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
Acked-by: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
Fix an RCU warning in the reading of user keys:
===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
security/keys/user_defined.c:202 invoked rcu_dereference_check() without protection!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 0
1 lock held by keyctl/3637:
#0: (&key->sem){+++++.}, at: [<ffffffff811a80ae>] keyctl_read_key+0x9c/0xcf
stack backtrace:
Pid: 3637, comm: keyctl Not tainted 2.6.34-rc5-cachefs #18
Call Trace:
[<ffffffff81051f6c>] lockdep_rcu_dereference+0xaa/0xb2
[<ffffffff811aa55f>] user_read+0x47/0x91
[<ffffffff811a80be>] keyctl_read_key+0xac/0xcf
[<ffffffff811a8a06>] sys_keyctl+0x75/0xb7
[<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
The ACPI dependency moved to the TPM, where it belongs. Although
IMA per-se does not require access to the bios measurement log,
verifying the IMA boot aggregate does, which requires ACPI.
This patch prereq's 'TPM: ACPI/PNP dependency removal'
http://lkml.org/lkml/2010/5/4/378.
Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Reported-by: Jean-Christophe Dubois <jcd@tribudubois.net>
Acked-by: Serge Hallyn <serue@us.ibm.com>
Tested-by: Serge Hallyn <serue@us.ibm.com>
Signed-off-by: James Morris <jmorris@namei.org>
|
|
On Tue, 2010-04-27 at 11:47 -0700, David Miller wrote:
> From: "Tom \"spot\" Callaway" <tcallawa@redhat.com>
> Date: Tue, 27 Apr 2010 14:20:21 -0400
>
> > [root@apollo ~]$ cat /proc/2174/maps
> > 00010000-00014000 r-xp 00000000 fd:00 15466577
> > /sbin/mingetty
> > 00022000-00024000 rwxp 00002000 fd:00 15466577
> > /sbin/mingetty
> > 00024000-00046000 rwxp 00000000 00:00 0
> > [heap]
>
> SELINUX probably barfs on the executable heap, the PLT is in the HEAP
> just like powerpc32 and that's why VM_DATA_DEFAULT_FLAGS has to set
> both executable and writable.
>
> You also can't remove the CONFIG_PPC32 ifdefs in selinux, since
> because of the VM_DATA_DEFAULT_FLAGS setting used still in that arch,
> the heap will always have executable permission, just like sparc does.
> You have to support those binaries forever, whether you like it or not.
>
> Let's just replace the CONFIG_PPC32 ifdef in SELINUX with CONFIG_PPC32
> || CONFIG_SPARC as in Tom's original patch and let's be done with
> this.
>
> In fact I would go through all the arch/ header files and check the
> VM_DATA_DEFAULT_FLAGS settings and add the necessary new ifdefs to the
> SELINUX code so that other platforms don't have the pain of having to
> go through this process too.
To avoid maintaining per-arch ifdefs, it seems that we could just
directly use (VM_DATA_DEFAULT_FLAGS & VM_EXEC) as the basis for deciding
whether to enable or disable these checks. VM_DATA_DEFAULT_FLAGS isn't
constant on some architectures but instead depends on
current->personality, but we want this applied uniformly. So we'll just
use the initial task state to determine whether or not to enable these
checks.
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
Acked-by: David S. Miller <davem@davemloft.net>
Signed-off-by: James Morris <jmorris@namei.org>
|
