Обсуждение: [PATCH] Fix bug when calling strncmp in check_authmethod_valid

Hi all,

Attached is a patch that fixes bug when calling strncmp function, in
which case the third argument (authmethod - strchr(authmethod, ' '))
may be negative, which is not as expected..


With Regards,
Jingxian Li.

> On 30 Apr 2024, at 04:41, Jingxian Li <aqktjcm@qq.com> wrote:

> Attached is a patch that fixes bug when calling strncmp function, in
> which case the third argument (authmethod - strchr(authmethod, ' '))
> may be negative, which is not as expected..

The calculation is indeed incorrect, but the lack of complaints of it being
broken made me wonder why this exist in the first place.  This dates back to
e7029b212755, just shy of 2 decades old, which added --auth with support for
strings with auth-options to ident and pam like --auth 'pam <servicename>' and
'ident sameuser'.  Support for options to ident was removed in 01c1a12a5bb4 but
options to pam is still supported (although not documented), but was AFAICT
broken in commit 8a02339e9ba3 some 12 years ago with this strncmp().

-  if (strncmp(authmethod, *p, (authmethod - strchr(authmethod, ' '))) == 0)
+  if (strncmp(authmethod, *p, (strchr(authmethod, ' ') - authmethod)) == 0)

This with compare "pam postgresql" with "pam" and not "pam " so the length
should be "(strchr(authmethod, ' ') - authmethod + 1)" since "pam " is a method
separate from "pam" in auth_methods_{host|local}.  We don't want to allow "md5
" as that's not a method in the array of valid methods.

But, since it's been broken in all supported versions of postgres and has
AFAICT never been documented to exist, should we fix it or just remove it?  We
don't support auth-options for any other methods, like clientcert to cert for
example.  If we fix it we should also document that it works IMHO.

--
Daniel Gustafsson

Hi Daniel，
Thank you for explaining the ins and outs of this problem.

On 2024/4/30 17:14, Daniel Gustafsson wrote:
>> On 30 Apr 2024, at 04:41, Jingxian Li <aqktjcm@qq.com> wrote:
>
>> Attached is a patch that fixes bug when calling strncmp function, in
>> which case the third argument (authmethod - strchr(authmethod, ' '))
>> may be negative, which is not as expected..
>
> The calculation is indeed incorrect, but the lack of complaints of it being
> broken made me wonder why this exist in the first place.  This dates back to
> e7029b212755, just shy of 2 decades old, which added --auth with support for
> strings with auth-options to ident and pam like --auth 'pam <servicename>' and
> 'ident sameuser'.  Support for options to ident was removed in 01c1a12a5bb4 but
> options to pam is still supported (although not documented), but was AFAICT
> broken in commit 8a02339e9ba3 some 12 years ago with this strncmp().
>
> -  if (strncmp(authmethod, *p, (authmethod - strchr(authmethod, ' '))) == 0)
> +  if (strncmp(authmethod, *p, (strchr(authmethod, ' ') - authmethod)) == 0)
>
> This with compare "pam postgresql" with "pam" and not "pam " so the length
> should be "(strchr(authmethod, ' ') - authmethod + 1)" since "pam " is a method
> separate from "pam" in auth_methods_{host|local}.  We don't want to allow "md5
> " as that's not a method in the array of valid methods.
>
> But, since it's been broken in all supported versions of postgres and has
> AFAICT never been documented to exist, should we fix it or just remove it?  We
> don't support auth-options for any other methods, like clientcert to cert for
> example.  If we fix it we should also document that it works IMHO.

You mentioned that auth-options are not supported for auth methods except pam,
but I found that  some methods (such as  ldap and radius etc.) also requires aut-options,
and there are no corresponding auth methods ending with space (such as  "ldap " and
radius ") present in auth_methods_host and auth_methods_local arrays.

--
Jingxian Li

> On 7 May 2024, at 06:46, Jingxian Li <aqktjcm@qq.com> wrote:

>> But, since it's been broken in all supported versions of postgres and has
>> AFAICT never been documented to exist, should we fix it or just remove it?  We
>> don't support auth-options for any other methods, like clientcert to cert for
>> example.  If we fix it we should also document that it works IMHO.
>
> You mentioned that auth-options are not supported for auth methods except pam,
> but I found that  some methods (such as  ldap and radius etc.) also requires aut-options,
> and there are no corresponding auth methods ending with space (such as  "ldap " and
> radius ") present in auth_methods_host and auth_methods_local arrays.

Correct, only pam and ident were ever supported (yet not documented) and ident
was removed a long time ago.

Searching the archives I was unable to find any complaints, and this has been
broken for the entire window of supported releases, so I propose we remove it
as per the attached patch.  If anyone is keen on making this work again for all
the types where it makes sense, it can be resurrected (probably with a better
implementation).

Any objections to fixing this in 17 by removing it? (cc:ing Michael from the RMT)

--
Daniel Gustafsson

Hi,

> Searching the archives I was unable to find any complaints, and this has been
> broken for the entire window of supported releases, so I propose we remove it
> as per the attached patch.  If anyone is keen on making this work again for all
> the types where it makes sense, it can be resurrected (probably with a better
> implementation).
>
> Any objections to fixing this in 17 by removing it? (cc:ing Michael from the RMT)

+1 Something that is not documented or used by anyone (apparently) and
is broken should just be removed.

-- 
Best regards,
Aleksander Alekseev

On Mon, May 13, 2024 at 01:01:21PM +0300, Aleksander Alekseev wrote:
>> Any objections to fixing this in 17 by removing it? (cc:ing Michael from the RMT)
>
> +1 Something that is not documented or used by anyone (apparently) and
> is broken should just be removed.

8a02339e9ba3 sounds like an argument good enough to prove there is no
demand in the field for being able to support options through initdb
--auth, and this does not concern only pam.  If somebody is interested
in that, that could always be done later.  My take is that this would
be simpler if implemented through a separate option, leaving the
checks between the options and the auth method up to the postmaster
when loading pg_hba.conf at startup.

Hence, no objections to clean up that now.  Thanks for asking.
--
Michael

> On 14 May 2024, at 07:12, Michael Paquier <michael@paquier.xyz> wrote:

> Hence, no objections to clean up that now.  Thanks for asking.

Thanks for verifying, I've pushed this now.

--
Daniel Gustafsson