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

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.

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".

> +         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?

> +     /* 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).

> +     upname = (char*)palloc(upnamesize);

I don't believe this cast is typically included.

> +     /* 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.

> +     /* 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.

Minus the few small things above, this looks good to me, assuming it
resolves the issue.

--Robbie