Thread: Question concerning backport of CVE-2022-2625

Greetings PGSQL hackers,

I am working on a backport of CVE-2022-2625 to PostgreSQL 9.6 and 9.4.
I am starting from commit 5919bb5a5989cda232ac3d1f8b9d90f337be2077.

The backport to 9.6 was relatively straightforward, the principal change
being to omit some of the hunks related to commands in 9.6 that did not
have support for 'IF NOT EXISTS'.  When it came to 9.4, things got a
little more interesting.  There were additional instances of commands
that did not have support for 'IF NOT EXISTS' and some of the
contructions were slightly different as well, but nothing insurmountable
there.

I did have to hack at the 9.4 test harness a bit since the
test_extensions sub-directory seems to have been introduced post-9.4 and
it seemed like a good idea to have the actual tests from the
aforementioned commit to help guard against some sort of unintended
change on my part.  However, after I got through the CINE changes and
started dealing with the COR changes I ran into something fairly
peculiar.  The test output included this:

 DROP VIEW ext_cor_view; 
 CREATE TYPE test_ext_type;
 CREATE EXTENSION test_ext_cor;  -- fail
 ERROR:  type test_ext_type is not a member of extension "test_ext_cor"
 DETAIL:  An extension is not allowed to replace an object that it does not own.
 DROP TYPE test_ext_type;
 -- this makes a shell "point <<@@ polygon" operator too
 CREATE OPERATOR @@>> ( PROCEDURE = poly_contain_pt,
   LEFTARG = polygon, RIGHTARG = point,
   COMMUTATOR = <<@@ );
 CREATE EXTENSION test_ext_cor;  -- fail
 ERROR:  operator <<@@(point,polygon) is not a member of extension "test_ext_cor"
 DETAIL:  An extension is not allowed to replace an object that it does not own.
 DROP OPERATOR <<@@ (point, polygon);
 CREATE EXTENSION test_ext_cor;  -- now it should work
+ERROR:  operator 16427 is not a member of extension "test_ext_cor"
+DETAIL:  An extension is not allowed to replace an object that it does not own.
 SELECT ext_cor_func();

This made me suspect that there was an issue with 'DROP OPERATOR'.
After a little scavenger hunt, I located a commit which appears to be
related, c94959d4110a1965472956cfd631082a96f64a84, and which was made
post-9.4.  So then, my question: is the existing behavior that produces
"ERROR:  operator ... is not a member of extension ..." a sufficient
guard against the CVE-2022-2625 vulnerability when it comes to
operators?  (My thought is that it might be sufficient, and if it is I
would need to add something like 'DROP OPERATOR @@>> (point, polygon);'
to allow the extension creation to work and the test to complete.)

If the apparently buggy behavior is not a sufficient guard, then is a
backport of c94959d4110a1965472956cfd631082a96f64a84 in conjunction with
the CVE-2022-2625 fix the correct solution?

Regards,

-Roberto

-- 
Roberto C. Sánchez

Roberto =?iso-8859-1?Q?C=2E_S=E1nchez?= <roberto@debian.org> writes:
>  -- this makes a shell "point <<@@ polygon" operator too
>  CREATE OPERATOR @@>> ( PROCEDURE = poly_contain_pt,
>    LEFTARG = polygon, RIGHTARG = point,
>    COMMUTATOR = <<@@ );
>  CREATE EXTENSION test_ext_cor;  -- fail
>  ERROR:  operator <<@@(point,polygon) is not a member of extension "test_ext_cor"
>  DETAIL:  An extension is not allowed to replace an object that it does not own.
>  DROP OPERATOR <<@@ (point, polygon);
>  CREATE EXTENSION test_ext_cor;  -- now it should work
> +ERROR:  operator 16427 is not a member of extension "test_ext_cor"
> +DETAIL:  An extension is not allowed to replace an object that it does not own.

That is ... odd.  Since 9.4 is long out of support I'm unenthused
about investigating it myself.  (Why is it that people will move heaven
and earth to fix "security" bugs in dead branches, but ignore even
catastrophic data-loss bugs?)  But if you're stuck with pursuing
this exercise, I think you'd better figure out exactly what's
happening.  I agree that it smells like c94959d41 could be related,
but I don't see just how that'd produce this symptom.  Before that
commit, the DROP OPERATOR <<@@ would have left a dangling link
behind in @@>> 's oprcom field, but there doesn't seem to be a
reason why that'd affect the test_ext_cor extension: it will not
be re-using the same operator OID, nor would it have any reason to
touch @@>>, since there's no COMMUTATOR clause in the extension.

