On Wed, Sep 23, 2026 at 4:48 PM Chao Li <[email protected]> wrote: > > > > > On Sep 24, 2026, at 03:14, Masahiko Sawada <[email protected]> wrote: > > > > On Tue, Sep 22, 2026 at 10:10 PM Chao Li <[email protected]> wrote: > >> > >> > >> > >>> On Sep 23, 2026, at 12:19, Masahiko Sawada <[email protected]> wrote: > >>> > >>> 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. > >> > >> 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
