Thread: Re: [HACKERS] [PATCH] Re: Setuid functions

Re: [HACKERS] [PATCH] Re: Setuid functions

From
Bruce Momjian
Date:
I am backing out this SET AUTHORIZATION patch until we can resolve the
security issues.  It will remain in the patch queue at:

    http://candle.pha.pa.us/cgi-bin/pgpatches



> > I updated the patch to use the SET AUTHORIZATION { INVOKER | DEFINER }
> > terminology. Also, the function owner is now determined and saved at compile
> > time (no gotchas here, right?). It is located at
> >
> > http://volpe.home.mindspring.com/pgsql/set_auth.patch
>
> OK, patch applied.  Can I have some docs with that burger?  :-)
>
> --
>   Bruce Momjian                        |  http://candle.pha.pa.us
>   pgman@candle.pha.pa.us               |  (610) 853-3000
>   +  If your life is a hard drive,     |  830 Blythe Avenue
>   +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

> Index: src/pl/plpgsql/src/gram.y
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/src/pl/plpgsql/src/gram.y,v
> retrieving revision 1.21
> diff -c -r1.21 gram.y
> *** src/pl/plpgsql/src/gram.y    2001/06/06 18:54:41    1.21
> --- src/pl/plpgsql/src/gram.y    2001/07/11 18:37:07
> ***************
> *** 122,132 ****
>   %type <stmts>    proc_sect, proc_stmts, stmt_else, loop_body
>   %type <stmt>    proc_stmt, pl_block
>   %type <stmt>    stmt_assign, stmt_if, stmt_loop, stmt_while, stmt_exit
> ! %type <stmt>    stmt_return, stmt_raise, stmt_execsql, stmt_fori
>   %type <stmt>    stmt_fors, stmt_select, stmt_perform
>   %type <stmt>    stmt_dynexecute, stmt_dynfors, stmt_getdiag
>   %type <stmt>    stmt_open, stmt_fetch, stmt_close
>
>   %type <intlist>    raise_params
>   %type <ival>    raise_level, raise_param
>   %type <str>        raise_msg
> --- 122,134 ----
>   %type <stmts>    proc_sect, proc_stmts, stmt_else, loop_body
>   %type <stmt>    proc_stmt, pl_block
>   %type <stmt>    stmt_assign, stmt_if, stmt_loop, stmt_while, stmt_exit
> ! %type <stmt>    stmt_return, stmt_raise, stmt_execsql, stmt_fori, stmt_setauth
>   %type <stmt>    stmt_fors, stmt_select, stmt_perform
>   %type <stmt>    stmt_dynexecute, stmt_dynfors, stmt_getdiag
>   %type <stmt>    stmt_open, stmt_fetch, stmt_close
>
> + %type <ival>    auth_level
> +
>   %type <intlist>    raise_params
>   %type <ival>    raise_level, raise_param
>   %type <str>        raise_msg
> ***************
> *** 172,177 ****
> --- 174,183 ----
>   %token    K_PERFORM
>   %token    K_ROW_COUNT
>   %token    K_RAISE
> + %token    K_SET
> + %token    K_AUTHORIZATION
> + %token    K_INVOKER
> + %token    K_DEFINER
>   %token    K_RECORD
>   %token    K_RENAME
>   %token    K_RESULT_OID
> ***************
> *** 726,731 ****
> --- 732,739 ----
>                           { $$ = $1; }
>                   | stmt_raise
>                           { $$ = $1; }
> +                 | stmt_setauth
> +                         { $$ = $1; }
>                   | stmt_execsql
>                           { $$ = $1; }
>                   | stmt_dynexecute
> ***************
> *** 1242,1247 ****
> --- 1250,1278 ----
>                           $$ = (PLpgSQL_stmt *)new;
>                       }
>                   ;
> +
> + stmt_setauth        : K_SET K_AUTHORIZATION auth_level lno ';'
> +                 {
> +                     PLpgSQL_stmt_setauth *new;
> +
> +                     new=malloc(sizeof(PLpgSQL_stmt_setauth));
> +
> +                     new->cmd_type = PLPGSQL_STMT_SETAUTH;
> +                     new->auth_level = $3;
> +                                         new->lineno = $4;
> +
> +                     $$ = (PLpgSQL_stmt *)new;
> +                 }
> +
> + auth_level : K_DEFINER
> +         {
> +             $$=PLPGSQL_AUTH_DEFINER;
> +                 }
> +        | K_INVOKER
> +                {
> +                     $$=PLPGSQL_AUTH_INVOKER;
> +                 }
> + ;
>
>   stmt_raise        : K_RAISE lno raise_level raise_msg raise_params ';'
>                       {
> Index: src/pl/plpgsql/src/pl_comp.c
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/src/pl/plpgsql/src/pl_comp.c,v
> retrieving revision 1.31
> diff -c -r1.31 pl_comp.c
> *** src/pl/plpgsql/src/pl_comp.c    2001/05/21 14:22:18    1.31
> --- src/pl/plpgsql/src/pl_comp.c    2001/07/11 18:37:07
> ***************
> *** 169,174 ****
> --- 169,175 ----
>
>       function->fn_functype = functype;
>       function->fn_oid = fn_oid;
> +         function->definer_uid = procStruct->proowner;
>       function->fn_name = strdup(DatumGetCString(DirectFunctionCall1(nameout,
>                                    NameGetDatum(&(procStruct->proname)))));
>
> Index: src/pl/plpgsql/src/pl_exec.c
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
> retrieving revision 1.44
> diff -c -r1.44 pl_exec.c
> *** src/pl/plpgsql/src/pl_exec.c    2001/05/28 19:33:24    1.44
> --- src/pl/plpgsql/src/pl_exec.c    2001/07/11 18:37:07
> ***************
> *** 47,52 ****
> --- 47,53 ----
>   #include "plpgsql.h"
>   #include "pl.tab.h"
>
> + #include "miscadmin.h"
>   #include "access/heapam.h"
>   #include "catalog/pg_proc.h"
>   #include "catalog/pg_type.h"
> ***************
> *** 105,110 ****
> --- 106,113 ----
>                  PLpgSQL_stmt_exit * stmt);
>   static int exec_stmt_return(PLpgSQL_execstate * estate,
>                    PLpgSQL_stmt_return * stmt);
> + static int exec_stmt_setauth(PLpgSQL_execstate * estate,
> +                 PLpgSQL_stmt_setauth * stmt);
>   static int exec_stmt_raise(PLpgSQL_execstate * estate,
>                   PLpgSQL_stmt_raise * stmt);
>   static int exec_stmt_execsql(PLpgSQL_execstate * estate,
> ***************
> *** 226,231 ****
> --- 229,237 ----
>                       case PLPGSQL_STMT_RETURN:
>                           stmttype = "return";
>                           break;
> +                     case PLPGSQL_STMT_SETAUTH:
> +                         stmttype = "setauth";
> +                         break;
>                       case PLPGSQL_STMT_RAISE:
>                           stmttype = "raise";
>                           break;
> ***************
> *** 277,283 ****
>       estate.retistuple = func->fn_retistuple;
>       estate.retisset = func->fn_retset;
>       estate.exitlabel = NULL;
> !
>       estate.found_varno = func->found_varno;
>       estate.ndatums = func->ndatums;
>       estate.datums = palloc(sizeof(PLpgSQL_datum *) * estate.ndatums);
> --- 283,292 ----
>       estate.retistuple = func->fn_retistuple;
>       estate.retisset = func->fn_retset;
>       estate.exitlabel = NULL;
> !     estate.invoker_uid = GetUserId();
> !     estate.definer_uid = func->definer_uid;
> !     estate.auth_level = PLPGSQL_AUTH_INVOKER;
> !
>       estate.found_varno = func->found_varno;
>       estate.ndatums = func->ndatums;
>       estate.datums = palloc(sizeof(PLpgSQL_datum *) * estate.ndatums);
> ***************
> *** 397,402 ****
> --- 406,414 ----
>           elog(ERROR, "control reaches end of function without RETURN");
>       }
>
> +     if (estate.auth_level!=PLPGSQL_AUTH_INVOKER)
> +         SetUserId(estate.invoker_uid);
> +
>       /*
>        * We got a return value - process it
>        */
> ***************
> *** 577,582 ****
> --- 589,597 ----
>       estate.retistuple = func->fn_retistuple;
>       estate.retisset = func->fn_retset;
>       estate.exitlabel = NULL;
> +     estate.invoker_uid = GetUserId();
> +     estate.definer_uid = func->definer_uid;
> +     estate.auth_level = PLPGSQL_AUTH_INVOKER;
>
>       estate.found_varno = func->found_varno;
>       estate.ndatums = func->ndatums;
> ***************
> *** 760,765 ****
> --- 775,783 ----
>           elog(ERROR, "control reaches end of trigger procedure without RETURN");
>       }
>
> +     if (estate.auth_level!=PLPGSQL_AUTH_INVOKER)
> +         SetUserId(estate.invoker_uid);
> +
>       /*
>        * Check that the returned tuple structure has the same attributes,
>        * the relation that fired the trigger has.
> ***************
> *** 1022,1027 ****
> --- 1040,1049 ----
>               rc = exec_stmt_return(estate, (PLpgSQL_stmt_return *) stmt);
>               break;
>
> +         case PLPGSQL_STMT_SETAUTH:
> +             rc = exec_stmt_setauth(estate, (PLpgSQL_stmt_setauth *) stmt);
> +             break;
> +
>           case PLPGSQL_STMT_RAISE:
>               rc = exec_stmt_raise(estate, (PLpgSQL_stmt_raise *) stmt);
>               break;
> ***************
> *** 1643,1648 ****
> --- 1665,1693 ----
>                                       &(estate->rettype));
>
>       return PLPGSQL_RC_RETURN;
> + }
> +
> + /* ----------
> +  * exec_stmt_setauth            Changes user ID to/from
> +  *                              that of the function owner's
> +  * ----------
> +  */
> +
> + static int
> + exec_stmt_setauth(PLpgSQL_execstate * estate, PLpgSQL_stmt_setauth * stmt)
> + {
> +     switch(stmt->auth_level)
> +         {
> +             case PLPGSQL_AUTH_DEFINER:
> +                     SetUserId(estate->definer_uid);
> +                         break;
> +                 case PLPGSQL_AUTH_INVOKER:
> +                     SetUserId(estate->invoker_uid);
> +                         break;
> +     }
> +
> +     estate->auth_level=stmt->auth_level;
> +     return PLPGSQL_RC_OK;
>   }
>
>
> Index: src/pl/plpgsql/src/pl_funcs.c
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/src/pl/plpgsql/src/pl_funcs.c,v
> retrieving revision 1.13
> diff -c -r1.13 pl_funcs.c
> *** src/pl/plpgsql/src/pl_funcs.c    2001/05/21 14:22:19    1.13
> --- src/pl/plpgsql/src/pl_funcs.c    2001/07/11 18:37:08
> ***************
> *** 382,387 ****
> --- 382,388 ----
>   static void dump_select(PLpgSQL_stmt_select * stmt);
>   static void dump_exit(PLpgSQL_stmt_exit * stmt);
>   static void dump_return(PLpgSQL_stmt_return * stmt);
> + static void dump_setauth(PLpgSQL_stmt_setauth * stmt);
>   static void dump_raise(PLpgSQL_stmt_raise * stmt);
>   static void dump_execsql(PLpgSQL_stmt_execsql * stmt);
>   static void dump_dynexecute(PLpgSQL_stmt_dynexecute * stmt);
> ***************
> *** 438,443 ****
> --- 439,447 ----
>           case PLPGSQL_STMT_RETURN:
>               dump_return((PLpgSQL_stmt_return *) stmt);
>               break;
> +         case PLPGSQL_STMT_SETAUTH:
> +             dump_setauth((PLpgSQL_stmt_setauth *) stmt);
> +             break;
>           case PLPGSQL_STMT_RAISE:
>               dump_raise((PLpgSQL_stmt_raise *) stmt);
>               break;
> ***************
> *** 719,724 ****
> --- 723,743 ----
>               dump_expr(stmt->expr);
>       }
>       printf("\n");
> + }
> +
> + static void
> + dump_setauth(PLpgSQL_stmt_setauth * stmt)
> + {
> +     dump_ind();
> +         switch (stmt->auth_level)
> +         {
> +             case PLPGSQL_AUTH_DEFINER:
> +                     printf("SET AUTHORIZATION DEFINER\n");
> +                         break;
> +                 case PLPGSQL_AUTH_INVOKER:
> +                     printf("SET AUTHORIZATION INVOKER\n");
> +                         break;
> +         }
>   }
>
>   static void
> Index: src/pl/plpgsql/src/plpgsql.h
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/src/pl/plpgsql/src/plpgsql.h,v
> retrieving revision 1.14
> diff -c -r1.14 plpgsql.h
> *** src/pl/plpgsql/src/plpgsql.h    2001/05/21 14:22:19    1.14
> --- src/pl/plpgsql/src/plpgsql.h    2001/07/11 18:37:08
> ***************
> *** 95,100 ****
> --- 95,101 ----
>       PLPGSQL_STMT_DYNEXECUTE,
>       PLPGSQL_STMT_DYNFORS,
>       PLPGSQL_STMT_GETDIAG,
> +     PLPGSQL_STMT_SETAUTH,
>       PLPGSQL_STMT_OPEN,
>       PLPGSQL_STMT_FETCH,
>       PLPGSQL_STMT_CLOSE
> ***************
> *** 112,117 ****
> --- 113,128 ----
>       PLPGSQL_RC_RETURN
>   };
>
> + /* ---------
> +  * Authorization levels
> +  * ---------
> +  */
> + enum
> + {
> +     PLPGSQL_AUTH_INVOKER,
> +         PLPGSQL_AUTH_DEFINER,
> + };
> +
>   /* ----------
>    * GET DIAGNOSTICS system attrs
>    * ----------
> ***************
> *** 425,430 ****
> --- 436,447 ----
>       int            retrecno;
>   }            PLpgSQL_stmt_return;
>
> + typedef struct
> + {                               /* SET AUTHORIZATION statement */
> +     int         cmd_type;
> +     int         lineno;
> +     int        auth_level;
> + }           PLpgSQL_stmt_setauth;
>
>   typedef struct
>   {                                /* RAISE statement            */
> ***************
> *** 480,485 ****
> --- 497,503 ----
>       int            tg_nargs_varno;
>
>       int            ndatums;
> +         Oid            definer_uid;
>       PLpgSQL_datum **datums;
>       PLpgSQL_stmt_block *action;
>       struct PLpgSQL_function *next;
> ***************
> *** 502,507 ****
> --- 520,528 ----
>       int            found_varno;
>       int            ndatums;
>       PLpgSQL_datum **datums;
> +     Oid        invoker_uid;
> +     Oid        definer_uid;
> +         int        auth_level;
>   }            PLpgSQL_execstate;
>
>
> Index: src/pl/plpgsql/src/scan.l
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/src/pl/plpgsql/src/scan.l,v
> retrieving revision 1.12
> diff -c -r1.12 scan.l
> *** src/pl/plpgsql/src/scan.l    2001/05/21 14:22:19    1.12
> --- src/pl/plpgsql/src/scan.l    2001/07/11 18:37:08
> ***************
> *** 121,126 ****
> --- 121,130 ----
>   open            { return K_OPEN;            }
>   perform            { return K_PERFORM;            }
>   raise            { return K_RAISE;            }
> + set            { return K_SET;                }
> + authorization        { return K_AUTHORIZATION;        }
> + invoker            { return K_INVOKER;            }
> + definer            { return K_DEFINER;            }
>   record            { return K_RECORD;            }
>   rename            { return K_RENAME;            }
>   result_oid        { return K_RESULT_OID;        }

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Re: [HACKERS] [PATCH] Re: Setuid functions

From
Mark Volpe
Date:
Might as well just get rid of that one; Peter's right about the security hole.

The simplest fix I see is to allow SET AUTHORIZATION only in superuser-owned
functions. It would still be potentially useful that way. Doing this the
"right" way (with users needing regrantable privileges, etc.) would involve
too much effort for too little reward, IMHO.

(Repost - I don't think this made it to the list)

Mark

Bruce Momjian wrote:
>
> I am backing out this SET AUTHORIZATION patch until we can resolve the
> security issues.  It will remain in the patch queue at:
>
>         http://candle.pha.pa.us/cgi-bin/pgpatches
>

Re: [HACKERS] [PATCH] Re: Setuid functions

From
Mark Volpe
Date:
Might as well just get rid of that one; Peter's right about the security hole.

The simplest fix I see is to allow SET AUTHORIZATION only in superuser-owned
functions. It would still be potentially useful that way. Doing this the
"right" way (with users needing regrantable privileges, etc.) would involve
too much effort for too little reward, IMHO.

Mark

Bruce Momjian wrote:
>
> I am backing out this SET AUTHORIZATION patch until we can resolve the
> security issues.  It will remain in the patch queue at:
>
>         http://candle.pha.pa.us/cgi-bin/pgpatches
>