kdc: cross realm s4u2self handling

[Isaac Boukris]

Hello Greg, all,

I've been working lately on cross-realm s4u2self in samba code and
would like to consult about MIT's current handling and possibly needed
changes.

See last emails on samba-technical thread (it's a long one, spans over
a few months):
https://lists.samba.org/archive/samba-technical/2018-September/130044.html

Heimdal and Samba PRs:
https://github.com/heimdal/heimdal/pull/403
https://github.com/samba-team/samba/pull/204


In short, for cross-realm s4u2self requests we need a way to sign and
verify the PAC including the realm, that is ***@realm for a regular
principal name, and ***@realm@realm for an enterprise one.

See attached prove-of-concept patches for mit-krb5 and samba with
which cross-realm s4u2self works both ways (tested manually against
windows too).

I'd like to start with the last commit in the MIT patch, which
partially reverts 8a9909ff9ef6b51c5ed09ead6713888fbb34072f commit,
that I don't fully understand.

In cross-realm s4u2self, a service starts by requesting the ticket
from a KDC in the impersonated principal's realm, and then follows
back referrals to its own realm.
The kdc code handles this correctly in several places apart of this
error when is_referral is true, introduced by the above commit.

I think it even contradicts the below sections in 'do_tgs_req.c':

if (isflagset(c_flags, KRB5_KDB_FLAG_PROTOCOL_TRANSITION) &&
!isflagset(c_flags, KRB5_KDB_FLAG_CROSS_REALM))
enc_tkt_reply.client = s4u_x509_user->user_id.user;
else
enc_tkt_reply.client = subject_tkt->client;

This code is a bit tricky as usually KRB5_KDB_FLAG_CROSS_REALM means
that the client is not in local realm so he probably came with a
referral tgt, but not necessarily that we are currently issuing a
referral.
But in cross-realm s4u2self, since the tgt-client and the server are
the same principal, it means we are issuing a referral and that's
exactly what this code is checking in order to set the returned cname
and crealm.

However, with the above commit we won't reach here in case of referral
as we'd error out before.
BTW, I think using the new is_referral instead of
KRB5_KDB_FLAG_CROSS_REALM in this code would be clearer and more
correct (as it is based on db lookup).

Thought?

Thank you!

[Greg Hudson]

Post by Isaac Boukris
I'd like to start with the last commit in the MIT patch, which
partially reverts 8a9909ff9ef6b51c5ed09ead6713888fbb34072f commit,
that I don't fully understand.

That was part of a series of commits made between 1.11 and 1.12, with
the goal of treating the client request as an immutable object (commit
9e37d01a0122904776fada43ec65425c375414d8). Prior to those commits,
process_tgs_req() would set the request server to the result of the
server search, which could be a referral. For the commit you are trying
to understand, there were two checks against request->server that I was
concerned with:

* In kdc_util.c:kdc_process_s4u2self_req(), we compare request->server
against the client principal in the header ticket and error out if it
doesn't match.

* In tgs_policy.c:check_tgs_nontgt(), we compare the header ticket
server against the request server and error out if it doesn't match.

I wanted to make sure that leaving request->server alone would not allow
requests through which would previously have been disallowed. I was
only considering within-realm S4U2Self requests, but I believe that in
the 1.11 code, the check in kdc_process_s4u2self_req() would have
prevented cross-realm S4U2Self requests from succeeding. It no longer
interferes bequest request->server is now the original request server.

I may have unwittingly been preserving a bug rather than an intentional
restriction, so we can probably relax it.
_______________________________________________
krbdev mailing list ***@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev

[Isaac Boukris]

Post by Isaac Boukris
I'd like to start with the last commit in the MIT patch, which
partially reverts 8a9909ff9ef6b51c5ed09ead6713888fbb34072f commit,
that I don't fully understand.

