Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Gwendal Roué-2
Hello,

This email contains a patch that introduces a new authorizer action code: SQLITE_READ_TABLE.

The goal of this new action code is to fill a hole in the current authorization API, which does not tell about all tables read by a statement. For example, the statement "SELECT COUNT(*) FROM table1" currently invokes the callback twice: SQLITE_SELECT, SQLITE_FUNCTION. Nothing is said about table1.

With the provided patch, we add a third invocation of the callback, with the new code SQLITE_READ_TABLE. Its 3d parameter is "table1", and the schema name is the 5th parameter.

Following the current practice which calls sqlite3AuthCheck() during the parsing phase, I have added a call to sqlite3AuthCheck() in the selectExpander() function, right after the call to sqlite3LocateTableItem().

Basically, for each table used by the select statement, either it is not found by sqlite3LocateTableItem(), either it has to be authorized by the authorization callback.

I'm not familiar with the way code and feature requests are handled within the SQLite community. If you are interested about this patch, let me know how I can help!

Gwendal Roué


$ fossil info
project-name: SQLite
repository:   /Users/groue/Documents/git/sqlite/sqlite.fossil
local-root:   /Users/groue/Documents/git/sqlite/
config-db:    /Users/groue/.fossil
project-code: 2ab58778c2967968b94284e989e43dc11791f548
checkout:     b9a58daca80a815e87e541cb5fff9bc8b93f131d 2017-05-04 11:13:50 UTC
parent:       e24b73820cdca07eee87853fe6dd9f60d76e039e 2017-05-03 19:36:50 UTC
tags:         trunk
comment:      Fix a collision of the "B0" identifier name between the termios.h header file and the SHA3 implementation in the shell. (user: drh)
check-ins:    18701


$ fossil diff
Index: src/select.c
==================================================================
--- src/select.c
+++ src/select.c
@@ -10,10 +10,11 @@
 **
 *************************************************************************
 ** This file contains C code routines that are called by the parser
 ** to handle SELECT statements in SQLite.
 */
+#include <stdio.h>
 #include "sqliteInt.h"
 
 /*
 ** Trace output macros
 */
