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

pkinit: Correctly pad Diffie-Hellman key #1177

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jsutton24
Copy link
Contributor

If the Diffie-Hellman key was ‘n’ bytes too short, we would shift it to the right ‘n’ places, padding it out to the correct length to compute the reply key.

Unfortunately, we forgot to increase the size of the key accordingly, so ‘n’ trailing key bytes would be discarded. This could mean failure to decrypt a reply when interoperating with a Kerberos implementation without this bug.

If the Diffie-Hellman key was ‘n’ bytes too short, we would shift it to
the right ‘n’ places, padding it out to the correct length to compute
the reply key.

Unfortunately, we forgot to increase the size of the key accordingly, so
‘n’ trailing key bytes would be discarded. This could mean failure to
decrypt a reply when interoperating with a Kerberos implementation
without this bug.

Signed-off-by: Joseph Sutton <[email protected]>
@@ -230,6 +230,7 @@ generate_dh_keyblock(krb5_context context,
size -= dh_gen_keylen;
memmove(dh_gen_key + size, dh_gen_key, dh_gen_keylen);
memset(dh_gen_key, 0, size);
dh_gen_keylen += size;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the buggy client mean we need to try both possibilities now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if we want to interoperate with buggy Heimdal clients without sporadic failures. For the other way round, interoperating with buggy Heimdal servers, the client probably would need to keep regenerating the key until it got a working one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back in 2010 there were interop problems with MIT which resulted in the submission of both 7639f83 and then 6e05462. Does the existing code successfully interoperate with MIT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the existing code successfully interoperate with MIT?

I don’t know. I haven’t tested that.

@mattwoodyard
Copy link

The client side of this patch seems to have fixed client side pkinit failures we were seeing against an AD KDC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants