default value of -k for hprop broken

[brianmay]

For more details see http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=712680#68

[brianmay]

Copied details from Debian bug report:

My current understanding of the details:

The default value of -k is "HDB:".

Heimdal retrieves the keytab with hdb_get_entry(). As the dbname is not given (dbname should appear to right of "HDB:"), the would recurse through the list of known databases (find_db) until a match is found. On a default Debian configuration this is:

# ./lib/hdb/test_dbinfo 
label: default
        realm: no realm
        dbname: /var/lib/heimdal-kdc/heimdal
        mkey_file: /var/lib/heimdal-kdc/m-key
        acl_file: /var/lib/heimdal-kdc/kadmind.acl

The value of dbname is used.

However before this happens, krb5_init_creds_set_keytab() is called first.

Since change 118f99e, krb5_init_creds_set_keytab() checks the result of the call to krb5_kt_start_seq_get(). Before if this failed, it didn't matter. Now it does. This function call ends up at hdb_start_seq_get(). Unfortunately this function does not like the fact it was not given a specific database to work on, and fails:

    if (dbname == NULL) {
        /*
         * We don't support enumerating without being told what
         * backend to enumerate on
         */
        ret = KRB5_KT_NOTFOUND;
        return ret;
    }

This failure is propagated back up to krb5_init_creds_set_keytab(), which calls _krb5_kt_principal_not_found(), which sets the error. This in turn get propagated back to hprop.c, get_creds() which prints the error:

hprop: krb5_get_init_creds: Failed to find kadmin/hprop@IN.VPAC.ORG in keytab HDB: (unknown enctype)

[brianmay]

Not really sure what the purpose of krb5_init_creds_set_keytab(). Or if it is really needed.

However, it appears to inspect every key in the keytab. It probably isn't really a good idea to do this for the master KDC HDB database, which may contain many keys.

So I would think that the following line in kdc/hprop.h:

#define HPROP_KEYTAB "HDB:"

Should get replaced with:

#define HPROP_KEYTAB KEYTAB_DEFAULT

Assuming we can see this definition at this point. Alternatively, delete the above, and change the reference in kdc/hprop.c

[jaltman]

Love, what is the intended behavior for the default hprop keytab?

[lha]

once upon a time HDB didm't support iteration, we should make a version that doesn't do that again.

the reason why we want to use the HDB instead of keytab is since the admin servers have access to the HDB and there is no reason have a partial copy that just will go out of sync at some time

How about renaming HDB to FULLHDB and drop hdb_start_seq_get() and friends from the HDB callback structure (hdb_kt_ops) ?

[jaltman]

I'm fine with that change provided the Samba team is ok with it.

[elric1]

On Mon, Oct 14, 2013 at 05:09:37AM -0700, Love H?rnquist ?strand wrote:

once upon a time HDB didm't support iteration, we should make a version
that doesn't do that again.

the reason why we want to use the HDB instead of keytab is since the
admin servers have access to the HDB and there is no reason have a
partial copy that just will go out of sync at some time

How about renaming HDB to FULLHDB and drop hdb_start_seq_get() and
friends from the HDB callback structure (hdb_kt_ops) ?

Is it clear that we need version that does iterate? What is the
use case? (Not saying that we shouldn't, just curious as it may
be simpler to elide the feature entirely if we can't think of a
use case for it.)

Roland Dowdeswell                      http://Imrryr.ORG/~elric/

[jaltman]

The functionality was added explicitly for Samba4 by Andrew Bartlett.
commit 788480d
Author: Love Hornquist Astrand lha@h5l.org
Date: Mon Aug 3 10:43:22 2009 +0200

heimdal Extend the 'hdb as a keytab' code [HEIMDAL-600]

This extends the hdb_keytab code to allow enumeration of all the keys.

The plan is to allow ktutil's copy command to copy from Samba4's
hdb_samba4 into a file-based keytab used in wireshark.

From Andrew Bartlett

[elric1]

On Mon, Oct 14, 2013 at 09:09:27AM -0700, Jeffrey Altman wrote:

The functionality was added explicitly for Samba4 by Andrew Bartlett.

The reason that I ask is that most of the Kerberos DBs that I use
are not really appropriate for enumeration but rather only for
searching due to their size. This is quite an inefficient way to
go about this as the keytab interface only allows enumeration rather
than proper search.

Perhaps, it would be appropriate to either add a proper search to
the keytab interface or do this another way? While we are at it,
it looks like krb5_kt_get_entry() isn't a proper search as it simply
iterates over the ``keytab'' contents.

Roland Dowdeswell                      http://Imrryr.ORG/~elric/

[lha]

Today I would have made a new ”dump” backend that converts a database into a keytab.

As for Andrew use case it most appropriate since it allows him to parse windows/windows communication.

The keytab interface is a proper search if the backend implements the ->get() method.

[elric1]

On Mon, Oct 14, 2013 at 12:05:55PM -0700, Love H?rnquist ?strand wrote:

The keytab interface is a proper search if the backend implements the
->get() method.

Right, missed that and skipped straight to the enumeration when I
looked at the code. Still, though, why does Samba need the
enumeration as mentioned in their change request. This seems to
me that they are either planning to copy the entire HDB into a
keytab or that they feel that the provided search functionality is
inadequate for their needs. I'd assume the latter, but not knowing
what they are planning it would be just an assumption.

As for Andrew use case it most appropriate since it allows him
to parse windows/windows communication.

I have always felt slightly uncomfortable about this sort of thing
as I have been hoping for a bit of the old PFS in Kerberos for
quite some time now. Although it is possible to snoop connexions
if you have a key or decrypt old saved evesdropping if you later
come into possession of the key (say, by guessing the user's very
old password), it probably shouldn't be---and it probably shouldn't
be relied on.

Roland Dowdeswell                      http://Imrryr.ORG/~elric/

[abartlet]

On Mon, 2013-10-14 at 12:36 -0700, Roland C. Dowdeswell wrote:

On Mon, Oct 14, 2013 at 12:05:55PM -0700, Love H?rnquist ?strand
wrote:

The keytab interface is a proper search if the backend implements
the
->get() method.

Right, missed that and skipped straight to the enumeration when I
looked at the code. Still, though, why does Samba need the
enumeration as mentioned in their change request. This seems to
me that they are either planning to copy the entire HDB into a
keytab or that they feel that the provided search functionality is
inadequate for their needs. I'd assume the latter, but not knowing
what they are planning it would be just an assumption.

No, it is the former. We use it to dump the database (our database)
into a keytab, so that we can decrypt all kerberos communication using
wireshark.

The 'samba-tool domain exportkeytab' command uses this.

As for Andrew use case it most appropriate since it allows him
to parse windows/windows communication.

I have always felt slightly uncomfortable about this sort of thing
as I have been hoping for a bit of the old PFS in Kerberos for
quite some time now. Although it is possible to snoop connexions
if you have a key or decrypt old saved evesdropping if you later
come into possession of the key (say, by guessing the user's very
old password), it probably shouldn't be---and it probably shouldn't
be relied on.

Regardless, we have and ship a command that allows this to our users.

I wouldn't be fussed if the database name we use changes, or if someone
wants to provide us a patch that uses a different interface. (a dump()
call would probably need less code copied into Samba).

The issue is that we can build with either a now quite old bundled
version, or use the system version. Removing this from upstream Heimdal
will break the 'use the system kerberos' use case on which Debian
relies.

We can of course patch that package, but I'm just worried that the fix
might get lost. Or instead add HDB_GETONLY instead.

I'm a little confused, shouldn't we just fix the caller not to search?

Andrew Bartlett

Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz

[brianmay]

It is perhaps worth noting that whatever krb5_init_creds_set_keytab() does, I don't think it can be very important, as before 118f99e it would have been basically a NOP, as the call to krb5_kt_start_seq_get() would have returned straight away with an error, resulting in krb5_init_creds_set_keytab() returning immediately with no error. Nobody seem to notice.

I don't think disabling enumeration at the HDB layer would help anything. If enumeration was not supported, krb5_kt_start_seq_get() would still generate an error, and the result would be exactly the same.

There does seem to be a good usage case for this enumeration too - debugging. So while regular code such as krb5_init_creds_set_keytab() probably should not be enumerating the entire database, it sounds like the enumeration code should stay.

[elric1]

On Mon, Oct 14, 2013 at 03:18:55PM -0700, Andrew Bartlett wrote:

Right, missed that and skipped straight to the enumeration when I
looked at the code. Still, though, why does Samba need the
enumeration as mentioned in their change request. This seems to
me that they are either planning to copy the entire HDB into a
keytab or that they feel that the provided search functionality is
inadequate for their needs. I'd assume the latter, but not knowing
what they are planning it would be just an assumption.
No, it is the former. We use it to dump the database (our database)
into a keytab, so that we can decrypt all kerberos communication using
wireshark.
The 'samba-tool domain exportkeytab' command uses this.

Wow, surely this must be shockingly inefficient for wireshark?
Keytabs are not designed for random access and Kerberos databases
can become a reasonable size.

As for Andrew use case it most appropriate since it allows him
to parse windows/windows communication.

I have always felt slightly uncomfortable about this sort of thing
as I have been hoping for a bit of the old PFS in Kerberos for
quite some time now. Although it is possible to snoop connexions
if you have a key or decrypt old saved evesdropping if you later
come into possession of the key (say, by guessing the user's very
old password), it probably shouldn't be---and it probably shouldn't
be relied on.
Regardless, we have and ship a command that allows this to our users.
I wouldn't be fussed if the database name we use changes, or if someone
wants to provide us a patch that uses a different interface. (a dump()
call would probably need less code copied into Samba).
The issue is that we can build with either a now quite old bundled
version, or use the system version. Removing this from upstream Heimdal
will break the 'use the system kerberos' use case on which Debian
relies.

I am not arguing that we remove the feature that we can allow the HDB:
keytab method to enumerate the keys here, but rather I am arguing that
we need to eventually fix the Kerberos encryption types to prevent
wireshark and programs like it from working in the first place even if
they have the keys as the fact that wireshark works (with the keys) is
a fundamental security weakness that should be addressed at some point.
And so, people that use wireshark in this way will, I hope, one day be
out of luck. As will the malicious third parties who can save a bit of
traffic and eventually succeed in a dictionary attack.

We can of course patch that package, but I'm just worried that the fix
might get lost. Or instead add HDB_GETONLY instead.
I'm a little confused, shouldn't we just fix the caller not to search?

I agree that we should simply fix the callers not to enumerate the
database in most places in the code where this is done. If we want
to preserve the ability to enumerate all keytab backends, this is
the cleanest fix to the issue. It is also likely the best given
that we do not have a limit on the size of ``normal'' keytabs and
we have just seen a use case (wireshark) where the keytab will be
the entire database.

Roland Dowdeswell                      http://Imrryr.ORG/~elric/

[lha]

Wow, surely this must be shockingly inefficient for wireshark?

sure, when testing you make sure that you don't have that many keys. also if running at any length, you'll get overrun by all sub keys that you need to keep track of.

again, only for diagnosing, so not an issue.

like it from working in the first place even if they have the keys as the fact that wireshark works (with the keys) is a fundamental security weakness that should be addressed at some point.

you can't one end coöperates with the the wireshark peer (I would say that handing out the kdb kind of hints of that).

I agree that we should do ForwardSecurity though, but that is a different feature then the problem.

[lha]

1d84562

Please re-open if this is not enough.

[lha]

15 okt 2013 kl. 00:18 skrev Andrew Bartlett notifications@github.com:

I'm a little confused, shouldn't we just fix the caller not to search?

No, because of aliases and other features you can’t assume the string that the KDC handed you is indeed what you’ll find in the keytab.

Kerberos is not aligned with reality there should really be an UUID assigned to each key and that is what the KDC told the server to use.

Love

[brianmay]

Sorry, I may be lost here.

However I really don't see the point behind 1d84562, nor does it solve the problem that caused me to open this ticket.krb5_init_creds_set_keytab

You say enumeration/search is required because [reason] however then you disable the enumeration support? I don't get this. It worked before Andrew's change to implement enumeration and before the "not supported" error was was checked. That would suggest to me that enumeration is not actually required, and code (krb5_init_creds_set_keytab()) that attempts to enumerate the entire HDB should be fixed not to do so.

As I explained above the problem is because krb5_init_creds_set_keytab() is trying to enumerate all keys, and this is failing because the enumeration is not supported for that particular case. Removing the enumeration support does not help, it is still going to fail because the enumeration is not supported, now for a more general case.

I just run hprop on the latest master, and as expected I get exactly the same error, and strace shows no attempt to open the HDB database as before.

Brian May

[brianmay]

This bug was not fixed. As per my last post. I can't reopen it. Can somebody with reopen privileges please reopen it?

Thanks

[jaltman]

Brian, please submit a pull request for a proposed fix if you can.

I believe the correct fix may be to explicitly ignore the not supported error in krb5_init_creds_set_keytab() when enumeration is attempted.

[brianmay]

I could create a patch, however not sure if anyone would like it.

e.g. I would start off by reversing 1d84562 as I don't understand what this was meant to achieve. It seems to be extra complication for no reason.

[jaltman]

1d84562 permits the Samba team to enumerate the database for their use case while disabling enumeration for the general case.

[brianmay]

What is the point of disabling enumeration for the general case? It doesn't resolve this bug. I don't see the point myself.

Need to change the code so that it doesn't call the enumeration in the first place I think.

[jaltman]

The bug can be resolved by not treating the lack of enumeration support as an error.
I don't understand all of the situations under which the Samba team is using enumeration so I am not prepared to remove it.

[nicowilliams]

See the fix_gic_kt_w_hdbget branch of my github clone of heimdal
(https://github.com/nicowilliams/heimdal.git). Does it work for you?

Nico

[jaltman]

Brian? Any feedback?

[brianmay]

Hello,

Sorry for the delay in responding, I have been busy.

I haven't yet had a chance to look at @nicowilliam changes yet. Assume he does as you suggested.

Yes, I agree that fixing a possible solution to fixing this bug would be not to treat lack of enumeration support as an error. I.e. the way it use to be.

There seems to be a bit of confusion here. What is the krb5_init_creds_set_keytab function? Why does it need to even try to enumerate every key in the database? I don't think this has anything to do with the Samba team or any changes they have supplied. If it really does need to be able to enumerate every key in the database, but not for HDB, then maybe HDB really does need to be split up into two. However it seems a odd requirement, and I haven't seen any justification for it here.

Thanks

[nicowilliams]

On Wed, Dec 04, 2013 at 08:22:54PM -0800, Brian May wrote:

There seems to be a bit of confusion here. What is the
krb5_init_creds_set_keytab function? Why does it need to even try to
enumerate every key in the database? I don't think this has anything
to do with the Samba team or any changes they have supplied. If it
really does need to be able to enumerate every key in the database,
but not for HDB, then maybe HDB really does need to be split up into
two. However it seems a odd requirement, and I haven't seen any
justification for it here.

HDB-as-keytab.

The idea is that to minimize the burden for configuring privileged
KDC-side services (setting up keytabs for them) the library supports
using the HDB as a keytab. (MIT Kerberos also does this.)

The keytab API is... limited. Anyways, by having an HDB that does not
permit enumeration we avoid the risk of the daemon spending a lot of
time enumerating the HDB.

Nico