NetBSD Problem Report #51038

From www@NetBSD.org  Sat Apr  2 21:28:27 2016
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id D93397A221
	for <gnats-bugs@gnats.NetBSD.org>; Sat,  2 Apr 2016 21:28:27 +0000 (UTC)
Message-Id: <20160402212826.AF0E37A495@mollari.NetBSD.org>
Date: Sat,  2 Apr 2016 21:28:26 +0000 (UTC)
From: er.abhinav.upadhyay@gmail.com
Reply-To: er.abhinav.upadhyay@gmail.com
To: gnats-bugs@NetBSD.org
Subject: makemandb(8) does not check for access permissions to the sqlite database
X-Send-Pr-Version: www-1.0

>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:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Apr 02 21:30:00 +0000 2016
>Closed-Date:    Wed Apr 13 07:53:03 +0000 2016
>Last-Modified:  Wed Apr 13 07:53:03 +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 *);

>Release-Note:

>Audit-Trail:
From: Abhinav Upadhyay <er.abhinav.upadhyay@gmail.com>
To: NetBSD GNATS <gnats-bugs@netbsd.org>
Cc: gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
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> 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

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51038 CVS commit: src/usr.sbin/makemandb
Date: Tue, 12 Apr 2016 21:37:50 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Wed Apr 13 01:37:50 UTC 2016

 Modified Files:
 	src/usr.sbin/makemandb: apropos-utils.c apropos-utils.h

 Log Message:
 PR/51038: Abhinav Upadhyay: check for access permissions to the sqlite database


 To generate a diff of this commit:
 cvs rdiff -u -r1.22 -r1.23 src/usr.sbin/makemandb/apropos-utils.c
 cvs rdiff -u -r1.9 -r1.10 src/usr.sbin/makemandb/apropos-utils.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: wiz@NetBSD.org
State-Changed-When: Wed, 13 Apr 2016 07:53:03 +0000
State-Changed-Why:
Committed by christos, thanks!


>Unformatted:

Home	PR Database Search