Re: [BUGS] Bug#333854: pg_group file update problems - Mailing list pgsql-patches

I have developed a small patch to 8.0.X that fixes your reported
problem:

    test=> CREATE GROUP g1;
    CREATE GROUP

    test=> CREATE USER u1 IN GROUP g1;
    CREATE USER
    test=> \! cat /u/pg/data/global/pg_group
    "g1"    "u1"

    test=> CREATE USER u2 IN GROUP g1;
    CREATE USER
    test=> \! cat /u/pg/data/global/pg_group
    "g1"    "u1" "u2"

    test=> ALTER USER u2 RENAME TO u3;
    ALTER USER
    test=> \! cat /u/pg/data/global/pg_group
    "g1"    "u1" "u3"

In the patch, notice the old comment that suggests we might need to use
CommandCounterIncrement().

This sesms safe to apply to back branches.

---------------------------------------------------------------------------

Dennis Vshivkov wrote:
> Package: postgresql-8.0
> Version: 8.0.3-13
> Severity: important
> Tags: patch, upstream
>
> Here's the problem:
>
> db=# CREATE GROUP g1;
> CREATE GROUP
> db=# CREATE USER u1 IN GROUP g1;                        (1)
> CREATE USER
>
> # cat /var/lib/postgresql/8.0/main/global/pg_group
> #
>
> The file gets rewritten, but the group `g1' line does not get
> added to the file.  Continue:
>
> db=# CREATE USER u2 IN GROUP g1;                        (2)
> CREATE USER
>
> # cat /var/lib/postgresql/8.0/main/global/pg_group
> "g1"    "u1"
> #
>
> Now the line is there, but it lacks the latest member.  Consider
> this also:
>
> db=# ALTER USER u2 RENAME TO u3;                        (3)
> ALTER USER
>
> # cat /var/lib/postgresql/8.0/main/global/pg_group
> "g1"    "u1" "u2"
> #
>
> The problem is that the code that updates pg_group file resolves
> group membership through the system user catalogue cache.  The
> file update happens shortly before the commit, but the caches
> only see updates after the commit.  Because of this, new users
> or changes in users' names often do not make it to pg_group.
> That leads to mysterious authentication failures subsequently.
> The problem can also have security implications for certain
> pg_hba.conf arrangements.
>
> The attached `98-6-pg_group-stale-data-fix.patch' makes the code
> in question access the system user table directly and thus fixes
> the cases (1) and (2), however (3) is doubly ill: the user
> renaming code does not even trigger a pg_group file update.
> Hence the other patch, `98-5-rename-user-update-pg_group.patch'.
>
> A byproduct of the main fix is removal of an unlikely system
> cache reference leak which happens if a group member name
> contains a newline.
>
> The problems were found and the fixes were done for PostgreSQL
> 8.0.3 release.  The flaws seem intact in 8.0.4 source code, too.
>
> Hope this helps.
>
> --
> /Awesome Walrus <walrus@amur.ru>

[ Attachment, skipping... ]

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
>                http://archives.postgresql.org

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/backend/commands/user.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/user.c,v
retrieving revision 1.147
diff -c -c -r1.147 user.c
*** src/backend/commands/user.c    31 Dec 2004 21:59:42 -0000    1.147
--- src/backend/commands/user.c    14 Oct 2005 15:42:17 -0000
***************
*** 175,183 ****

      /*
       * Read pg_group and write the file.  Note we use SnapshotSelf to
!      * ensure we see all effects of current transaction.  (Perhaps could
!      * do a CommandCounterIncrement beforehand, instead?)
       */
      scan = heap_beginscan(grel, SnapshotSelf, 0, NULL);
      while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
      {
--- 175,183 ----

      /*
       * Read pg_group and write the file.  Note we use SnapshotSelf to
!      * ensure we see all effects of current transaction.
       */
+     CommandCounterIncrement();
      scan = heap_beginscan(grel, SnapshotSelf, 0, NULL);
      while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
      {
***************
*** 781,786 ****
--- 781,787 ----
       * Set flag to update flat password file at commit.
       */
      user_file_update_needed();
+     group_file_update_needed();
  }


***************
*** 1200,1205 ****
--- 1201,1207 ----
       * Set flag to update flat password file at commit.
       */
      user_file_update_needed();
+     group_file_update_needed();
  }


***************
*** 1286,1291 ****
--- 1288,1294 ----
      heap_close(rel, NoLock);

      user_file_update_needed();
+     group_file_update_needed();
  }