@@ -4370,10 +4371,14 @@
     }else{
       /* An ordinary table or view name in the FROM clause */
       assert( pFrom->pTab==0 );
       pFrom->pTab = pTab = sqlite3LocateTableItem(pParse, 0, pFrom);
       if( pTab==0 ) return WRC_Abort;
+      int iDb = sqlite3SchemaToIndex(db, pTab->pSchema);
+      if( sqlite3AuthCheck(pParse, SQLITE_READ_TABLE, pTab->zName, 0, db->aDb[iDb].zDbSName) ){
+        return WRC_Abort;
+      }
       if( pTab->nTabRef>=0xffff ){
         sqlite3ErrorMsg(pParse, "too many references to \"%s\": max 65535",
            pTab->zName);
         pFrom->pTab = 0;
         return WRC_Abort;

Index: src/sqlite.h.in
==================================================================
--- src/sqlite.h.in
+++ src/sqlite.h.in
@@ -2824,10 +2824,11 @@
 #define SQLITE_DROP_VTABLE          30   /* Table Name      Module Name     */
 #define SQLITE_FUNCTION             31   /* NULL            Function Name   */
 #define SQLITE_SAVEPOINT            32   /* Operation       Savepoint Name  */
 #define SQLITE_COPY                  0   /* No longer used */
 #define SQLITE_RECURSIVE            33   /* NULL            NULL            */
+#define SQLITE_READ_TABLE           34   /* Table Name      NULL            */
 
 /*
 ** CAPI3REF: Tracing And Profiling Functions
 ** METHOD: sqlite3
 **

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Gwendal Roué-2

> Le 6 mai 2017 à 15:12, Gwendal Roué <[hidden email]> a écrit :
>
> Hello,
>
> This email contains a patch that introduces a new authorizer action code: SQLITE_READ_TABLE.

My patch did not work when the authorizer callback would not return SQLITE_OK.

Please find the fixed patch below:

$ fossil info
project-name: SQLite
repository:   /Users/groue/Documents/git/sqlite/sqlite.fossil
local-root:   /Users/groue/Documents/git/sqlite/
config-db:    /Users/groue/.fossil
project-code: 2ab58778c2967968b94284e989e43dc11791f548
checkout:     b9a58daca80a815e87e541cb5fff9bc8b93f131d 2017-05-04 11:13:50 UTC
parent:       e24b73820cdca07eee87853fe6dd9f60d76e039e 2017-05-03 19:36:50 UTC
tags:         trunk
comment:      Fix a collision of the "B0" identifier name between the termios.h header file and the SHA3 implementation in the shell. (user: drh)
check-ins:    18701

$ fossil diff
Index: src/select.c
==================================================================
--- src/select.c
+++ src/select.c
@@ -4370,10 +4370,15 @@
     }else{
       /* An ordinary table or view name in the FROM clause */
       assert( pFrom->pTab==0 );
       pFrom->pTab = pTab = sqlite3LocateTableItem(pParse, 0, pFrom);
       if( pTab==0 ) return WRC_Abort;
+      int iDb = sqlite3SchemaToIndex(db, pTab->pSchema);
+      if( sqlite3AuthCheck(pParse, SQLITE_READ_TABLE, pTab->zName, 0, db->aDb[iDb].zDbSName) ){
+        pFrom->pTab = 0;
+        return WRC_Abort;
+      }
       if( pTab->nTabRef>=0xffff ){
         sqlite3ErrorMsg(pParse, "too many references to \"%s\": max 65535",
            pTab->zName);
         pFrom->pTab = 0;
         return WRC_Abort;

Index: src/sqlite.h.in
==================================================================
--- src/sqlite.h.in
+++ src/sqlite.h.in
@@ -2824,10 +2824,11 @@
 #define SQLITE_DROP_VTABLE          30   /* Table Name      Module Name     */
 #define SQLITE_FUNCTION             31   /* NULL            Function Name   */
 #define SQLITE_SAVEPOINT            32   /* Operation       Savepoint Name  */
 #define SQLITE_COPY                  0   /* No longer used */
 #define SQLITE_RECURSIVE            33   /* NULL            NULL            */
+#define SQLITE_READ_TABLE           34   /* Table Name      NULL            */
 
 /*
 ** CAPI3REF: Tracing And Profiling Functions
 ** METHOD: sqlite3
 **

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

petern
Gwendal.  Your proposal last month for adding column names to the callback
parameters seemed more sensible.  The first question that comes to mind
when new callback modes are to being proposed is what else would be missing
if the same standard were applied to every possible operation?

My thought.  A cursory read of the relevant code comments (see below)
suggests the author had in mind only precise security control of views and
triggers - the ubiquitous 6th parameter mentioned in the comment.  If
that's the idea, then one presumably denies everything by default and then
handles requests only to a purpose built secure view and trigger layer.

It would be nice to hear from the author about what they actually had in
mind for those who need total iron clad security of every row or aggregate
query of any table.  For example, if SQLITE_READ authorization is not being
tested, why not?  Is it tested later?  Perhaps the architecture of the
authorizer is not self explanatory from the names of the #defines or is
described elsewhere.

/*
** CAPI3REF: Authorizer Action Codes
**
** The [sqlite3_set_authorizer()] interface registers a callback function
** that is invoked to authorize certain SQL statement actions.  The
** second parameter to the callback is an integer code that specifies
** what action is being authorized.  These are the integer action codes that
** the authorizer callback may be passed.
**
** These action code values signify what kind of operation is to be
** authorized.  The 3rd and 4th parameters to the authorization
** callback function will be parameters or NULL depending on which of these
** codes is used as the second parameter.  ^(The 5th parameter to the
** authorizer callback is the name of the database ("main", "temp",
** etc.) if applicable.)^  ^The 6th parameter to the authorizer callback
** is the name of the inner-most trigger or view that is responsible for
** the access attempt or NULL if this access attempt is directly from
** top-level SQL code.
*/
/******************************************* 3rd ************ 4th
***********/
#define SQLITE_CREATE_INDEX          1   /* Index Name      Table Name
*/
#define SQLITE_CREATE_TABLE          2   /* Table Name      NULL
*/
#define SQLITE_CREATE_TEMP_INDEX     3   /* Index Name      Table Name
*/
#define SQLITE_CREATE_TEMP_TABLE     4   /* Table Name      NULL
*/
#define SQLITE_CREATE_TEMP_TRIGGER   5   /* Trigger Name    Table Name
*/
#define SQLITE_CREATE_TEMP_VIEW      6   /* View Name       NULL
*/
#define SQLITE_CREATE_TRIGGER        7   /* Trigger Name    Table Name
*/
#define SQLITE_CREATE_VIEW           8   /* View Name       NULL
*/
#define SQLITE_DELETE                9   /* Table Name      NULL
*/
#define SQLITE_DROP_INDEX           10   /* Index Name      Table Name
*/
#define SQLITE_DROP_TABLE           11   /* Table Name      NULL
*/
#define SQLITE_DROP_TEMP_INDEX      12   /* Index Name      Table Name
*/
#define SQLITE_DROP_TEMP_TABLE      13   /* Table Name      NULL
*/
#define SQLITE_DROP_TEMP_TRIGGER    14   /* Trigger Name    Table Name
*/
#define SQLITE_DROP_TEMP_VIEW       15   /* View Name       NULL
*/
#define SQLITE_DROP_TRIGGER         16   /* Trigger Name    Table Name
*/
#define SQLITE_DROP_VIEW            17   /* View Name       NULL
*/
#define SQLITE_INSERT               18   /* Table Name      NULL
*/
#define SQLITE_PRAGMA               19   /* Pragma Name     1st arg or NULL
*/
#define SQLITE_READ                 20   /* Table Name      Column Name
*/
#define SQLITE_SELECT               21   /* NULL            NULL
*/
#define SQLITE_TRANSACTION          22   /* Operation       NULL
*/
#define SQLITE_UPDATE               23   /* Table Name      Column Name
*/
#define SQLITE_ATTACH               24   /* Filename        NULL
*/
#define SQLITE_DETACH               25   /* Database Name   NULL
*/
#define SQLITE_ALTER_TABLE          26   /* Database Name   Table Name
*/
#define SQLITE_REINDEX              27   /* Index Name      NULL
*/
#define SQLITE_ANALYZE              28   /* Table Name      NULL
*/
#define SQLITE_CREATE_VTABLE        29   /* Table Name      Module Name
*/
#define SQLITE_DROP_VTABLE          30   /* Table Name      Module Name
*/
#define SQLITE_FUNCTION             31   /* NULL            Function Name
*/
#define SQLITE_SAVEPOINT            32   /* Operation       Savepoint Name
*/
#define SQLITE_COPY                  0   /* No longer used */
#define SQLITE_RECURSIVE            33   /* NULL            NULL
*/


On Sat, May 6, 2017 at 6:24 AM, Gwendal Roué <[hidden email]> wrote:

>
> > Le 6 mai 2017 à 15:12, Gwendal Roué <[hidden email]> a écrit :
> >
> > Hello,
> >
> > This email contains a patch that introduces a new authorizer action
> code: SQLITE_READ_TABLE.
>
> My patch did not work when the authorizer callback would not return
> SQLITE_OK.
>
> Please find the fixed patch below:
>
> $ fossil info
> project-name: SQLite
> repository:   /Users/groue/Documents/git/sqlite/sqlite.fossil
> local-root:   /Users/groue/Documents/git/sqlite/
> config-db:    /Users/groue/.fossil
> project-code: 2ab58778c2967968b94284e989e43dc11791f548
> checkout:     b9a58daca80a815e87e541cb5fff9bc8b93f131d 2017-05-04
> 11:13:50 UTC
> parent:       e24b73820cdca07eee87853fe6dd9f60d76e039e 2017-05-03
> 19:36:50 UTC
> tags:         trunk
> comment:      Fix a collision of the "B0" identifier name between the
> termios.h header file and the SHA3 implementation in the shell. (user: drh)
> check-ins:    18701
>
> $ fossil diff
> Index: src/select.c
> ==================================================================
> --- src/select.c
> +++ src/select.c
> @@ -4370,10 +4370,15 @@
>      }else{
>        /* An ordinary table or view name in the FROM clause */
>        assert( pFrom->pTab==0 );
>        pFrom->pTab = pTab = sqlite3LocateTableItem(pParse, 0, pFrom);
>        if( pTab==0 ) return WRC_Abort;
> +      int iDb = sqlite3SchemaToIndex(db, pTab->pSchema);
> +      if( sqlite3AuthCheck(pParse, SQLITE_READ_TABLE, pTab->zName, 0,
> db->aDb[iDb].zDbSName) ){
> +        pFrom->pTab = 0;
> +        return WRC_Abort;
> +      }
>        if( pTab->nTabRef>=0xffff ){
>          sqlite3ErrorMsg(pParse, "too many references to \"%s\": max
> 65535",
>             pTab->zName);
>          pFrom->pTab = 0;
>          return WRC_Abort;
>
> Index: src/sqlite.h.in
> ==================================================================
> --- src/sqlite.h.in
> +++ src/sqlite.h.in
> @@ -2824,10 +2824,11 @@
>  #define SQLITE_DROP_VTABLE          30   /* Table Name      Module Name
>    */
>  #define SQLITE_FUNCTION             31   /* NULL            Function
> Name   */
>  #define SQLITE_SAVEPOINT            32   /* Operation       Savepoint
> Name  */
>  #define SQLITE_COPY                  0   /* No longer used */
>  #define SQLITE_RECURSIVE            33   /* NULL            NULL
>   */
> +#define SQLITE_READ_TABLE           34   /* Table Name      NULL
>   */
>
>  /*
>  ** CAPI3REF: Tracing And Profiling Functions
>  ** METHOD: sqlite3
>  **
>
> _______________________________________________
> sqlite-users mailing list
> [hidden email]
> http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
>
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Gwendal Roué-2
In reply to this post by Gwendal Roué-2
Hello Peter,

It's the generally the responsability of the callback implementor to test or not each authorization, depending on her needs. See https://sqlite.org/c3ref/set_authorizer.html

    -- Allow user to run select statements, and read col1 of t1:
    -- SQLITE_SELECT
    -- SQLITE_READ t1 col1 main
    SELECT col1 FROM t1;

    -- Allow user to run select statements, read col1 of t1, and insert in t2:
    -- SQLITE_INSERT t2 main
    -- SQLITE_SELECT
    -- SQLITE_READ t1 col1 main
    INSERT INTO t2 SELECT col1 FROM t1;

There are also authorization callbacks for functions:

    -- Allow user to run select statements, read col1 of t1, execute count function:
    -- SQLITE_SELECT
    -- SQLITE_FUNCTION max
    -- SQLITE_READ t1 col1 main
    SELECT MAX(col1) FROM t1

But here is why I'm suggesting a new code SQLITE_READ_TABLE:

    -- Allow user to run select statements, and execute count function:
    -- SQLITE_SELECT
    -- SQLITE_FUNCTION count
    SELECT COUNT(*) FROM t1

In the previous query, no one knows that the table t1 is about to be used.

The authorizer callback can not be extended so that it tells everything about a function arguments. That's because a function arguments can be too complex to fit in the callback arguments:

    -- SQLITE_SELECT
    -- SQLITE_FUNCTION count
    SELECT COUNT(*) FROM t1, t2, t3, t4, t5

    -- SQLITE_SELECT
    -- SQLITE_FUNCTION count
    -- SQLITE_READ t1 col1 main
    -- SQLITE_READ t2 col1 main
    -- SQLITE_READ t3 col1 main
    SELECT COUNT(DISTINCT t1.col1 + t2.col1 + t3.col1) FROM t1, t2, t3

With the newly introduced SQLITE_READ_TABLE code, we have instead:

    -- SQLITE_SELECT
    -- SQLITE_READ t1 main
    -- SQLITE_FUNCTION count
    SELECT COUNT(*) FROM t1

And now the client knows that the table t1 is used, and can forbid this access.

Gwendal Roué

> Gwendal.  Your proposal last month for adding column names to the callback parameters seemed more sensible.
>
> The first question that comes to mind when new callback modes are to being proposed is what else would be missing if the same standard were applied to every possible operation?
>
> My thought.  A cursory read of the relevant code comments (see below) suggests the author had in mind only precise security control of views and triggers - the ubiquitous 6th parameter mentioned in the comment.  If that's the idea, then one presumably denies everything by default and then handles requests only to a purpose built secure view and trigger layer.
>
> It would be nice to hear from the author about what they actually had in mind for those who need total iron clad security of every row or aggregate query of any table.  For example, if SQLITE_READ authorization is not being tested, why not?  Is it tested later?  Perhaps the architecture of the authorizer is not self explanatory from the names of the #defines or is described elsewhere.
>
>
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

petern
Gwendal.  I understand all that.  It's also good that you've confirmed how
SQLITE_READ is actually queried by the authorizer callback interface.  I
was wondering about that.  Reading your earlier post, one might get the
impression that the SQLITE_READ authorizer action was not queried by the
engine for aggregate table reads for some reason.  Presumably that would be
a bug.

My question about your solution is illustrated by looking at the existing
defines for orthogonal operations.  Consider how SELECT, INSERT, and UPDATE
are currently defined as below.

#define SQLITE_INSERT               18   /* Table Name      NULL
*/
#define SQLITE_SELECT               21   /* NULL            NULL
*/
#define SQLITE_UPDATE               23   /* Table Name      Column Name
*/

If this interface is logically missing SQLITE_READ_TABLE then shouldn't all
the orthogonal authorizer action codes in that same dimension also be
implemented?  Thus, why not also add authorizer action codes for
SQLITE_WRITE_TABLE, SQLITE_READ_COLUMN, SQLITE_WRITE_COLUMN,
SQLITE_READ_SCHEMA, and SQLITE_WRITE_SCHEMA?  Why only just
SQLITE_READ_TABLE?   If SQLITE_READ_TABLE is missing why aren't the others
also missing?

Who is the author of the Authorizer Action Code source?  Does anyone know?
Does the author have an opinion?  Are these new SQLITE_READ_X and
SQLITE_WRITE_X authorizer codes truly missing from the intended design? If
so, are they on the development road map?  Or, was the presumption that
practical applications would handle access control by denying everything
except operations within an application defined view and trigger layer?
[See idea about the 6th parameter of the callback, view or trigger name, I
mentioned in the previous post.]

Why is this forum so silent on this question?  Usually there are half a
dozen responses on the "correct way" to do it.  This time, crickets.

On Mon, May 8, 2017 at 8:04 AM, Gwendal Roué <[hidden email]> wrote:

> Hello Peter,
>
> It's the generally the responsability of the callback implementor to test
> or not each authorization, depending on her needs. See
> https://sqlite.org/c3ref/set_authorizer.html
>
>     -- Allow user to run select statements, and read col1 of t1:
>     -- SQLITE_SELECT
>     -- SQLITE_READ t1 col1 main
>     SELECT col1 FROM t1;
>
>     -- Allow user to run select statements, read col1 of t1, and insert in
> t2:
>     -- SQLITE_INSERT t2 main
>     -- SQLITE_SELECT
>     -- SQLITE_READ t1 col1 main
>     INSERT INTO t2 SELECT col1 FROM t1;
>
> There are also authorization callbacks for functions:
>
>     -- Allow user to run select statements, read col1 of t1, execute count
> function:
>     -- SQLITE_SELECT
>     -- SQLITE_FUNCTION max
>     -- SQLITE_READ t1 col1 main
>     SELECT MAX(col1) FROM t1
>
> But here is why I'm suggesting a new code SQLITE_READ_TABLE:
>
>     -- Allow user to run select statements, and execute count function:
>     -- SQLITE_SELECT
>     -- SQLITE_FUNCTION count
>     SELECT COUNT(*) FROM t1
>
> In the previous query, no one knows that the table t1 is about to be used.
>
> The authorizer callback can not be extended so that it tells everything
> about a function arguments. That's because a function arguments can be too
> complex to fit in the callback arguments:
>
>     -- SQLITE_SELECT
>     -- SQLITE_FUNCTION count
>     SELECT COUNT(*) FROM t1, t2, t3, t4, t5
>
>     -- SQLITE_SELECT
>     -- SQLITE_FUNCTION count
>     -- SQLITE_READ t1 col1 main
>     -- SQLITE_READ t2 col1 main
>     -- SQLITE_READ t3 col1 main
>     SELECT COUNT(DISTINCT t1.col1 + t2.col1 + t3.col1) FROM t1, t2, t3
>
> With the newly introduced SQLITE_READ_TABLE code, we have instead:
>
>     -- SQLITE_SELECT
>     -- SQLITE_READ t1 main
>     -- SQLITE_FUNCTION count
>     SELECT COUNT(*) FROM t1
>
> And now the client knows that the table t1 is used, and can forbid this
> access.
>
> Gwendal Roué
>
> > Gwendal.  Your proposal last month for adding column names to the
> callback parameters seemed more sensible.
> >
> > The first question that comes to mind when new callback modes are to
> being proposed is what else would be missing if the same standard were
> applied to every possible operation?
> >
> > My thought.  A cursory read of the relevant code comments (see below)
> suggests the author had in mind only precise security control of views and
> triggers - the ubiquitous 6th parameter mentioned in the comment.  If
> that's the idea, then one presumably denies everything by default and then
> handles requests only to a purpose built secure view and trigger layer.
> >
> > It would be nice to hear from the author about what they actually had in
> mind for those who need total iron clad security of every row or aggregate
> query of any table.  For example, if SQLITE_READ authorization is not being
> tested, why not?  Is it tested later?  Perhaps the architecture of the
> authorizer is not self explanatory from the names of the #defines or is
> described elsewhere.
> >
> >
> _______________________________________________
> sqlite-users mailing list
> [hidden email]
> http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
>
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Simon Slavin-3

On 8 May 2017, at 10:11pm, petern <[hidden email]> wrote:

> Who is the author of the Authorizer Action Code source?

Although SQLite is in the public domain, development of it is not typical for an open source project.  Almost everything you download when you download SQLite was written by a development team of three or four people.  Contributions from outside that group are rarely (? ever ?) incorporated into the project as source code supplied.  Instead the development group takes suggestions submitted on this list and sometimes decides to write code to implement them.

Instead, SQLite provides many hooks and callback opportunities, and people are encouraged to write their own extensions and host them themselves.

> Why is this forum so silent on this question?  Usually there are half a
> dozen responses on the "correct way" to do it.  This time, crickets.

Hardly anyone uses the authentication system, so far fewer people know the answers.

Simon.
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Gwendal Roué-2
In reply to this post by petern

> Le 8 mai 2017 à 23:11, petern <[hidden email]> a écrit :
>
> Gwendal.  I understand all that.  It's also good that you've confirmed how
> SQLITE_READ is actually queried by the authorizer callback interface.  I
> was wondering about that.  Reading your earlier post, one might get the
> impression that the SQLITE_READ authorizer action was not queried by the
> engine for aggregate table reads for some reason.  Presumably that would be
> a bug.
>
> My question about your solution is illustrated by looking at the existing
> defines for orthogonal operations.  Consider how SELECT, INSERT, and UPDATE
> are currently defined as below.
>
> #define SQLITE_INSERT               18   /* Table Name      NULL
> */
> #define SQLITE_SELECT               21   /* NULL            NULL
> */
> #define SQLITE_UPDATE               23   /* Table Name      Column Name
> */
>
> If this interface is logically missing SQLITE_READ_TABLE then shouldn't all
> the orthogonal authorizer action codes in that same dimension also be
> implemented?  Thus, why not also add authorizer action codes for
> SQLITE_WRITE_TABLE, SQLITE_READ_COLUMN, SQLITE_WRITE_COLUMN,
> SQLITE_READ_SCHEMA, and SQLITE_WRITE_SCHEMA?  Why only just
> SQLITE_READ_TABLE?   If SQLITE_READ_TABLE is missing why aren't the others
> also missing?

Because they are not missing: existing authorizer callbacks already provide a detailed information for all possible updates. We just miss information about selected tables.

> Why is this forum so silent on this question?  Usually there are half a
> dozen responses on the "correct way" to do it.  This time, crickets.

I did propose a patch as a way to show that my proposal doesn't come from thin air, but can be implemented.

Yes, I wish the core team would give at least an acknowledgement that something could be improved.

Gwendal Roué

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Gwendal Roué-2
In reply to this post by Simon Slavin-3

> Le 9 mai 2017 à 00:21, Simon Slavin <[hidden email]> a écrit :
>
>
> On 8 May 2017, at 10:11pm, petern <[hidden email]> wrote:
>
>> Who is the author of the Authorizer Action Code source?
>
> Although SQLite is in the public domain, development of it is not typical for an open source project.  Almost everything you download when you download SQLite was written by a development team of three or four people.  Contributions from outside that group are rarely (? ever ?) incorporated into the project as source code supplied.  Instead the development group takes suggestions submitted on this list and sometimes decides to write code to implement them.

Thanks for the information: I didn't know that.

> Instead, SQLite provides many hooks and callback opportunities, and people are encouraged to write their own extensions and host them themselves.

Forking SQLite is a very hard path, unfortunately :-)

Gwendal Roué

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Gwendal Roué-2
In reply to this post by Simon Slavin-3

> Le 9 mai 2017 à 00:21, Simon Slavin <[hidden email]> a écrit :
>
> Hardly anyone uses the authentication system, so far fewer people know the answers.

As a reminder, I intend to use the authorisation system in order to tell if a statement has an opportunity to impact on another statement, as a support for a general database observation feature.

Here is the general process:

1. During the compilation of a statement S1, authorizer callbacks tell which tables and columns are read by the statement.
2. During the compilation of any other statement S2, authorizer callbacks tell which tables and columns are written by the statement.
3. If the two sets of tables and columns do not intersect, then S2 can not change the results of S1: exit.
4. Until callbacks registered by sqlite3_commit_hook or sqlite3_rollback_hook [2] are invoked, authoriser callbacks allows the app to follow the savepoint stack. This is important for the next step:
5. During the execution of S2, sqlite3_update_hook [1] tell if S2 actually performs any change. Those changes are remembered until the transaction commits [2], or the eventual current savepoint is rollbacked (see step 4 above).
6. After the transaction has been committed, if S2 has performed changes, then S1 is reputed "dirty", and the application is notified that S1 results may have changed.

The process above is able to provide false positives: for example `UPDATE TABLE t1 SET a = a` will trigger a notification, even though no change did occur.

What is important is that the process above doesn't miss any potential change.

Because of the current authorizer callbacks for queries like `SELECT COUNT(*) FROM t1`, which do not tell anything about t1, the step 3 above has to assume that *any* statement has the opportunity to modify the results of this select. This yields too many false positives.

My suggestion is there to allow a less paranoid observation of statements that use the COUNT function.

It is important to stress that I perfectly know that all those authorizer, update, commit, rollback hooks *can not* help observing a database as soon as there are several writer connections. But they *can* help as soon as there is a single writer connection.

That's exactly why I'm here: the above algorithm is already used by GRDB.swift [3], a database library focused on application development that puts all bets on SQLite. GRDB has a serious and robust concurrency model [4] which supports a single connection to a regular database, or a pool of several connections on a WAL database. Both use a single writer connection. Both would be improved with my suggestion.

Of course, the core team may prefer not implementing my suggested SQLITE_READ_TABLE. But I wish they would react to the above scenario.

Gwendal Roué

[1] https://www.sqlite.org/c3ref/update_hook.html <https://www.sqlite.org/c3ref/update_hook.html>
[2] https://sqlite.org/c3ref/commit_hook.html <https://sqlite.org/c3ref/commit_hook.html>
[3] http://github.com/groue/GRDB.swift <http://github.com/groue/GRDB.swift>
[4] https://github.com/groue/GRDB.swift/blob/master/README.md#concurrency <https://github.com/groue/GRDB.swift/blob/master/README.md#concurrency>


_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Gwendal Roué-2

> Le 9 mai 2017 à 08:23, Gwendal Roué <[hidden email]> a écrit :
>
> As a reminder, I intend to use the authorisation system in order to tell if a statement has an opportunity to impact on another statement, as a support for a general database observation feature.
>
> Here is the general process:
>
> 1. During the compilation of a statement S1, authorizer callbacks tell which tables and columns are read by the statement.
> 2. During the compilation of any other statement S2, authorizer callbacks tell which tables and columns are written by the statement.
> 3. If the two sets of tables and columns do not intersect, then S2 can not change the results of S1: exit.
> 4. Until callbacks registered by sqlite3_commit_hook or sqlite3_rollback_hook [2] are invoked, authoriser callbacks allows the app to follow the savepoint stack. This is important for the next step:
> 5. During the execution of S2, sqlite3_update_hook [1] tell if S2 actually performs any change. Those changes are remembered until the transaction commits [2], or the eventual current savepoint is rollbacked (see step 4 above).
> 6. After the transaction has been committed, if S2 has performed changes, then S1 is reputed "dirty", and the application is notified that S1 results may have changed.

Let me please rewrite the above process, to make it more clear.

Goal: notify that the last commit may have changed the results of statement S1, without any false negative, and a number of false positives that is as low as possible.

Let's use `SELECT col1 FROM t1` as S1.


# LEVEL 1: notify after each commit with sqlite3_commit_hook.

Need for improvement: there are many many false positives. Even an empty transaction `BEGIN; COMMIT;` triggers a notification.


# LEVEL 2: notify only for statements that write in the columns and tables read by S1 (thanks to the authorizer callbacks)

Good: `INSERT INTO t2 ...` and `UPDATE t1 SET col2 = 'foo'` no longer trigger a change notification, because authorizer callbacks prove that they have no impact of S1.

Need for improvement 1: if S1 uses the COUNT function, then all statements may have an impact on it, and we're back to level 1 above (read my previous emails about his trouble that SQLITE_READ_TABLE aims at solving)

Need for improvement 2: a statement that may modify S1 may not modify the database: for example `DELETE FROM t1 WHERE 0` won't delete a single line, but the change notification will be triggered anyway.


# LEVEL 3: notify only for statements that perform database modifications (thanks to the database change callbacks)

Good: `DELETE FROM t1 WHERE 0` no longer triggers a change notification, because it did not update, delete, or insert a single row.

Need for improvement: a database change callback may be invalidated by a rollbacked savepoint.


# LEVEL 4: filter out database changes that are rollbacked by a savepoint (thanks to the authorizer callbacks that allow the sqlite3 client to track the savepoint stack)

Good: `INSERT INTO t1 ...` no longer triggers a change notification if it is rollbacked.


# LEVEL 5: there is no level 5, I think we've gone as far as SQLite can do, and this is already quite tremendous.


Again, the problem is with the COUNT function. By I should stop repeating myself :-)

Thanks for reading,
Gwendal Roué

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Simon Slavin-3
In reply to this post by Gwendal Roué-2

On 9 May 2017, at 7:23am, Gwendal Roué <[hidden email]> wrote:

> As a reminder, I intend to use the authorisation system in order to tell if a statement has an opportunity to impact on another statement, as a support for a general database observation feature.

I’ve read your proposed mechanism.  Providing support for all those callbacks looks like it would considerably slow down SQLite.

How are you going to handle TRIGGERs ?  The authoriser analyses each statement individually.  It will not tell you everything that’s in the TRIGGER until it decides to execute those statements.

I don’t think the authorisation system can be efficiently used in this way.  If you’re analysing statements speculatively you’ll get more information out of EXPLAIN.

Simon.
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Gwendal Roué-2

> Le 9 mai 2017 à 15:02, Simon Slavin <[hidden email]> a écrit :
>
> On 9 May 2017, at 7:23am, Gwendal Roué <[hidden email] <mailto:[hidden email]>> wrote:
>
>> As a reminder, I intend to use the authorisation system in order to tell if a statement has an opportunity to impact on another statement, as a support for a general database observation feature.
>
> I’ve read your proposed mechanism.  Providing support for all those callbacks looks like it would considerably slow down SQLite.

Observation is an opt-in service. The user decides which queries are observed.

I haven't done any benchmark yet. Still, I care about performance, and GRDB is the fastest wrapper in its category: https://github.com/groue/GRDB.swift/wiki/Performance.

Database observation is such an awesome service I could pay a little bit for it, though. If performance hit is too high (yet to be measured), then the user can still send change notifications in another way.

> How are you going to handle TRIGGERs ?  The authoriser analyses each statement individually.  It will not tell you everything that’s in the TRIGGER until it decides to execute those statements.

That's a very good question. I have indeed to think about triggers, and also foreign key cascades.

Unless I'm wrong, cascades can be handled through foreign keys introspection. Triggers, on the other side...

Well, if triggers can not be handled, then the documentation will have to tell it in an explicit way: this does not invalidate the whole idea.

> I don’t think the authorisation system can be efficiently used in this way.  If you’re analysing statements speculatively you’ll get more information out of EXPLAIN.

Thanks for the suggestion. Even if EXPLAIN can give enough information, this does not invalidate the information provided by authorizer callbacks.

Gwendal

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Dominique Devienne
In reply to this post by Simon Slavin-3
On Tue, May 9, 2017 at 3:02 PM, Simon Slavin <[hidden email]> wrote:

> On 9 May 2017, at 7:23am, Gwendal Roué <[hidden email]> wrote:
> > As a reminder, I intend to use the authorisation system in order to tell
> if a statement has an opportunity to impact on another statement, as a
> support for a general database observation feature.
> How are you going to handle TRIGGERs ?  The authoriser analyses each
> statement individually.  It will not tell you everything that’s in the
> TRIGGER until it decides to execute those statements.
>
> Aren't triggers compiled into statements themselves? In that case, if
authorizers are called on the full statement, I don't see why it wouldn't
work.


> I don’t think the authorisation system can be efficiently used in this
> way.  If you’re analysing statements speculatively you’ll get more
> information out of EXPLAIN.


On the contrary, a few months bad I was machine-parsing EXPLAIN, and was
told to use authorizers instead.
So it's probably completely appropriate, and I don't see why it wouldn't be
efficient. --DD
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Gwendal Roué-2
In reply to this post by Gwendal Roué-2

> Le 9 mai 2017 à 15:41, Gwendal Roué <[hidden email]> a écrit :
>
>>> As a reminder, I intend to use the authorisation system in order to tell if a statement has an opportunity to impact on another statement, as a support for a general database observation feature.
>>
>> How are you going to handle TRIGGERs ?  The authoriser analyses each statement individually.  It will not tell you everything that’s in the TRIGGER until it decides to execute those statements.
>
> That's a very good question.

Very good news: foreign keys and triggers are 100% handled by authorizer callbacks, for free :-D

How much a boon is SQLite, frankly ???

Look:

CREATE TABLE table1 ( ... );
CREATE TABLE table2 ( ... );
CREATE TRIGGER trigger1 AFTER INSERT ON table2 BEGIN DELETE FROM table1; END;

INSERT INTO table2 (id) VALUES (NULL);
-- SQLITE_INSERT table2 main
-- SQLITE_DELETE table1 main trigger1 :-)

Gwendal Roué

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Dominique Devienne
On Wed, May 10, 2017 at 1:35 PM, Gwendal Roué <[hidden email]>
wrote:

> > Le 9 mai 2017 à 15:41, Gwendal Roué <[hidden email]> a écrit :
> >> How are you going to handle TRIGGERs ?
> >
> > That's a very good question.
>
> Very good news: foreign keys and triggers are 100% handled by authorizer
> callbacks, for free :-D
>

Thanks for confirming. That's what I thought, from previous discussions on
this list.

We haven't heard from Richard, but I hope we will eventually.

I very much agree with you the authorizer API should be "complete" and
"exhaustive"
in what is truly accessed. A flag could be set to not issue the new calls
you propose, for
backward compatibility purposes (and also perhaps because both FKs and
TRIGGERs
are an implementation "detail" of the schema, that not all clients should
be aware of,
unless explicitly requested by the client, perhaps even checked by the
authorizer itself,
to complete the circle :)). --DD
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Gwendal Roué-2

