Re: Adding a range check on the sequence index from the publisher.

On Tue, Sep 22, 2026 at 7:08 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
> > On Sep 23, 2026, at 04:27, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Mon, Sep 21, 2026 at 10:00 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> >>
> >>
> >>
> >>> On Sep 22, 2026, at 03:51, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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