MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:kroki Date:June 20 2006 12:11pm
Subject:bk commit into 5.0 tree (kroki:1.2168) BUG#18630
View as plain text  
Below is the list of changes that have just been committed into a local
5.0 repository of tomash. When tomash does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet
  1.2168 06/06/20 16:11:30 kroki@stripped +8 -0
  Bug#18630: Arguments of suid routine calculated in wrong security context.
  
  Routine arguments were evaluated in the security context of the routine
  itself, not in the caller's context.
  
  The bug is fixed the following way:
  
    - Item_func_sp::find_and_check_access() has been split into two functions:
      Item_func_sp::find_and_check_access() itself only finds the
      function and check that the caller have EXECUTE privilege on it.
      New function set_routine_security_ctx() changes security context for
      SUID routines and checks that definer have EXECUTE privilege too.
  
    - new function sp_head::execute_trigger() is called from
      Table_triggers_list::process_triggers() instead of
      sp_head::execute_function(), and is effectively just as the
      sp_head::execute_function() is, with all non-trigger related code
      (i.e. dead code in case of trigger) removed.
  
    - call to Item_func_sp::find_and_check_access() stays outside
      of sp_head::execute_function(), and there is a code in
      sql_parse.cc before the call to sp_head::execute_procedure() that
      checks that the caller have EXECUTE privilege, but both
      sp_head::execute_function() and sp_head::execute_procedure() call
      set_routine_security_ctx() after evaluating their parameters,
      and restore the context after the body is executed.

  sql/sql_trigger.cc
    1.50 06/06/20 16:11:23 kroki@stripped +1 -1
    Call new sp_head::execute_trigger() instead of sp_head::execute_function().

  sql/sql_parse.cc
    1.548 06/06/20 16:11:23 kroki@stripped +1 -17
    Do not change security context before executing the procedure, as it will be
    changed after argument evaluation.

  sql/sp_head.h
    1.85 06/06/20 16:11:23 kroki@stripped +3 -0
    Add declaration for sp_head::execute_trigger().

  sql/sp_head.cc
    1.215 06/06/20 16:11:23 kroki@stripped +130 -0
    Add set_routine_security_ctx() function.
    Add sp_head::execute_trigger() method.
    Change security context in sp_head::execute_function() and
    sp_head::execute_procedure() after argument evaluation.

  sql/item_func.h
    1.141 06/06/20 16:11:23 kroki@stripped +1 -2
    Change prototype for Item_func_sp::find_and_check_access().

  sql/item_func.cc
    1.288 06/06/20 16:11:23 kroki@stripped +18 -49
    Do not change security context before executing the function, as it will be
    changed after argument evaluation.
    Do not change security context in Item_func_sp::find_and_check_access().
    
    Relax check in Item_func_sp::fix_fields(): only check that view definer has
    the privilege to call the function, do not check what privileges routine
    definer have at this moment.

  mysql-test/t/sp-security.test
    1.33 06/06/20 16:11:22 kroki@stripped +66 -1
    Add test case for bug#18630.

  mysql-test/r/sp-security.result
    1.30 06/06/20 16:11:22 kroki@stripped +46 -0
    Add result for bug#18630.

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	kroki
# Host:	moonlight.intranet
# Root:	/home/tomash/src/mysql_ab/mysql-5.0-bug18630

--- 1.287/sql/item_func.cc	2006-05-18 00:51:53 +04:00
+++ 1.288/sql/item_func.cc	2006-06-20 16:11:23 +04:00
@@ -4832,7 +4832,7 @@
 {
   bool err_status= TRUE;
   Sub_statement_state statement_state;
-  Security_context *save_security_ctx= thd->security_ctx, *save_ctx_func;
+  Security_context *save_security_ctx= thd->security_ctx;
 
   DBUG_ENTER("Item_func_sp::execute_impl");
 
@@ -4843,7 +4843,7 @@
     thd->security_ctx= context->security_ctx;
   }
 #endif
-  if (find_and_check_access(thd, EXECUTE_ACL, &save_ctx_func))
+  if (find_and_check_access(thd))
     goto error;
 
   /*
@@ -4855,13 +4855,11 @@
   err_status= m_sp->execute_function(thd, args, arg_count, return_value_fld);
   thd->restore_sub_statement_state(&statement_state);
 
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-  sp_restore_security_context(thd, save_ctx_func);
 error:
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
   thd->security_ctx= save_security_ctx;
-#else
-error:
 #endif
+
   DBUG_RETURN(err_status);
 }
 
@@ -4998,48 +4996,22 @@
 */
 
 bool
