lkml.org 
[lkml]   [2008]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [TOMOYO #12 (2.6.28-rc2-mm1) 06/11] Common functions for TOMOYO Linux.
Andrew Morton wrote:
> > +static bool is_byte_range(const char *str)
> > +{
> > + return *str >= '0' && *str++ <= '3' &&
> > + *str >= '0' && *str++ <= '7' &&
> > + *str >= '0' && *str <= '7';
> > +}
>
> Well... why?
>
> I cannot think of any kernel interfaces which use octal strings. What
> is special about Tomoyo?
>
TOMOYO uses \ooo style representation for 0x01 - 0x20 and 0x7F - 0xFF.
This function verifies that \ooo is in valid range.

> > +static bool is_decimal(const char c)
> > +{
> > + return c >= '0' && c <= '9';
> > +}
>
> This duplicates a standard ctype.h function.
>
Replaced "is_decimal" by "isdigit".

> > +static bool is_hexadecimal(const char c)
> > +{
> > + return (c >= '0' && c <= '9') ||
> > + (c >= 'A' && c <= 'F') ||
> > + (c >= 'a' && c <= 'f');
> > +}
>
> And so does this.
>
Replaced "is_hexadecimal" by "isxdigit".

> > +static bool is_alphabet_char(const char c)
> > +{
> > + return (c >= 'A' && c <= 'F') || (c >= 'a' && c <= 'f');
> > +}
>
> As does this.
>
Oops! "(c >= 'A' && c <= 'F') || (c >= 'a' && c <= 'f');" was wrong. X-p
It is "(c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');".
But, not found in a standard ctype.h function.

> > +static bool str_starts(char **src, const char *find)
> > +{
> > + const int len = strlen(find);
> > + char *tmp = *src;
> > +
> > + if (strncmp(tmp, find, len))
> > + return false;
> > + tmp += len;
> > + *src = tmp;
> > + return true;
> > +}
>
> hrm. Isn't there a standard string.h way of doing this?
>
> If not, it looks like a pretty common thing. I'd suggest that it a) be
> coded to not do two passes across the input and b) proposed as a
> generic addition to the kernel's string library functions.
>
> > +static void normalize_line(unsigned char *buffer)
> > +{
> > + unsigned char *sp = buffer;
> > + unsigned char *dp = buffer;
> > + bool first = true;
> > +
> > + while (*sp && (*sp <= ' ' || *sp >= 127))
> > + sp++;
> > + while (*sp) {
> > + if (!first)
> > + *dp++ = ' ';
> > + first = false;
> > + while (*sp > ' ' && *sp < 127)
> > + *dp++ = *sp++;
> > + while (*sp && (*sp <= ' ' || *sp >= 127))
> > + sp++;
> > + }
> > + *dp = '\0';
> > +}
>
> that looks pretty generic as well.
>
If you think TOMOYO's way of string representation (described below) is useful,
I'd like to propose these functions as generic functions.

> It seems to have duplicated isprint() in lots of places.
>
It is different from "isprint()".

TOMOYO uses only 0x21 - 0x7E (as printable characters) and 0x20 (as word
delimiter) and 0x0A (as line delimiter).

> What happens if I have a filename which includes a character in the
> 128->255 range?

0x01 - 0x20 and 0x80 - 0xFF will be handled in \ooo style representation.
The reason to use \ooo is to guarantee that "%s" won't damage logs.
Userland program can request

open("/tmp/file granted.\nAccess /tmp/file ", O_WRONLY | O_CREAT)
and auditing such crazy pathname using "Access %s denied.\n" format
will results in "fabrication of audit logs" like

Access /tmp/file granted.
Access /tmp/file rejected.

TOMOYO converts such characters to \ooo so that the auditing will generate

Access /tmp/file\040granted.\012Access\040/tmp/file rejected.

and the administrator can read the audited logs safely using /bin/cat .

