Re: bin/51038: makemandb(8) does not check for access permissions to the sqlite database

[Abhinav Upadhyay <er.abhinav.upadhyay%gmail.com@localhost>]

On Sun, Apr 3, 2016 at 3:00 AM,  <er.abhinav.upadhyay%gmail.com@localhost> wrote:
>>Number:         51038
>>Category:       bin
>>Synopsis:       makemandb(8) does not check for access permissions to the sqlite database
>>Confidential:   no
>>Severity:       non-critical
>>Priority:       low
>>Responsible:    bin-bug-people
>>State:          open
>>Class:          sw-bug
>>Submitter-Id:   net
>>Arrival-Date:   Sat Apr 02 21:30:00 +0000 2016
>>Originator:     Abhinav Upadhyay
>>Release:        CURRENT
>>Organization:
>>Environment:
>>Description:
> makemandb(8) when run as a normal user, doesn't have write permissions to the sqlite database. Sqlite API doesn't check for the write permissions when opening the connection, as a result makemandb(8) goes ahead with parsing the man pages and each time it tries to store the parsed data in the database, it gets write permission error.
>
> It would be better, if makemandb(8) just reported an error and exited in such a case.
>
> The attached patch should fix this.
>
> Also, the SQLite documentation says that if the call to sqlite3_open_v2() doesn't return SQLITE_OK, we should still call sqlite3_close. The patch address that issue as well.
>
>
>>How-To-Repeat:
> Try to run makemandb(8) as a normal user who doesn't have write permission to /var/db/man.db.
>>Fix:
> Index: apropos-utils.c
> ===================================================================
> RCS file: /cvsroot/src/usr.sbin/makemandb/apropos-utils.c,v
> retrieving revision 1.22
> diff -u -r1.22 apropos-utils.c
> --- apropos-utils.c     31 Mar 2016 20:16:58 -0000      1.22
> +++ apropos-utils.c     2 Apr 2016 21:27:06 -0000
> @@ -300,18 +300,19 @@
>   *     In normal cases the function should return a handle to the db.
>   */
>  sqlite3 *
> -init_db(int db_flag, const char *manconf)
> +init_db(mandb_access_mode db_flag, const char *manconf)
>  {
>         sqlite3 *db = NULL;
>         sqlite3_stmt *stmt;
>         struct stat sb;
>         int rc;
>         int create_db_flag = 0;
> +       int access_mode;
>
>         char *dbpath = get_dbpath(manconf);
>         if (dbpath == NULL)
>                 errx(EXIT_FAILURE, "_mandb entry not found in man.conf");
> -       /* Check if the database exists or not */
> +
>         if (!(stat(dbpath, &sb) == 0 && S_ISREG(sb.st_mode))) {
>                 /* Database does not exist, check if DB_CREATE was specified, and set
>                  * flag to create the database schema
> @@ -322,16 +323,23 @@
>                         return NULL;
>                 }
>                 create_db_flag = 1;
> +       } else {
> +               /*
> +                * Database exists. Check if we have the permissions to read/write the files
> +                */
> +               access_mode = db_flag == MANDB_CREATE || db_flag == MANDB_WRITE? R_OK | W_OK: R_OK;
> +               if ((access(dbpath, access_mode)) != 0) {
> +                       warnx("Unable to access the database, please check permissions for %s", dbpath);
> +                       return NULL;
> +               }
>         }
>
> -       /* Now initialize the database connection */
>         sqlite3_initialize();
>         rc = sqlite3_open_v2(dbpath, &db, db_flag, NULL);
>
>         if (rc != SQLITE_OK) {
>                 warnx("%s", sqlite3_errmsg(db));
> -               sqlite3_shutdown();
> -               return NULL;
> +               goto error;
>         }
>
>         if (create_db_flag && create_db(db) < 0) {
> @@ -379,8 +387,7 @@
>         return db;
>
>  error:
> -       sqlite3_close(db);
> -       sqlite3_shutdown();
> +       close_db(db);
>         return NULL;
>  }
>
> Index: apropos-utils.h
> ===================================================================
> RCS file: /cvsroot/src/usr.sbin/makemandb/apropos-utils.h,v
> retrieving revision 1.9
> diff -u -r1.9 apropos-utils.h
> --- apropos-utils.h     2 Apr 2013 17:16:50 -0000       1.9
> +++ apropos-utils.h     2 Apr 2016 21:27:06 -0000
> @@ -39,9 +39,12 @@
>  #define SECMAX 9
>
>  /* Flags for opening the database */
> -#define MANDB_READONLY SQLITE_OPEN_READONLY
> -#define MANDB_WRITE SQLITE_OPEN_READWRITE
> -#define MANDB_CREATE SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE
> +typedef enum mandb_access_mode {
> +       MANDB_READONLY = SQLITE_OPEN_READONLY,
> +       MANDB_WRITE = SQLITE_OPEN_READWRITE,
> +       MANDB_CREATE = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE
> +} mandb_access_mode;
> +
>
>  #define APROPOS_SCHEMA_VERSION 20120507
>
> @@ -92,7 +95,7 @@
>  char *lower(char *);
>  void concat(char **, const char *);
>  void concat2(char **, const char *, size_t);
> -sqlite3 *init_db(int, const char *);
> +sqlite3 *init_db(mandb_access_mode, const char *);
>  void close_db(sqlite3 *);
>  char *get_dbpath(const char *);
>  int run_query(sqlite3 *, query_format, query_args *);
>

Hi,

Any comments on this one (and also bin/51040)? I have another patch
that I want to send in but it will include these changes as well
unless this gets committed.

--
Abhinav