-Item_func_sp::find_and_check_access(THD *thd, ulong want_access,
-                                    Security_context **save)
+Item_func_sp::find_and_check_access(THD *thd)
 {
-  bool res= TRUE;
-
-  *save= 0;                                     // Safety if error
   if (! m_sp && ! (m_sp= sp_find_routine(thd, TYPE_ENUM_FUNCTION, m_name,
                                          &thd->sp_func_cache, TRUE)))
   {
     my_error(ER_SP_DOES_NOT_EXIST, MYF(0), "FUNCTION", m_name->m_qname.str);
-    goto error;
+    return TRUE;
   }
 
 #ifndef NO_EMBEDDED_ACCESS_CHECKS
-  if (check_routine_access(thd, want_access,
+  if (check_routine_access(thd, EXECUTE_ACL,
 			   m_sp->m_db.str, m_sp->m_name.str, 0, FALSE))
-    goto error;
-
-  sp_change_security_context(thd, m_sp, save);
-  /*
-    If we changed context to run as another user, we need to check the
-    access right for the new context again as someone may have deleted
-    this person the right to use the procedure
-
-    TODO:
-      Cache if the definer has the right to use the object on the first
-      usage and only reset the cache if someone does a GRANT statement
-      that 'may' affect this.
-  */
-  if (*save &&
-      check_routine_access(thd, want_access,
-			   m_sp->m_db.str, m_sp->m_name.str, 0, FALSE))
-  {
-    sp_restore_security_context(thd, *save);
-    *save= 0;                                   // Safety
-    goto error;
-  }
+    return TRUE;
 #endif
-  res= FALSE;                                   // no error
 
-error:
-  return res;
+  return FALSE;
 }
 
 bool
@@ -5052,19 +5024,16 @@
   {
     /*
       Here we check privileges of the stored routine only during view
-      creation, in order to validate the view. A runtime check is perfomed
-      in Item_func_sp::execute(), and this method is not called during
-      context analysis. We do not need to restore the security context
-      changed in find_and_check_access because all view structures created
-      in CREATE VIEW are not used for execution.  Notice, that during view
-      creation we do not infer into stored routine bodies and do not check
-      privileges of its statements, which would probably be a good idea
-      especially if the view has SQL SECURITY DEFINER and the used stored
-      procedure has SQL SECURITY DEFINER
+      creation, in order to validate the view. A runtime check is
+      perfomed in Item_func_sp::execute(), and this method is not
+      called during context analysis.  We do not check privileges of
+      the routine definer here.  Notice, that during view creation we
+      do not infer into stored routine bodies and do not check
+      privileges of its statements, which would probably be a good
+      idea especially if the view has SQL SECURITY DEFINER and the
+      used stored procedure has SQL SECURITY DEFINER
     */
-    Security_context *save_ctx;
-    if (!(res= find_and_check_access(thd, EXECUTE_ACL, &save_ctx)))
-      sp_restore_security_context(thd, save_ctx);
+    res= find_and_check_access(thd);
   }
   return res;
 }

--- 1.140/sql/item_func.h	2006-05-15 15:28:10 +04:00
+++ 1.141/sql/item_func.h	2006-06-20 16:11:23 +04:00
@@ -1465,8 +1465,7 @@
     { context= (Name_resolution_context *)cntx; return FALSE; }
 
   void fix_length_and_dec();
-  bool find_and_check_access(THD * thd, ulong want_access,
-                             Security_context **backup);
+  bool find_and_check_access(THD * thd);
   virtual enum Functype functype() const { return FUNC_SP; }
 
   bool fix_fields(THD *thd, Item **ref);

--- 1.547/sql/sql_parse.cc	2006-05-30 16:22:06 +04:00
+++ 1.548/sql/sql_parse.cc	2006-06-20 16:11:23 +04:00
@@ -4360,9 +4360,6 @@
       }
       else
       {
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-	Security_context *save_ctx;
-#endif
 	ha_rows select_limit;
         /* bits that should be cleared in thd->server_status */
 	uint bits_to_be_cleared= 0;
@@ -4404,21 +4401,11 @@
 
 #ifndef NO_EMBEDDED_ACCESS_CHECKS
 	if (check_routine_access(thd, EXECUTE_ACL,
-				 sp->m_db.str, sp->m_name.str, TRUE, 0) ||
-          sp_change_security_context(thd, sp, &save_ctx))
+				 sp->m_db.str, sp->m_name.str, TRUE, FALSE))
 	{
 	  thd->net.no_send_ok= nsok;
 	  goto error;
 	}
