public inbox for [email protected]
help / color / mirror / Atom feedFrom: Daniel Gustafsson <[email protected]>
To: Jingxian Li <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Fix bug when calling strncmp in check_authmethod_valid
Date: Tue, 30 Apr 2024 11:14:37 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
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]>
> 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.
--
Daniel Gustafsson
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