Bug #43730 There is no way to add new privileges to sql_acl code
Submitted: 18 Mar 2009 15:12 Modified: 29 May 2009 14:46
Reporter: Chuck Bell Email Updates:
Status: Won't fix Impact on me:
None 
Category:MySQL Server: General Severity:S3 (Non-critical)
Version:6.0.11, 5.0, 5.1 bzr OS:Any
Assigned to: Chuck Bell CPU Architecture:Any
Tags: Security

[18 Mar 2009 15:12] Chuck Bell
Description:
The Backup team needs to add one or possibly two new global ACL items. (See BUG#39580). Unfortunately, the defines for the ACL flags have reached their maximum thus there is no (easy) way to add new ACL items.

The current *_ACL defines in sql_acl.h are:

#define SELECT_ACL	(1L << 0)
#define INSERT_ACL	(1L << 1)
#define UPDATE_ACL	(1L << 2)
#define DELETE_ACL	(1L << 3)

...

#define EXTRA_ACL	(1L << 29)
#define NO_ACCESS	(1L << 30)

<out of space>

Furthermore, loops that traverse the user table are using this shift thereby complicating the issue:

  for (tmp_field= table->field+3, priv = SELECT_ACL;
       *tmp_field && (*tmp_field)->real_type() == MYSQL_TYPE_ENUM &&
	 ((Field_enum*) (*tmp_field))->typelib->count == 2 ;
       tmp_field++, priv <<= 1)
  {
    if (priv & rights)				 // set requested privileges
      (*tmp_field)->store(&what, 1, &my_charset_latin1);
  }

NOTICE: This bug report is needed for BUG#39580 which is tagged as SR60BETA.

How to repeat:
Inspect the code in sql_acl.h/.cc

Suggested fix:
We need to refactor the sql_acl.h/.cc code as well as the code that iterates over the user table and the ACLs in general to allow for more global ACLs to be defined. This is a seemingly complicated task as there are several places in the code where this could go horribly wrong if done improperly.
[18 Mar 2009 15:12] Chuck Bell
It is possible to simply add new defines using the skipped bits like:

#define BACKUP_ACL  3L
#define RESTORE_ACL 5L

...but this doesn't work because the loops that iterate over the user table and the ACLs in general are using the bit shift.
[18 Mar 2009 20:22] Sveta Smirnova
Thank you for the report.

Verified as described.
[20 Mar 2009 12:49] Chuck Bell
I made this a bug because while the design of the ACL code is sound, I think the design is flawed to have such a small limit on the number of ACLs that can be defined and I cannot solve a security issue in backup until this is fixed. See BUG#39580 (which has been escalated). If the runtime team wants to make it a worklog I am certainly fine with that provided I get a solution in a timely manner.
[7 Apr 2009 15:19] Lars Thalmann
The suggested solution is to change to longlong for storing the
defined bits (one bit per privilege) instead of long.
[29 Apr 2009 1:08] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/72970

2698 Chuck Bell	2009-04-28
      BUG#43730 : There is no way to add new privileges to sql_acl code
      
      This patch alters the ACL #defines, methods, and variables that
      store ACL values from ulong to ulonglong.
      modified:
        sql/event_data_objects.cc
        sql/events.cc
        sql/mysql_priv.h
        sql/sql_acl.cc
        sql/sql_acl.h
        sql/sql_class.h
        sql/sql_db.cc
        sql/sql_lex.h
        sql/sql_parse.cc
        sql/sql_prepare.cc
        sql/sql_show.cc
        sql/sql_update.cc
        sql/table.cc
        sql/table.h
[11 May 2009 19:20] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/73785

2809 Chuck Bell	2009-05-11
      BUG#43730 : There is no way to add new privileges to sql_acl code
      
      This patch alters the ACL #defines, methods, and variables that
      store ACL values from ulong to ulonglong.
      modified:
        sql/event_data_objects.cc
        sql/events.cc
        sql/mysql_priv.h
        sql/sql_acl.cc
        sql/sql_acl.h
        sql/sql_class.h
        sql/sql_connect.cc
        sql/sql_db.cc
        sql/sql_lex.h
        sql/sql_parse.cc
        sql/sql_prepare.cc
        sql/sql_show.cc
        sql/sql_update.cc
        sql/table.cc
        sql/table.h
[19 May 2009 18:45] Jørgen Løland
Good to push
[25 May 2009 7:10] Lars Thalmann
Chuck, please check if we can use bits 29, 31 for BUG#39580.

On Sat, May 23, 2009 at 08:19:00PM +0200, Sergei Golubchik wrote:
> although I agree that 32 bits for privileges is too little and we need
> to extend that, I don't see the urgency in doing that now for Bug#39580.
> 
> There are at least two unused bits now (29th and 31st, counting from 0),
> and the need for the 30th (NO_ACCESS) is dubious, we could probably make
> this one free.
> 
> This should be enough for the immediate need of Bug#39580.
[29 May 2009 14:13] Chuck Bell
Alternative solution

Attachment: 43730_alt.diff (application/octet-stream, text), 2.56 KiB.

[29 May 2009 14:46] Chuck Bell
An alternative to changing the data type has been proposed, accepted, and tested. A file has been attached to the bug report with the alternative solution.