-	if (save_ctx &&
-            check_routine_access(thd, EXECUTE_ACL,
-                                 sp->m_db.str, sp->m_name.str, TRUE, 0))
-	{
-	  thd->net.no_send_ok= nsok;
-	  sp_restore_security_context(thd, save_ctx);
-	  goto error;
-	}
-
 #endif
 	select_limit= thd->variables.select_limit;
 	thd->variables.select_limit= HA_POS_ERROR;
@@ -4442,9 +4429,6 @@
 	  thd->total_warn_count= 0;
 
 	thd->variables.select_limit= select_limit;
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-	sp_restore_security_context(thd, save_ctx);
-#endif
 
 	thd->net.no_send_ok= nsok;
         thd->server_status&= ~bits_to_be_cleared;

--- 1.49/sql/sql_trigger.cc	2006-03-28 01:01:48 +04:00
+++ 1.50/sql/sql_trigger.cc	2006-06-20 16:11:23 +04:00
@@ -1515,7 +1515,7 @@
 #endif // NO_EMBEDDED_ACCESS_CHECKS
 
     thd->reset_sub_statement_state(&statement_state, SUB_STMT_TRIGGER);
-    err_status= sp_trigger->execute_function(thd, 0, 0, 0);
+    err_status= sp_trigger->execute_trigger(thd);
     thd->restore_sub_statement_state(&statement_state);
 
 #ifndef NO_EMBEDDED_ACCESS_CHECKS

--- 1.29/mysql-test/r/sp-security.result	2006-04-18 20:10:42 +04:00
+++ 1.30/mysql-test/r/sp-security.result	2006-06-20 16:11:22 +04:00
@@ -420,3 +420,49 @@
 ---> connection: root
 DROP USER mysqltest_2@localhost;
 DROP DATABASE mysqltest;
+DROP TABLE IF EXISTS t1;
+DROP VIEW IF EXISTS v1;
+DROP FUNCTION IF EXISTS f_suid;
+DROP PROCEDURE IF EXISTS p_suid;
+DROP FUNCTION IF EXISTS f_evil;
+CREATE TABLE t1 (i INT);
+CREATE FUNCTION f_suid(i INT) RETURNS INT RETURN 0;
+CREATE PROCEDURE p_suid(IN i INT) SET @c:= 0;
+CREATE USER testuser1@localhost;
+GRANT EXECUTE ON test.* TO testuser1@localhost;
+CREATE DEFINER=testuser1@localhost FUNCTION f_evil () RETURNS INT
+SQL SECURITY INVOKER
+BEGIN
+SET @a:= CURRENT_USER();
+SET @b:= (SELECT COUNT(*) FROM t1);
+RETURN @b;
+END|
+CREATE SQL SECURITY INVOKER VIEW v1 AS SELECT f_evil();
+SELECT COUNT(*) FROM t1;
+ERROR 42000: SELECT command denied to user 'testuser1'@'localhost' for table 't1'
+SELECT f_evil();
+ERROR 42000: SELECT command denied to user 'testuser1'@'localhost' for table 't1'
+SELECT @a, @b;
+@a	@b
+testuser1@localhost	NULL
+SELECT f_suid(f_evil());
+ERROR 42000: SELECT command denied to user 'testuser1'@'localhost' for table 't1'
+SELECT @a, @b;
+@a	@b
+testuser1@localhost	NULL
+CALL p_suid(f_evil());
+ERROR 42000: SELECT command denied to user 'testuser1'@'localhost' for table 't1'
+SELECT @a, @b;
+@a	@b
+testuser1@localhost	NULL
+SELECT * FROM v1;
+ERROR 42000: SELECT command denied to user 'testuser1'@'localhost' for table 'v1'
+SELECT @a, @b;
+@a	@b
+testuser1@localhost	NULL
+DROP VIEW v1;
+DROP FUNCTION f_evil;
+DROP USER testuser1@localhost;
+DROP PROCEDURE p_suid;
+DROP FUNCTION f_suid;
+DROP TABLE t1;