> Le 10 mai 2017 à 15:06, Dominique Devienne <[hidden email]> a écrit :
>
> On Wed, May 10, 2017 at 1:35 PM, Gwendal Roué <[hidden email]>
> wrote:
>
>>> Le 9 mai 2017 à 15:41, Gwendal Roué <[hidden email]> a écrit :
>>>> How are you going to handle TRIGGERs ?
>>>
>>> That's a very good question.
>>
>> Very good news: foreign keys and triggers are 100% handled by authorizer
>> callbacks, for free :-D
>>
>
> Thanks for confirming. That's what I thought, from previous discussions on
> this list.
>
> We haven't heard from Richard, but I hope we will eventually.

So do I.

Since my real topic is answering the question "were the results of statement S modified by transaction T?", I can imagine how this SQLITE_READ_TABLE proposal looks indirect, and potentially misled.

I tried to explain before that this question requires a lot of careful use of commit/rollback/update hooks on top of authorizer callbacks in order to provide as few false positives as possible.

A better written feature request could remain strictly at the statement level. Here is a documentation for a potential sqlite3_stmt_independent() function, inspired from the documentation of sqlite3_stmt_readonly:

        # Statement Independence Test
       
        int sqlite3_stmt_independent(sqlite3_stmt*, sqlite3_stmt*);
       
        The sqlite3_stmt_independent(S1, S2) returns true (non-zero) if and only if the prepared statement S1 makes no changes to the database table and columns used by the prepared statement S2. Changes that are taken in account are direct changes to the table and columns used by S2, but also indirect changes through foreign keys cascades, and indirect changes through triggers.
       
        For example, given the following SQL statement:
       
                SELECT a, b FROM t1;
       
        The following statements are considered independent:
               
                SELECT * FROM t1;
                INSERT INTO t2 (...);
                UPDATE t1 SET c = c + 1;

        The following statements are considered dependent:
               
                DELETE FROM t1;
                UPDATE t1 SET a = 1;
                INSERT INTO t1 (a, b) VALUES (1, 2);
       
        Note that application-defined SQL functions or virtual tables might change the database indirectly as a side effect. For example, if an application defines a function "eval()" that calls sqlite3_exec(), then the following SQL statement would change the database through side-effects:

                SELECT eval('DELETE FROM t1') FROM t2;

        But because the SELECT statement does not change the database directly, sqlite3_stmt_independent would still return true, regardless of the second parameter.
       

