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

> 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 not
seemto 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. 

>
>> But from another perspective, an Assert is normally used for an internal invariant. Here, however, seqidx is
receivedfrom 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/