lkml.org 
[lkml]   [2009]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectTOMOYO in linux-next
    Hi!

    I don't think merging that is good idea. Security should be doable
    without making shell-like glob matching...

    +/**
    + * tomoyo_file_matches_to_pattern2 - Pattern matching without '/' character
    + * and "\-" pattern.
    + *
    + * @filename: The start of string to check.
    + * @filename_end: The end of string to check.
    + * @pattern: The start of pattern to compare.
    + * @pattern_end: The end of pattern to compare.
    + *
    + * Returns true if @filename matches @pattern, false otherwise.
    + */
    +static bool tomoyo_file_matches_to_pattern2(const char *filename,


    ...or code like...

    +/**
    + * tomoyo_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 tomoyo_path_depth(const char *pathname)

    ...or code like...

    +/* String table for functionality that takes 2 modes. */
    +static const char *tomoyo_mode_2[4] = {
    + "disabled", "enabled", "enabled", "enabled"
    +};

    Are those interfaces documented somewhere?

    This is quite nasty. I don't think turning off enforcement in
    interrupt is good idea. ("fails open").


    +/**
    + * tomoyo_check_flags - Check mode for specified functionality.
    + *
    + * @domain: Pointer to "struct tomoyo_domain_info".
    + * @index: The functionality to check mode.
    + *
    + * TOMOYO checks only process context.
    + * This code disables TOMOYO's enforcement in case the function is
    called from
    + * interrupt context.
    + */
    +unsigned int tomoyo_check_flags(const struct tomoyo_domain_info
    *domain,
    + const u8 index)
    +{
    + const u8 profile = domain->profile;
    +
    + if (WARN_ON(in_interrupt()))
    + return 0;

    Comments leave something to be desired:

    + /***** EXCLUSIVE SECTION START *****/

    +/**
    + * tomoyo_close_control - close() for /sys/kernel/security/tomoyo/
    interface.
    + *
    + * @file: Pointer to "struct file".


    I'm not sure basing security on pids is good idea...

    + /* Don't allow updating policies by non manager programs. */
    + if (head->write != tomoyo_write_pid &&

    Hmm, barrier is spelled otherwise, and I'm not sure I'd trust this:

    +struct tomoyo_path_info_with_data {
    + /* Keep "head" first, for this pointer is passed to tomoyo_free(). */
    + struct tomoyo_path_info head;
    + char bariier1[16]; /* Safeguard for overrun. */

    I guess constants should be used here:

    +#ifdef TOMOYO_DEBUG_DOMAIN_UNDELETE
    + if (domain2->is_deleted != 255)
    + printk(KERN_DEBUG
    + "Marked %p as non undeletable\n",
    + domain2);
    +#endif
    + domain2->is_deleted = 255;

    (I don't know why we want undelete in tomoyo.)

    If it contains copyright, it should contain copyright. It probably
    should not contain version numbers.

    +/*
    + * security/tomoyo/file.c
    + *
    + * Implementation of the Domain-Based Mandatory Access Control.
    + *
    + * Copyright (C) 2005-2009 NTT DATA CORPORATION
    + *
    + * Version: 2.2.0-pre 2009/02/01

    Code is full of tables such as:

    +/* Keyword array for single path operations. */
    +static const char
    *tomoyo_sp_keyword[TOMOYO_MAX_SINGLE_PATH_OPERATION] = {
    + [TOMOYO_TYPE_READ_WRITE_ACL] = "read/write",
    + [TOMOYO_TYPE_EXECUTE_ACL] = "execute",
    + [TOMOYO_TYPE_READ_ACL] = "read",
    + [TOMOYO_TYPE_WRITE_ACL] = "write",
    + [TOMOYO_TYPE_CREATE_ACL] = "create",
    + [TOMOYO_TYPE_UNLINK_ACL] = "unlink",
    + [TOMOYO_TYPE_MKDIR_ACL] = "mkdir",
    + [TOMOYO_TYPE_RMDIR_ACL] = "rmdir",
    + [TOMOYO_TYPE_MKFIFO_ACL] = "mkfifo",
    + [TOMOYO_TYPE_MKSOCK_ACL] = "mksock",
    + [TOMOYO_TYPE_MKBLOCK_ACL] = "mkblock",
    + [TOMOYO_TYPE_MKCHAR_ACL] = "mkchar",
    + [TOMOYO_TYPE_TRUNCATE_ACL] = "truncate",
    + [TOMOYO_TYPE_SYMLINK_ACL] = "symlink",
    + [TOMOYO_TYPE_REWRITE_ACL] = "rewrite",
    +};

    Can we get an interface that does not need as many strings/ as much
    string parsing?

    That's my main complaint: Documentation.*tomoyo nor
    Documentation.*TOMOYO does not exist, still this adds a *lot* of new
    user<->kernel interfaces.

    New user<->kernel interaces should be documented and very carefuly
    reviewed; I don't think that happened here.
    Pavel

    --
    (english) http://www.livejournal.com/~pavelmachek
    (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


    \
     
     \ /
      Last update: 2009-03-26 22:51    [W:0.034 / U:0.292 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site