> I very much agree with you the authorizer API should be "complete" and "exhaustive" in what is truly accessed.

That's my current strategy :-)

The core team may prefer the sqlite3_stmt_independent(S1, S2) function above. But I'm not sure: currently authorizer callbacks are called during the parsing phase: SQLite doesn't internally keep any information about the impact of a statement besides its execution plan. An eventual sqlite3_stmt_independent function may be difficult to implement, depending on how obsfuscated is the information it needs after the statement has been compiled. I don't know the inner SQLite guts enough.

With SQLITE_READ_TABLE, we have a way to let the library user gather the needed information, with a simple implementation (the only one I could provide with my current knowledge of the library).

> A flag could be set to not issue the new calls you propose, for backward compatibility purposes (and also perhaps because both FKs and TRIGGERs are an implementation "detail" of the schema, that not all clients should be aware of, unless explicitly requested by the client, perhaps even checked by the authorizer itself, to complete the circle :)). --DD

I don't know if backward compatibility is an issue here: code that did not check for SQLITE_READ_TABLE has no reason to break after SQLITE_READ_TABLE has been introduced.

Gwendal Roué

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Richard Hipp-3
In reply to this post by Dominique Devienne
On 5/10/17, Dominique Devienne <[hidden email]> wrote:
>
> We haven't heard from Richard, but I hope we will eventually.
>

