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
    removed, and added trigger-specific security context switch.

  - 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.

6 files changed, 227 insertions, 111 deletions

diff --git a/sql/item_func.cc b/sql/item_func.cc
index 194d62b1183..57d3b0ac5f3 100644
--- a/sql/item_func.cc
+++ b/sql/item_func.cc
@@ -4832,7 +4832,9 @@ Item_func_sp::execute_impl(THD *thd, Field *return_value_fld)
 {
   bool err_status= TRUE;
   Sub_statement_state statement_state;
-  Security_context *save_security_ctx= thd->security_ctx, *save_ctx_func;
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  Security_context *save_security_ctx= thd->security_ctx;
+#endif
 
   DBUG_ENTER("Item_func_sp::execute_impl");
 
@@ -4843,7 +4845,7 @@ Item_func_sp::execute_impl(THD *thd, Field *return_value_fld)
     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 +4857,11 @@ Item_func_sp::execute_impl(THD *thd, Field *return_value_fld)
   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);
 }
 
@@ -4978,70 +4978,38 @@ Item_func_sp::tmp_table_field(TABLE *t_arg)
   SYNOPSIS
     find_and_check_access()
     thd           thread handler
-    want_access   requested access
-    save          backup of security context
 
   RETURN
     FALSE    Access granted
     TRUE     Requested access can't be granted or function doesn't exists
-	     In this case security context is not changed and *save = 0
 
   NOTES
     Checks if requested access to function can be granted to user.
     If function isn't found yet, it searches function first.
     If function can't be found or user don't have requested access
     error is raised.
-    If security context sp_ctx is provided and access can be granted then
-    switch back to previous context isn't performed.
-    In case of access error or if context is not provided then
-    find_and_check_access() switches back to previous security context.
 */
 
 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,
-			   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,
+  if (check_routine_access(thd, EXECUTE_ACL,
 			   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
 Item_func_sp::fix_fields(THD *thd, Item **ref)
 {
@@ -5052,19 +5020,23 @@ Item_func_sp::fix_fields(THD *thd, Item **ref)
   {
     /*
       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.  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);
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+    Security_context *save_secutiry_ctx;
+    if (!res && !(res= set_routine_security_ctx(thd, m_sp, false,
+                                                &save_secutiry_ctx)))
+    {
+      sp_restore_security_context(thd, save_secutiry_ctx);
+    }
+#endif /* ! NO_EMBEDDED_ACCESS_CHECKS */
   }
   return res;
 }
diff --git a/sql/item_func.h b/sql/item_func.h
index 1d8a1bd5e22..f70becb79fc 100644
--- a/sql/item_func.h
+++ b/sql/item_func.h
@@ -1465,8 +1465,7 @@ public:
     { 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);
diff --git a/sql/sp_head.cc b/sql/sp_head.cc
index 9965c48935a..e39efd78d7e 100644
--- a/sql/sp_head.cc
+++ b/sql/sp_head.cc
@@ -1097,6 +1097,7 @@ sp_head::execute(THD *thd)
 
   thd->restore_active_arena(&execute_arena, &backup_arena);
 
+  thd->spcont->pop_all_cursors(); // To avoid memory leaks after an error
 
   /* Restore all saved */
   old_packet.swap(thd->packet);
@@ -1158,6 +1159,161 @@ sp_head::execute(THD *thd)
                m_first_instance->m_first_free_instance->m_recursion_level ==
                m_recursion_level + 1));
   m_first_instance->m_first_free_instance= this;
+
+  DBUG_RETURN(err_status);
+}
+
+
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+/*
+  set_routine_security_ctx() changes routine security context, and
+  checks if there is an EXECUTE privilege in new context.  If there is
+  no EXECUTE privilege, it changes the context back and returns a
+  error.
+
+  SYNOPSIS
+    set_routine_security_ctx()
+      thd         thread handle
+      sp          stored routine to change the context for
+      is_proc     TRUE is procedure, FALSE if function
+      save_ctx    pointer to an old security context
+   
+  RETURN
+    TRUE if there was a error, and the context wasn't changed.
+    FALSE if the context was changed.
+*/
+
+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 // ! NO_EMBEDDED_ACCESS_CHECKS
+
+
+/*
+  Execute a trigger:
+   - changes security context for triggers
+   - switch to new memroot
+   - call sp_head::execute
+   - restore old memroot
+   - restores security context
+
+  SYNOPSIS
+    sp_head::execute_trigger()
+      thd               Thread handle
+      db                database name
+      table             table name
+      grant_info        GRANT_INFO structure to be filled with
+                        information about definer's privileges
+                        on subject table
+   
+  RETURN
+    FALSE  on success
+    TRUE   on error
+*/
+
+bool
+sp_head::execute_trigger(THD *thd, const char *db, const char *table,
+                         GRANT_INFO *grant_info)
+{
+  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));
+
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  Security_context *save_ctx;
+  if (sp_change_security_context(thd, this, &save_ctx))
+    DBUG_RETURN(TRUE);
+
+  /*
+    NOTE: TRIGGER_ACL should be used here.
+  */
+  if (check_global_access(thd, SUPER_ACL))
+  {
+    sp_restore_security_context(thd, save_ctx);
+    DBUG_RETURN(TRUE);
+  }
+
+  /*
+    Fetch information about table-level privileges to GRANT_INFO
+    structure for subject table. Check of privileges that will use it
+    and information about column-level privileges will happen in
+    Item_trigger_field::fix_fields().
+  */
+  fill_effective_table_privileges(thd, grant_info, db, table);
+#endif // NO_EMBEDDED_ACCESS_CHECKS
+
+  /*
+    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))
+  {
+    err_status= TRUE;
+    goto err_with_cleanup;
+  }
+
+#ifndef DBUG_OFF
+  nctx->sp= this;
+#endif
+
+  thd->spcont= nctx;
+
+  err_status= execute(thd);
+
+err_with_cleanup:
+  thd->restore_active_arena(&call_arena, &backup_arena);
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  sp_restore_security_context(thd, save_ctx);
+#endif // NO_EMBEDDED_ACCESS_CHECKS
+  delete nctx;
+  call_arena.free_items();
+  free_root(&call_mem_root, MYF(0));
+  thd->spcont= octx;
+
   DBUG_RETURN(err_status);
 }
 
@@ -1165,8 +1321,12 @@ sp_head::execute(THD *thd)
 /*
   Execute a function:
    - evaluate parameters
+   - changes security context for SUID routines
+   - switch to new memroot
    - call sp_head::execute
+   - restore old memroot
    - evaluate the return value
+   - restores security context
 
   SYNOPSIS
     sp_head::execute_function()
@@ -1293,6 +1453,15 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount,
   }
   thd->spcont= nctx;
 
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  Security_context *save_security_ctx;
+  if (set_routine_security_ctx(thd, this, FALSE, &save_security_ctx))
+  {
+    err_status= TRUE;
+    goto err_with_cleanup;
+  }
+#endif
+
   binlog_save_options= thd->options;
   if (need_binlog_call)
   {
@@ -1333,7 +1502,7 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount,
     reset_dynamic(&thd->user_var_events);
   }
 
-  if (m_type == TYPE_ENUM_FUNCTION && !err_status)
+  if (!err_status)
   {
     /* We need result only in function but not in trigger */
 