--- 1.32/mysql-test/t/sp-security.test	2006-04-18 20:10:42 +04:00
+++ 1.33/mysql-test/t/sp-security.test	2006-06-20 16:11:22 +04:00
@@ -743,5 +743,70 @@
 
 DROP DATABASE mysqltest;
 
+--disconnect con1root
+--connection default
 
-# End of 5.0 bugs.
+
+#
+# BUG#18630: Arguments of suid routine calculated in wrong security
+# context
+#
+--disable_warnings
+DROP TABLE IF EXISTS t1;
+DROP VIEW IF EXISTS v1;
+DROP FUNCTION IF EXISTS f_suid;
+DROP PROCEDURE IF EXISTS p_suid;
+DROP FUNCTION IF EXISTS f_evil;
+--enable_warnings
+
+CREATE TABLE t1 (i INT);
+CREATE FUNCTION f_suid(i INT) RETURNS INT RETURN 0;
+CREATE PROCEDURE p_suid(IN i INT) SET @c:= 0;
+
+CREATE USER testuser1@localhost;
+GRANT EXECUTE ON test.* TO testuser1@localhost;
+
+delimiter |;
+CREATE DEFINER=testuser1@localhost FUNCTION f_evil () RETURNS INT
+  SQL SECURITY INVOKER
+BEGIN
+  SET @a:= CURRENT_USER();
+  SET @b:= (SELECT COUNT(*) FROM t1);
+  RETURN @b;
+END|
+delimiter ;|
+
+CREATE SQL SECURITY INVOKER VIEW v1 AS SELECT f_evil();
+
+connect (conn1, localhost, testuser1,,);
+
+--error ER_TABLEACCESS_DENIED_ERROR
+SELECT COUNT(*) FROM t1;
+
+--error ER_TABLEACCESS_DENIED_ERROR
+SELECT f_evil();
+SELECT @a, @b;
+
+--error ER_TABLEACCESS_DENIED_ERROR
+SELECT f_suid(f_evil());
+SELECT @a, @b;
+
+--error ER_TABLEACCESS_DENIED_ERROR
+CALL p_suid(f_evil());
+SELECT @a, @b;
+
+--error ER_TABLEACCESS_DENIED_ERROR
+SELECT * FROM v1;
+SELECT @a, @b;
+
+disconnect conn1;
+connection default;
+
+DROP VIEW v1;
+DROP FUNCTION f_evil;
+DROP USER testuser1@localhost;
+DROP PROCEDURE p_suid;
+DROP FUNCTION f_suid;
+DROP TABLE t1;
+
+# End of 5.0 tests.

