how to "ban" clients?

[Chris Hecker]

The patch below has a bug I just found with more testing:

+ request->client) == KRB5_NT_ENTERPRISE_PRINCIPAL) {

should be

+ header_enc_tkt->client) == KRB5_NT_ENTERPRISE_PRINCIPAL) {

The former line will deference a null pointer and crash (request->client 
is 0 on tgs_req).

Chris



On 2011/07/27 04:02, Chris Hecker wrote:
>
> Oh, and I should point out there's the obvious code in
> validate_tgs_request that uses the client:
>
> + /* Client must not be locked out */
> + if (client && isflagset(client->attributes, KRB5_KDB_DISALLOW_ALL_TIX)) {
> + *status = "CLIENT LOCKED OUT";
> + return(KDC_ERR_CLIENT_REVOKED);
> + }
>
> Chris
>
>
>
> On 2011/07/27 03:35, Chris Hecker wrote:
>>
>> Okay, I implemented this today. Most of the patch is loading the bool
>> from kdc.conf and updating all the places those config variables are
>> declared and copied, but I've put the business end of things below.
>> I'll put the full patch up somewhere, but I wanted more clued people to
>> take a look at this and see if it looks right. It works perfectly, and
>> it correctly rejects new tickets if somebody's been -allow_tix'd even if
>> they have a valid TGT.
>>
>> I haven't done any perf testing yet.
>>
>> As you can see from the comment, I've got a couple questions that maybe
>> Greg or somebody can answer...
>>
>> Also, do I need to set the canonicalize flags? Basically, I only need
>> to be able to check is_local_principal, and get the attributes to check
>> the allow_tix flag...
>>
>> Thanks,
>> Chris
>>
>> === modified file 'src/kdc/do_tgs_req.c'
>> --- old/src/kdc/do_tgs_req.c 2011-07-26 10:46:03 +0000
>> +++ new/src/kdc/do_tgs_req.c 2011-07-27 07:37:15 +0000
>> @@ -201,6 +201,63 @@
>> * decrypted with the session key.
>> */
>>
>> + if (validate_tgs_req_local_client &&
>> + is_local_principal(header_enc_tkt->client)) {
>> +
>> + /*
>> + * If validate_tgs_req_local_client is set in kdc.conf, we
>> + * will check KRB5_KDB_DISALLOW_ALL_TIX on any local clients.
>> + *
>> + * This client db_entry get code is basically copied from
>> + * process_as_req. We free the client below after passing it
>> + * to validate_tgs_request, before the s4u2self request sets
>> + * it, and we use a local flags for getting the entry.
>> + *
>> + * Note: I have to admit to not understanding all the
>> + * subtleties of this code, so somebody with more of a clue
>> + * should review it! Some things I don't understand yet:
>> + *
>> + * - We really only need to be able to check if the client is
>> + * local, and if the tix flag is set, but the
>> + * KRB5_KDB_FLAG_CLIENT_REFERRALS_ONLY flag is only valid
>> + * for AS_REQ, so is the db doing more work than it needs
>> + * to? Is there a fast path here I could use?
>> + *
>> + * - The s4u code below also is confusing, and reusing the
>> + * client local variable is a bit cheesy. Do they refer to
>> + * the same client? From looking at the code below, I don't
>> + * think so, but I'm not sure. If they are, would we save
>> + * any work by reusing it below if we get it here?
>> + *
>> + */
>> +
>> + unsigned int local_c_flags = 0;
>> +
>> + /*
>> + * Note that according to the referrals draft we should
>> + * always canonicalize enterprise principal names.
>> + */
>> + if (isflagset(request->kdc_options, KDC_OPT_CANONICALIZE) ||
>> + krb5_princ_type(kdc_context,
>> + request->client) ==
>> KRB5_NT_ENTERPRISE_PRINCIPAL) {
>> + setflag(local_c_flags, KRB5_KDB_FLAG_CANONICALIZE);
>> + setflag(local_c_flags, KRB5_KDB_FLAG_ALIAS_OK);
>> + }
>> + errcode = krb5_db_get_principal(kdc_context,
>> header_enc_tkt->client,
>> + local_c_flags, &client);
>> + if (errcode == KRB5_KDB_NOENTRY) {
>> + status = "CLIENT_NOT_FOUND";
>> + if (vague_errors)
>> + errcode = KRB5KRB_ERR_GENERIC;
>> + else
>> + errcode = KRB5KDC_ERR_C_PRINCIPAL_UNKNOWN;
>> + goto cleanup;
>> + } else if (errcode) {
>> + status = "LOOKING_UP_CLIENT";
>> + goto cleanup;
>> + }
>> + }
>> +
>> /* XXX make sure server here has the proper realm...taken from AP_REQ
>> header? */
>>
>> @@ -276,7 +333,7 @@
>> goto cleanup;
>> }
>>
>> - if ((retval = validate_tgs_request(request, 0, server, header_ticket,
>> + if ((retval = validate_tgs_request(request, client, server,
>> header_ticket,
>> kdc_time, &status, &e_data))) {
>> if (!status)
>> status = "UNKNOWN_REASON";
>> @@ -284,6 +341,14 @@
>> goto cleanup;
>> }
>>
>> + /* free the client if we created one above, so the below s4u code
>> + works exactly the same as before the validate flag was added. */
>> + if (client) {
>> + krb5_db_free_principal(kdc_context, client);
>> + client = 0;
>> + }
>> +
>> +
>> if (!is_local_principal(header_enc_tkt->client))
>> setflag(c_flags, KRB5_KDB_FLAG_CROSS_REALM);
>>
>>