> > +struct domain_info *tmy_find_domain(const char *domainname)
> > +{
> > + struct domain_info *domain;
> > + struct path_info name;
> > +
> > + name.name = domainname;
> > + tmy_fill_path_info(&name);
> > + list1_for_each_entry(domain, &domain_list, list) {
> > + if (!domain->is_deleted &&
> > + !tmy_pathcmp(&name, domain->domainname))
> > + return domain;
> > + }
> > + return NULL;
> > +}
>
> No lock was taken to protect that list.
>
No lock needed for protecting "list1" list.
list1 was reviewed by Paul E. McKenney. ( http://lkml.org/lkml/2008/10/20/4 ).

> If the caller must take some lock then that precondition should be
> documented in the function's comment.
>
To your surprise, most functions are lock free, due to use of
"append only singly linked list" named "list1".
Only tmy_real_domain() requires the caller to take "tasklist_lock".
tmy_read_control() and tmy_write_control take "struct tmy_io_buffer"->io_sem
before calling "struct tmy_io_buffer"->read and "struct tmy_io_buffer"->write
methods.
All other functions don't require the caller to take some lock.

> > +/**
> > + * path_depth - Evaluate the number of '/' in a string.
> > + *
> > + * @pathname: The string to evaluate.
> > + *
> > + * Returns path depth of the string.
> > + *
> > + * I score 2 for each of the '/' in the @pathname
> > + * and score 1 if the @pathname ends with '/'.
> > + */
> > +static int path_depth(const char *pathname)
> > +{
> > + int i = 0;
> > +
> > + if (pathname) {
> > + char *ep = strchr(pathname, '\0');
>
> what? Does that even work? strchr(p, 0) should always return NULL:
>
> RETURN VALUE
> The strchr() and strrchr() functions return a pointer to the matched
> character or NULL if the character is not found.
>
>
> Using
>
> pathname + strlen(pathname)
>
> would be saner, no?
>
strchr(p, 0) returns the location of '\0', not NULL.
But, "p + strlen(p)" could generate faster assembly code than "strchr(p, 0)".
Replaced "strchr(p, 0)" by "p + strlen(p)".

> > + if (pathname < ep--) {
> > + if (*ep != '/')
> > + i++;
> > + while (pathname <= ep)
> > + if (*ep-- == '/')
> > + i += 2;
> > + }
> > + }
> > + return i;
> > +}
>
> I cannot imagine why this function exists :(
>
To hash string for faster comparison.

> > [vast amounts of string hacking snipped]
>
> This seems like madness, sorry.
>
> Why the heck is so much string bashing going on in here???
>
Yeah, You would go crazy with functions that handle string data.
But these functions are needed to stay inside the kernel for validating,
hashing and comparing string data.

> > +/**
> > + * tmy_io_printf - Transactional printf() to "struct tmy_io_buffer" structure.
> > + *
> > + * @head: Pointer to "struct tmy_io_buffer".
> > + * @fmt: The printf()'s format string, followed by parameters.
> > + *
> > + * Returns true on success, false otherwise.
>
> This comment should explain what the terms "success" and "failure"
> refer to. Perhaps "success"=="the output didn't overflow" or something.
>
Replaced "Returns true on success" by "Returns true if output was written".

> > +static const char *tmy_get_exe(void)
> > +{
> > + struct mm_struct *mm = current->mm;
> > + struct vm_area_struct *vma;
> > + const char *cp = NULL;
> > +
> > + if (!mm)
> > + return NULL;
> > + down_read(&mm->mmap_sem);
> > + for (vma = mm->mmap; vma; vma = vma->vm_next) {
> > + if ((vma->vm_flags & VM_EXECUTABLE) && vma->vm_file) {
> > + cp = tmy_realpath_from_path(&vma->vm_file->f_path);
> > + break;
> > + }
> > + }
> > + up_read(&mm->mmap_sem);
> > + return cp;
> > +}
>
> What guarantees that the first executable mapping in the mapping list
> is the correct one for the executable?
>
It is just for auditing. Not for security decision.

> What prevents us from accidentally breaking that guarantee in the
> future? I wasn't even aware that this was the case.
>
> What happens if the executable was unlinked?
>
audit_log_task_info() is doing the same thing.

> > +/**
> > + * tmy_check_flags - Check mode for specified functionality.
> > + *
> > + * @domain: Pointer to "struct domain_info".
> > + * @index: The functionality to check mode.
> > + *
> > + * Returns the mode of specified functionality.
>
> That description is rather meaningless.
>
Removed the description.

> > +unsigned int tmy_check_flags(const struct domain_info *domain, const u8 index)
> > +{
> > + const u8 profile = domain->profile;
> > +
> > + if (unlikely(in_interrupt())) {
> > + static u8 count = 20;
> > + if (count) {
> > + count--;
> > + printk(KERN_ERR "BUG: sleeping function called "
> > + "from invalid context.\n");
> > + dump_stack();
> > + }
> > + return 0;
> > + }
>
> a) WARN_ON is preferred
>
> b) WARN_ON_ONCE might be usable here
>
> c) what on earth is this code doing??
>
Replaced "count" with "WARN_ON".

TOMOYO checks only process context.
This code disables TOMOYO's enforcement in case the function is called from
interrupt context.

> > + return sbin_init_started && index < TMY_MAX_CONTROL_INDEX
> > +#if MAX_PROFILES != 256
> > + && profile < MAX_PROFILES
> > +#endif
> > + && profile_ptr[profile] ?
> > + profile_ptr[profile]->value[index] : 0;
> > +}
>
> And this. I cannot imagine why Tomoyo cares whether /sbin/init has
> started yet. This sort of thing should be commented!
>
TOMOYO loads policy using /sbin/tomoyo-init when /sbin/init starts.
Replaced "sbin_init_started" by "tmy_policy_loaded".

> What happens in a cgroups environment where there will be multiple
> /sbin/inits running?
Nothing. /sbin/tomoyo-init is called only for the first time /sbin/init
is requested and /sbin/tomoyo-init exists.

> > +/**
> > + * tmy_check_domain_quota - Check for domain's quota.
> > + *
> > + * @domain: Pointer to "struct domain_info".
> > + *
> > + * Returns true if the domain is not exceeded quota, false otherwise.
> > + */
>
> This function is poorly named.
>
Replaced "tmy_check_domain_quota" by "tomoyo_domain_quota_is_ok".

> > +/**
> > + * tmy_find_or_assign_new_profile - Create a new profile.
> > + *
> > + * @profile: Profile number to create.
> > + *
> > + * Returns pointer to "struct profile" on success, NULL otherwise.
> > + */
> > +static struct profile *tmy_find_or_assign_new_profile(const unsigned int
> > + profile)
> > +{
> > + static DEFINE_MUTEX(lock);
> > + struct profile *ptr = NULL;
> > +
> > + /***** EXCLUSIVE SECTION START *****/
> > + mutex_lock(&lock);
> > + if (profile < MAX_PROFILES) {
>
> This check didn't need to be inside the lock.
>
Moved the "if" to before the lock.

> > +/**
> > + * write_profile - Write profile table.
>
> where to?
>
Write to profile table. Fixed.

> > +/**
> > + * read_profile - Read profile table.
>
> Where from?
>
Read from profile table. Fixed.

> These functions appear to be implementing more userspace interfaces.
>
> The userspace interface is the most important part of any kernel code.
> We can change all the internal details, but the interfaces will live
> forever.
>
> Hence we should review the proposed interfaces before even looking at
> the code. Indeed, before even writing the code.
>
> What are the Tomoyo kernel interfaces?

I'll describe it in other posting.

> > +/* Structure for policy manager. */
> > +struct policy_manager_entry {
> > + struct list1_head list;
> > + /* A path to program or a domainname. */
> > + const struct path_info *manager;
> > + bool is_domain; /* True if manager is a domainname. */
> > + bool is_deleted; /* True if this entry is deleted. */
> > +};
> > +
> > +/*
> > + * The list for "struct policy_manager_entry".
> > + *
> > + * This list is updated only inside update_manager_entry(), thus
> > + * no global mutex exists.
> > + */
> > +static LIST1_HEAD(policy_manager_list);
> > +
> > +/**
> > + * update_manager_entry - Add a manager entry.
> > + *
> > + * @manager: The path to manager or the domainnamme.
> > + * @is_delete: True if it is a delete request.
> > + *
> > + * Returns 0 on success, negative value otherwise.
> > + */
>
> eh? So deleted entries get their "is_deleted" flag set but they are
> never actually removed from the list nor freed? So over time the list
> gets longer and longer and consumes more and more memory?
>
Right. Most of elements are allocated when /sbin/init starts, and they
are referred during lifetime of the kernel. Deleted entries get their
"is_deleted" flag set but they are never actually removed from the list
nor freed. But don't worry. The amount of memory used by TOMOYO is quite
small (about 1MB or so).

> > +/**
> > + * write_manager_policy - Write manager policy.
> > + *
> > + * @head: Pointer to "struct tmy_io_buffer"
> > + *
> > + * Returns 0 on success, negative value otherwise.
> > + */
>
> More userspace ABI proposals?
>
Yes. But, it is only for TOMOYO's management tools.
TOMOYO requires no modification of existing userland programs
and provides no API for existing userland programs.

> > +/**
> > + * is_policy_manager - Check whether the current process is a policy manager.
> > + *
> > + * Returns true if the current process is permitted to modify policy
> > + * via /sys/kernel/security/tomoyo/ interface.
> > + */
> > +static bool is_policy_manager(void)
> > +{
> > + struct policy_manager_entry *ptr;
> > + const char *exe;
> > + const struct task_struct *task = current;
> > + const struct path_info *domainname = tmy_domain()->domainname;
> > + bool found = false;
> > +
> > + if (!sbin_init_started)
> > + return true;
> > + if (!manage_by_non_root && (task->cred->uid || task->cred->euid))
> > + return false;
>
> What happens in a containerised environment where uids are non-unique
> and where there are multiple /sbin/inits?
>
This interface is designed to be accessed by processes having init_pid_ns
namespace.

> > + if (!found) { /* Reduce error messages. */
> > + static pid_t last_pid;
> > + const pid_t pid = current->pid;
> > + if (last_pid != pid) {
> > + printk(KERN_WARNING "%s ( %s ) is not permitted to "
> > + "update policies.\n", domainname->name, exe);
>
> It appears that unprivileged userspace can cause this messge to be
> printed at will. That can cause the logs to fill and is considered to
> be a small denial of service security hole.
>
By default, this pseudo file is "root:root" and it's permission is 0600.

> > +static bool is_select_one(struct tmy_io_buffer *head, const char *data)
> > +{
> > + unsigned int pid;
> > + struct domain_info *domain = NULL;
> > +
> > + if (sscanf(data, "pid=%u", &pid) == 1) {
>
> PIDs are no longer system-wide unique, and here we appear to be
> implementing new userspace ABIs using PIDs.
>
This interface is designed to be accessed by processes having init_pid_ns
namespace.

> > +static int write_domain_profile(struct tmy_io_buffer *head)
> > +{
> > + char *data = head->write_buf;
> > + char *cp = strchr(data, ' ');
> > + struct domain_info *domain;
> > + unsigned long profile;
> > +
> > + if (!cp)
> > + return -EINVAL;
> > + *cp = '\0';
> > + domain = tmy_find_domain(cp + 1);
> > + strict_strtoul(data, 10, &profile);
>
> Unchecked return value?
>
Added checking.

> > +/* path to policy loader */
> > +static const char *tmy_loader = "/sbin/tomoyo-init";
>
> hm, hard-wired knowledge of filesytem layout.
>
> We did this in a few places already, reluctantly. We did at least make
> them configurable (eg: /proc/sys/kernel/modprobe).
>
> It's rather ugly to be doing this sort of thing.
>
This pathname is embedded into the kernel to avoid modification of
userland program.
/proc/sys/kernel/tmy_loader seems redundant. Should we use __setup()?

> > +static bool policy_loader_exists(void)
> > +{
> > + /*
> > + * Don't activate MAC if the policy loader doesn't exist.
> > + * If the initrd includes /sbin/init but real-root-dev has not
> > + * mounted on / yet, activating MAC will block the system since
> > + * policies are not loaded yet.
> > + * Thus, let do_execve() call this function everytime.
> > + */
> > + struct nameidata nd;
> > +
> > + if (path_lookup(tmy_loader, LOOKUP_FOLLOW, &nd)) {
> > + printk(KERN_INFO "Not activating Mandatory Access Control now "
> > + "since %s doesn't exist.\n", tmy_loader);
> > + return false;
> > + }
> > + path_put(&nd.path);
> > + return true;
> > +}
>
> If you really really have to do this then a simple call to sys_access()
> might suffice.
>
To use sys_access(), we need to add get_fs()/set_fs() stuff.
It's not simple.

> But it is of course racy against concurrent rename, unlink, etc.
>
Racing is not a problem. Policy loader is called *only once* upon boot.

> > +void tmy_load_policy(const char *filename)
> > +{
> > + char *argv[2];
> > + char *envp[3];
> > +
> > + if (sbin_init_started)
> > + return;
> > + /*
> > + * Check filename is /sbin/init or /sbin/tomoyo-start.
> > + * /sbin/tomoyo-start is a dummy filename in case where /sbin/init can't
> > + * be passed.
> > + * You can create /sbin/tomoyo-start by
> > + * "ln -s /bin/true /sbin/tomoyo-start".
> > + */
> > + if (strcmp(filename, "/sbin/init") &&
> > + strcmp(filename, "/sbin/tomoyo-start"))
> > + return;
> > + if (!policy_loader_exists())
> > + return;
>
> Why do this? call_usermodehelper() will simply fail if the file isn't here.
>
Without policy_loader_exists(), the system will panic() if
/sbin/init is requested but the policy loader doesn't exist.

> > + printk(KERN_INFO "Calling %s to load policy. Please wait.\n",
> > + tmy_loader);
> > + argv[0] = (char *) tmy_loader;
> > + argv[1] = NULL;
> > + envp[0] = "HOME=/";
> > + envp[1] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
> > + envp[2] = NULL;
> > + call_usermodehelper(argv[0], argv, envp, 1);
> > +
> > + printk(KERN_INFO "TOMOYO: 2.2.0-pre 2008/10/10\n");
> > + printk(KERN_INFO "Mandatory Access Control activated.\n");
> > + sbin_init_started = true;
> > + { /* Check all profiles currently assigned to domains are defined. */
> > + struct domain_info *domain;
> > + list1_for_each_entry(domain, &domain_list, list) {
> > + const u8 profile = domain->profile;
> > + if (profile_ptr[profile])
> > + continue;
> > + panic("Profile %u (used by '%s') not defined.\n",
> > + profile, domain->domainname->name);
> > + }
> > + }
> > +}

> > +/**
> > + * read_updates_counter - Check for policy change counter.
> > + *
> > + * @head: Pointer to "struct tmy_io_buffer".
> > + *
> > + * Returns how many times policy has changed since the previous check.
> > + */
> > +static int read_updates_counter(struct tmy_io_buffer *head)
> > +{
> > + if (head->read_eof)
> > + return 0;
> > + tmy_io_printf(head,
> > + "/sys/kernel/security/tomoyo/domain_policy: %10u\n"
> > + "/sys/kernel/security/tomoyo/exception_policy: %10u\n"
> > + "/sys/kernel/security/tomoyo/profile: %10u\n"
> > + "/sys/kernel/security/tomoyo/manager: %10u\n",
> > + atomic_xchg(&updates_counter
> > + [TMY_UPDATES_COUNTER_DOMAIN_POLICY], 0),
> > + atomic_xchg(&updates_counter
> > + [TMY_UPDATES_COUNTER_EXCEPTION_POLICY], 0),
> > + atomic_xchg(&updates_counter
> > + [TMY_UPDATES_COUNTER_PROFILE], 0),
> > + atomic_xchg(&updates_counter
> > + [TMY_UPDATES_COUNTER_MANAGER], 0));
> > + head->read_eof = true;
> > + return 0;
> > +}
>
> What is this doing? We print the absolute pathnames of sysfs files via
> another sysfs file?
>
This is an interface to allow GUI management tool to examine policy changes.
But removed because GUI management tool is not ready to support this version.

> > +static int tmy_read_control(struct file *file, char __user *buffer,
> > + const int buffer_len)
> > +{
> > + int len = 0;
> > + struct tmy_io_buffer *head = file->private_data;
> > + char *cp;
> > +
> > + if (!head->read)
> > + return -ENOSYS;
> > + if (!access_ok(VERIFY_WRITE, buffer, buffer_len))
>
> Unneeded - copy_to_user() checks this.
>
Removed.

> > +/**
> > + * tmy_write_control - write() for /sys/kernel/security/tomoyo/ interface.
> > + *
> > + * @file: Pointer to "struct file".
> > + * @buffer: Pointer to buffer to read from.
> > + * @buffer_len: Size of @buffer.
> > + *
> > + * Returns @buffer_len on success, negative value otherwise.
> > + */
> > +static int tmy_write_control(struct file *file, const char __user *buffer,
> > + const int buffer_len)
> > +{
> > + struct tmy_io_buffer *head = file->private_data;
> > + int error = buffer_len;
> > + int avail_len = buffer_len;
> > + char *cp0 = head->write_buf;
> > +
> > + if (!head->write)
> > + return -ENOSYS;
> > + if (!access_ok(VERIFY_READ, buffer, buffer_len))
>
> Unneeded.
>
I know. But to avoid partial copy, I check here too.

> > +/* Common header for holding ACL entries. */
> > +struct acl_info {
> > + struct list1_head list;
> > + /*
> > + * Type of this ACL entry.
> > + *
> > + * MSB is is_deleted flag.
> > + */
> > + u8 type;
> > +} __attribute__((__packed__));
>
> I cannot tell from reading the code why this is packed. Hence a comment
> is needed.
>
Packing "struct acl_info" allows "single_path_acl_record" to embed "u16" and
"struct double_path_acl_record" to embed "u8" without enlarging their structure
size. I added a comment.

> Please use __packed.
Replaced "__attribute__((__packed__))" with "__packed".

> > +/* The kernel's domain. */
> > +extern struct domain_info KERNEL_DOMAIN;
>
> Why upper-case?
>
Replaced "KERNEL_DOMAIN" by "kernel_domain".

> Many of the symbols which this header defines have quite
> generic-sounding names. it would be better if their names were to
> identify the symbols as being part of Tomoyo.
>
Added "tomoyo_" to all symbols.




\
 
 \ /
  Last update: 2008-11-10 11:37    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site