public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jingxian Li <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: [PATCH] Fix bug when calling strncmp in check_authmethod_valid
Date: Tue, 7 May 2024 12:46:27 +0800
Message-ID: <[email protected]> (raw)
References: <CAMbWs4-mWCGbbE_bne5=AfqjYGDaUZmjCw2+soLjrdNA0xUDFw@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAMbWs48uTLZT14aaBaEg7cpB6ercU5wm9+WfGLGC3VoP28gfCA@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAMbWs48adH1Am=tszAqyq3UtEhDCsFL22U8Jzq0EpPAA6tOd9A@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
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 <[email protected]> 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
view thread (21+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: [PATCH] Fix bug when calling strncmp in check_authmethod_valid
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox