On Tue, Sep 22, 2026 at 7:08 PM Chao Li <[email protected]> wrote: > > > > > On Sep 23, 2026, at 04:27, Masahiko Sawada <[email protected]> wrote: > > > > On Mon, Sep 21, 2026 at 10:00 PM Chao Li <[email protected]> wrote: > >> > >> > >> > >>> On Sep 22, 2026, at 03:51, Masahiko Sawada <[email protected]> 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
