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

This bug will be fixed in the next 8.0.X release.  I have not fixed
7.4.X because the risk/benefit does not warrant it, and 8.1 does not
have this problem.

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

Bruce Momjian wrote:
>
> 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();
>   }
>
>

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend

--
  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