No new authorizer codes will be added, since that would present
compatibility problems for legacy authorizer callbacks.  Instead, the
fix is to invoke the authorizer callback with SQLITE_READ but with a
NULL column name for any table that is referenced but for which no
columns are extracted.

This change is more likely to be compatible with legacy authorizer
callbacks.  In particular, the authorizer callback used by Fossil
(https://www.fossil-scm.org/fossil/artifact/ee53ffbf7?ln=161-221)
continues to work fine, and with the enhanced SQLITE_READ, no prevents
users from creating a report using

     SELECT count(*) FROM user

That returns the number of users, for example.

The fix is implemented by https://www.sqlite.org/src/timeline?c=92ab1f72
--
D. Richard Hipp
[hidden email]
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Gwendal Roué-2

> Le 10 mai 2017 à 18:22, Richard Hipp <[hidden email]> a écrit :
>
> On 5/10/17, Dominique Devienne <[hidden email]> wrote:
>>
>> We haven't heard from Richard, but I hope we will eventually.
>>
>
> No new authorizer codes will be added, since that would present
> compatibility problems for legacy authorizer callbacks.  Instead, the
> fix is to invoke the authorizer callback with SQLITE_READ but with a
> NULL column name for any table that is referenced but for which no
> columns are extracted.
>
> This change is more likely to be compatible with legacy authorizer
> callbacks.  In particular, the authorizer callback used by Fossil
> (https://www.fossil-scm.org/fossil/artifact/ee53ffbf7?ln=161-221)
> continues to work fine, and with the enhanced SQLITE_READ, no prevents
> users from creating a report using
>
>     SELECT count(*) FROM user
>
> That returns the number of users, for example.
>
> The fix is implemented by https://www.sqlite.org/src/timeline?c=92ab1f72 <https://www.sqlite.org/src/timeline?c=92ab1f72>

Thanks a lot, Richard!

I would naively expect SQLITE_READ with a NULL column a bigger threat for backward compatibility than a new authorizer code, because:

1. Existing callbacks that catch SQLITE_READ expect a non-NULL column

2. Existing callbacks are more likely to implement a blacklist, and use the authorizer code in a switch statement that does not do anything in its default case. The reason for favouring blacklisting over whitelisting is that the list of authorizer codes is not documented as closed, and that the documentation shows that the list of authoriser codes has already changed in the past (the no-longer used SQLITE_COPY).

The example below (a callback that denies access to the `token` column of the `users` table) would be harmed by SQLITE_READ with a NULL column because strcmp() isn't supposed to accept NULL input:

int denyTokenAccess(void *pUserData, int code, const char *s1, const char *s2, const char *s3, const char *s4) {
        switch(code) {
        case SQLITE_READ:
                if (strcmp(s1, "users") == 0 && strcmp(s2, "token") == 0) {
                        return SQLITE_DENIED;
                } else {
                        return SQLITE_OK;
                }
        default:
                return SQLITE_OK
        }
}

What do you think?
Gwendal Roué

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Clemens Ladisch
In reply to this post by Gwendal Roué-2
Richard Hipp wrote:
> ** ^When a table is referenced by a [SELECT] but no column values are
> ** extracted from that table (for example in a query like
> ** "SELECT count(*) FROM tab") then the [SQLITE_READ] authorizer callback
> ** is invoked once for that table with a NULL column name.

The documentation fails to mention that in this case, the authorizer
callback does not authorize anything, i.e., that its return value is
ignored.  (Assuming that this the desired behaviour ...)


Regards,
Clemens
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Proposition: introduce a new SQLITE_READ_TABLE Authorizer Action Code

Richard Hipp-3
On 5/11/17, Clemens Ladisch <[hidden email]> wrote:
> Richard Hipp wrote:
>> ** ^When a table is referenced by a [SELECT] but no column values are
>> ** extracted from that table (for example in a query like
>> ** "SELECT count(*) FROM tab") then the [SQLITE_READ] authorizer callback
>> ** is invoked once for that table with a NULL column name.
>
> The documentation fails to mention that in this case, the authorizer
> callback does not authorize anything, i.e., that its return value is
> ignored.  (Assuming that this the desired behaviour ...)

I don't understand what you are saying.  Can you give an example of
the undocumented behavior?
--
D. Richard Hipp
[hidden email]
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
12