Re: [bug fix] pg_ctl fails with config-only directory

Hello,

I've found a bug and would like to fix it, but I cannot figure out how to do
that well. Could you give me any advice? I encountered this on PG 9.2, but
it will probably exist in later versions.

[Problem]
On Windows, a user with Administrator privileges can start the database
server. However, when he uses config-only directory, the database server
cannot be started. "pg_ctl start" fails with the following messages:

Execution of PostgreSQL by a user with administrative permissions is not
permitted.
The server must be started under an unprivileged user ID to prevent
possible system security compromises. See the documentation for
more information on how to properly start the server.

[Cause]
pg_ctl runs "postgres -C data_directory" to know the data directory. But
postgres cannot be run by a user with Administrator privileges, and displays
the above messages.

[Fix]
It is ideal that users with administrative privileges can start postgres,
with the Administrator privileges removed.

Currently, initdb and pg_ctl take trouble to invoke postgres in a process
with restricted privileges. I understand this improvement was done in 8.2
or 8.3 for convenience. The same convenience should be available when
running postgres directly, at least "postgres -C",
"postgres --describe-config", and "postgres --single".

Then, how can we do this? Which approach should we take?

* Approach 1
When postgres starts, it removes Administrator privileges from its own
process. But is this possible at all? Windows security API is complex and
provides many functions. It seems difficult to understand them. I'm afraid
it would take a long time to figure out the solution. Is there any good web
page to look at?

* Approach 2
Do not call check_root() on Windows when -C, --describe-config, or --single
is specified when running postgres. This would be easy, and should not be
dangerous in terms of security because attackers cannot get into the server
process via network.

I'll try to find a solution based on approach 1, but I doubt there's one.
If okay, I want to take approach 2. Could you give me your thoughts?

Regards
MauMau

On Wed, Dec 4, 2013 at 7:57 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> Hello,
>
> I've found a bug and would like to fix it, but I cannot figure out how to do
> that well. Could you give me any advice? I encountered this on PG 9.2, but
> it will probably exist in later versions.
>
> [Problem]
> On Windows, a user with Administrator privileges can start the database
> server. However, when he uses config-only directory, the database server
> cannot be started. "pg_ctl start" fails with the following messages:
>
> Execution of PostgreSQL by a user with administrative permissions is not
> permitted.
> The server must be started under an unprivileged user ID to prevent
> possible system security compromises. See the documentation for
> more information on how to properly start the server.
>
>
> [Cause]
> pg_ctl runs "postgres -C data_directory" to know the data directory. But
> postgres cannot be run by a user with Administrator privileges, and displays
> the above messages.
>
>
> [Fix]
> It is ideal that users with administrative privileges can start postgres,
> with the Administrator privileges removed.
>
> Currently, initdb and pg_ctl take trouble to invoke postgres in a process
> with restricted privileges. I understand this improvement was done in 8.2
> or 8.3 for convenience. The same convenience should be available when
> running postgres directly, at least "postgres -C", "postgres
> --describe-config", and "postgres --single".
>
> Then, how can we do this? Which approach should we take?
>
> * Approach 1
> When postgres starts, it removes Administrator privileges from its own
> process. But is this possible at all? Windows security API is complex and
> provides many functions. It seems difficult to understand them. I'm afraid
> it would take a long time to figure out the solution. Is there any good web
> page to look at?
>
> * Approach 2
> Do not call check_root() on Windows when -C, --describe-config, or --single
> is specified when running postgres. This would be easy, and should not be
> dangerous in terms of security because attackers cannot get into the server
> process via network.

Approach-2 has been discussed previously to resolve it and it doesn't seem to be
a good way to handle it. Please refer link:
http://www.postgresql.org/message-id/1339601668-sup-4658@alvh.no-ip.org

