From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 97CDF24B3A for ; Wed, 16 May 2018 15:27:29 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 2wB0zsvQY3EX for ; Wed, 16 May 2018 15:27:29 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 4C1F7243D3 for ; Wed, 16 May 2018 15:27:29 -0400 (EDT) Date: Wed, 16 May 2018 22:27:26 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [security 2/2] security: Refactor system space access checks Message-ID: <20180516192726.GB9640@atlas> References: <409d4eb26a35f40309ef25b1fe291f3ec5ddb911.1526474053.git.imarkov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <409d4eb26a35f40309ef25b1fe291f3ec5ddb911.1526474053.git.imarkov@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Ilya Markov Cc: georgy@tarantool.org, tarantool-patches@freelists.org * Ilya Markov [18/05/16 15:39]: > /* > - * XXX: pre 1.7.7 there was no specific 'CREATE' or > - * 'ALTER' ACL, instead, read and write access on universe > - * was used to allow create/alter. > - * For backward compatibility, if a user has read and write > - * access on the universe, grant it CREATE access > - * automatically. > - * The legacy fix does not affect sequences since they > - * were added in 1.7.7 only. > + * If a user has write access on the universe, > + * grant it CREATE, DROP, ALTER access automatically. > */ > - if (is_17_compat_mode && has_access & PRIV_R && has_access & PRIV_W) > - has_access |= PRIV_C | PRIV_A; > + if (has_access & PRIV_W) > + has_access |= PRIV_C | PRIV_A | PRIV_D; > I see no point in such automatic grant other than 1.7.7 compatibility. > - user_access_t access = ((PRIV_U | (user_access_t) priv_type) & > - ~has_access); > + user_access_t access = ((user_access_t) priv_type & ~has_access); > bool is_owner = owner_uid == cr->uid || cr->uid == ADMIN; > /* > * Only the owner of the object or someone who has > - * specific DDL privilege on the object can execute > - * DDL. If a user has no USAGE access and is owner, > - * deny access as well. > + * specific DDL privilege on the object can execute DDL. > */ > - if (access == 0 || (is_owner && !(access & PRIV_U))) > + if (access == 0 || is_owner) I don't understand this change. Why did 'usage' disappear? Even the owner of the object can do nothing if they have no 'usage' access. > access_check_ddl(old_space->def->name, old_space->def->uid, SC_SPACE, > - priv_type, true); > + priv_type, BOX_SPACE_ID); Why did you need these changes? Could you describe in the changeset comment the idea of this patch? > /* > - * Check if a write privilege was given, raise an error if not. > + * Perform checks specific for space. Then perform usual check_ddl. > */ > - access_check_space_xc(old_space, PRIV_W); > + credentials *cr = effective_user(); > + uint32_t has_access = cr->universal_access | > + old_space->access[cr->auth_token].effective; > + if (!(has_access & (PRIV_W | PRIV_D))) { > + access_check_ddl(old_space->def->name, old_space->def->uid, > + SC_SPACE, > + PRIV_D, BOX_SPACE_ID); > + } (stopped the review) This is a huge patch. Any chance of splitting it? -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov