Re: Question concerning backport of CVE-2022-2625
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 0x5563cc733f25 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 0x5563cc735b72 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(, true); + recordDependencyOnCurrentExtension(, 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 0dab5ef39b3d9d86e452f6ea60b4f5517d9a, 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
Re: Question concerning backport of CVE-2022-2625
Hi Tom, On Sun, Nov 20, 2022 at 11:43:41AM -0500, Tom Lane wrote: > Roberto =?iso-8859-1?Q?C=2E_S=E1nchez?= 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
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
Re: Request for assistance to backport CVE-2022-1552 fixes to 9.6 and 9.4
Hello pgsql-hackers, Is there anyone willing to review the patches that I prepared? I'd have substatntially more confidence in the patches with a review from an upstream developer who is familiar with the code. Regards, -Roberto On Mon, Jul 04, 2022 at 06:06:58PM -0400, Roberto C. Sánchez wrote: > On Wed, Jun 08, 2022 at 05:31:22PM -0400, Roberto C. Sánchez wrote: > > On Wed, Jun 08, 2022 at 04:15:47PM -0400, Tom Lane wrote: > > > Roberto =?iso-8859-1?Q?C=2E_S=E1nchez?= writes: > > > > I am investigating backporting the fixes for CVE-2022-1552 to 9.6 and > > > > 9.4 as part of Debian LTS and Extended LTS. I am aware that these > > > > releases are no longer supported upstream, but I have made an attempt at > > > > adapting commits ef792f7856dea2576dcd9cab92b2b05fe955696b and > > > > f26d5702857a9c027f84850af48b0eea0f3aa15c from the REL_10_STABLE branch. > > > > I would appreciate a review of the attached patches and any comments on > > > > things that may have been missed and/or adapted improperly. > > > > > > FWIW, I would not recommend being in a huge hurry to back-port those > > > changes, pending the outcome of this discussion: > > > > > > https://www.postgresql.org/message-id/flat/f8a4105f076544c180a87ef0c4822352%40stmuk.bayern.de > > > > > Thanks for the pointer. > > > > > We're going to have to tweak that code somehow, and it's not yet > > > entirely clear how. > > > > > I will monitor the discussion to see what comes of it. > > > Based on the discussion in the other thread, I have made an attempt to > backport commit 88b39e61486a8925a3861d50c306a51eaa1af8d6 to 9.6 and 9.4. > The only significant change that I had to make was to add the full > function signatures for the REVOKE/GRANT in the citext test. > > One question that I had about the change as committed is whether a > REVOKE is needed on s.citext_ne, like so: > > REVOKE ALL ON FUNCTION s.citext_ne FROM PUBLIC; > > Or (for pre-10): > > REVOKE ALL ON FUNCTION s.citext_ne(s.citext, s.citext) FROM PUBLIC; > > I ask because the comment immediately preceding the sequence of REVOKEs > includes the comment "Revoke all conceivably-relevant ACLs within the > extension." I am not especially knowledgable about deep internals, but > that function seems like it would belong in the same group with the > others. > > In any event, would someone be willing to review the attached patches > for correctness? I would like to shortly publish updates to 9.6 and 9.4 > in Debian and a review would be most appreciated. > > Regards, > > -Roberto > > -- > Roberto C. Sánchez > From ef792f7856dea2576dcd9cab92b2b05fe955696b Mon Sep 17 00:00:00 2001 > From: Noah Misch > Date: Mon, 9 May 2022 08:35:08 -0700 > Subject: [PATCH] Make relation-enumerating operations be security-restricted > operations. > > When a feature enumerates relations and runs functions associated with > all found relations, the feature's user shall not need to trust every > user having permission to create objects. BRIN-specific functionality > in autovacuum neglected to account for this, as did pg_amcheck and > CLUSTER. An attacker having permission to create non-temp objects in at > least one schema could execute arbitrary SQL functions under the > identity of the bootstrap superuser. CREATE INDEX (not a > relation-enumerating operation) and REINDEX protected themselves too > late. This change extends to the non-enumerating amcheck interface. > Back-patch to v10 (all supported versions). > > Sergey Shinderuk, reviewed (in earlier versions) by Alexander Lakhin. > Reported by Alexander Lakhin. > > Security: CVE-2022-1552 > --- > src/backend/access/brin/brin.c | 30 - > src/backend/catalog/index.c | 41 +-- > src/backend/commands/cluster.c | 35 > src/backend/commands/indexcmds.c | 53 > +-- > src/backend/utils/init/miscinit.c| 24 -- > src/test/regress/expected/privileges.out | 42 > src/test/regress/sql/privileges.sql | 36 + > 7 files changed, 231 insertions(+), 30 deletions(-) > > --- a/src/backend/access/brin/brin.c > +++ b/src/backend/access/brin/brin.c > @@ -28,6 +28,7 @@ > #include "pgstat.h" > #include "storage/bufmgr.h" > #include "storage/freespace.h" > +#include "utils/guc.h" > #include "utils/index_selfuncs.h" > #include "utils/memutils.h" > #include "u
Re: Request for assistance to backport CVE-2022-1552 fixes to 9.6 and 9.4
On Wed, Jun 08, 2022 at 05:31:22PM -0400, Roberto C. Sánchez wrote:
> On Wed, Jun 08, 2022 at 04:15:47PM -0400, Tom Lane wrote:
> > Roberto =?iso-8859-1?Q?C=2E_S=E1nchez?= writes:
> > > I am investigating backporting the fixes for CVE-2022-1552 to 9.6 and
> > > 9.4 as part of Debian LTS and Extended LTS. I am aware that these
> > > releases are no longer supported upstream, but I have made an attempt at
> > > adapting commits ef792f7856dea2576dcd9cab92b2b05fe955696b and
> > > f26d5702857a9c027f84850af48b0eea0f3aa15c from the REL_10_STABLE branch.
> > > I would appreciate a review of the attached patches and any comments on
> > > things that may have been missed and/or adapted improperly.
> >
> > FWIW, I would not recommend being in a huge hurry to back-port those
> > changes, pending the outcome of this discussion:
> >
> > https://www.postgresql.org/message-id/flat/f8a4105f076544c180a87ef0c4822352%40stmuk.bayern.de
> >
> Thanks for the pointer.
>
> > We're going to have to tweak that code somehow, and it's not yet
> > entirely clear how.
> >
> I will monitor the discussion to see what comes of it.
>
Based on the discussion in the other thread, I have made an attempt to
backport commit 88b39e61486a8925a3861d50c306a51eaa1af8d6 to 9.6 and 9.4.
The only significant change that I had to make was to add the full
function signatures for the REVOKE/GRANT in the citext test.
One question that I had about the change as committed is whether a
REVOKE is needed on s.citext_ne, like so:
REVOKE ALL ON FUNCTION s.citext_ne FROM PUBLIC;
Or (for pre-10):
REVOKE ALL ON FUNCTION s.citext_ne(s.citext, s.citext) FROM PUBLIC;
I ask because the comment immediately preceding the sequence of REVOKEs
includes the comment "Revoke all conceivably-relevant ACLs within the
extension." I am not especially knowledgable about deep internals, but
that function seems like it would belong in the same group with the
others.
In any event, would someone be willing to review the attached patches
for correctness? I would like to shortly publish updates to 9.6 and 9.4
in Debian and a review would be most appreciated.
Regards,
-Roberto
--
Roberto C. Sánchez
>From ef792f7856dea2576dcd9cab92b2b05fe955696b Mon Sep 17 00:00:00 2001
From: Noah Misch
Date: Mon, 9 May 2022 08:35:08 -0700
Subject: [PATCH] Make relation-enumerating operations be security-restricted
operations.
When a feature enumerates relations and runs functions associated with
all found relations, the feature's user shall not need to trust every
user having permission to create objects. BRIN-specific functionality
in autovacuum neglected to account for this, as did pg_amcheck and
CLUSTER. An attacker having permission to create non-temp objects in at
least one schema could execute arbitrary SQL functions under the
identity of the bootstrap superuser. CREATE INDEX (not a
relation-enumerating operation) and REINDEX protected themselves too
late. This change extends to the non-enumerating amcheck interface.
Back-patch to v10 (all supported versions).
Sergey Shinderuk, reviewed (in earlier versions) by Alexander Lakhin.
Reported by Alexander Lakhin.
Security: CVE-2022-1552
---
src/backend/access/brin/brin.c | 30 -
src/backend/catalog/index.c | 41 +--
src/backend/commands/cluster.c | 35
src/backend/commands/indexcmds.c | 53 +--
src/backend/utils/init/miscinit.c| 24 --
src/test/regress/expected/privileges.out | 42
src/test/regress/sql/privileges.sql | 36 +
7 files changed, 231 insertions(+), 30 deletions(-)
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -28,6 +28,7 @@
#include "pgstat.h"
#include "storage/bufmgr.h"
#include "storage/freespace.h"
+#include "utils/guc.h"
#include "utils/index_selfuncs.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -786,6 +787,9 @@
Oid heapoid;
Relation indexRel;
Relation heapRel;
+ Oid save_userid;
+ int save_sec_context;
+ int save_nestlevel;
double numSummarized = 0;
if (RecoveryInProgress())
@@ -799,10 +803,28 @@
* passed indexoid isn't an index then IndexGetRelation() will fail.
* Rather than emitting a not-very-helpful error message, postpone
* complaining, expecting that the is-it-an-index test below will fail.
+ *
+ * unlike brin_summarize_range(), autovacuum never calls this. hence, we
+ * don't switch userid.
*/
heapoid = IndexGetRelation(indexoid, true);
if (OidIsValid(heapoid))
+ {
heapRel = heap_open(heapoid, ShareUpdateExclusiveLock);
+
+ /*
+ * Autovacuum calls us. For its benefit, switch to t
Re: Request for assistance to backport CVE-2022-1552 fixes to 9.6 and 9.4
On Wed, Jun 08, 2022 at 04:15:47PM -0400, Tom Lane wrote: > Roberto =?iso-8859-1?Q?C=2E_S=E1nchez?= writes: > > I am investigating backporting the fixes for CVE-2022-1552 to 9.6 and > > 9.4 as part of Debian LTS and Extended LTS. I am aware that these > > releases are no longer supported upstream, but I have made an attempt at > > adapting commits ef792f7856dea2576dcd9cab92b2b05fe955696b and > > f26d5702857a9c027f84850af48b0eea0f3aa15c from the REL_10_STABLE branch. > > I would appreciate a review of the attached patches and any comments on > > things that may have been missed and/or adapted improperly. > > FWIW, I would not recommend being in a huge hurry to back-port those > changes, pending the outcome of this discussion: > > https://www.postgresql.org/message-id/flat/f8a4105f076544c180a87ef0c4822352%40stmuk.bayern.de > Thanks for the pointer. > We're going to have to tweak that code somehow, and it's not yet > entirely clear how. > I will monitor the discussion to see what comes of it. Regards, -Roberto -- Roberto C. Sánchez
Request for assistance to backport CVE-2022-1552 fixes to 9.6 and 9.4
Hello Devs,
I am investigating backporting the fixes for CVE-2022-1552 to 9.6 and
9.4 as part of Debian LTS and Extended LTS. I am aware that these
releases are no longer supported upstream, but I have made an attempt at
adapting commits ef792f7856dea2576dcd9cab92b2b05fe955696b and
f26d5702857a9c027f84850af48b0eea0f3aa15c from the REL_10_STABLE branch.
I would appreciate a review of the attached patches and any comments on
things that may have been missed and/or adapted improperly.
The first thing I did was to adapt the full patches, with functional
changes and regression tests. Since amcheck was new to version 10, I
dropped that part of the patch. Additionally, since partitioned tables
were new in 10 I dropped those parts of the tests. The absence of block
range indices in 9.4 means I also dropped that part of the change and
associated test as well.
Once everything built successfully, I built again with only the
regression tests to confirm that the vulnerability was presented and
triggerred by the regression test [*].
When building with only the adapted regression tests, the 9.6 build
failed with this in the test output:
+ ERROR: sro_ifun(10) called by pbuilder
+ CONTEXT: PL/pgSQL function sro_ifun(integer) line 4 at ASSERT
This seems to indicate that the vulnerability was encountered and that
the function was called as the invoking user rather than the owning
user. Naturally, there were further differneces in the test output
owing to the index creation failure.
For 9.4, the error looked like this:
+ ERROR: called by pbuilder
As a result of ASSERT not being present in 9.4 I had to resort to an IF
statement with a RAISE. However, it appears to be the identical
problem.
There are 4 patches attached to this mail, one for each of the two
commits referenced above as adapted for 9.6 and 9.4. Please advise on
whether adjustments are needed or whether I can proceed with publishing
updated 9.6 and 9.4 packages for Debian with said patches.
Regards,
-Roberto
[*] Side note: my approach revealed that the adapted regression tests
trigger the vulnerability in both 9.6 and 9.4. However, the SUSE
security information page for CVE-2022-1552 [0] lists 9.6 as "not
affected". Presumably this is based on the language in the upstream
advisory "Versions Affected: 10 - 14."
[0] https://www.suse.com/security/cve/CVE-2022-1552.html
--
Roberto C. Sánchez
>From ef792f7856dea2576dcd9cab92b2b05fe955696b Mon Sep 17 00:00:00 2001
From: Noah Misch
Date: Mon, 9 May 2022 08:35:08 -0700
Subject: [PATCH] Make relation-enumerating operations be security-restricted
operations.
When a feature enumerates relations and runs functions associated with
all found relations, the feature's user shall not need to trust every
user having permission to create objects. BRIN-specific functionality
in autovacuum neglected to account for this, as did pg_amcheck and
CLUSTER. An attacker having permission to create non-temp objects in at
least one schema could execute arbitrary SQL functions under the
identity of the bootstrap superuser. CREATE INDEX (not a
relation-enumerating operation) and REINDEX protected themselves too
late. This change extends to the non-enumerating amcheck interface.
Back-patch to v10 (all supported versions).
Sergey Shinderuk, reviewed (in earlier versions) by Alexander Lakhin.
Reported by Alexander Lakhin.
Security: CVE-2022-1552
---
src/backend/access/brin/brin.c | 30 -
src/backend/catalog/index.c | 41 +--
src/backend/commands/cluster.c | 35
src/backend/commands/indexcmds.c | 53 +--
src/backend/utils/init/miscinit.c| 24 --
src/test/regress/expected/privileges.out | 42
src/test/regress/sql/privileges.sql | 36 +
7 files changed, 231 insertions(+), 30 deletions(-)
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -28,6 +28,7 @@
#include "pgstat.h"
#include "storage/bufmgr.h"
#include "storage/freespace.h"
+#include "utils/guc.h"
#include "utils/index_selfuncs.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -786,6 +787,9 @@
Oid heapoid;
Relation indexRel;
Relation heapRel;
+ Oid save_userid;
+ int save_sec_context;
+ int save_nestlevel;
double numSummarized = 0;
if (RecoveryInProgress())
@@ -799,10 +803,28 @@
* passed indexoid isn't an index then IndexGetRelation() will fail.
* Rather than emitting a not-very-helpful error message, postpone
* complaining, expecting that the is-it-an-index test below will fail.
+ *
+ * unlike brin_summarize_range(), autovacuum never calls this. hence, we
+ * don't switch userid.
*/
heapoid = IndexGetRelation(indexoid, true);
if (OidIsValid(heapoid))
+ {
heapRel = heap_open(he