It'd likely be a good idea to reproduce this with a gdb breakpoint
set at errfinish, and see exactly what's leading up to the error.

            regards, tom lane

Hi Tom,

On Sun, Nov 20, 2022 at 11:43:41AM -0500, Tom Lane wrote:
> Roberto =?iso-8859-1?Q?C=2E_S=E1nchez?= <roberto@debian.org> writes:
> >  -- this makes a shell "point <<@@ polygon" operator too
> >  CREATE OPERATOR @@>> ( PROCEDURE = poly_contain_pt,
> >    LEFTARG = polygon, RIGHTARG = point,
> >    COMMUTATOR = <<@@ );
> >  CREATE EXTENSION test_ext_cor;  -- fail
> >  ERROR:  operator <<@@(point,polygon) is not a member of extension "test_ext_cor"
> >  DETAIL:  An extension is not allowed to replace an object that it does not own.
> >  DROP OPERATOR <<@@ (point, polygon);
> >  CREATE EXTENSION test_ext_cor;  -- now it should work
> > +ERROR:  operator 16427 is not a member of extension "test_ext_cor"
> > +DETAIL:  An extension is not allowed to replace an object that it does not own.
> 
> That is ... odd.  Since 9.4 is long out of support I'm unenthused
> about investigating it myself.  (Why is it that people will move heaven
> and earth to fix "security" bugs in dead branches, but ignore even
> catastrophic data-loss bugs?)  But if you're stuck with pursuing
> this exercise, I think you'd better figure out exactly what's
> happening.  I agree that it smells like c94959d41 could be related,
> but I don't see just how that'd produce this symptom.  Before that
> commit, the DROP OPERATOR <<@@ would have left a dangling link
> behind in @@>> 's oprcom field, but there doesn't seem to be a
> reason why that'd affect the test_ext_cor extension: it will not
> be re-using the same operator OID, nor would it have any reason to
> touch @@>>, since there's no COMMUTATOR clause in the extension.
> 
I understand your reticence to dive into a branch that is long dead from
your perspective.  That said, I am grateful for the insights you
provided here.

> It'd likely be a good idea to reproduce this with a gdb breakpoint
> set at errfinish, and see exactly what's leading up to the error.
> 
Thanks for this suggestion.  I will see if I am able to isolate the
precise cause of the failure with this.

Regards,

-Roberto

-- 
Roberto C. Sánchez

Hi Tom,

On Sun, Nov 20, 2022 at 11:43:41AM -0500, Tom Lane wrote:
> 
> It'd likely be a good idea to reproduce this with a gdb breakpoint
> set at errfinish, and see exactly what's leading up to the error.
> 
So, I did as you suggested.  The top few frames of the backtrace were:

#0  errfinish (dummy=0)
    at /build/postgresql-9.4-9.4.26/build/../src/backend/utils/error/elog.c:419
#1  0x00005563cc733f25 in recordDependencyOnCurrentExtension (
    object=object@entry=0x7ffcfc649310, isReplace=isReplace@entry=1 '\001')
    at /build/postgresql-9.4-9.4.26/build/../src/backend/catalog/pg_depend.c:184
#2  0x00005563cc735b72 in makeOperatorDependencies (tuple=0x5563cd10aaa8)
    at /build/postgresql-9.4-9.4.26/build/../src/backend/catalog/pg_operator.c:862

The code at pg_depend.c:184 came directly from the CVE-2022-2625 commit,
5919bb5a5989cda232ac3d1f8b9d90f337be2077.  However, when I looked at
pg_operator.c:862 I saw that I had had to omit the following change in
backporting to 9.4:

        /* Dependency on extension */
-       recordDependencyOnCurrentExtension(&myself, true);
+       recordDependencyOnCurrentExtension(&myself, isUpdate);

The reason is that the function makeOperatorDependencies() does not have
the parameter isUpdate in 9.4.  I found that the parameter was
introduced in 0dab5ef39b3d9d86e45bbbb2f6ea60b4f5517d9a, which fixed a
problem with the ALTER OPERATOR command, but which also seems to bring
some structural changes as well and it wasn't clear they would be
particularly beneficial in resolving the issue.

In the end, what I settled on was a minor change to pg_operator.c to add
the isUpdate parameter to the signature of makeOperatorDependencies(),
along with updates to the invocations of makeOperatorDependencies() so
that when it is invoked in OperatorCreate() the parameter is passed in.
After that I was able to include the change I had originally omitted and
all the tests passed as written (with appropriate adjustments for
commands that did not support CINE in 9.4).

Thanks again for the suggestion of where to look for the failure!

Regards,

-Roberto

-- 
Roberto C. Sánchez