Обсуждение: Increase psql's password buffer size
Folks, At least two cloud providers are now stuffing large amounts of information into the password field. This change makes it possible to accommodate that usage in interactive sessions. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Вложения
David Fetter <david@fetter.org> writes:
> At least two cloud providers are now stuffing large amounts of
> information into the password field. This change makes it possible to
> accommodate that usage in interactive sessions.
Like who? It seems like a completely silly idea. And if 2K is sane,
why not much more?
(I can't say that s/100/2048/ in one place is a particularly evil change;
what bothers me is the likelihood that there are other places that won't
cope with arbitrarily long passwords. Not all of them are necessarily
under our control, either.)
regards, tom lane
On Mon, Jan 20, 2020 at 01:12:35PM -0500, Tom Lane wrote: > David Fetter <david@fetter.org> writes: > > At least two cloud providers are now stuffing large amounts of > > information into the password field. This change makes it possible to > > accommodate that usage in interactive sessions. > > Like who? AWS and Azure are two examples I know of. > It seems like a completely silly idea. And if 2K is sane, why not > much more? Good question. Does it make sense to rearrange these things so they're allocated at runtime instead of compile time? > (I can't say that s/100/2048/ in one place is a particularly evil > change; what bothers me is the likelihood that there are other > places that won't cope with arbitrarily long passwords. Not all of > them are necessarily under our control, either.) I found one that is, so please find attached the next revision of the patch. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Вложения
On Mon, Jan 20, 2020 at 07:44:25PM +0100, David Fetter wrote: > On Mon, Jan 20, 2020 at 01:12:35PM -0500, Tom Lane wrote: > > David Fetter <david@fetter.org> writes: > > > At least two cloud providers are now stuffing large amounts of > > > information into the password field. This change makes it possible to > > > accommodate that usage in interactive sessions. > > > > Like who? > > AWS and Azure are two examples I know of. > > > It seems like a completely silly idea. And if 2K is sane, why not > > much more? > > Good question. Does it make sense to rearrange these things so they're > allocated at runtime instead of compile time? > > > (I can't say that s/100/2048/ in one place is a particularly evil > > change; what bothers me is the likelihood that there are other > > places that won't cope with arbitrarily long passwords. Not all of > > them are necessarily under our control, either.) > > I found one that is, so please find attached the next revision of the > patch. I found another place that assumes 100 bytes and upped it to 2048. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Вложения
David Fetter <david@fetter.org> writes:
> On Mon, Jan 20, 2020 at 07:44:25PM +0100, David Fetter wrote:
>> On Mon, Jan 20, 2020 at 01:12:35PM -0500, Tom Lane wrote:
>>> (I can't say that s/100/2048/ in one place is a particularly evil
>>> change; what bothers me is the likelihood that there are other
>>> places that won't cope with arbitrarily long passwords. Not all of
>>> them are necessarily under our control, either.)
>> I found one that is, so please find attached the next revision of the
>> patch.
> I found another place that assumes 100 bytes and upped it to 2048.
So this is pretty much exactly what I expected. And have you tried
it with e.g. PAM, or LDAP?
I think the AWS guys are fools to imagine that this will work in very
many places, and I don't see why we should be leading the charge to
make it work for them. What's the point of having a huge amount of
data in a password, anyway? You can't expect to get it back out
again, and there's no reason to believe that it adds any security
after a certain point. If they want a bunch of different things
contributing to the password, OK, but they could just hash those
things together and thereby keep their submitted password to a length
that will work with most services.
regards, tom lane
Hi, I think I should add my two cents. On Mon, 20 Jan 2020 at 20:38, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > I found another place that assumes 100 bytes and upped it to 2048. There one more place, in the code which is parsing .pgpass > > So this is pretty much exactly what I expected. And have you tried > it with e.g. PAM, or LDAP? > > I think the AWS guys are fools to imagine that this will work in very > many places, and I don't see why we should be leading the charge to > make it work for them. What's the point of having a huge amount of > data in a password, anyway? We at Zalando are using JWT tokens as passwords. JWT tokens are self-contained and therefore quite huge (up to 700-800 bytes in our case). Tokens have a limited lifetime (1 hour) and we are using PAM to verify them. Altogether the whole thing works like a charm. The only problem that it is not possible to copy&paste the token into psql password prompt, but there is a workaround, export PGPASSWORD=verylongtokenstring && psql JWT: https://jwt.io/ PAM module to verify OAuth tokens: https://github.com/CyberDem0n/pam-oauth2 Regards, -- Alexander Kukushkin
Alexander Kukushkin <cyberdemn@gmail.com> writes:
> I think I should add my two cents.
> We at Zalando are using JWT tokens as passwords. JWT tokens are
> self-contained and therefore quite huge (up to 700-800 bytes in our
> case). Tokens have a limited lifetime (1 hour) and we are using PAM to
> verify them.
> Altogether the whole thing works like a charm. The only problem that
> it is not possible to copy&paste the token into psql password prompt,
> but there is a workaround, export PGPASSWORD=verylongtokenstring &&
> psql
I remain unconvinced that this is a good design, as compared to the
alternative of hashing $large_secret_data down to a more typical
length for a password.
Quite aside from whether or not you run into any implementation
restrictions on password length, using externally-sourced secret
data directly as a password seems like a lousy idea from a pure
security standpoint. What happens if somebody compromises your
database, or even just your connection to the database, and is
able to read out the raw password? The damage is worse than the
ordinary case of just being able to get into your database account,
because now the attacker has info about a formerly-secure upstream
datum, which probably lets him into some other things. It's not
unlike using the same password across multiple services.
In the case you describe, you're also exposing that data to wherever
the PAM mechanism is keeping its secrets, hence presenting an even
larger attack surface. Hashing the data before it goes to any of
those places would go a long way towards mitigating the risk.
regards, tom lane
On Mon, Jan 20, 2020 at 09:17:47PM +0100, Alexander Kukushkin wrote:
> Hi,
>
> I think I should add my two cents.
>
> On Mon, 20 Jan 2020 at 20:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > > I found another place that assumes 100 bytes and upped it to 2048.
>
> There one more place, in the code which is parsing .pgpass
What I found that seems like it might be related was on line 6900 of
src/interfaces/libpq/fe-connect.c (passwordFromFile):
#define LINELEN NAMEDATALEN*5
which is 315 (63*5) by default and isn't 100 on any sane setup. What
did I miss?
In any case, having the lengths be different in different places
seems sub-optimal. PGPASSWORD is just a const char *, so could be
quite long. The password prompted for by psql can be up to 100 bytes,
and the one read from .pgpass is bounded from above by
315
- 4 (colons)
- 4 (shortest possible hostname)
- 4 (usual port number)
- 1 (shortest db name)
- 1 (shortest possible username)
-------------------------------
301
> > So this is pretty much exactly what I expected. And have you tried
> > it with e.g. PAM, or LDAP?
> >
> > I think the AWS guys are fools to imagine that this will work in very
> > many places, and I don't see why we should be leading the charge to
> > make it work for them. What's the point of having a huge amount of
> > data in a password, anyway?
>
> We at Zalando are using JWT tokens as passwords. JWT tokens are
> self-contained and therefore quite huge (up to 700-800 bytes in our
> case). Tokens have a limited lifetime (1 hour) and we are using PAM to
> verify them.
> Altogether the whole thing works like a charm. The only problem that
> it is not possible to copy&paste the token into psql password prompt,
> but there is a workaround, export PGPASSWORD=verylongtokenstring &&
> psql
>
> JWT: https://jwt.io/
> PAM module to verify OAuth tokens: https://github.com/CyberDem0n/pam-oauth2
This reminds me of a patch that implemented PGPASSCOMMAND.
https://www.postgresql.org/message-id/flat/CAE35ztOGZqgwae3mBA%3DL97pSg3kvin2xycQh%3Dir%3D5NiwCApiYQ%40mail.gmail.com
Discussion of that seems to have trailed off, though. My thought on
that was that it was making a decision about the presence of both a
.pgpass file and a PGPASSCOMMAND setting that it shouldn't have made,
i.e. it decided which took precedence. I think it should fail when
presented with both, as there's not a single right answer that will
cover all cases.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
On 2020/01/21 4:21, David Fetter wrote: > On Mon, Jan 20, 2020 at 07:44:25PM +0100, David Fetter wrote: >> On Mon, Jan 20, 2020 at 01:12:35PM -0500, Tom Lane wrote: >>> David Fetter <david@fetter.org> writes: >>>> At least two cloud providers are now stuffing large amounts of >>>> information into the password field. This change makes it possible to >>>> accommodate that usage in interactive sessions. >>> >>> Like who? >> >> AWS and Azure are two examples I know of. >> >>> It seems like a completely silly idea. And if 2K is sane, why not >>> much more? >> >> Good question. Does it make sense to rearrange these things so they're >> allocated at runtime instead of compile time? >> >>> (I can't say that s/100/2048/ in one place is a particularly evil >>> change; what bothers me is the likelihood that there are other >>> places that won't cope with arbitrarily long passwords. Not all of >>> them are necessarily under our control, either.) >> >> I found one that is, so please find attached the next revision of the >> patch. > > I found another place that assumes 100 bytes and upped it to 2048. There are other places that 100 bytes password length is assumed. It's better to check the 0001 patch that posted in the following thread. https://www.postgresql.org/message-id/09512C4F-8CB9-4021-B455-EF4C4F0D55A0@amazon.com I have no strong opinion about the maximum length of password, for now. But IMO it's worth committing that 0001 patch as the first step for this problem. Also IMO the more problematic thing is that psql silently truncates the password specified in the prompt into 99B if its length is more than 99B. I think that psql should emit a warning in this case so that users can notice that. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
On Tue, Jan 21, 2020 at 02:42:07PM +0900, Fujii Masao wrote: > I have no strong opinion about the maximum length of password, > for now. But IMO it's worth committing that 0001 patch as the first step > for this problem. > > Also IMO the more problematic thing is that psql silently truncates > the password specified in the prompt into 99B if its length is > more than 99B. I think that psql should emit a warning in this case > so that users can notice that. I think we should be using a macro to define the maximum length, rather than have 100 used in various places. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
On Tue, Jan 21, 2020 at 10:12:52AM -0500, Bruce Momjian wrote: > On Tue, Jan 21, 2020 at 02:42:07PM +0900, Fujii Masao wrote: > > I have no strong opinion about the maximum length of password, > > for now. But IMO it's worth committing that 0001 patch as the first step > > for this problem. > > > > Also IMO the more problematic thing is that psql silently truncates > > the password specified in the prompt into 99B if its length is > > more than 99B. I think that psql should emit a warning in this case > > so that users can notice that. > > I think we should be using a macro to define the maximum length, rather > than have 100 used in various places. It's not just 100 in some places. It's different in different places, which goes to your point. How about using a system that doesn't meaningfully impose a maximum length? The shell variable is a const char *, so why not just re(p)alloc as needed? Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Tue, Jan 21, 2020 at 04:19:13PM +0100, David Fetter wrote: > On Tue, Jan 21, 2020 at 10:12:52AM -0500, Bruce Momjian wrote: > > I think we should be using a macro to define the maximum length, rather > > than have 100 used in various places. > > It's not just 100 in some places. It's different in different places, > which goes to your point. > > How about using a system that doesn't meaningfully impose a maximum > length? The shell variable is a const char *, so why not just > re(p)alloc as needed? Uh, how do you know how big to make the buffer that receives the read? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
On Tue, Jan 21, 2020 at 10:23:59AM -0500, Bruce Momjian wrote:
> On Tue, Jan 21, 2020 at 04:19:13PM +0100, David Fetter wrote:
> > On Tue, Jan 21, 2020 at 10:12:52AM -0500, Bruce Momjian wrote:
> > > I think we should be using a macro to define the maximum length, rather
> > > than have 100 used in various places.
> >
> > It's not just 100 in some places. It's different in different places,
> > which goes to your point.
> >
> > How about using a system that doesn't meaningfully impose a maximum
> > length? The shell variable is a const char *, so why not just
> > re(p)alloc as needed?
>
> Uh, how do you know how big to make the buffer that receives the read?
You can start at any size, possibly even 100, and then increase the
size in a loop along the lines of (untested)
my_size = 100;
my_buf = char[my_size];
curr_size = 0;
while (c = getchar() != '\0')
{
my_buf[curr_size++] = c;
if (curr_size == my_size) /* If we want an absolute maximum,
this'd be the place to test for it.
*/
{
my_size *= 2;
repalloc(my_buf, my_size);
}
}
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
On Tue, Jan 21, 2020 at 07:05:47PM +0100, David Fetter wrote:
> On Tue, Jan 21, 2020 at 10:23:59AM -0500, Bruce Momjian wrote:
> > On Tue, Jan 21, 2020 at 04:19:13PM +0100, David Fetter wrote:
> > > On Tue, Jan 21, 2020 at 10:12:52AM -0500, Bruce Momjian wrote:
> > > > I think we should be using a macro to define the maximum length, rather
> > > > than have 100 used in various places.
> > >
> > > It's not just 100 in some places. It's different in different places,
> > > which goes to your point.
> > >
> > > How about using a system that doesn't meaningfully impose a maximum
> > > length? The shell variable is a const char *, so why not just
> > > re(p)alloc as needed?
> >
> > Uh, how do you know how big to make the buffer that receives the read?
>
> You can start at any size, possibly even 100, and then increase the
> size in a loop along the lines of (untested)
[and unworkable]
I should have tested the code, but my point about using rep?alloc()
remains.
Best,
David.
Working code:
int main(int argc, char **argv)
{
size_t my_size = 2,
curr_size = 0;
char *buf;
int c;
buf = (char *) malloc(my_size);
printf("Enter a nice, long string.\n");
while( (c = getchar()) != '\0' )
{
buf[curr_size++] = c;
if (curr_size == my_size)
{
my_size *= 2;
buf = (char *) realloc(buf, my_size);
}
}
printf("The string %s is %zu bytes long.\n", buf, curr_size);
}
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
On 2020/01/22 0:12, Bruce Momjian wrote: > On Tue, Jan 21, 2020 at 02:42:07PM +0900, Fujii Masao wrote: >> I have no strong opinion about the maximum length of password, >> for now. But IMO it's worth committing that 0001 patch as the first step >> for this problem. >> >> Also IMO the more problematic thing is that psql silently truncates >> the password specified in the prompt into 99B if its length is >> more than 99B. I think that psql should emit a warning in this case >> so that users can notice that. > > I think we should be using a macro to define the maximum length, rather > than have 100 used in various places. +1 as the first step for this issue. The patch that I mentioned upthread actually does that. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
On 2020/01/22 9:41, David Fetter wrote: > On Tue, Jan 21, 2020 at 07:05:47PM +0100, David Fetter wrote: >> On Tue, Jan 21, 2020 at 10:23:59AM -0500, Bruce Momjian wrote: >>> On Tue, Jan 21, 2020 at 04:19:13PM +0100, David Fetter wrote: >>>> On Tue, Jan 21, 2020 at 10:12:52AM -0500, Bruce Momjian wrote: >>>>> I think we should be using a macro to define the maximum length, rather >>>>> than have 100 used in various places. >>>> >>>> It's not just 100 in some places. It's different in different places, >>>> which goes to your point. >>>> >>>> How about using a system that doesn't meaningfully impose a maximum >>>> length? The shell variable is a const char *, so why not just >>>> re(p)alloc as needed? >>> >>> Uh, how do you know how big to make the buffer that receives the read? >> >> You can start at any size, possibly even 100, and then increase the >> size in a loop along the lines of (untested) That's possible, but I like having the (reasonable) upper limit on that rather than arbitrary size. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
> On 22 Jan 2020, at 01:41, David Fetter <david@fetter.org> wrote: > > On Tue, Jan 21, 2020 at 07:05:47PM +0100, David Fetter wrote: >> On Tue, Jan 21, 2020 at 10:23:59AM -0500, Bruce Momjian wrote: >>> On Tue, Jan 21, 2020 at 04:19:13PM +0100, David Fetter wrote: >>>> On Tue, Jan 21, 2020 at 10:12:52AM -0500, Bruce Momjian wrote: >>>>> I think we should be using a macro to define the maximum length, rather >>>>> than have 100 used in various places. >>>> >>>> It's not just 100 in some places. It's different in different places, >>>> which goes to your point. >>>> >>>> How about using a system that doesn't meaningfully impose a maximum >>>> length? The shell variable is a const char *, so why not just >>>> re(p)alloc as needed? >>> >>> Uh, how do you know how big to make the buffer that receives the read? >> >> You can start at any size, possibly even 100, and then increase the >> size in a loop along the lines of (untested) It doesn't seem like a terribly safe pattern to have the client decide the read buffer without disclosing the size, and have the server resize the input buffer to an arbitrary size as input comes in. > my_size *= 2; > buf = (char *) realloc(buf, my_size); I know it's just example code, but using buf as the input to realloc like this risks a memleak when realloc fails as the original buf pointer is overwritten. Using a temporary pointer for ther returnvalue avoids that, which is how pg_repalloc and repalloc does it. cheers ./daniel
On 2020/01/22 11:01, Fujii Masao wrote: > > > On 2020/01/22 0:12, Bruce Momjian wrote: >> On Tue, Jan 21, 2020 at 02:42:07PM +0900, Fujii Masao wrote: >>> I have no strong opinion about the maximum length of password, >>> for now. But IMO it's worth committing that 0001 patch as the first step >>> for this problem. >>> >>> Also IMO the more problematic thing is that psql silently truncates >>> the password specified in the prompt into 99B if its length is >>> more than 99B. I think that psql should emit a warning in this case >>> so that users can notice that. >> >> I think we should be using a macro to define the maximum length, rather >> than have 100 used in various places. > > +1 as the first step for this issue. The patch that I mentioned > upthread actually does that. Attached is the patch that Nathan proposed at [1] and I think that it's worth applying. I'd like to add this to next CommitFest. Thought? [1] https://www.postgresql.org/message-id/09512C4F-8CB9-4021-B455-EF4C4F0D55A0@amazon.com Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
Вложения
Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> Attached is the patch that Nathan proposed at [1] and I think that
> it's worth applying. I'd like to add this to next CommitFest.
> Thought?
I can't get excited about this in the least. For any "normal" use of
passwords, 100 bytes is surely far more than sufficient. Furthermore,
if there is someone out there for whom it isn't sufficient, they're not
going to want to build custom versions of Postgres to change it.
If we think that longer passwords are actually a thing to be concerned
about, then what we need to do is change all these places to support
expansible buffers. I'm not taking a position on whether that's worth
the trouble ... but I do take the position that just inserting a
#define is a waste of time.
regards, tom lane