@@ -1344,8 +1513,9 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount,
     }
   }
 
-
-  nctx->pop_all_cursors();	// To avoid memory leaks after an error
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  sp_restore_security_context(thd, save_security_ctx);
+#endif
 
 err_with_cleanup:
   delete nctx;
@@ -1368,8 +1538,10 @@ err_with_cleanup:
 
   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
@@ -1490,6 +1662,12 @@ sp_head::execute_procedure(THD *thd, List<Item> *args)
 
   thd->spcont= nctx;
 
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  Security_context *save_security_ctx= 0;
+  if (!err_status)
+    err_status= set_routine_security_ctx(thd, this, TRUE, &save_security_ctx);
+#endif
+
   if (!err_status)
     err_status= execute(thd);
 
@@ -1534,10 +1712,14 @@ sp_head::execute_procedure(THD *thd, List<Item> *args)
     }
   }
 
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  if (save_security_ctx)
+    sp_restore_security_context(thd, save_security_ctx);
+#endif
+
   if (!save_spcont)
     delete octx;
 
-  nctx->pop_all_cursors();	// To avoid memory leaks after an error
   delete nctx;
   thd->spcont= save_spcont;
 
diff --git a/sql/sp_head.h b/sql/sp_head.h
index 073cca2cd12..36747716bdc 100644
--- a/sql/sp_head.h
+++ b/sql/sp_head.h
@@ -207,6 +207,10 @@ public:
   destroy();
 
   bool
+  execute_trigger(THD *thd, const char *db, const char *table,
+                  GRANT_INFO *grant_onfo);
+
+  bool
   execute_function(THD *thd, Item **args, uint argcount, Field *return_fld);
 
   bool
@@ -1149,6 +1153,10 @@ sp_change_security_context(THD *thd, sp_head *sp,
                            Security_context **backup);
 void
 sp_restore_security_context(THD *thd, Security_context *backup);
+
+bool
+set_routine_security_ctx(THD *thd, sp_head *sp, bool is_proc,
+                         Security_context **save_ctx);
 #endif /* NO_EMBEDDED_ACCESS_CHECKS */
 
 TABLE_LIST *
diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
index cd57c280950..9a3b1e1234e 100644
--- a/sql/sql_parse.cc
+++ b/sql/sql_parse.cc
@@ -4383,9 +4383,6 @@ end_with_restore_list:
       }
       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;
@@ -4427,21 +4424,11 @@ end_with_restore_list:
 
 #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;
@@ -4465,9 +4452,6 @@ end_with_restore_list:
 	  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;
diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc
index 28d7dc0bb9d..6effa6e0644 100644
--- a/sql/sql_trigger.cc
+++ b/sql/sql_trigger.cc
@@ -1495,40 +1495,11 @@ bool Table_triggers_list::process_triggers(THD *thd, trg_event_type event,
       old_field= table->field;
     }
 
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-    Security_context *save_ctx;
-
-    if (sp_change_security_context(thd, sp_trigger, &save_ctx))
-      return TRUE;
-
-    /*
-      NOTE: TRIGGER_ACL should be used below.
-    */
-
-    if (check_global_access(thd, SUPER_ACL))
-    {
-      sp_restore_security_context(thd, save_ctx);
-      return TRUE;
-    }
-
-    /*
-      Fetch information about table-level privileges to GRANT_INFO structure for
-      subject table. Check of privileges that will use it and information about
-      column-level privileges will happen in Item_trigger_field::fix_fields().
-    */
-
-    fill_effective_table_privileges(thd,
-                                    &subject_table_grants[event][time_type],
-                                    table->s->db, table->s->table_name);
-#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, table->s->db, table->s->table_name,
+       &subject_table_grants[event][time_type]);
     thd->restore_sub_statement_state(&statement_state);
-
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-    sp_restore_security_context(thd, save_ctx);
-#endif // NO_EMBEDDED_ACCESS_CHECKS
   }
 
   return err_status;