Thread: Possible buffer overrun in src/backend/libpq/hba.c gethba_options()

Hi,

During a routine Coverity scan of our internal PostgreSQL fork, it
issued a buffer overrun warning for src/backend/libpq/hba.c,
gethba_options()[0]:

MAIN_ISSUE EventDescription: Overrunning array "options" of 12 8-byte
elements at element index 12 (byte offset 96) using index "noptions++"
(which evaluates to 12).
[...]
        if (hba->ldapscope)
            options[noptions++] =
                CStringGetTextDatum(psprintf("ldapscope=%d", hba->ldapscope));
[...]

This is because earlier in the function[1], if hba->usermap,
hba->clientcert, and hba->pamservice were set then noptions would
exceed MAX_HBA_OPTIONS. Of course, if those options are mutually
exclusive with hba->auth_method == uaLDAP, then it's a false positive.
Is that the case, or should MAX_HBA_OPTIONS be increased?

Thanks.

[0] https://github.com/postgres/postgres/blob/master/src/backend/libpq/hba.c#L2307
[1] https://github.com/postgres/postgres/blob/master/src/backend/libpq/hba.c#L2249

On Tue, Nov 13, 2018 at 3:02 PM Julian Hsiao <jhsiao@salesforce.com> wrote:
> During a routine Coverity scan of our internal PostgreSQL fork, it
> issued a buffer overrun warning for src/backend/libpq/hba.c,
> gethba_options()[0]:
>
> MAIN_ISSUE EventDescription: Overrunning array "options" of 12 8-byte
> elements at element index 12 (byte offset 96) using index "noptions++"
> (which evaluates to 12).
> [...]
>         if (hba->ldapscope)
>             options[noptions++] =
>                 CStringGetTextDatum(psprintf("ldapscope=%d", hba->ldapscope));
> [...]
>
> This is because earlier in the function[1], if hba->usermap,
> hba->clientcert, and hba->pamservice were set then noptions would
> exceed MAX_HBA_OPTIONS. Of course, if those options are mutually
> exclusive with hba->auth_method == uaLDAP, then it's a false positive.
> Is that the case, or should MAX_HBA_OPTIONS be increased?

Right, thank you.  It seems clear that MAX_HBA_OPTIONS should be
increased and the comment near its definition is wrong.  Will fix.

-- 
Thomas Munro
http://www.enterprisedb.com

On Tue, Nov 13, 2018 at 3:42 PM Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Tue, Nov 13, 2018 at 3:02 PM Julian Hsiao <jhsiao@salesforce.com> wrote:
> > During a routine Coverity scan of our internal PostgreSQL fork, it
> > issued a buffer overrun warning for src/backend/libpq/hba.c,
> > gethba_options()[0]:
> >
> > MAIN_ISSUE EventDescription: Overrunning array "options" of 12 8-byte
> > elements at element index 12 (byte offset 96) using index "noptions++"
> > (which evaluates to 12).
> > [...]
> >         if (hba->ldapscope)
> >             options[noptions++] =
> >                 CStringGetTextDatum(psprintf("ldapscope=%d", hba->ldapscope));
> > [...]
> >
> > This is because earlier in the function[1], if hba->usermap,
> > hba->clientcert, and hba->pamservice were set then noptions would
> > exceed MAX_HBA_OPTIONS. Of course, if those options are mutually
> > exclusive with hba->auth_method == uaLDAP, then it's a false positive.
> > Is that the case, or should MAX_HBA_OPTIONS be increased?
>
> Right, thank you.  It seems clear that MAX_HBA_OPTIONS should be
> increased and the comment near its definition is wrong.  Will fix.

I'm pretty sure that's not a live bug, but I pushed a fix to master,
11 and 10 (slightly different numbers) because it was still wrong.  I
hope Coverity is happier now.  Thanks for the report.

Gee, it'd be nice to be able to build dynamically sized arrays of
datums in something more like a C++ vector and get rid of code like
this.

-- 
Thomas Munro
http://www.enterprisedb.com