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

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

The following reply was made to PR bin/51038; it has been noted by GNATS.

From: Abhinav Upadhyay <er.abhinav.upadhyay%gmail.com@localhost>
To: NetBSD GNATS <gnats-bugs%netbsd.org@localhost>
Cc: gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
Subject: Re: bin/51038: makemandb(8) does not check for access permissions to
 the sqlite database
Date: Tue, 12 Apr 2016 19:47:00 +0530

 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 wh=
 en 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 datab=
 ase, 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 ad=
 dress that issue as well.
 >
 >
 >>How-To-Repeat:
 > Try to run makemandb(8) as a normal user who doesn't have write permissio=
 n to /var/db/man.db.
 >>Fix:
 > Index: apropos-utils.c
 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
 > 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 =3D NULL;
 >         sqlite3_stmt *stmt;
 >         struct stat sb;
 >         int rc;
 >         int create_db_flag =3D 0;
 > +       int access_mode;
 >
 >         char *dbpath =3D get_dbpath(manconf);
 >         if (dbpath =3D=3D NULL)
 >                 errx(EXIT_FAILURE, "_mandb entry not found in man.conf");
 > -       /* Check if the database exists or not */
 > +
 >         if (!(stat(dbpath, &sb) =3D=3D 0 && S_ISREG(sb.st_mode))) {
 >                 /* Database does not exist, check if DB_CREATE was specif=
 ied, and set
 >                  * flag to create the database schema
 > @@ -322,16 +323,23 @@
 >                         return NULL;
 >                 }
 >                 create_db_flag =3D 1;
 > +       } else {
 > +               /*
 > +                * Database exists. Check if we have the permissions to r=
 ead/write the files
 > +                */
 > +               access_mode =3D db_flag =3D=3D MANDB_CREATE || db_flag =
 =3D=3D MANDB_WRITE? R_OK | W_OK: R_OK;
 > +               if ((access(dbpath, access_mode)) !=3D 0) {
 > +                       warnx("Unable to access the database, please chec=
 k permissions for %s", dbpath);
 > +                       return NULL;
 > +               }
 >         }
 >
 > -       /* Now initialize the database connection */
 >         sqlite3_initialize();
 >         rc =3D sqlite3_open_v2(dbpath, &db, db_flag, NULL);
 >
 >         if (rc !=3D 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
 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
 > 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 =3D SQLITE_OPEN_READONLY,
 > +       MANDB_WRITE =3D SQLITE_OPEN_READWRITE,
 > +       MANDB_CREATE =3D 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