That was part of a series of commits made between 1.11 and 1.12, with the
goal of treating the client request as an immutable object (commit
9e37d01a0122904776fada43ec65425c375414d8). Prior to those commits,
process_tgs_req() would set the request server to the result of the server
search, which could be a referral. For the commit you are trying to
understand, there were two checks against request->server that I was
* In kdc_util.c:kdc_process_s4u2self_req(), we compare request->server
against the client principal in the header ticket and error out if it
doesn't match.
* In tgs_policy.c:check_tgs_nontgt(), we compare the header ticket server
against the request server and error out if it doesn't match.
I wanted to make sure that leaving request->server alone would not allow
requests through which would previously have been disallowed. I was only
considering within-realm S4U2Self requests, but I believe that in the 1.11
code, the check in kdc_process_s4u2self_req() would have prevented
cross-realm S4U2Self requests from succeeding. It no longer interferes
bequest request->server is now the original request server.
I may have unwittingly been preserving a bug rather than an intentional
restriction, so we can probably relax it.

Ok, it makes sense now, I can see it'd have failed the comparison in
kdc_process_s4u2self_req() (thanks for the detailed clarification).

Note that in my heimdal work, I have assumed that we can skip this
comparison check when the service isn't local since we are not issuing a
ticket to self but a referral, so let the KDC of the service do the check
when it issues the service ticket.
The reason it came up was, because heimdal has a more relaxed check
implemented via hdb plugin which allows matching different names, like an
SPN to a regular principal name, by looking up both name in db and calling
the plugin callback with both entries, then samba plugin just compares the
SID to confirm it's the same principal.
However, when the service is not local, we don't have these db handles so
we can't do that.
I think we should implement something similar in MIT too (in a later
stage), do you think the above assumption is correct?


Secondly, I'm wondering if we need to change the sign_authdata() hdb
callback to also supply the tgt-client principal name (see the first commit
in samba patch).

The reason is that in s4u2self, when the impersonated principal is from
local realm, then the PAC is of the tgt-client and we may need to verify it
(heimdal does, though I'm unsure why).
For in-realm request, we could use server->princ, since they are the same
principal, but when the service is foreign then it will be the cross realm
trust principal and we cannot verify the PAC.
I guess my main question is whether we need to verify this PAC at, or we
can just discard it, which is more of a Windows question.


Other than that, what do you think of the pac_verify/sign_ex() routines,
does it look ok?


Thanks!
_______________________________________________
krbdev mailing list ***@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev

[Greg Hudson]

Post by Isaac Boukris

Post by Greg Hudson
I may have unwittingly been preserving a bug rather than an intentional
restriction, so we can probably relax it.

Ok, it makes sense now, I can see it'd have failed the comparison in
kdc_process_s4u2self_req() (thanks for the detailed clarification).

It occurs to me that a within-realm S4U2Self request (i.e. one using a
local TGT header ticket rather than a cross-TGT one) should still fail
if it results in a referral. I will try to put together a test case for
that.

Post by Isaac Boukris
Note that in my heimdal work, I have assumed that we can skip this
comparison check when the service isn't local since we are not issuing a
ticket to self but a referral, so let the KDC of the service do the
check when it issues the service ticket.
The reason it came up was, because heimdal has a more relaxed check
implemented via hdb plugin which allows matching different names, like
an SPN to a regular principal name, by looking up both name in db and
calling the plugin callback with both entries, then samba plugin just
compares the SID to confirm it's the same principal.
However, when the service is not local, we don't have these db handles
so we can't do that.

Maybe. I can't find a flaw in your logic--in the cross-realm case, we
aren't issuing a ticket for the requested service name and we don't have
a DB entry for it, so surely it can't matter too much what it was.

Post by Isaac Boukris
I guess my main question is whether we need to verify this PAC at, or we
can just discard it, which is more of a Windows question.

I can't think of a reason to verify the PAC in the TGT unless the KDC is
going to use its contents.

Post by Isaac Boukris
Other than that, what do you think of the pac_verify/sign_ex() routines,
does it look ok?

I looked over them briefly and don't have a problem with them. If you
submit a PR I will examine them more closely and cross-check against
[MS-PAC] and [MS-SFU].
_______________________________________________
krbdev mailing list ***@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev

[Isaac Boukris]

It occurs to me that a within-realm S4U2Self request (i.e. one using a local
TGT header ticket rather than a cross-TGT one) should still fail if it
results in a referral. I will try to put together a test case for that.

I see, though I'm not sure I understand how this would happen.

At any case, would it suffice to condition the check on:
is_local_principal(kdc_active_realm, header_ticket->server)
Or perhaps on (are those two necessarily equivalent here btw?):
!is_cross_tgs_principal(header_ticket->server)


Note, in case of a local TGT header ticket, I think we could add:
if (client == NULL)
KRB5KDC_ERR_POLICY;
The client here being the principal to impersonate, which must be
local in that case.

This would help to return the same error as Windows in case when bad
implementation (e.g. current heimdal), use a local TGT to request a
s4u2self ticket from its own KDC on behalf of a foreign principal.
I'll need to add that logic to my heimdal kdc changes as well, as
currently it only fails there on PAC logon-name mismatch.

Post by Isaac Boukris
Other than that, what do you think of the pac_verify/sign_ex() routines,
does it look ok?

I looked over them briefly and don't have a problem with them. If you
submit a PR I will examine them more closely and cross-check against
[MS-PAC] and [MS-SFU].

I'll submit a PR soon, thanks a lot for all the feedback.
_______________________________________________
krbdev mailing list ***@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev

[Isaac Boukris]

Post by Isaac Boukris

It occurs to me that a within-realm S4U2Self request (i.e. one using a local
TGT header ticket rather than a cross-TGT one) should still fail if it
results in a referral. I will try to put together a test case for that.

I see, though I'm not sure I understand how this would happen.
is_local_principal(kdc_active_realm, header_ticket->server)
!is_cross_tgs_principal(header_ticket->server)

Also note, another assumption I made in Heimdal, was to skip the check
of 'ok_to_auth_as_delegate' before copying the forwardable flag when
issuing a referral.
For similar reasons; technically we don't have a DB entry to check,
and logically we are not issuing the service ticket but a referral so
let the final KDC do the check.
To my understanding, MIT code already has this assumption, see (which
suggests again that referrals are intended to work for s4u2self):
https://buildfarm.opencsw.org/source/xref/krb5/src/kdc/do_tgs_req.c#434

Post by Isaac Boukris

Post by Isaac Boukris
Other than that, what do you think of the pac_verify/sign_ex() routines,
does it look ok?

I looked over them briefly and don't have a problem with them. If you
submit a PR I will examine them more closely and cross-check against
[MS-PAC] and [MS-SFU].

This is now PR #852.
I got into some troubles when I tried to add unit-tests, first I
noticed that if a PAC is already signed, then the sign function would
just verify instead of re-signing, which would fail if it was signed
without realm and we want to sign it with realm.
It wasn't a problem in my tests with samba plugin, since the latter
always creates a new PAC and populates it anew, so I left that
unsupported.

Another issue was, enterprise principal with realm, as it got unparsed
to this when signing:
(gdb) p pac_princname
$25 = 0x60a8b0 "w2003final$\\@***@WIN2K3.THINKER.LOCAL"

So for this case I added KRB5_PRINCIPAL_UNPARSE_DISPLAY flag when
parsing the name to avoid the escaping.
I think we might want to add it generally (heimdal does it).
_______________________________________________
krbdev mailing list ***@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev

[Isaac Boukris]

Hi Greg,

Post by Isaac Boukris

It occurs to me that a within-realm S4U2Self request (i.e. one using a local
TGT header ticket rather than a cross-TGT one) should still fail if it
results in a referral. I will try to put together a test case for that.

I see, though I'm not sure I understand how this would happen.
is_local_principal(kdc_active_realm, header_ticket->server)
!is_cross_tgs_principal(header_ticket->server)
if (client == NULL)
KRB5KDC_ERR_POLICY;
The client here being the principal to impersonate, which must be
local in that case.
This would help to return the same error as Windows in case when bad
implementation (e.g. current heimdal), use a local TGT to request a
s4u2self ticket from its own KDC on behalf of a foreign principal.
I'll need to add that logic to my heimdal kdc changes as well, as
currently it only fails there on PAC logon-name mismatch.

I've submitted PR #853 to follow up on this. I have tested it manually
in trust with Windows, and will try to add a test case in t_s4u for it
(without PAC, as I've suggested).
If there are other tests you have in mind, I can try to implement as well.

I hope this seems reasonable.
Thanks!
_______________________________________________
krbdev mailing list ***@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev