Re: BUG #13854: SSPI authentication failure: wrong realm name used

* From: Robbie Harwood [mailto:rharwood@redhat.com]

> Christian Ullrich <chris@chrullrich.net> writes:
>
> > Updated patch attached.
>
> I unfortunately don't have windows machines to test this on, but I
> thought it might be helpful to review this anyway since I'm touching
> code in the same general area (GSSAPI).  And as far as I can tell, you
> don't break anything there; master continues to behave as expected.

Thanks for the review.

> Some comments inline:
>
> >   pg_SSPI_recvauth(Port *port)
> >   {
> >       int            mtype;
> > +     int            status;
>
> The section of this function for include_realm checking already uses an
> int status return code (retval).  I would expect to see them share a
> variable rather than have both "retval" and "status".

I would not, because retval is local to that last if, but you are right, status
does not need to be in function scope.

> > +         status = pg_SSPI_make_upn(accountname, sizeof(accountname),
> > +                                   domainname,
> sizeof(domainname),
> > +                                   port->hba->upn_username);
>
> This is the only place I see that this function is called.  That being
> the case, why bother with the sizes of parameters?  Why doesn't
> pg_SSPI_make_upn() just call sizeof() by itself rather than taking those
> as arguments?

sizeof(array) != sizeof(pointer).

> > +     /* Build SAM name (DOMAIN\\user), then translate to UPN
> > +        (user@kerberos.realm). The realm name is returned in
> > +        lower case, but that is fine because in SSPI auth,
> > +        string comparisons are always case-insensitive. */
>
> Since we're already considering changing things: this is not the comment
> style for this file (though it is otherwise a good comment).

True. Will fix.

> > +     upname = (char*)palloc(upnamesize);
>
> I don't believe this cast is typically included.

Left over from when this was malloc() before Magnus' first look at it.

> > +     /* Replace domainname with realm name. */
> > +     if (upnamerealmsize > domainnamesize)
> > +     {
> > +         pfree(upname);
> > +         ereport(LOG,
> > +                 (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
> > +                  errmsg("realm name too long")));
> > +                  return STATUS_ERROR;
> > +     }
> > +
> > +     /* Length is now safe. */
> > +     strcpy(domainname, p+1);
>
> Is this an actual fail state or something born out of convenience?  A
> naive reading of this code doesn't explain why it's forbidden for the
> upn realm to be longer than the domain name.

Because it's copied *into* domainname right there on the last line.

That said, sizeof(domainname) is MAXPGPATH, which is 1024, so there is
absolutely no chance that the realm could be longer -- it would need an
AD forest at least 16 domains deep.

> > +     /* Replace account name as well (in case UPN != SAM)? */
> > +     if (update_accountname)
> > +     {
> > +         if ((p - upname + 1) > accountnamesize)
> > +         {
> > +             pfree(upname);
> > +             ereport(LOG,
> > +                     (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
> > +                      errmsg("translated account name too
> long")));
> > +                      return STATUS_ERROR;
> > +         }
> > +
> > +         *p = 0;
> > +         strcpy(accountname, upname);
>
> Same as above.

Yup.

--
Christian