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
Attachments:
v1-0001-Increase-psql-s-password-buffer-size.patchtext/x-diff; charset=us-asciiDownload
From 168c380548d1f97e4f6e9245851c22029931e8b5 Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Mon, 20 Jan 2020 09:58:19 -0800
Subject: [PATCH v1] Increase psql's password buffer size
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.24.1"
This is a multi-part message in MIME format.
--------------2.24.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
At least two cloud providers are now stuffing large amounts of
information into the password field. This makes it possible to
accommodate that usage in interactive sessions.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index e111cee556..84c2a0a37f 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2845,7 +2845,7 @@ copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
static char *
prompt_for_password(const char *username)
{
- char buf[100];
+ char buf[2048];
if (username == NULL || username[0] == '\0')
simple_prompt("Password: ", buf, sizeof(buf), false);
--------------2.24.1--
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
Attachments:
v2-0001-Increase-psql-s-password-buffer-size.patchtext/x-diff; charset=us-asciiDownload
From dfe72e1f7b3af646ba3d0bff017c9574eb54eb4c Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Mon, 20 Jan 2020 09:58:19 -0800
Subject: [PATCH v2] Increase psql's password buffer size
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.24.1"
This is a multi-part message in MIME format.
--------------2.24.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
At least two cloud providers are now stuffing large amounts of
information into the password field. This makes it possible to
accommodate that usage in interactive sessions.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index e111cee556..61386fe4ae 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1827,8 +1827,8 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
{
char *opt0 = psql_scan_slash_option(scan_state,
OT_SQLID, NULL, true);
- char pw1[100];
- char pw2[100];
+ char pw1[2048];
+ char pw2[2048];
simple_prompt("Enter new password: ", pw1, sizeof(pw1), false);
simple_prompt("Enter it again: ", pw2, sizeof(pw2), false);
@@ -2845,7 +2845,7 @@ copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
static char *
prompt_for_password(const char *username)
{
- char buf[100];
+ char buf[2048];
if (username == NULL || username[0] == '\0')
simple_prompt("Password: ", buf, sizeof(buf), false);
--------------2.24.1--
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
Attachments:
v3-0001-Increase-psql-s-password-buffer-size.patchtext/x-diff; charset=us-asciiDownload
From fb05bf709df0a67a63bca413cd7f0f276cab78b9 Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Mon, 20 Jan 2020 09:58:19 -0800
Subject: [PATCH v3] Increase psql's password buffer size
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.24.1"
This is a multi-part message in MIME format.
--------------2.24.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
At least two cloud providers are now stuffing large amounts of
information into the password field. This makes it possible to
accommodate that usage in interactive sessions.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index e111cee556..61386fe4ae 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1827,8 +1827,8 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
{
char *opt0 = psql_scan_slash_option(scan_state,
OT_SQLID, NULL, true);
- char pw1[100];
- char pw2[100];
+ char pw1[2048];
+ char pw2[2048];
simple_prompt("Enter new password: ", pw1, sizeof(pw1), false);
simple_prompt("Enter it again: ", pw2, sizeof(pw2), false);
@@ -2845,7 +2845,7 @@ copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
static char *
prompt_for_password(const char *username)
{
- char buf[100];
+ char buf[2048];
if (username == NULL || username[0] == '\0')
simple_prompt("Password: ", buf, sizeof(buf), false);
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 3302bd4dd3..a7e3263979 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -120,7 +120,7 @@ main(int argc, char *argv[])
struct adhoc_opts options;
int successResult;
bool have_password = false;
- char password[100];
+ char password[2048];
bool new_pass;
pg_logging_init(argv[0]);
--------------2.24.1--
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 &&
psqlJWT: https://jwt.io/
PAM module to verify OAuth tokens: https://github.com/CyberDem0n/pam-oauth2
This reminds me of a patch that implemented PGPASSCOMMAND.
/messages/by-id/CAE35ztOGZqgwae3mBA=L97pSg3kvin2xycQh=ir=5NiwCApiYQ@mail.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.
/messages/by-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]/messages/by-id/09512C4F-8CB9-4021-B455-EF4C4F0D55A0@amazon.com and I think that
it's worth applying. I'd like to add this to next CommitFest.
Thought?
[1]: /messages/by-id/09512C4F-8CB9-4021-B455-EF4C4F0D55A0@amazon.com
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
Attachments:
v1-0001-Refactor-maximum-password-length-enforced-by-clie.patchtext/plain; charset=UTF-8; name=v1-0001-Refactor-maximum-password-length-enforced-by-clie.patch; x-mac-creator=0; x-mac-type=0Download
From ba8fb45bfad75ebba9b22bfaf397d2647a416f8e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Fri, 12 Oct 2018 15:31:31 +0000
Subject: [PATCH v1 1/3] Refactor maximum password length enforced by client
utilities.
---
contrib/oid2name/oid2name.c | 2 +-
contrib/vacuumlo/vacuumlo.c | 2 +-
src/bin/initdb/initdb.c | 4 ++--
src/bin/pg_basebackup/streamutil.c | 2 +-
src/bin/pg_dump/pg_backup_db.c | 4 ++--
src/bin/pg_dump/pg_dumpall.c | 2 +-
src/bin/pgbench/pgbench.c | 2 +-
src/bin/psql/command.c | 6 +++---
src/bin/psql/startup.c | 2 +-
src/bin/scripts/common.c | 2 +-
src/bin/scripts/createuser.c | 4 ++--
src/common/saslprep.c | 4 ++--
src/include/postgres_fe.h | 11 +++++++++++
13 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index aa122ca0e9..08904ffd12 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -285,7 +285,7 @@ sql_conn(struct options *my_opts)
{
PGconn *conn;
bool have_password = false;
- char password[100];
+ char password[PROMPT_MAX_PASSWORD_LENGTH];
bool new_pass;
PGresult *res;
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index 3075781abe..5acfa65289 100644
--- a/contrib/vacuumlo/vacuumlo.c
+++ b/contrib/vacuumlo/vacuumlo.c
@@ -70,7 +70,7 @@ vacuumlo(const char *database, const struct _param *param)
bool new_pass;
bool success = true;
static bool have_password = false;
- static char password[100];
+ static char password[PROMPT_MAX_PASSWORD_LENGTH];
/* Note: password can be carried over from a previous call */
if (param->pg_prompt == TRI_YES && !have_password)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index ab5cb7f0c1..a9663531de 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -1528,8 +1528,8 @@ setup_auth(FILE *cmdfd)
static void
get_su_pwd(void)
{
- char pwd1[100];
- char pwd2[100];
+ char pwd1[PROMPT_MAX_PASSWORD_LENGTH];
+ char pwd2[PROMPT_MAX_PASSWORD_LENGTH];
if (pwprompt)
{
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 52f1e559b7..8ecb412ab8 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -51,7 +51,7 @@ char *dbport = NULL;
char *dbname = NULL;
int dbgetpassword = 0; /* 0=auto, -1=never, 1=always */
static bool have_password = false;
-static char password[100];
+static char password[PROMPT_MAX_PASSWORD_LENGTH];
PGconn *conn = NULL;
/*
diff --git a/src/bin/pg_dump/pg_backup_db.c b/src/bin/pg_dump/pg_backup_db.c
index 5e32ee8a5b..402ee3cff1 100644
--- a/src/bin/pg_dump/pg_backup_db.c
+++ b/src/bin/pg_dump/pg_backup_db.c
@@ -126,7 +126,7 @@ _connectDB(ArchiveHandle *AH, const char *reqdb, const char *requser)
const char *newdb;
const char *newuser;
char *password;
- char passbuf[100];
+ char passbuf[PROMPT_MAX_PASSWORD_LENGTH];
bool new_pass;
if (!reqdb)
@@ -246,7 +246,7 @@ ConnectDatabase(Archive *AHX,
{
ArchiveHandle *AH = (ArchiveHandle *) AHX;
char *password;
- char passbuf[100];
+ char passbuf[PROMPT_MAX_PASSWORD_LENGTH];
bool new_pass;
if (AH->connection)
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 5176626476..0521f8c700 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -1536,7 +1536,7 @@ connectDatabase(const char *dbname, const char *connection_string,
const char **values = NULL;
PQconninfoOption *conn_opts = NULL;
static bool have_password = false;
- static char password[100];
+ static char password[PROMPT_MAX_PASSWORD_LENGTH];
if (prompt_password == TRI_YES && !have_password)
{
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 436764b9c9..097666169b 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1179,7 +1179,7 @@ doConnect(void)
PGconn *conn;
bool new_pass;
static bool have_password = false;
- static char password[100];
+ static char password[PROMPT_MAX_PASSWORD_LENGTH];
/*
* Start the connection. Loop until we have a password if requested by
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 5b4d54a442..879fa1f494 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1790,8 +1790,8 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
{
char *opt0 = psql_scan_slash_option(scan_state,
OT_SQLID, NULL, true);
- char pw1[100];
- char pw2[100];
+ char pw1[PROMPT_MAX_PASSWORD_LENGTH];
+ char pw2[PROMPT_MAX_PASSWORD_LENGTH];
simple_prompt("Enter new password: ", pw1, sizeof(pw1), false);
simple_prompt("Enter it again: ", pw2, sizeof(pw2), false);
@@ -2808,7 +2808,7 @@ copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
static char *
prompt_for_password(const char *username)
{
- char buf[100];
+ char buf[PROMPT_MAX_PASSWORD_LENGTH];
if (username == NULL || username[0] == '\0')
simple_prompt("Password: ", buf, sizeof(buf), false);
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index be57574cd3..dfc1377382 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -100,7 +100,7 @@ main(int argc, char *argv[])
struct adhoc_opts options;
int successResult;
bool have_password = false;
- char password[100];
+ char password[PROMPT_MAX_PASSWORD_LENGTH];
bool new_pass;
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("psql"));
diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c
index ba6120706d..0b31bf3672 100644
--- a/src/bin/scripts/common.c
+++ b/src/bin/scripts/common.c
@@ -73,7 +73,7 @@ connectDatabase(const char *dbname, const char *pghost,
PGconn *conn;
bool new_pass;
static bool have_password = false;
- static char password[100];
+ static char password[PROMPT_MAX_PASSWORD_LENGTH];
if (!allow_password_reuse)
have_password = false;
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 3420e62fdd..ced7386af8 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -66,7 +66,7 @@ main(int argc, char *argv[])
bool pwprompt = false;
char *newpassword = NULL;
char newuser_buf[128];
- char newpassword_buf[100];
+ char newpassword_buf[PROMPT_MAX_PASSWORD_LENGTH];
/* Tri-valued variables. */
enum trivalue createdb = TRI_DEFAULT,
@@ -201,7 +201,7 @@ main(int argc, char *argv[])
if (pwprompt)
{
- char pw2[100];
+ char pw2[PROMPT_MAX_PASSWORD_LENGTH];
simple_prompt("Enter password for new role: ",
newpassword_buf, sizeof(newpassword_buf), false);
diff --git a/src/common/saslprep.c b/src/common/saslprep.c
index 4cf574fed8..1f90df250d 100644
--- a/src/common/saslprep.c
+++ b/src/common/saslprep.c
@@ -39,7 +39,7 @@
* Limit on how large password's we will try to process. A password
* larger than this will be treated the same as out-of-memory.
*/
-#define MAX_PASSWORD_LENGTH 1024
+#define SASLPREP_MAX_PASSWORD_LENGTH 1024
/*
* In backend, we will use palloc/pfree. In frontend, use malloc, and
@@ -1085,7 +1085,7 @@ pg_saslprep(const char *input, char **output)
*output = NULL;
/* Check that the password isn't stupendously long */
- if (strlen(input) > MAX_PASSWORD_LENGTH)
+ if (strlen(input) > SASLPREP_MAX_PASSWORD_LENGTH)
{
#ifndef FRONTEND
ereport(ERROR,
diff --git a/src/include/postgres_fe.h b/src/include/postgres_fe.h
index 14567953f2..0c9293f942 100644
--- a/src/include/postgres_fe.h
+++ b/src/include/postgres_fe.h
@@ -26,4 +26,15 @@
#include "common/fe_memutils.h"
+/*
+ * Limit on the length of passwords we will try to process. Note that this does
+ * not restrict the creation of longer passwords via commands such as CREATE
+ * ROLE and ALTER ROLE. It merely restricts the length of passwords accepted by
+ * client utility prompts (e.g. psql).
+ *
+ * One byte is reserved at the end for the '\0' byte, so the user-facing maximum
+ * length is actually 99.
+ */
+#define PROMPT_MAX_PASSWORD_LENGTH 100
+
#endif /* POSTGRES_FE_H */
--
2.16.2
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