BUG #10680: LDAP bind password leaks to log on failed authentication

Greetings,

* smsiebe@gmail.com (smsiebe@gmail.com) wrote:

When a user fails to login when the LDAP method is used, the ldapbindpasswd
(in plain text) is leaked to the log, even when the log level is set to
warning.

If you don't want the server to see the user's password, don't use LDAP
authentication. A much better approach is Kerberos or client-side SSL
certificates.

There may be something which is done to improve the specific case
mentioned here (or perhaps not..), but if LDAP is used then the PG
server will see the user's password because that's how that
authentication system works.

Thanks,

Stephen

Thanks for the reply.

If you don't want the server to see the user's password, don't use LDAP
authentication. A much better approach is Kerberos or client-side SSL
certificates.

Sadly, all other authentication options will not work for us.

I'm not seeing the user password in the log, I'm seeing the bind
password (ldapbindpasswd) that in the pg_hba.conf file. There is a
line in auth.c that, on every failed attempt, prints the full (raw)
configuration line to the log at all log levels. So, this isn't just
a problem with LDAP (with ldapbindpasswd) but also the RADIUS method
(radiussecret).

I've submitted a patch and we're discussing the problem further on the
pgsql-hackers distro. Really, I think it all comes down to finding
the right balance of security and convenience of the administrator.
I'm hopeful we'll come up with the right answer soon and I can submit
a new patch.

S

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Steven,

* Steven Siebert (smsiebe@gmail.com) wrote:

If you don't want the server to see the user's password, don't use LDAP
authentication. A much better approach is Kerberos or client-side SSL
certificates.

Sadly, all other authentication options will not work for us.

I'm not seeing the user password in the log, I'm seeing the bind
password (ldapbindpasswd) that in the pg_hba.conf file. There is a
line in auth.c that, on every failed attempt, prints the full (raw)
configuration line to the log at all log levels. So, this isn't just
a problem with LDAP (with ldapbindpasswd) but also the RADIUS method
(radiussecret).

Ah, ok. Kerberos and SSL certs aren't immune to that problem, though
the secrets don't ever end up in the logs- but they still must be
visible to the server process in order. Of course, if you already
have access to the server process, there shouldn't be much to gain
from the Kerberos secret, the RADIUS secret, the SSL private key, or
the LDAP bind password..

I've submitted a patch and we're discussing the problem further on the
pgsql-hackers distro. Really, I think it all comes down to finding
the right balance of security and convenience of the administrator.
I'm hopeful we'll come up with the right answer soon and I can submit
a new patch.

Oh, yeah, I saw that discussion but hadn't quite put it together with
this bug report (somehow I saw the bug report after the hackers
discussion...).

Thanks,

Stephen

Ah, ok. Kerberos and SSL certs aren't immune to that problem, though
the secrets don't ever end up in the logs- but they still must be
visible to the server process in order. Of course, if you already
have access to the server process, there shouldn't be much to gain
from the Kerberos secret, the RADIUS secret, the SSL private key, or
the LDAP bind password..

Agreed. In our situation (government), though, we must export out
logs to enterprise logging services where auditors (that wouldn't
otherwise have access to the server/process) would be able to see it.

Despite the arguments of it being in another file...generally, having
clear-text secrets copied around to multiple places is a bad thing. I
think it should be easy to come to compromise...and we're willing to
put in the work once we do figure out the best course of action =)

Thanks!

S

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Steven Siebert <smsiebe@gmail.com> writes:

Agreed. In our situation (government), though, we must export out
logs to enterprise logging services where auditors (that wouldn't
otherwise have access to the server/process) would be able to see it.

The thing is that the postmaster logs will certainly contain all manner
of sensitive information. A few examples:

* Occasionally, people mess up and enter their username as their password
and vice versa. Logging of connection failures, or indeed mere logging of
error messages, will therefore expose their password --- admittedly, not
identified as such, but if you see a subsequent successful connection you
know whose it was.

* Logging of queries is likely to expose sensitive user data in the form
of constants in the queries, eg "INSERT INTO customers (name, address,
credit_card_number) VALUES (...)". Even if you're not logging all
queries, failed queries could still expose such data.

* An example pretty directly connected to yours is that people have
complained about how statement logging will capture "ALTER USER joe
WITH PASSWORD 'joes-new-password'".

So basically, making the logs safe to show to untrusted auditors is a
fool's errand. You need to deal with this problem in some other,
nontechnical, way. IOW, why exactly don't you trust the auditors,
and how will you fix that?

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

* Occasionally, people mess up and enter their username as their password
and vice versa. Logging of connection failures, or indeed mere logging of
error messages, will therefore expose their password --- admittedly, not
identified as such, but if you see a subsequent successful connection you
know whose it was.

* Logging of queries is likely to expose sensitive user data in the form
of constants in the queries, eg "INSERT INTO customers (name, address,
credit_card_number) VALUES (...)". Even if you're not logging all
queries, failed queries could still expose such data.

* An example pretty directly connected to yours is that people have
complained about how statement logging will capture "ALTER USER joe
WITH PASSWORD 'joes-new-password'".

Sadly, we (devs/administrators) realize all these things. Big picture
logic plays no role in the way individuals develop the security
checklists or those accreditors that interpret those checklists. It's
very black-and-white...either the database supports xyz (in this case,
no clear-text passwords in the log) or it fails that check.

I can give you specific reasons why the points you made above (which
are quite good points) are not applicable to us (ie users don't
directly log into the database, we use service accounts and handle
user auth/authz with PKI at the application level....and we don't log
individual statements because auditing changes are being done in a
different, approved, manner)...but we're digging down to such a
specific corner case it gets silly to argue scenarios.

So basically, making the logs safe to show to untrusted auditors is a
fool's errand. You need to deal with this problem in some other,
nontechnical, way. IOW, why exactly don't you trust the auditors,
and how will you fix that?

Right. We (my team) agree. We think it's stupid. It doesn't matter
what we think. (Welcome to my world). I'm sorry if I seem frustrated,
it's not with you...it's purely with the situation we're in having to
deal with this ourselves.

Believe me, I really hate to look at it like this, but it comes down
to: is there anything we can do within postgres / the postgres
community to eliminate this one specific 'vulnerability'? Yes, we're
focusing on just the one vulnerability right now - where clear text
passwords are, arguably, *intentionally* sent into the log. It's
something that can be fixed...and you have a developer (me) willing to
fix it if given direction on how he should proceed.

There are currently three suggestions on a fix put forth already:
1) remove the raw line from the log entirely, just keeping the line number
2) log that one specific event containing the raw log at a lower log
level (ie debug)
3) parse out the password and continue to log the sanitized line at
the same "level" (all)

I'm OK with the fact that the patch I provided using the first
approach seems to be denied. Can we consider either approach 2, 3, or
perhaps a combination or 2/3?

I do have alternative means at my disposal (ie use flume, or something
similar, to filter out just the log events I'm interested in and
forward off)...but we wanted to be able to help those behind us that
had similar concerns by fixing it at the source of the 'problem'. I
want postgres to be unequivocally be approved software for the
government - not conditionally based on complex usages of 3rd party
applications to get it into an approved state.

S

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

* Steven Siebert (smsiebe@gmail.com) wrote:

There are currently three suggestions on a fix put forth already:
1) remove the raw line from the log entirely, just keeping the line number
2) log that one specific event containing the raw log at a lower log
level (ie debug)
3) parse out the password and continue to log the sanitized line at
the same "level" (all)

I'm OK with the fact that the patch I provided using the first
approach seems to be denied. Can we consider either approach 2, 3, or
perhaps a combination or 2/3?

I actually don't really see a huge problem with 1, but I need to go
review the thread in more detail...

I do have alternative means at my disposal (ie use flume, or something
similar, to filter out just the log events I'm interested in and
forward off)...but we wanted to be able to help those behind us that
had similar concerns by fixing it at the source of the 'problem'. I
want postgres to be unequivocally be approved software for the
government - not conditionally based on complex usages of 3rd party
applications to get it into an approved state.

Yeah, I tend to agree- mistakes and errors are different considerations
when it comes to auditing, etc.

Thanks,

Stephen

On Thu, Jun 19, 2014 at 5:37 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Steven Siebert (smsiebe@gmail.com) wrote:

There are currently three suggestions on a fix put forth already:
1) remove the raw line from the log entirely, just keeping the line

number

2) log that one specific event containing the raw log at a lower log
level (ie debug)
3) parse out the password and continue to log the sanitized line at
the same "level" (all)

I'm OK with the fact that the patch I provided using the first
approach seems to be denied. Can we consider either approach 2, 3, or
perhaps a combination or 2/3?

I actually don't really see a huge problem with 1, but I need to go
review the thread in more detail...

The reason the raw line was added in the first place was debugging cases
where the running pg_hba.conf might not be the same as the one in the
filesystem - either because of a reload not being done, or a reload of a
broken file.

I think 3 is a good option of these, assuming we can do it in a reasonably
good way.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

* Magnus Hagander (magnus@hagander.net) wrote:

On Thu, Jun 19, 2014 at 5:37 PM, Stephen Frost <sfrost@snowman.net> wrote:

I actually don't really see a huge problem with 1, but I need to go
review the thread in more detail...

The reason the raw line was added in the first place was debugging cases
where the running pg_hba.conf might not be the same as the one in the
filesystem - either because of a reload not being done, or a reload of a
broken file.

erm, not entirely convinced that's a great reason to log the whole line,
but..

I think 3 is a good option of these, assuming we can do it in a reasonably
good way.

I'd be fine with this approach. I'd definitely like to see this
addressed in some manner because it's, clearly, not going to go away as
a request (I remember dealing with similar issues quite a few years ago
and all the arguments about how it "should" be ok to log passwords
didn't fly and we ended up having to address it also).

Thanks,

Stephen

Thanks for the continued discussion on this issue.

It seems like, generally, fixing this vulnerability is getting a green light.

I wouldn't mind re-working the patch for this bug if I knew the
consensus on the preferred implementation. As I mentioned previously,
I'm new here, so how do I go about soliciting "votes" (or otherwise)
the preferred approach so that I may move forward.

Thanks!

Steve

On Thu, Jun 19, 2014 at 12:09 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Magnus Hagander (magnus@hagander.net) wrote:

On Thu, Jun 19, 2014 at 5:37 PM, Stephen Frost <sfrost@snowman.net> wrote:

I actually don't really see a huge problem with 1, but I need to go
review the thread in more detail...

The reason the raw line was added in the first place was debugging cases
where the running pg_hba.conf might not be the same as the one in the
filesystem - either because of a reload not being done, or a reload of a
broken file.

erm, not entirely convinced that's a great reason to log the whole line,
but..

I think 3 is a good option of these, assuming we can do it in a reasonably
good way.

I'd be fine with this approach. I'd definitely like to see this
addressed in some manner because it's, clearly, not going to go away as
a request (I remember dealing with similar issues quite a few years ago
and all the arguments about how it "should" be ok to log passwords
didn't fly and we ended up having to address it also).

Thanks,

Stephen

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Mon, Jun 23, 2014 at 10:26 PM, Steven Siebert <smsiebe@gmail.com> wrote:

Thanks for the continued discussion on this issue.

It seems like, generally, fixing this vulnerability is getting a green
light.

I wouldn't mind re-working the patch for this bug if I knew the
consensus on the preferred implementation. As I mentioned previously,
I'm new here, so how do I go about soliciting "votes" (or otherwise)
the preferred approach so that I may move forward.

I think the current summary is that "option c" is the one that people would
accept if you submit it (provided the regular caveats about it being
correctly implemented etc, of course). It should of course cover other
potentially sensitive fields as well (such as the radius encryption key).

If you implement a patch for that option, I will be happy to review and
apply it.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Thanks Magnus =) I'll move forward with this guidance.

On Mon, Jun 23, 2014 at 4:35 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Mon, Jun 23, 2014 at 10:26 PM, Steven Siebert <smsiebe@gmail.com> wrote:

Thanks for the continued discussion on this issue.

It seems like, generally, fixing this vulnerability is getting a green
light.

I wouldn't mind re-working the patch for this bug if I knew the
consensus on the preferred implementation. As I mentioned previously,
I'm new here, so how do I go about soliciting "votes" (or otherwise)
the preferred approach so that I may move forward.

I think the current summary is that "option c" is the one that people would
accept if you submit it (provided the regular caveats about it being
correctly implemented etc, of course). It should of course cover other
potentially sensitive fields as well (such as the radius encryption key).

If you implement a patch for that option, I will be happy to review and
apply it.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Was any progress made on this, the reporting of LDAP/RADIUS passwords in
our server logs?

---------------------------------------------------------------------------

On Mon, Jun 23, 2014 at 04:42:24PM -0400, Steven Siebert wrote:

Thanks Magnus =) I'll move forward with this guidance.

On Mon, Jun 23, 2014 at 4:35 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Mon, Jun 23, 2014 at 10:26 PM, Steven Siebert <smsiebe@gmail.com> wrote:

Thanks for the continued discussion on this issue.

It seems like, generally, fixing this vulnerability is getting a green
light.

I wouldn't mind re-working the patch for this bug if I knew the
consensus on the preferred implementation. As I mentioned previously,
I'm new here, so how do I go about soliciting "votes" (or otherwise)
the preferred approach so that I may move forward.

I think the current summary is that "option c" is the one that people would
accept if you submit it (provided the regular caveats about it being
correctly implemented etc, of course). It should of course cover other
potentially sensitive fields as well (such as the radius encryption key).

If you implement a patch for that option, I will be happy to review and
apply it.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Dropped off my radar I'm afraid, but the customer is still quite interested
in getting this fixed. What we finally worked out should be quick work,
I'll throw up a patch tonight. Thanks for the ping!

Thanks,

S

On Sat, Oct 11, 2014 at 2:35 PM, Bruce Momjian <bruce@momjian.us> wrote:

Attached is the patch (against master) for the approach we discussed:
sanitizing the log message by removing the sensitive information out of the
hba raw line.

Three things to note:

- I really only saw need to sanitize out the ldapbindpasswd and the
radiussecret (although I agree that can be argued). Is there anything else
that should be redacted?
- I substitute the sensitive phrase with the string "[sanitized]" -- this
is based on domain language of my customer...might be better named
something else?
- I couldn't for the life of me find a generic, simplified, replace_str
function either already imported from an existing dependency or within
postgresql code itself. Of course, some related functions in varlena.c and
regex, but I didn't think this routine warranted the overhead. Instead, I
settled for adding a simple single-purpose sanitizeLine function...but
being new here, I don't want to impose my design decisions over big picture
maintenance....especially since, as I noted, there is a potential (very
unlikely IMO) chance there could be a buffer overflow if someone makes a
single hba line over 4kb. I leave it in the experts hands...if it's
preferred an alternate way and it's simpler for you to just tell me what to
change in the patch, I can fix and resubmit.

Code successfully builds and existing regression tests pass. I did not add
any new tests here, because it seemed like a fairly large undertaking or
such a small change, since I couldn't find an existing set of tests to
piggy back on. I would be happy to add another todo request and work on
another path to add tests for auth.c as a whole if there is interest in it.

Thanks,

S

On Sat, Oct 11, 2014 at 8:44 PM, Steven Siebert <smsiebe@gmail.com> wrote:

Steven Siebert <smsiebe@gmail.com> writes:

Attached is the patch (against master) for the approach we discussed:
sanitizing the log message by removing the sensitive information out of the
hba raw line.

I still say that this is an ill-considered, unmaintainable, and
fundamentally insecure approach to solving the wrong problem.

As a single example of what's wrong with it, suppose that you
fat-finger some syntax detail of an LDAP line in pg_hba.conf.
When you issue "pg_ctl reload", will the postmaster log the broken
line in the postmaster log? I sure hope so, because not doing so
would be a major usability fail. Will it obscure the RADIUS secret?
No, because the syntax error will prevent it from correctly
identifying which part of the line is the secret, if indeed it
even realizes that the line might contain a secret.

If we go down this path, we'll be battling "oh, but what about that
other scenario?" cases till kingdom come; there will never be any
reason to think we've covered them all.

The right problem to be solving, to my mind, is that you feel a need
to give access to the postmaster log to untrusted people. Now maybe
that's just a problem of wrong administrative procedures, but let's
consider what we might do in PG to improve your ability to do that
safely. Perhaps what we should be entertaining is a proposal to have
multiple log channels, some containing more security-relevant messages
and others less so. Then you could give people the ability to read only
the non-security-relevant messages. If we arranged for *all* messages
relevant to pg_hba.conf to go into a secure log, it'd be a lot easier to
convince ourselves that we would not leak any security-critical info
than if we take the approach this patch proposes.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Steven Siebert <smsiebe@gmail.com> writes:

Attached is the patch (against master) for the approach we discussed:
sanitizing the log message by removing the sensitive information out of the
hba raw line.

I still say that this is an ill-considered, unmaintainable, and
fundamentally insecure approach to solving the wrong problem.

I agree that this approach is pretty grotty.

As a single example of what's wrong with it, suppose that you
fat-finger some syntax detail of an LDAP line in pg_hba.conf.

I've never liked that we tell users to put passwords in pg_hba.conf, be
they LDAP passwords or RADIUS secrets, it's just not the right place.

Perhaps what we should be entertaining is a proposal to have
multiple log channels, some containing more security-relevant messages
and others less so. Then you could give people the ability to read only
the non-security-relevant messages. If we arranged for *all* messages
relevant to pg_hba.conf to go into a secure log, it'd be a lot easier to
convince ourselves that we would not leak any security-critical info
than if we take the approach this patch proposes.

I definitely like the idea of having multiple log channels with a way to
control what kinds of messages go to which- but I've also got a simpler
proposal:

Let's stop having passwords and secrets in a complex configuration file
which can have parsing and other failures.

How about allowing users to put that information in an independent file,
as do for SSL (admittedly, probably more because it's easier for us to
deal with OpenSSL that way, but still)?

ldapbindpwfile=/etc/whatever
radiussecretfile=/etc/whatever

as examples. The files would contain *only* the password or secret
(ignoring any newline) and must be readable by the PG user. We could
happily log any issues with pg_hba, etc, as long as we don't log what
came from those files. Keeping the passwords out of pg_hba.conf also
makes it easier for admins to manage those files across multiple systems
(eg: using puppet, chef, etc) while having local PWs for each box too.
Might not be perfect for LDAP (unless we also provide a
ldapbinduserfile), though the bind user could be handled through puppet
or similar and it isn't as secure a concern, and it'd work directly for
RADIUS which directly support different secrets for different hosts.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

Let's stop having passwords and secrets in a complex configuration file
which can have parsing and other failures.

How about allowing users to put that information in an independent file,
as do for SSL (admittedly, probably more because it's easier for us to
deal with OpenSSL that way, but still)?

And then what? This other file can't possibly be so simple that
it's immune to having syntax errors, for example. You're just moving
the same problems from point A to point B. More, this would complicate
configuration and thereby create a whole new set of possible config
errors, which we'd then feel pressure to ameliorate by adding more
logging showing what the postmaster is doing. And that logging would
have this same issue of maybe it's exposing information that person A
doesn't want logged ... even though person B needs that very same info
to help him figure out his configuration mistake. A likely example of
that is feeding the wrong password/secret to some auth infrastructure
service, because you referenced the wrong item in this secondary file.

The core problem here is that we *need* to put security-relevant info
into the postmaster log in order to help people resolve problems. Moving
around configuration details isn't going to fix that; indeed, the more
complicated the configuration files, the worse the problem will be.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

Let's stop having passwords and secrets in a complex configuration file
which can have parsing and other failures.

How about allowing users to put that information in an independent file,
as do for SSL (admittedly, probably more because it's easier for us to
deal with OpenSSL that way, but still)?

And then what? This other file can't possibly be so simple that
it's immune to having syntax errors, for example.

Uh, no, that's exactly the point of the independent file.

It's *only* the password.

fopen() - fails, then log that you can't open the file
fgets() - fails, then log that you can't read the file

(check for newline and remove it, if it's there)

If it grows to be a complex configuration file which can have syntax
errors, then it loses the point, I agree, but I don't hear anyone
complaining about SSL keys or SSH keys or Kerberos keytabs being leaked
in log files- and I'm pretty darn sure they would be complaining if it
was happening, regardless of any "keep your log files secure and only
let trusted people look at them" requirement.

You're just moving
the same problems from point A to point B. More, this would complicate
configuration and thereby create a whole new set of possible config
errors, which we'd then feel pressure to ameliorate by adding more
logging showing what the postmaster is doing. And that logging would
have this same issue of maybe it's exposing information that person A
doesn't want logged ... even though person B needs that very same info
to help him figure out his configuration mistake. A likely example of
that is feeding the wrong password/secret to some auth infrastructure
service, because you referenced the wrong item in this secondary file.

This is done in other systems and has worked well from my experience.

The core problem here is that we *need* to put security-relevant info
into the postmaster log in order to help people resolve problems. Moving
around configuration details isn't going to fix that; indeed, the more
complicated the configuration files, the worse the problem will be.

Having pg_hba be complicated but not having passwords in secrets in it
isn't making this problem worse- we can log all we want about pg_hba.
The point is to remove the sensetive information from the complicated
config file, even if that makes the complicated config file a bit more
complicated.

Thanks,

Stephen