--- 1.214/sql/sp_head.cc	2006-05-23 23:00:59 +04:00
+++ 1.215/sql/sp_head.cc	2006-06-20 16:11:23 +04:00
@@ -1189,11 +1189,118 @@
 }
 
 
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+static
+bool
+set_routine_security_ctx(THD *thd, sp_head *sp, bool is_proc,
+                         Security_context **save_ctx)
+{
+  *save_ctx= 0;
+  if (sp_change_security_context(thd, sp, save_ctx))
+    return TRUE;
+
+  /*
+    If we changed context to run as another user, we need to check the
+    access right for the new context again as someone may have revoked
+    the right to use the procedure from this user.
+
+    TODO:
+      Cache if the definer has the right to use the object on the
+      first usage and only reset the cache if someone does a GRANT
+      statement that 'may' affect this.
+  */
+  if (*save_ctx &&
+      check_routine_access(thd, EXECUTE_ACL,
+                           sp->m_db.str, sp->m_name.str, is_proc, FALSE))
+  {
+    sp_restore_security_context(thd, *save_ctx);
+    *save_ctx= 0;
+    return TRUE;
+  }
+
+  return FALSE;
+}
+#endif
+
+
+/*
+  Execute a trigger:
+   - call sp_head::execute
+
+  SYNOPSIS
+    sp_head::execute_trigger()
+      thd               Thread handle
+   
+  RETURN
+    FALSE  on success
+    TRUE   on error
+*/
+
+bool
+sp_head::execute_trigger(THD *thd)
+{
+  sp_rcontext *octx = thd->spcont;
+  sp_rcontext *nctx = NULL;
+  bool err_status= FALSE;
+  MEM_ROOT call_mem_root;
+  Query_arena call_arena(&call_mem_root, Query_arena::INITIALIZED_FOR_SP);
+  Query_arena backup_arena;
+
+  DBUG_ENTER("sp_head::execute_trigger");
+  DBUG_PRINT("info", ("trigger %s", m_name.str));
+
+  /*
+    Prepare arena and memroot for objects which lifetime is whole
+    duration of trigger call (sp_rcontext, it's tables and items,
+    sp_cursor and Item_cache holders for case expressions).  We can't
+    use caller's arena/memroot for those objects because in this case
+    some fixed amount of memory will be consumed for each trigger
+    invocation and so statements which involve lot of them will hog
+    memory.
+
+    TODO: we should create sp_rcontext once per command and reuse it
+    on subsequent executions of a trigger.
+  */
+  init_sql_alloc(&call_mem_root, MEM_ROOT_BLOCK_SIZE, 0);
+  thd->set_n_backup_active_arena(&call_arena, &backup_arena);
+
+  if (!(nctx= new sp_rcontext(m_pcont, 0, octx)) ||
+      nctx->init(thd))
+  {
+    thd->restore_active_arena(&call_arena, &backup_arena);
+    err_status= TRUE;
+    goto err_with_cleanup;
+  }
+
+#ifndef DBUG_OFF
+  nctx->sp= this;
+#endif
+
+  thd->spcont= nctx;
+
+  err_status= execute(thd);
+
+  thd->restore_active_arena(&call_arena, &backup_arena);
+
+  nctx->pop_all_cursors();	// To avoid memory leaks after an error
+
+err_with_cleanup:
+  delete nctx;
+  call_arena.free_items();
+  free_root(&call_mem_root, MYF(0));
+  thd->spcont= octx;
+
+  DBUG_RETURN(err_status);
+}
+
+
 /*
   Execute a function:
    - evaluate parameters
+   - changes security context for SUID routines
    - call sp_head::execute
    - evaluate the return value
+   - restores security context
 
   SYNOPSIS
     sp_head::execute_function()
@@ -1320,6 +1427,15 @@
   }
   thd->spcont= nctx;
 
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  Security_context *save_ctx;
+  if (set_routine_security_ctx(thd, this, FALSE, &save_ctx))
+  {
+    err_status= TRUE;
+    goto err_with_cleanup;
+  }
+#endif
+
   binlog_save_options= thd->options;
   if (need_binlog_call)
   {
@@ -1371,6 +1487,9 @@
     }
   }
 
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  sp_restore_security_context(thd, save_ctx);
+#endif
 
   nctx->pop_all_cursors();	// To avoid memory leaks after an error
 
@@ -1395,8 +1514,10 @@
 
   The function does the following steps:
    - Set all parameters 
+   - changes security context for SUID routines
    - call sp_head::execute
    - copy back values of INOUT and OUT parameters
+   - restores security context
 
   RETURN
     FALSE  on success
@@ -1517,6 +1638,11 @@
 
   thd->spcont= nctx;
 
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  Security_context *save_ctx;
+  err_status= set_routine_security_ctx(thd, this, TRUE, &save_ctx);
+#endif
+
   if (!err_status)
     err_status= execute(thd);
 
@@ -1560,6 +1686,10 @@
       }
     }
   }
+
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  sp_restore_security_context(thd, save_ctx);
+#endif
 
   if (!save_spcont)
     delete octx;

--- 1.84/sql/sp_head.h	2006-05-15 14:01:52 +04:00
+++ 1.85/sql/sp_head.h	2006-06-20 16:11:23 +04:00
@@ -216,6 +216,9 @@
   destroy();
 
   bool
+  execute_trigger(THD *thd);
+
+  bool
   execute_function(THD *thd, Item **args, uint argcount, Field *return_fld);
 
   bool
Thread
bk commit into 5.0 tree (kroki:1.2168) BUG#18630kroki20 Jun