You can go through that mail chain and see if there can be a better
solution than Approach-2.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> On Wed, Dec 4, 2013 at 7:57 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>> * Approach 1
>> When postgres starts, it removes Administrator privileges from its own
>> process. But is this possible at all? Windows security API is complex
>> and
>> provides many functions. It seems difficult to understand them. I'm
>> afraid
>> it would take a long time to figure out the solution. Is there any good
>> web
>> page to look at?
>>
>> * Approach 2
>> Do not call check_root() on Windows when -C, --describe-config,
>> or --single
>> is specified when running postgres. This would be easy, and should not
>> be
>> dangerous in terms of security because attackers cannot get into the
>> server
>> process via network.
>
> Approach-2 has been discussed previously to resolve it and it doesn't seem
> to be
> a good way to handle it. Please refer link:
> http://www.postgresql.org/message-id/1339601668-sup-4658@alvh.no-ip.org
>
> You can go through that mail chain and see if there can be a better
> solution than Approach-2.

Thanks for the info. I understand your feeling, but we need to be
practical. I believe we should not leave a bug and inconvenience by
worrying about theory too much. In addition to the config-only directory,
the DBA with admin privs will naturally want to run "postgres -C" and
"postgres --describe-config", because they are useful and so described in
the manual. I don't see any (at least big) risk in allowing
postgres -C/--describe-config to run with admin privs. In addition, recent
Windows versions help to secure the system by revoking admin privs with UAC,
don't they? Disabling UAC is not recommended.

I couldn't find a way to let postgres delete its token groups from its own
primary access token. There doesn't seem to be a reasonably clean and good
way.

So I had to choose approach 2. Please find attached the patch. This simple
and not-complex change worked well. I'd like to add this to 2014-1
commitfest this weekend unless a better approach is proposed.

Regards
MauMau

On Thu, Dec 5, 2013 at 6:30 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>>
>> On Wed, Dec 4, 2013 at 7:57 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>>>
>>
>> Approach-2 has been discussed previously to resolve it and it doesn't seem
>> to be
>> a good way to handle it. Please refer link:
>> http://www.postgresql.org/message-id/1339601668-sup-4658@alvh.no-ip.org
>>
>> You can go through that mail chain and see if there can be a better
>> solution than Approach-2.
>
>
> Thanks for the info. I understand your feeling, but we need to be
> practical. I believe we should not leave a bug and inconvenience by
> worrying about theory too much. In addition to the config-only directory,
> the DBA with admin privs will naturally want to run "postgres -C" and
> "postgres --describe-config", because they are useful and so described in
> the manual. I don't see any (at least big) risk in allowing postgres
> -C/--describe-config to run with admin privs.

Today, I had again gone through all the discussion that happened at
that time related to this problem
and I found that later in discussion it was discussed something on
lines as your Approach-2,
please see the link
http://www.postgresql.org/message-id/503A879C.6070703@dunslane.net

> In addition, recent Windows
> versions help to secure the system by revoking admin privs with UAC, don't
> they? Disabling UAC is not recommended.
>
> I couldn't find a way to let postgres delete its token groups from its own
> primary access token. There doesn't seem to be a reasonably clean and good
> way.

Wouldn't the other way to resolve this problem be reinvoke pg_ctl in
non-restricted mode for the case in question?

> So I had to choose approach 2. Please find attached the patch. This simple
> and not-complex change worked well. I'd like to add this to 2014-1
> commitfest this weekend unless a better approach is proposed.

I think it is important to resolve this problem, so please godhead and
upload this patch to next CF.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> Today, I had again gone through all the discussion that happened at
> that time related to this problem
> and I found that later in discussion it was discussed something on
> lines as your Approach-2,
> please see the link
> http://www.postgresql.org/message-id/503A879C.6070703@dunslane.net

Thank you again. I'm a bit relieved to see similar discussion.

> Wouldn't the other way to resolve this problem be reinvoke pg_ctl in
> non-restricted mode for the case in question?

It's effectively the same as not checking admin privileges. And direct
invocation of postgres -C/--describe-config still cannot be helped.
> I think it is important to resolve this problem, so please godhead and
> upload this patch to next CF.

Thanks.

Regards
MauMau

On Sat, Dec 7, 2013 at 6:02 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>
>> Today, I had again gone through all the discussion that happened at
>> that time related to this problem
>> and I found that later in discussion it was discussed something on
>> lines as your Approach-2,
>> please see the link
>> http://www.postgresql.org/message-id/503A879C.6070703@dunslane.net
>
>
> Thank you again. I'm a bit relieved to see similar discussion.
>
>
>
>> Wouldn't the other way to resolve this problem be reinvoke pg_ctl in
>> non-restricted mode for the case in question?
>
>
> It's effectively the same as not checking admin privileges. And direct
> invocation of postgres -C/--describe-config still cannot be helped.

Yeah, that is the only point of reinvoke pg_ctl in non-restricted
mode, that it should be only allowed through
pg_ctl, but not directly with postgres -C.
In anycase, I think now we have link for the discussion and patch
as well for other Approach, so if the consensus
is to modify postgres code such that we don't need to check for
admin privs for -C/--describe-config option, then the
patch proposed by you can be taken else the other patch can be used.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi,

personally I really dislike constructs like you used:

#ifdef WIN32
if (check_if_admin)
#endif
check_root(progname);

It is hard to read and may confuse editors. Can you rewrite it?

The rest looks fine to me.

Best regards,

--
Christian Kruse http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

From: "Christian Kruse" <christian(at)2ndQuadrant(dot)com>
> personally I really dislike constructs like you used:

Thanks for reviewing the patch. Fixed. I'll add this revised patch to the
CommitFest entry soon.

Regards
MauMau

Hi,

On 31/01/14 22:17, MauMau wrote:
> Thanks for reviewing the patch. Fixed. I'll add this revised patch to the
> CommitFest entry soon.

Looks fine for me. Set it to „waiting for commit.“

Best regards,

--
Christian Kruse http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 02/01/2014 12:28 PM, Christian Kruse wrote:
> On 31/01/14 22:17, MauMau wrote:
>> Thanks for reviewing the patch. Fixed. I'll add this revised patch to the
>> CommitFest entry soon.
>
> Looks fine for me. Set it to „waiting for commit.“

Hmm, why do this only on Windows? If "postgres -C" is safe enough to run
as Administrator on Windows, why not allow running it as root on Unix as
well? Even if there's no particular need to allow it as root on Unix,
fewer #ifdefs is good.

- Heikki

From: "Heikki Linnakangas" <hlinnakangas(at)vmware(dot)com>
> Hmm, why do this only on Windows? If "postgres -C" is safe enough to run
> as Administrator on Windows, why not allow running it as root on Unix as
> well? Even if there's no particular need to allow it as root on Unix,
> fewer #ifdefs is good.

Yes, I limited the change only to Windows, because I thought I might get get
objection against unnecessary change. Plus, --single should not be allowed
for root, because root cannot be the PostgreSQL user account.

Regards
MauMau

"MauMau" <maumau307(at)gmail(dot)com> writes:
> From: "Heikki Linnakangas" <hlinnakangas(at)vmware(dot)com>
>> Hmm, why do this only on Windows? If "postgres -C" is safe enough to run
>> as Administrator on Windows, why not allow running it as root on Unix as
>> well? Even if there's no particular need to allow it as root on Unix,
>> fewer #ifdefs is good.

> Yes, I limited the change only to Windows, because I thought I might get get
> objection against unnecessary change. Plus, --single should not be allowed
> for root, because root cannot be the PostgreSQL user account.

Indeed, and why would that not apply to Windows as well? AFAIK, inclusion
of --single in this list is just plain nuts. The most likely result of
running that way is creation of root-owned files inside $PGDATA, which
would cause havoc for later normally-privileged postmasters.

I will go and commit this, without the #ifdefs and without the --single
exclusion.

regards, tom lane

I wrote:
> I will go and commit this, without the #ifdefs and without the --single
> exclusion.

On closer inspection I realized that the switch parsing was still far too
risky, because it would treat "-C" in any word of the process command line
as a reason not to check for root. Quite aside from the fact that some of
those words might be switch arguments not switches, main.c is also the
front end for other operating modes that have switches unrelated to the
postmaster's switches. --boot mode doesn't have any -C switch today, but
it might do so tomorrow, and that would result in a hard-to-notice hole in
our root protections.

However, there is a reasonably simple way around that objection, which is
to only skip the root check if -C is the first switch. pg_ctl can easily
be changed to call it that way, and we're not really here to make -C easy
for root users to call manually, so I'm not too concerned about that
aspect of it. --describe-config is only accepted as the first switch
anyway, so there's no issue there either.

Committed with appropriate changes.

regards, tom lane