Re: Adding a range check on the sequence index from the publisher. - Mailing list pgsql-hackers

> 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
checkand 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
notseem 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
cannotprotect 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/