pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedAdding a range check on the sequence index from the publisher.
11+ messages / 3 participants
[nested] [flat]
* Adding a range check on the sequence index from the publisher.
@ 2026-09-21 19:51 Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 2 replies; 11+ messages in thread
From: Masahiko Sawada @ 2026-09-21 19:51 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Amit Kapila <amit.kapila16@gmail.com>
Hi all,
(CCing Amit as the committer of this feature)
This was originally reported to pgsql-security by Anthropic OSS
program but the security team considered it as a non-vuln bug since
it's a v19-beta code, and I'm reporting here on behalf of them as it's
permitted now.
The reported problem is in sequencesync.c; the sequence
synchronization worker uses an integer that came back from the
publisher as a list subscript without checking it, and then writes
through the resulting pointer.
While it's not a problem in normal cases where the publisher is a
normal PostgreSQL, it could lead to out-of-bounds writes when the
publisher is a malicious server looking like a publisher.
Other fields that we get through get_and_validate_seq_info() could
also get the wrong value but they just show the wrong values rather
than OOB writes. So I think we need a safeguard only for seqidx.
I've attached the patch to fix it. Feedback is very welcome.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-patch] v1-0001-Add-a-range-check-on-the-sequence-index-from-the-.patch (2.4K, ../../CAD21AoBWDMuMmnevZaZ1xSOi75eUx8X7LbVQzmUZudk6F8Bdgg@mail.gmail.com/2-v1-0001-Add-a-range-check-on-the-sequence-index-from-the-.patch)
download | inline diff:
From f79d9aa01bbb1f1fffff6a9b81bd9b9de48bac07 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 21 Sep 2026 12:25:28 -0700
Subject: [PATCH v1] Add a range check on the sequence index from the
publisher.
The sequencesync worker asks the publisher about a batch of sequences,
tagging each one with its position in the worker's own list, and the
publisher returns that position alongside the sequence's data. The
received position is used to subscript the list using list_nth(),
which bounds-checks only under assertinos, so it was possible that an
index we never sent made the worker read a pointer from past the end
of the list and then store the remote last_value through it.
Check the position against the list before using it. No sane publisher
can trigger this, but we should not let a remote server steer a memory
access. An in-range position from another batch still gets through and
would attach one sequence's data to another, but that's a wrong value
rather than a corrupt process.
Backpatch v19, where the sequence synchronization was introduced.
Reported-by: Anthropic OSS program
Discussion: https://postgr.es/m/
Backpatch-through: 19
---
src/backend/replication/logical/sequencesync.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/backend/replication/logical/sequencesync.c b/src/backend/replication/logical/sequencesync.c
index 6d551d45791..c226af246b9 100644
--- a/src/backend/replication/logical/sequencesync.c
+++ b/src/backend/replication/logical/sequencesync.c
@@ -285,6 +285,17 @@ get_and_validate_seq_info(TupleTableSlot *slot, Relation *sequence_rel,
*seqidx = DatumGetInt32(slot_getattr(slot, ++col, &isnull));
Assert(!isnull);
+ /*
+ * The publisher only echoes back an index that we put in the VALUES list,
+ * so this should always identify an entry of seqinfos. Check it anyway
+ * before using it as a list subscript, since list_nth() does not
+ * bounds-check outside assert-enabled builds and we would then write the
+ * remote sequence state through a pointer fetched from beyond the list.
+ */
+ if (*seqidx < 0 || *seqidx >= list_length(seqinfos))
+ elog(ERROR, "invalid sequence index %d received from the publisher",
+ *seqidx);
+
/* Identify the corresponding local sequence for the given index. */
*seqinfo = seqinfo_local =
(LogicalRepSequenceInfo *) list_nth(seqinfos, *seqidx);
--
2.55.0
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Adding a range check on the sequence index from the publisher.
@ 2026-09-22 04:09 Amit Kapila <amit.kapila16@gmail.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
1 sibling, 1 reply; 11+ messages in thread
From: Amit Kapila @ 2026-09-22 04:09 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: pgsql-hackers
On Tue, Sep 22, 2026 at 1:22 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've attached the patch to fix it. Feedback is very welcome.
>
Thanks, fix LGTM. BTW, on similar lines, we can change seqRow[0] to
INT4OID from INT8OID in copy_sequences()[1]. Since
libpqrcv_processTuples uses the declared type's input function, the
value is parsed by int8in and then silently truncated to its low 32
bits. This can be risky too.
[1]:
Oid seqRow[REMOTE_SEQ_COL_COUNT] = {INT8OID,
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Adding a range check on the sequence index from the publisher.
@ 2026-09-22 04:59 Chao Li <li.evan.chao@gmail.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
1 sibling, 1 reply; 11+ messages in thread
From: Chao Li @ 2026-09-22 04:59 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: pgsql-hackers; Amit Kapila <amit.kapila16@gmail.com>
> On Sep 22, 2026, at 03:51, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Hi all,
> (CCing Amit as the committer of this feature)
>
> This was originally reported to pgsql-security by Anthropic OSS
> program but the security team considered it as a non-vuln bug since
> it's a v19-beta code, and I'm reporting here on behalf of them as it's
> permitted now.
>
> The reported problem is in sequencesync.c; the sequence
> synchronization worker uses an integer that came back from the
> publisher as a list subscript without checking it, and then writes
> through the resulting pointer.
>
> While it's not a problem in normal cases where the publisher is a
> normal PostgreSQL, it could lead to out-of-bounds writes when the
> publisher is a malicious server looking like a publisher.
>
> Other fields that we get through get_and_validate_seq_info() could
> also get the wrong value but they just show the wrong values rather
> than OOB writes. So I think we need a safeguard only for seqidx.
>
> I've attached the patch to fix it. Feedback is very welcome.
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com
> <v1-0001-Add-a-range-check-on-the-sequence-index-from-the-.patch>
If the concern here is a malicious publisher, does it also make sense to replace Assert(!isnull) with a runtime check and fail if seqidx is NULL?
Also there is a typo in the commit message:
```
which bounds-checks only under assertinos, so it was possible that an
```
assertinos -> assertions
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Adding a range check on the sequence index from the publisher.
@ 2026-09-22 20:22 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 0 replies; 11+ messages in thread
From: Masahiko Sawada @ 2026-09-22 20:22 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: pgsql-hackers
On Mon, Sep 21, 2026 at 9:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Sep 22, 2026 at 1:22 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached the patch to fix it. Feedback is very welcome.
> >
>
> Thanks, fix LGTM. BTW, on similar lines, we can change seqRow[0] to
> INT4OID from INT8OID in copy_sequences()[1]. Since
> libpqrcv_processTuples uses the declared type's input function, the
> value is parsed by int8in and then silently truncated to its low 32
> bits. This can be risky too.
Right. It doesn't actually lead to OOB writes so I was thinking of
fixing it in a separate patch. Having said that, it would be
reasonable to fix both at the same time.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Adding a range check on the sequence index from the publisher.
@ 2026-09-22 20:27 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Chao Li <li.evan.chao@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Masahiko Sawada @ 2026-09-22 20:27 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: pgsql-hackers; Amit Kapila <amit.kapila16@gmail.com>
On Mon, Sep 21, 2026 at 10:00 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Sep 22, 2026, at 03:51, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Hi all,
> > (CCing Amit as the committer of this feature)
> >
> > This was originally reported to pgsql-security by Anthropic OSS
> > program but the security team considered it as a non-vuln bug since
> > it's a v19-beta code, and I'm reporting here on behalf of them as it's
> > permitted now.
> >
> > The reported problem is in sequencesync.c; the sequence
> > synchronization worker uses an integer that came back from the
> > publisher as a list subscript without checking it, and then writes
> > through the resulting pointer.
> >
> > While it's not a problem in normal cases where the publisher is a
> > normal PostgreSQL, it could lead to out-of-bounds writes when the
> > publisher is a malicious server looking like a publisher.
> >
> > Other fields that we get through get_and_validate_seq_info() could
> > also get the wrong value but they just show the wrong values rather
> > than OOB writes. So I think we need a safeguard only for seqidx.
> >
> > I've attached the patch to fix it. Feedback is very welcome.
> >
> > Regards,
> >
> > --
> > Masahiko Sawada
> > Amazon Web Services: https://aws.amazon.com
> > <v1-0001-Add-a-range-check-on-the-sequence-index-from-the-.patch>
>
> If the concern here is a malicious publisher, does it also make sense to replace Assert(!isnull) with a runtime check and fail if seqidx is NULL?
I don't think we need it from a security perspective. Even if a
malicious publisher returns NULL as seqidx, a garbage value is stored
to *seqidx and will fail the new range check.
>
> Also there is a typo in the commit message:
> ```
> which bounds-checks only under assertinos, so it was possible that an
> ```
>
> assertinos -> assertions
Will fix it.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Adding a range check on the sequence index from the publisher.
@ 2026-09-23 02:08 Chao Li <li.evan.chao@gmail.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Chao Li @ 2026-09-23 02:08 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: pgsql-hackers; Amit Kapila <amit.kapila16@gmail.com>
> On Sep 23, 2026, at 04:27, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Sep 21, 2026 at 10:00 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>>
>>
>>> On Sep 22, 2026, at 03:51, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>
>>> Hi all,
>>> (CCing Amit as the committer of this feature)
>>>
>>> This was originally reported to pgsql-security by Anthropic OSS
>>> program but the security team considered it as a non-vuln bug since
>>> it's a v19-beta code, and I'm reporting here on behalf of them as it's
>>> permitted now.
>>>
>>> The reported problem is in sequencesync.c; the sequence
>>> synchronization worker uses an integer that came back from the
>>> publisher as a list subscript without checking it, and then writes
>>> through the resulting pointer.
>>>
>>> While it's not a problem in normal cases where the publisher is a
>>> normal PostgreSQL, it could lead to out-of-bounds writes when the
>>> publisher is a malicious server looking like a publisher.
>>>
>>> Other fields that we get through get_and_validate_seq_info() could
>>> also get the wrong value but they just show the wrong values rather
>>> than OOB writes. So I think we need a safeguard only for seqidx.
>>>
>>> I've attached the patch to fix it. Feedback is very welcome.
>>>
>>> Regards,
>>>
>>> --
>>> Masahiko Sawada
>>> Amazon Web Services: https://aws.amazon.com
>>> <v1-0001-Add-a-range-check-on-the-sequence-index-from-the-.patch>
>>
>> If the concern here is a malicious publisher, does it also make sense to replace Assert(!isnull) with a runtime check and fail if seqidx is NULL?
>
> I don't think we need it from a security perspective. Even if a
> malicious publisher returns NULL as seqidx, a garbage value is stored
> to *seqidx and will fail the new range check.
>
If a malicious publisher returns NULL for seqidx, the resulting *seqidx will likely be 0. Since 0 passes the range check, the first sequence could be silently selected, which might be incorrect.
On second thought, however, a malicious publisher could directly return a valid but incorrect seqidx, and we do not seem to have a way to protect against that. From this perspective, checking isnull would not help much.
But from another perspective, an Assert is normally used for an internal invariant. Here, however, seqidx is received from external, so a runtime check seems more reasonable.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Adding a range check on the sequence index from the publisher.
@ 2026-09-23 04:19 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Chao Li <li.evan.chao@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Masahiko Sawada @ 2026-09-23 04:19 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: pgsql-hackers; Amit Kapila <amit.kapila16@gmail.com>
On Tue, Sep 22, 2026 at 7:08 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Sep 23, 2026, at 04:27, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Sep 21, 2026 at 10:00 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >>
> >>
> >>
> >>> On Sep 22, 2026, at 03:51, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >>>
> >>> Hi all,
> >>> (CCing Amit as the committer of this feature)
> >>>
> >>> This was originally reported to pgsql-security by Anthropic OSS
> >>> program but the security team considered it as a non-vuln bug since
> >>> it's a v19-beta code, and I'm reporting here on behalf of them as it's
> >>> permitted now.
> >>>
> >>> The reported problem is in sequencesync.c; the sequence
> >>> synchronization worker uses an integer that came back from the
> >>> publisher as a list subscript without checking it, and then writes
> >>> through the resulting pointer.
> >>>
> >>> While it's not a problem in normal cases where the publisher is a
> >>> normal PostgreSQL, it could lead to out-of-bounds writes when the
> >>> publisher is a malicious server looking like a publisher.
> >>>
> >>> Other fields that we get through get_and_validate_seq_info() could
> >>> also get the wrong value but they just show the wrong values rather
> >>> than OOB writes. So I think we need a safeguard only for seqidx.
> >>>
> >>> I've attached the patch to fix it. Feedback is very welcome.
> >>>
> >>> Regards,
> >>>
> >>> --
> >>> Masahiko Sawada
> >>> Amazon Web Services: https://aws.amazon.com
> >>> <v1-0001-Add-a-range-check-on-the-sequence-index-from-the-.patch>
> >>
> >> If the concern here is a malicious publisher, does it also make sense to replace Assert(!isnull) with a runtime check and fail if seqidx is NULL?
> >
> > I don't think we need it from a security perspective. Even if a
> > malicious publisher returns NULL as seqidx, a garbage value is stored
> > to *seqidx and will fail the new range check.
> >
>
> If a malicious publisher returns NULL for seqidx, the resulting *seqidx will likely be 0. Since 0 passes the range check, the first sequence could be silently selected, which might be incorrect.
Right.
> On second thought, however, a malicious publisher could directly return a valid but incorrect seqidx, and we do not seem to have a way to protect against that. From this perspective, checking isnull would not help much.
Agreed, and I think that is the important point. We have no way to
tell a malicious value from a buggy one, so validating the value
doesn't really make sense. A publisher reporting a wrong last_value is
indistinguishable from a publisher whose sequence really holds that
value, so it can change the sequence on the subscriber whatever we
check.
I think what we need to fix here is narrower: the case where the
damage goes beyond the sequence being synchronized. The other columns
only lead to a wrong sequence value or a wrong report. seqidx is the
only one that becomes a list subscript, and so we write last_value
through a pointer taken from outside of the list.
> But from another perspective, an Assert is normally used for an internal invariant. Here, however, seqidx is received from external, so a runtime check seems more reasonable.
I agree with this in general. But get_and_validate_seq_info() has nine
Assert(!isnull) on columns that all come from the publisher, so
converting only the seqidx one doesn't make the function any more
consistent. A null seqidx leads to a wrong sequence value, which is
the same class of problem as a wrong last_value.
So I'd like to keep this patch to the range check. If we want to
convert those Asserts I think we should do all nine, as a separate
patch.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Adding a range check on the sequence index from the publisher.
@ 2026-09-23 05:09 Chao Li <li.evan.chao@gmail.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Chao Li @ 2026-09-23 05:09 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: pgsql-hackers; Amit Kapila <amit.kapila16@gmail.com>
> On Sep 23, 2026, at 12:19, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Sep 22, 2026 at 7:08 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>>
>>
>>> On Sep 23, 2026, at 04:27, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>
>>> On Mon, Sep 21, 2026 at 10:00 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>>> On Sep 22, 2026, at 03:51, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>>>
>>>>> Hi all,
>>>>> (CCing Amit as the committer of this feature)
>>>>>
>>>>> This was originally reported to pgsql-security by Anthropic OSS
>>>>> program but the security team considered it as a non-vuln bug since
>>>>> it's a v19-beta code, and I'm reporting here on behalf of them as it's
>>>>> permitted now.
>>>>>
>>>>> The reported problem is in sequencesync.c; the sequence
>>>>> synchronization worker uses an integer that came back from the
>>>>> publisher as a list subscript without checking it, and then writes
>>>>> through the resulting pointer.
>>>>>
>>>>> While it's not a problem in normal cases where the publisher is a
>>>>> normal PostgreSQL, it could lead to out-of-bounds writes when the
>>>>> publisher is a malicious server looking like a publisher.
>>>>>
>>>>> Other fields that we get through get_and_validate_seq_info() could
>>>>> also get the wrong value but they just show the wrong values rather
>>>>> than OOB writes. So I think we need a safeguard only for seqidx.
>>>>>
>>>>> I've attached the patch to fix it. Feedback is very welcome.
>>>>>
>>>>> Regards,
>>>>>
>>>>> --
>>>>> Masahiko Sawada
>>>>> Amazon Web Services: https://aws.amazon.com
>>>>> <v1-0001-Add-a-range-check-on-the-sequence-index-from-the-.patch>
>>>>
>>>> If the concern here is a malicious publisher, does it also make sense to replace Assert(!isnull) with a runtime check and fail if seqidx is NULL?
>>>
>>> I don't think we need it from a security perspective. Even if a
>>> malicious publisher returns NULL as seqidx, a garbage value is stored
>>> to *seqidx and will fail the new range check.
>>>
>>
>> If a malicious publisher returns NULL for seqidx, the resulting *seqidx will likely be 0. Since 0 passes the range check, the first sequence could be silently selected, which might be incorrect.
>
> Right.
>
>> On second thought, however, a malicious publisher could directly return a valid but incorrect seqidx, and we do not seem to have a way to protect against that. From this perspective, checking isnull would not help much.
>
> Agreed, and I think that is the important point. We have no way to
> tell a malicious value from a buggy one, so validating the value
> doesn't really make sense. A publisher reporting a wrong last_value is
> indistinguishable from a publisher whose sequence really holds that
> value, so it can change the sequence on the subscriber whatever we
> check.
>
> I think what we need to fix here is narrower: the case where the
> damage goes beyond the sequence being synchronized. The other columns
> only lead to a wrong sequence value or a wrong report. seqidx is the
> only one that becomes a list subscript, and so we write last_value
> through a pointer taken from outside of the list.
How about explaining that more explicitly in the comment? For example, the check prevents out-of-bounds access, but cannot protect against an incorrect index that is still within the valid range.
>
>> But from another perspective, an Assert is normally used for an internal invariant. Here, however, seqidx is received from external, so a runtime check seems more reasonable.
>
> I agree with this in general. But get_and_validate_seq_info() has nine
> Assert(!isnull) on columns that all come from the publisher, so
> converting only the seqidx one doesn't make the function any more
> consistent. A null seqidx leads to a wrong sequence value, which is
> the same class of problem as a wrong last_value.
>
> So I'd like to keep this patch to the range check. If we want to
> convert those Asserts I think we should do all nine, as a separate
> patch.
>
No objection here.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Adding a range check on the sequence index from the publisher.
@ 2026-09-23 19:14 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Chao Li <li.evan.chao@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Masahiko Sawada @ 2026-09-23 19:14 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: pgsql-hackers; Amit Kapila <amit.kapila16@gmail.com>
On Tue, Sep 22, 2026 at 10:10 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Sep 23, 2026, at 12:19, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, Sep 22, 2026 at 7:08 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >>
> >>
> >>
> >>> On Sep 23, 2026, at 04:27, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >>>
> >>> On Mon, Sep 21, 2026 at 10:00 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>>> On Sep 22, 2026, at 03:51, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >>>>>
> >>>>> Hi all,
> >>>>> (CCing Amit as the committer of this feature)
> >>>>>
> >>>>> This was originally reported to pgsql-security by Anthropic OSS
> >>>>> program but the security team considered it as a non-vuln bug since
> >>>>> it's a v19-beta code, and I'm reporting here on behalf of them as it's
> >>>>> permitted now.
> >>>>>
> >>>>> The reported problem is in sequencesync.c; the sequence
> >>>>> synchronization worker uses an integer that came back from the
> >>>>> publisher as a list subscript without checking it, and then writes
> >>>>> through the resulting pointer.
> >>>>>
> >>>>> While it's not a problem in normal cases where the publisher is a
> >>>>> normal PostgreSQL, it could lead to out-of-bounds writes when the
> >>>>> publisher is a malicious server looking like a publisher.
> >>>>>
> >>>>> Other fields that we get through get_and_validate_seq_info() could
> >>>>> also get the wrong value but they just show the wrong values rather
> >>>>> than OOB writes. So I think we need a safeguard only for seqidx.
> >>>>>
> >>>>> I've attached the patch to fix it. Feedback is very welcome.
> >>>>>
> >>>>> Regards,
> >>>>>
> >>>>> --
> >>>>> Masahiko Sawada
> >>>>> Amazon Web Services: https://aws.amazon.com
> >>>>> <v1-0001-Add-a-range-check-on-the-sequence-index-from-the-.patch>
> >>>>
> >>>> If the concern here is a malicious publisher, does it also make sense to replace Assert(!isnull) with a runtime check and fail if seqidx is NULL?
> >>>
> >>> I don't think we need it from a security perspective. Even if a
> >>> malicious publisher returns NULL as seqidx, a garbage value is stored
> >>> to *seqidx and will fail the new range check.
> >>>
> >>
> >> If a malicious publisher returns NULL for seqidx, the resulting *seqidx will likely be 0. Since 0 passes the range check, the first sequence could be silently selected, which might be incorrect.
> >
> > Right.
> >
> >> On second thought, however, a malicious publisher could directly return a valid but incorrect seqidx, and we do not seem to have a way to protect against that. From this perspective, checking isnull would not help much.
> >
> > Agreed, and I think that is the important point. We have no way to
> > tell a malicious value from a buggy one, so validating the value
> > doesn't really make sense. A publisher reporting a wrong last_value is
> > indistinguishable from a publisher whose sequence really holds that
> > value, so it can change the sequence on the subscriber whatever we
> > check.
> >
> > I think what we need to fix here is narrower: the case where the
> > damage goes beyond the sequence being synchronized. The other columns
> > only lead to a wrong sequence value or a wrong report. seqidx is the
> > only one that becomes a list subscript, and so we write last_value
> > through a pointer taken from outside of the list.
>
> How about explaining that more explicitly in the comment? For example, the check prevents out-of-bounds access, but cannot protect against an incorrect index that is still within the valid range.
How about the following?
/*
* The publisher only echoes back an index that we put in the VALUES list,
* so this should always identify an entry of seqinfos. Check it anyway
* before using it as a list subscript, since list_nth() does not
* bounds-check on non-assert builds and we would then write the remote
* sequence state through a pointer fetched from beyond the list.
*
* This only keeps the subscript inside the list. An index that is wrong
* but still in range is not detected, and cannot be; the sequence it
* points at then receives another sequence's data. That is the same kind
* of damage as the publisher reporting a wrong value in any other column,
* and is likewise beyond what we can check.
*/
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Adding a range check on the sequence index from the publisher.
@ 2026-09-23 23:48 Chao Li <li.evan.chao@gmail.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Chao Li @ 2026-09-23 23:48 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: pgsql-hackers; Amit Kapila <amit.kapila16@gmail.com>
> On Sep 24, 2026, at 03:14, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Sep 22, 2026 at 10:10 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>>
>>
>>> On Sep 23, 2026, at 12:19, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>
>>> On Tue, Sep 22, 2026 at 7:08 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>>> On Sep 23, 2026, at 04:27, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>>>
>>>>> On Mon, Sep 21, 2026 at 10:00 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>> On Sep 22, 2026, at 03:51, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>>>>>
>>>>>>> Hi all,
>>>>>>> (CCing Amit as the committer of this feature)
>>>>>>>
>>>>>>> This was originally reported to pgsql-security by Anthropic OSS
>>>>>>> program but the security team considered it as a non-vuln bug since
>>>>>>> it's a v19-beta code, and I'm reporting here on behalf of them as it's
>>>>>>> permitted now.
>>>>>>>
>>>>>>> The reported problem is in sequencesync.c; the sequence
>>>>>>> synchronization worker uses an integer that came back from the
>>>>>>> publisher as a list subscript without checking it, and then writes
>>>>>>> through the resulting pointer.
>>>>>>>
>>>>>>> While it's not a problem in normal cases where the publisher is a
>>>>>>> normal PostgreSQL, it could lead to out-of-bounds writes when the
>>>>>>> publisher is a malicious server looking like a publisher.
>>>>>>>
>>>>>>> Other fields that we get through get_and_validate_seq_info() could
>>>>>>> also get the wrong value but they just show the wrong values rather
>>>>>>> than OOB writes. So I think we need a safeguard only for seqidx.
>>>>>>>
>>>>>>> I've attached the patch to fix it. Feedback is very welcome.
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> --
>>>>>>> Masahiko Sawada
>>>>>>> Amazon Web Services: https://aws.amazon.com
>>>>>>> <v1-0001-Add-a-range-check-on-the-sequence-index-from-the-.patch>
>>>>>>
>>>>>> If the concern here is a malicious publisher, does it also make sense to replace Assert(!isnull) with a runtime check and fail if seqidx is NULL?
>>>>>
>>>>> I don't think we need it from a security perspective. Even if a
>>>>> malicious publisher returns NULL as seqidx, a garbage value is stored
>>>>> to *seqidx and will fail the new range check.
>>>>>
>>>>
>>>> If a malicious publisher returns NULL for seqidx, the resulting *seqidx will likely be 0. Since 0 passes the range check, the first sequence could be silently selected, which might be incorrect.
>>>
>>> Right.
>>>
>>>> On second thought, however, a malicious publisher could directly return a valid but incorrect seqidx, and we do not seem to have a way to protect against that. From this perspective, checking isnull would not help much.
>>>
>>> Agreed, and I think that is the important point. We have no way to
>>> tell a malicious value from a buggy one, so validating the value
>>> doesn't really make sense. A publisher reporting a wrong last_value is
>>> indistinguishable from a publisher whose sequence really holds that
>>> value, so it can change the sequence on the subscriber whatever we
>>> check.
>>>
>>> I think what we need to fix here is narrower: the case where the
>>> damage goes beyond the sequence being synchronized. The other columns
>>> only lead to a wrong sequence value or a wrong report. seqidx is the
>>> only one that becomes a list subscript, and so we write last_value
>>> through a pointer taken from outside of the list.
>>
>> How about explaining that more explicitly in the comment? For example, the check prevents out-of-bounds access, but cannot protect against an incorrect index that is still within the valid range.
>
> How about the following?
>
> /*
> * The publisher only echoes back an index that we put in the VALUES list,
> * so this should always identify an entry of seqinfos. Check it anyway
> * before using it as a list subscript, since list_nth() does not
> * bounds-check on non-assert builds and we would then write the remote
> * sequence state through a pointer fetched from beyond the list.
> *
> * This only keeps the subscript inside the list. An index that is wrong
> * but still in range is not detected, and cannot be; the sequence it
> * points at then receives another sequence's data. That is the same kind
> * of damage as the publisher reporting a wrong value in any other column,
> * and is likewise beyond what we can check.
> */
>
WFM
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Adding a range check on the sequence index from the publisher.
@ 2026-09-24 18:08 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Chao Li <li.evan.chao@gmail.com>
0 siblings, 0 replies; 11+ messages in thread
From: Masahiko Sawada @ 2026-09-24 18:08 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: pgsql-hackers; Amit Kapila <amit.kapila16@gmail.com>
On Wed, Sep 23, 2026 at 4:48 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Sep 24, 2026, at 03:14, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, Sep 22, 2026 at 10:10 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >>
> >>
> >>
> >>> On Sep 23, 2026, at 12:19, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >>>
> >>> On Tue, Sep 22, 2026 at 7:08 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>>> On Sep 23, 2026, at 04:27, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >>>>>
> >>>>> On Mon, Sep 21, 2026 at 10:00 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> On Sep 22, 2026, at 03:51, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >>>>>>>
> >>>>>>> Hi all,
> >>>>>>> (CCing Amit as the committer of this feature)
> >>>>>>>
> >>>>>>> This was originally reported to pgsql-security by Anthropic OSS
> >>>>>>> program but the security team considered it as a non-vuln bug since
> >>>>>>> it's a v19-beta code, and I'm reporting here on behalf of them as it's
> >>>>>>> permitted now.
> >>>>>>>
> >>>>>>> The reported problem is in sequencesync.c; the sequence
> >>>>>>> synchronization worker uses an integer that came back from the
> >>>>>>> publisher as a list subscript without checking it, and then writes
> >>>>>>> through the resulting pointer.
> >>>>>>>
> >>>>>>> While it's not a problem in normal cases where the publisher is a
> >>>>>>> normal PostgreSQL, it could lead to out-of-bounds writes when the
> >>>>>>> publisher is a malicious server looking like a publisher.
> >>>>>>>
> >>>>>>> Other fields that we get through get_and_validate_seq_info() could
> >>>>>>> also get the wrong value but they just show the wrong values rather
> >>>>>>> than OOB writes. So I think we need a safeguard only for seqidx.
> >>>>>>>
> >>>>>>> I've attached the patch to fix it. Feedback is very welcome.
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>>
> >>>>>>> --
> >>>>>>> Masahiko Sawada
> >>>>>>> Amazon Web Services: https://aws.amazon.com
> >>>>>>> <v1-0001-Add-a-range-check-on-the-sequence-index-from-the-.patch>
> >>>>>>
> >>>>>> If the concern here is a malicious publisher, does it also make sense to replace Assert(!isnull) with a runtime check and fail if seqidx is NULL?
> >>>>>
> >>>>> I don't think we need it from a security perspective. Even if a
> >>>>> malicious publisher returns NULL as seqidx, a garbage value is stored
> >>>>> to *seqidx and will fail the new range check.
> >>>>>
> >>>>
> >>>> If a malicious publisher returns NULL for seqidx, the resulting *seqidx will likely be 0. Since 0 passes the range check, the first sequence could be silently selected, which might be incorrect.
> >>>
> >>> Right.
> >>>
> >>>> On second thought, however, a malicious publisher could directly return a valid but incorrect seqidx, and we do not seem to have a way to protect against that. From this perspective, checking isnull would not help much.
> >>>
> >>> Agreed, and I think that is the important point. We have no way to
> >>> tell a malicious value from a buggy one, so validating the value
> >>> doesn't really make sense. A publisher reporting a wrong last_value is
> >>> indistinguishable from a publisher whose sequence really holds that
> >>> value, so it can change the sequence on the subscriber whatever we
> >>> check.
> >>>
> >>> I think what we need to fix here is narrower: the case where the
> >>> damage goes beyond the sequence being synchronized. The other columns
> >>> only lead to a wrong sequence value or a wrong report. seqidx is the
> >>> only one that becomes a list subscript, and so we write last_value
> >>> through a pointer taken from outside of the list.
> >>
> >> How about explaining that more explicitly in the comment? For example, the check prevents out-of-bounds access, but cannot protect against an incorrect index that is still within the valid range.
> >
> > How about the following?
> >
> > /*
> > * The publisher only echoes back an index that we put in the VALUES list,
> > * so this should always identify an entry of seqinfos. Check it anyway
> > * before using it as a list subscript, since list_nth() does not
> > * bounds-check on non-assert builds and we would then write the remote
> > * sequence state through a pointer fetched from beyond the list.
> > *
> > * This only keeps the subscript inside the list. An index that is wrong
> > * but still in range is not detected, and cannot be; the sequence it
> > * points at then receives another sequence's data. That is the same kind
> > * of damage as the publisher reporting a wrong value in any other column,
> > * and is likewise beyond what we can check.
> > */
> >
>
> WFM
I've pushed the patch after modifying the comment and changing INT8OID
to INT4OID.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2026-09-24 18:08 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-09-21 19:51 Adding a range check on the sequence index from the publisher. Masahiko Sawada <sawada.mshk@gmail.com>
2026-09-22 04:09 ` Amit Kapila <amit.kapila16@gmail.com>
2026-09-22 20:22 ` Masahiko Sawada <sawada.mshk@gmail.com>
2026-09-22 04:59 ` Chao Li <li.evan.chao@gmail.com>
2026-09-22 20:27 ` Masahiko Sawada <sawada.mshk@gmail.com>
2026-09-23 02:08 ` Chao Li <li.evan.chao@gmail.com>
2026-09-23 04:19 ` Masahiko Sawada <sawada.mshk@gmail.com>
2026-09-23 05:09 ` Chao Li <li.evan.chao@gmail.com>
2026-09-23 19:14 ` Masahiko Sawada <sawada.mshk@gmail.com>
2026-09-23 23:48 ` Chao Li <li.evan.chao@gmail.com>
2026-09-24 18:08 ` Masahiko Sawada <sawada.mshk@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox