Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Magnus Hagander]

On Apr 8, 2016 1:13 AM, "Tom Lane"  wrote:
>
> Magnus Hagander  writes:
> > On Apr 7, 2016 9:14 PM, "Christian Ullrich" 
wrote:
> >> Magnus, do you intend to commit the patch before the feature freeze?
>
> > It's on my list of things to work on this weekend, yeah.
>
> But the stated feature freeze deadline is tomorrow (Friday), not the
> weekend or later.
>
> To the extent that this can be called a bug fix, it might be exempt
> from feature freeze, but I'm not on the RMT so I'm not going to make
> that call.
>

Oh, dang, I had put it down as Sunday in my calendar :S

I'll have to see what dish the travel-gods hand out today, and try to get
it done before.

/Magnus

Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Tom Lane]

Magnus Hagander  writes:
> On Apr 7, 2016 9:14 PM, "Christian Ullrich"  wrote:
>> Magnus, do you intend to commit the patch before the feature freeze?

> It's on my list of things to work on this weekend, yeah.

But the stated feature freeze deadline is tomorrow (Friday), not the
weekend or later.

To the extent that this can be called a bug fix, it might be exempt
from feature freeze, but I'm not on the RMT so I'm not going to make
that call.

regards, tom lane


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

Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Anyway, as things stand, elog(ERROR) will abort the session safely but
>> you won't necessarily get the kind of logging you want, so expected
>> auth-failure cases should try to go the STATUS_ERROR route.

> In other words, the use of palloc() and friends (psprintf in the patch)
> should be acceptable here.

Sure, no problem with that.

regards, tom lane


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

Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> > So, it seems that ClientAuthentication() expects to raise ereport(FATAL)
> > in case of authentication failures.  But what's the code path that
> > causes that to happen if a ereport(ERROR) happens in there?  Because all
> > that code is pretty careful to not do ereport(ERROR) directly and
> > instead return STATUS_ERROR which makes ClientAuthentication do the
> > FATAL report.  If this doesn't matter, then isn't this whole code overly
> > complicated for no reason?
> 
> The reason why elog(ERROR) will become a FATAL is that no outer setjmp
> has been executed yet, so elog.c will realize it has noplace to longjmp
> to.

Ah, I was looking callers up-stack and found nothing.  That should have
cued me that that was happening :-)

> Anyway, as things stand, elog(ERROR) will abort the session safely but
> you won't necessarily get the kind of logging you want, so expected
> auth-failure cases should try to go the STATUS_ERROR route.

In other words, the use of palloc() and friends (psprintf in the patch)
should be acceptable here.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Tom Lane]

Alvaro Herrera  writes:
> So, it seems that ClientAuthentication() expects to raise ereport(FATAL)
> in case of authentication failures.  But what's the code path that
> causes that to happen if a ereport(ERROR) happens in there?  Because all
> that code is pretty careful to not do ereport(ERROR) directly and
> instead return STATUS_ERROR which makes ClientAuthentication do the
> FATAL report.  If this doesn't matter, then isn't this whole code overly
> complicated for no reason?

The reason why elog(ERROR) will become a FATAL is that no outer setjmp
has been executed yet, so elog.c will realize it has noplace to longjmp
to.

Whether it's overcomplicated I dunno.  I think the idea behind returning
STATUS_ERROR is to allow a centralized reporting site to decorate the
errors with additional info, as indeed auth_fail does.  Certainly that
could be done another way (errcontext?), but that's the way we've got.

Anyway, as things stand, elog(ERROR) will abort the session safely but
you won't necessarily get the kind of logging you want, so expected
auth-failure cases should try to go the STATUS_ERROR route.

regards, tom lane


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

Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Alvaro Herrera]

So, it seems that ClientAuthentication() expects to raise ereport(FATAL)
in case of authentication failures.  But what's the code path that
causes that to happen if a ereport(ERROR) happens in there?  Because all
that code is pretty careful to not do ereport(ERROR) directly and
instead return STATUS_ERROR which makes ClientAuthentication do the
FATAL report.  If this doesn't matter, then isn't this whole code overly
complicated for no reason?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Alvaro Herrera]

Christian Ullrich wrote:

> To be honest, I'm not sure what can and cannot be done in auth code. I
> took inspiration from the existing SSPI code and nearly every error
> check in pg_SSPI_recvauth() ends up doing ereport(ERROR) already,
> directly or via pg_SSPI_error(). If this could cause serious trouble,
> someone would have noticed yet.

I think the problem is whether the report is sent to the client or not,
but I may be confusing with something else (COMMERROR reports?).

> What *could* happen, anyway? Can ereport(ERROR) in a backend make the
> postmaster panic badly enough to force a shared memory reset?

Probably not, since it's running in a backend already at that point, not
in postmaster.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* From: Alvaro Herrera [mailto:alvhe...@2ndquadrant.com]

> Christian Ullrich wrote:

> > * Christian Ullrich wrote:
> >
> > >* From: Magnus Hagander [mailto:mag...@hagander.net]
> 
> > >>Code uses a mix of malloc() and palloc() (through sprintf). Is there
> > >>a reason for that?
> > >
> > >I wasn't sure which to prefer, so I looked around in auth.c, and
> > >other than RADIUS, everything seems to use malloc() (although the
> > >sample size is not too great). Should I use palloc() instead?
> >
> > The single instance of malloc() has been replaced with palloc().
> 
> I'm wary of palloc() in this code actually ... if the allocation fails,
> I'm not sure it's okay to use ereport(ERROR) which is what would happen
> with palloc.  With the malloc code, you report the problem with
> elog(LOG) and then return STATUS_ERROR which lets the calling code
> handle the failure in a different way.  I didn't actually review your
> new code, but I recall this from previous readings of auth code; so if
> you're going to use palloc(), you better audit what happens on OOM.
> 
> For the same reason, using psprintf is probably not acceptable either.

To be honest, I'm not sure what can and cannot be done in auth code. I took 
inspiration from the existing SSPI code and nearly every error check in 
pg_SSPI_recvauth() ends up doing ereport(ERROR) already, directly or via 
pg_SSPI_error(). If this could cause serious trouble, someone would have 
noticed yet.

What *could* happen, anyway? Can ereport(ERROR) in a backend make the 
postmaster panic badly enough to force a shared memory reset?

-- 
Christian



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

Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Alvaro Herrera]

Christian Ullrich wrote:
> * Christian Ullrich wrote:
> 
> >* From: Magnus Hagander [mailto:mag...@hagander.net]

> >>Code uses a mix of malloc() and palloc() (through sprintf). Is there a
> >>reason for that?
> >
> >I wasn't sure which to prefer, so I looked around in auth.c, and other than
> >RADIUS, everything seems to use malloc() (although the sample size is not
> >too great). Should I use palloc() instead?
> 
> The single instance of malloc() has been replaced with palloc().

I'm wary of palloc() in this code actually ... if the allocation fails,
I'm not sure it's okay to use ereport(ERROR) which is what would happen
with palloc.  With the malloc code, you report the problem with
elog(LOG) and then return STATUS_ERROR which lets the calling code
handle the failure in a different way.  I didn't actually review your
new code, but I recall this from previous readings of auth code; so if
you're going to use palloc(), you better audit what happens on OOM.

For the same reason, using psprintf is probably not acceptable either.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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