Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix +1 error in storing krbPrincipalKey LDAP attr. #181

Closed
wants to merge 1 commit into from

Conversation

tkuthan
Copy link
Contributor

@tkuthan tkuthan commented Aug 1, 2014

For principal entries having keys with multiple kvnos (due to use of
-keepold), LDAP backend plugin makes an attempt to store all the keys
having the same kvno into single krbPrincipalKey attribute value.
There is an +1 error in the loop, causing currkvno variable being set
to the just processed value, instead of the next kvno.
As a result, second and all following groups of keys by kvnos are each
stored in two krbPrincipalKey attribute values.

Fixed to use correct kvno value.

For principal entries having keys with multiple kvnos (due to use of
-keepold), LDAP backend plugin makes an attempt to store all the keys
having the same kvno into single krbPrincipalKey attribute value.
There is an +1 error in the loop, causing currkvno variable being set
to the just processed value, instead of the next kvno.
As a result, second and all following groups of keys by kvnos are each
stored in two krbPrincipalKey attribute values.

Fixed to use correct kvno value.
@greghudson
Copy link
Member

This looks correct, although there are some consequences beyond just inefficient storage of the keys due to the way the array is set up. I will want to add a test case.

@tkuthan
Copy link
Contributor Author

tkuthan commented Aug 4, 2014

Do you mean a regression test case for -keepold option?

I can imagine the test would make sure, that TGT still works after "cpw -keepold krbtgt/REALM":

  1. kinit as princA
  2. cpw -keepold krbtgt/REALM
  3. kvno princB (expect success)

Is that what you are looking for?

@greghudson
Copy link
Member

Pushed to master as 81c332e. There is an associated CVE and advisory, and commit 0d78da2 has an automated regression test.

@greghudson greghudson closed this Aug 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants