Thread: Minor issues in .pgpass

Hi,

When I was researching the maximum length of password in PostgreSQL
to answer the question from my customer, I found that there are two
minor issues in .pgpass file.

(1) If the length of a line in .pgpass file is larger than 319B,
        libpq silently treats each 319B in the line as a separate
        setting line.

(2) The document explains that a line beginning with # is treated
        as a comment in .pgpass. But as far as I read the code,
        there is no code doing such special handling. Whether a line
        begins with # or not, libpq just checks that the first token
        in the line match with the host. That is, if you try to connect
        to the host with the hostname beginning with #,
        it can match to the line beginning with # in .pgpass.

        Also if the length of that "comment" line is larger than 319B,
        the latter part of the line can be treated as valid setting.

You may think that these unexpected behaviors are not so harmful
in practice because "usually" the length of password setting line is
less than 319B and the hostname beginning with # is less likely to be
used. But the problem exists. And there are people who want to use
large password or to write a long comment (e.g., with multibyte
characters like Japanese) in .pgass, so these may be more harmful
in the near future.

For (1), I think that we should make libpq warn if the length of a line
is larger than 319B, and throw away the remaining part beginning from
320B position. Whether to enlarge the length of a line should be
a separate discussion, I think.

For (2), libpq should treat any lines beginning with # as comments.

I've not created the patch yet, but will do if we reach to
the consensus.

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

> On 21 Jan 2020, at 07:27, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

> For (2), libpq should treat any lines beginning with # as comments.

I haven't read the code to confirm that it really isn't, but +1 on making it
so.  I can't see a reason for allowing a hostname to start with #, but allowing
comments does seem useful.

cheers ./daniel

On Tue, Jan 21, 2020 at 03:27:50PM +0900, Fujii Masao wrote:
> Hi,
> 
> When I was researching the maximum length of password in PostgreSQL
> to answer the question from my customer, I found that there are two
> minor issues in .pgpass file.
> 
> (1) If the length of a line in .pgpass file is larger than 319B,
>        libpq silently treats each 319B in the line as a separate
>        setting line.

This seems like a potentially serious bug. For example, a truncated
password could get retried enough times to raise intruder alarms, and
it wouldn't be easy to track down.

> (2) The document explains that a line beginning with # is treated
>        as a comment in .pgpass. But as far as I read the code,
>        there is no code doing such special handling.

This is a flat-out bug, as it violates a promise the documentation has
made.

>        Also if the length of that "comment" line is larger than 319B,
>        the latter part of the line can be treated as valid setting.

> You may think that these unexpected behaviors are not so harmful
> in practice because "usually" the length of password setting line is
> less than 319B and the hostname beginning with # is less likely to be
> used. But the problem exists. And there are people who want to use
> large password or to write a long comment (e.g., with multibyte
> characters like Japanese) in .pgass, so these may be more harmful
> in the near future.
> 
> For (1), I think that we should make libpq warn if the length of a line
> is larger than 319B, and throw away the remaining part beginning from
> 320B position. Whether to enlarge the length of a line should be
> a separate discussion, I think.

Agreed.

> For (2), libpq should treat any lines beginning with # as comments.

Would it make sense for lines starting with whitespace and then # to
be treated as comments, too, e.g.:

 # Please don't treat this as a parameter

?

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/22 9:06, David Fetter wrote:
> On Tue, Jan 21, 2020 at 03:27:50PM +0900, Fujii Masao wrote:
>> Hi,
>>
>> When I was researching the maximum length of password in PostgreSQL
>> to answer the question from my customer, I found that there are two
>> minor issues in .pgpass file.
>>
>> (1) If the length of a line in .pgpass file is larger than 319B,
>>         libpq silently treats each 319B in the line as a separate
>>         setting line.
> 
> This seems like a potentially serious bug. For example, a truncated
> password could get retried enough times to raise intruder alarms, and
> it wouldn't be easy to track down.
> 
>> (2) The document explains that a line beginning with # is treated
>>         as a comment in .pgpass. But as far as I read the code,
>>         there is no code doing such special handling.
> 
> This is a flat-out bug, as it violates a promise the documentation has
> made.
> 
>>         Also if the length of that "comment" line is larger than 319B,
>>         the latter part of the line can be treated as valid setting.
> 
>> You may think that these unexpected behaviors are not so harmful
>> in practice because "usually" the length of password setting line is
>> less than 319B and the hostname beginning with # is less likely to be
>> used. But the problem exists. And there are people who want to use
>> large password or to write a long comment (e.g., with multibyte
>> characters like Japanese) in .pgass, so these may be more harmful
>> in the near future.
>>
>> For (1), I think that we should make libpq warn if the length of a line
>> is larger than 319B, and throw away the remaining part beginning from
>> 320B position. Whether to enlarge the length of a line should be
>> a separate discussion, I think.
> 
> Agreed.
> 
>> For (2), libpq should treat any lines beginning with # as comments.

Patch attached. This patch does the above (1) and (2).

> Would it make sense for lines starting with whitespace and then # to
> be treated as comments, too, e.g.:

Could you tell me why you want to treat such a line as comment?
Basically I don't want to change the existing rules for parsing
.pgpass file more thane necessary.

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low, but it is still
betterto fix this problem.
 

I've not tested the patch in any detail, however, there are a couple of comments I have before I proceed on with
detailedtesting.
 

1. pgindent is showing a few issues with formatting. Please have a look and resolve those.
2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables. Also, I would
choosea more appropriate name for "tmp" variable.
 

I believe if you move the following lines before the conditional statement and simply and change the if statement to
"if(len >= sizeof(buf) - 1)", it will serve the purpose.
 
========================================
/* strip trailing newline and carriage return */
len = pg_strip_crlf(buf);

if (len == 0)
    continue;
========================================

So, the patch should look like this in my opinion (ignore the formatting issues as this is just to give you an idea of
whatI mean):
 

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 408000a..6ca262f 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -6949,6 +6949,7 @@ passwordFromFile(const char *hostname, const char *port, const char *dbname,
 {
        FILE       *fp;
        struct stat stat_buf;
+       int             line_number = 0;
 
 #define LINELEN NAMEDATALEN*5
        char            buf[LINELEN];
@@ -7018,10 +7019,40 @@ passwordFromFile(const char *hostname, const char *port, const char *dbname,
                if (fgets(buf, sizeof(buf), fp) == NULL)
                        break;
 
-               /* strip trailing newline and carriage return */
-               len = pg_strip_crlf(buf);
+               line_number++;
 
-               if (len == 0)
+                /* strip trailing newline and carriage return */
+                len = pg_strip_crlf(buf);
+
+                if (len == 0)
+                        continue;
+
+               if (len >= sizeof(buf) - 1)
+               {
+                       char    tmp[LINELEN];
+
+                       /*
+                        * Warn if this password setting line is too long,
+                        * because it's unexpectedly truncated.
+                        */
+                       if (buf[0] != '#')
+                               fprintf(stderr,
+                                               libpq_gettext("WARNING: line %d too long in password file \"%s\"\n"),
+                                               line_number, pgpassfile);
+
+                       /* eat rest of the line */
+                       while (!feof(fp) && !ferror(fp))
+                       {
+                               if (fgets(tmp, sizeof(tmp), fp) == NULL)
+                                       break;
+                               len = strlen(tmp);
+                               if (len < sizeof(tmp) -1 || tmp[len - 1] == '\n')
+                                       break;
+                       }
+               }
+
+               /* ignore comments */
+               if (buf[0] == '#')

---
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950  EMAIL: mailto:hamid.akhtar@highgo.ca
SKYPE: engineeredvirus

The new status of this patch is: Waiting on Author

On 2020/02/29 0:46, Hamid Akhtar wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:       not tested
> Spec compliant:           not tested
> Documentation:            not tested
> 
> First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low, but it is
stillbetter to fix this problem.
 
> 
> I've not tested the patch in any detail, however, there are a couple of comments I have before I proceed on with
detailedtesting.
 

Thanks for the review and comments!

> 1. pgindent is showing a few issues with formatting. Please have a look and resolve those.

Yes.

> 2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables.

Basically I don't want to use the same variable for several purposes
because which would decrease the code readability.

> Also, I would choose a more appropriate name for "tmp" variable.

Yeah, so what about "rest" as the variable name?

> I believe if you move the following lines before the conditional statement and simply and change the if statement to
"if(len >= sizeof(buf) - 1)", it will serve the purpose.
 

ISTM that this doesn't work correctly when the "buf" contains
trailing carriage returns but not newlines (i.e., this line is too long
so the "buf" doesn't include newline). In this case, pg_strip_crlf()
shorten the "buf" and then its return value "len" should become
less than sizeof(buf). So the following condition always becomes
false unexpectedly in that case even though there is still rest of
the line to eat.

> +               if (len >= sizeof(buf) - 1)
> +               {
> +                       char    tmp[LINELEN];

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On 2020/03/03 21:38, Hamid Akhtar wrote:
> 
> 
> On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>> wrote:
> 
> 
> 
>     On 2020/02/29 0:46, Hamid Akhtar wrote:
>      > The following review has been posted through the commitfest application:
>      > make installcheck-world:  not tested
>      > Implements feature:       not tested
>      > Spec compliant:           not tested
>      > Documentation:            not tested
>      >
>      > First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low, but it
isstill better to fix this problem.
 
>      >
>      > I've not tested the patch in any detail, however, there are a couple of comments I have before I proceed on
withdetailed testing.
 
> 
>     Thanks for the review and comments!
> 
>      > 1. pgindent is showing a few issues with formatting. Please have a look and resolve those.
> 
>     Yes.
> 
>      > 2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables.
> 
>     Basically I don't want to use the same variable for several purposes
>     because which would decrease the code readability.
> 
>      > Also, I would choose a more appropriate name for "tmp" variable.
> 
>     Yeah, so what about "rest" as the variable name?
> 
>      > I believe if you move the following lines before the conditional statement and simply and change the if
statementto "if (len >= sizeof(buf) - 1)", it will serve the purpose.
 
> 
>     ISTM that this doesn't work correctly when the "buf" contains
>     trailing carriage returns but not newlines (i.e., this line is too long
>     so the "buf" doesn't include newline). In this case, pg_strip_crlf()
>     shorten the "buf" and then its return value "len" should become
>     less than sizeof(buf). So the following condition always becomes
>     false unexpectedly in that case even though there is still rest of
>     the line to eat.
> 
> 
> Per code comments for pg_strip_crlf:
> "pg_strip_crlf -- Remove any trailing newline and carriage return"
> If the buf read contains a newline or a carriage return at the end, then clearly the line
> is not exceeding the sizeof(buf).

No if the length of the setting line exceeds sizeof(buf) and
the buf contains only a carriage return at the end and not newline.
This case can happen because fgets() stops reading when a newline
(not a carriage return) is found. Normal users are very unlikely to
add a carriage return into the middle of the pgpass setting line
in practice, though. But IMO the code should handle even this
case because it *can* happen, if the code is not so complicated.

Regards,


-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On 2020/03/03 22:07, Hamid Akhtar wrote:
> On Tue, Mar 3, 2020 at 5:38 PM Hamid Akhtar <hamid.akhtar@gmail.com <mailto:hamid.akhtar@gmail.com>> wrote:
> 
> 
> 
>     On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>>
wrote:
> 
> 
> 
>         On 2020/02/29 0:46, Hamid Akhtar wrote:
>          > The following review has been posted through the commitfest application:
>          > make installcheck-world:  not tested
>          > Implements feature:       not tested
>          > Spec compliant:           not tested
>          > Documentation:            not tested
>          >
>          > First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low,
butit is still better to fix this problem.
 
>          >
>          > I've not tested the patch in any detail, however, there are a couple of comments I have before I proceed
onwith detailed testing.
 
> 
>         Thanks for the review and comments!
> 
>          > 1. pgindent is showing a few issues with formatting. Please have a look and resolve those.
> 
>         Yes.

Fixed. Attached is the updated version of the patch.
I marked this CF entry as "Needs Review" again.

>          > 2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables.
> 
>         Basically I don't want to use the same variable for several purposes
>         because which would decrease the code readability.
> 
> 
> That is fine.
> 
> 
>          > Also, I would choose a more appropriate name for "tmp" variable.
> 
>         Yeah, so what about "rest" as the variable name?
> 
> 
> May be something like "excess_buf" or any other one that describes that these bytes are to be discarded.

Thanks for the comment! But IMO that "rest" is not
so bad choice, so for now I used "rest" in the latest patch.

Regards,


-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On 2020/03/04 20:39, Hamid Akhtar wrote:
> 
> 
> On Tue, Mar 3, 2020 at 8:57 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>> wrote:
> 
> 
> 
>     On 2020/03/03 21:38, Hamid Akhtar wrote:
>      >
>      >
>      > On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>
<mailto:masao.fujii@oss.nttdata.com<mailto:masao.fujii@oss.nttdata.com>>> wrote:
 
>      >
>      >
>      >
>      >     On 2020/02/29 0:46, Hamid Akhtar wrote:
>      >      > The following review has been posted through the commitfest application:
>      >      > make installcheck-world:  not tested
>      >      > Implements feature:       not tested
>      >      > Spec compliant:           not tested
>      >      > Documentation:            not tested
>      >      >
>      >      > First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low,
butit is still better to fix this problem.
 
>      >      >
>      >      > I've not tested the patch in any detail, however, there are a couple of comments I have before I
proceedon with detailed testing.
 
>      >
>      >     Thanks for the review and comments!
>      >
>      >      > 1. pgindent is showing a few issues with formatting. Please have a look and resolve those.
>      >
>      >     Yes.
>      >
>      >      > 2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen"
variables.
>      >
>      >     Basically I don't want to use the same variable for several purposes
>      >     because which would decrease the code readability.
>      >
>      >      > Also, I would choose a more appropriate name for "tmp" variable.
>      >
>      >     Yeah, so what about "rest" as the variable name?
>      >
>      >      > I believe if you move the following lines before the conditional statement and simply and change the if
statementto "if (len >= sizeof(buf) - 1)", it will serve the purpose.
 
>      >
>      >     ISTM that this doesn't work correctly when the "buf" contains
>      >     trailing carriage returns but not newlines (i.e., this line is too long
>      >     so the "buf" doesn't include newline). In this case, pg_strip_crlf()
>      >     shorten the "buf" and then its return value "len" should become
>      >     less than sizeof(buf). So the following condition always becomes
>      >     false unexpectedly in that case even though there is still rest of
>      >     the line to eat.
>      >
>      >
>      > Per code comments for pg_strip_crlf:
>      > "pg_strip_crlf -- Remove any trailing newline and carriage return"
>      > If the buf read contains a newline or a carriage return at the end, then clearly the line
>      > is not exceeding the sizeof(buf).
> 
>     No if the length of the setting line exceeds sizeof(buf) and
>     the buf contains only a carriage return at the end and not newline.
>     This case can happen because fgets() stops reading when a newline
>     (not a carriage return) is found. Normal users are very unlikely to
>     add a carriage return into the middle of the pgpass setting line
>     in practice, though. But IMO the code should handle even this
>     case because it *can* happen, if the code is not so complicated.
> 
> 
> I'm not sure if I understand your comment here. From the code of pg_strip_crlf
> I see that it is handling both carriage return and/or new line at the end of a
> string:

So if "buf" contains a carriage return at the end, it's removed and
the "len" that pg_strip_crlf() returns obviously should be smaller
than sizeof(buf). This causes the following condition that you
proposed as follows to always be false (i.e., len < sizeof(buf) - 1)
even when there are still rest of line. So we cannot eat rest of
the line even though it exists. I'm missing something?

+               if (len >= sizeof(buf) - 1)
+               {
+                       char    tmp[LINELEN];
+
+                       /*
+                        * Warn if this password setting line is too long,
+                        * because it's unexpectedly truncated.
+                        */
+                       if (buf[0] != '#')
+                               fprintf(stderr,
+                                               libpq_gettext("WARNING: line %d too long in password file \"%s\"\n"),
+                                               line_number, pgpassfile);
+
+                       /* eat rest of the line */
+                       while (!feof(fp) && !ferror(fp))

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

Tested and looks fine to me.

The new status of this patch is: Ready for Committer

On 2020/03/04 23:01, Hamid Akhtar wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            tested, passed
> 
> Tested and looks fine to me.
> 
> The new status of this patch is: Ready for Committer

Many thanks for testing and reviewing the patch!
I pushed it.

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters