Re: [BUGS] Bug#333854: pg_group file update problems

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(at)amur(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073