gss_unwrap_iov with CFX krb5 fails to unwrap if wrap token includes nonzero RRC

[sthaber]

Describe the bug

There's really two bugs here, as there are two separate code paths affected.

First, unwrapping when not sealed (no encryption) is unimplemented as far as I can tell. Take a look at:

https://github.com/heimdal/heimdal/blob/master/lib/gssapi/krb5/cfx.c#L602
https://github.com/heimdal/heimdal/blob/master/lib/gssapi/krb5/cfx.c#L968

I believe this should be supported, as RFC4121 states:

The rotation count value is chosen by the sender based on
implementation details.  The receiver MUST be able to interpret all
possible rotation count values, including rotation counts greater
than the length of the token.

We've observed that, for example, Windows Server LDAP likes to send signed responses with a rotation applied. Furthermore, Heimdal's own gss_wrap implementation applies a rotation to signed messages. The IOV wrap equivalent does not, however! This means that if you take any of the unit tests in test_context.c and round a trip a gss_wrap followed by a gss_unwrap_iov, with sealing turned off, it will fail in the code path I linked to above.

The second bug is that unwrapping when sealed (with encryption) also seems to be broken. There exists a code path that is supposed to unrotate an IOV (iov_unrotate(), but it doesn't seem to work correctly. I haven't debugged exactly why, but the same set of circumstances described above (gss_wrap + gss_unwrap_iov) will produce the failure.

As a workaround, we've discovered it is sufficient to unrotate the buffers ourselves before passing them to the IOV unwrap in Heimdal. However, this is less than ideal, as it means we're interpreting RFC4121 data structures directly in our application, and we'd rather than Heimdal do this for us.

To Reproduce
Steps to reproduce the behavior:

1. Set up Kerberos using AES256/SHA1 as the encryption/checksumming type. This is standard if you use the Linux KDC.
2. Set up a GSS context using a Kerberos ticket from the KDC. (gss_init_sec_context() and gss_accept_sec_context())
3. Wrap any message of any length using gss_wrap(). We used "abc", for example.
4. (Optional) Provide CONF_REQ_ENCRYPTION to seal the message when wrapping.
5. Take the wrapped message and attempt to unwrap it using gss_unwrap_iov(). This requires you set up the header, data, padding, and trailer appropriate per the IOV interface.
6. Observe a GSS error is returned.

Expected behavior
The message is unwrapped back to its original contents without error.

Desktop (please complete the following information):

• OS: Ubuntu 18.04 LTS, up to date, nothing special

Additional context
We're running a relatively recent version of Heimdal: https://github.com/chapeltech/heimdal/. This fork is intended to be merged with master ASAP and I don't see anything in here that has meaningfully changed since it was forked in the wrap/unwrap areas. We have our own test framework at Qumulo that we are running these tests through which is not open source. If it would be helpful I could implement the test in your framework. Please let me know. Thanks for reading!

[lhoward]

Thanks for the thorough report, we should fix this. Does MIT implement this properly? (I did work on that implements but can’t recall offhand.)

[sthaber]

Good question. I don't have an easy way to swap in MIT to my tests. I read the code a little. MIT has a totally different interface for unwrapping an IOV. They support the GSS_IOV_BUFFER_TYPE_STREAM flag which is an alternative way to construct the input so that you don't have to parse out the header, padding, and trailer yourself:

https://github.com/krb5/krb5/blob/3bb429b1d61dcd017537e27d7572dcf9114a5613/src/lib/gssapi/krb5/k5unsealiov.c#L368

Heimdal has the STREAM flag defined but it doesn't really seem to be used. That's another thing that might be nice to have, since constructing the unwrap IOV in an application is a bit gross :).

It looks like if you opt to construct the full vec (don't use STREAM) then it's not going to unrotate for you, same as with Heimdal:

https://github.com/krb5/krb5/blob/3bb429b1d61dcd017537e27d7572dcf9114a5613/src/lib/gssapi/krb5/k5sealv3iov.c#L378

So maybe there's actually parity here except I think it's by accident since there is code in the unwrap path that looks like it's attempting to unrotate the IOV and it's just not working. In any case, if we fixed it, it would strictly be an improvement upon MIT...

[lhoward]

That makes sense, I did implement this for MIT in 2009 but my memory is hazy (and am traveling with limited access to internet at the moment). I think the intention was to use STREAM when you have a single buffer and individual IOVs where you need to reorder them, integrity protect associated data (AEAD), or the application needs to be aware of the trailer (DCE RPC).

In the absence of implementing STREAM support in Heimdal, could you just use gss_unwrap() instead of gss_unwrap_iov()? It does remove the ability to decrypt in-place but there are probably plenty of allocations going on anyway...

[sthaber]

Okay, that makes sense, thanks for explaining. I'm pretty new to this stuff.

Our hope was to use IOV in-place modification for both wrap and unwrap in our application (Kerberized NFSv4.1) in order to to improve performance and reduce heap thrashing for large buffers (between 1MiB and 16MiB). We don't have the whole stack wired up to using NFSv4.1 with privacy quite yet but probably will in a few weeks, at which point we can evaluate just how much heap stuff happens on these big requests. In theory, there shouldn't be too much heap stuff much since the primary paths for the IOV endpoints in Heimdal seem to actually be implemented in-place all the way down through GSS and KRB5. At least for AESxxx/SHA1. The other algorithms do seem to call iov_coalesce() which looks it just flattens the whole IOV into a malloc'd buffer. That's fine though as AES/SHA1 is what everything uses (Windows, Linux). Sounds like maybe you expect us to hit some other mallocs along the way though? Maybe I need to read a little more closely.

Thanks for engaging with us on this!

[sthaber]

Actually, here's some memory stats of the wrap/unwrap IOV. It's not bad at all! This is for a wrapping and unwrapping a 16MiB IOV. Resetting the stats after every operation.

before: {
    external_calls=0,
    external_bytes_allocated=0
}
after wrap: {
    external_calls=113,
    external_bytes_allocated=36144
}
after unwrap: {
    external_calls=53,
    external_bytes_allocated=30664
}
after doing it all again: {
    external_calls=12,
    external_bytes_allocated=50108
}

[lhoward]

Yeah, in-place definitely makes sense in terms of avoiding allocation, but rotating in-place assumes the memory passed by the caller is mutable (which is probably not part of the API contract, even if we were to rotate back).

[sthaber]

Oh, right! You could unrotate it in the IOV format inside Heimdal but you'd also need to unrotate it in the result data somehow, which would require either modifying the memory or returning another IOV pointing to the memory in a different way. Yeah maybe the API contract isn't sufficient to do rotation at all. Good point.

[mingw-io]

Hi there.

OS: Windows 10 x64

Heimdal version: master branch

We are integrating Cyrus SASL & Heimdal and while running the SASL testsuite program we got a segmentation fault:

We know nothing about the codebase as to fix (even attempt to fix) this issue but thought useful to make you aware of it.

Thanks in advance.

[abartlet]

This last comment I think is the different issue that this has strayed into discussing, which is as @josephsutton1 also noticed, the rotation happening on the input buffer changes potentially read only memory (Python memory in our case).

If there is to be a fix here, we should avoid modification of constant memory.

[lhoward]

Definitely a bug if this is happening.

[lhoward]

I haven’t looked closely enough at this issue recently but, I did make some changes to stream support in the lukeh/aes-gcm branch so, if it’s easy to test against that might be worth a look.

[sthaber]

I ended up just implementing the rotation and streaming and all that jazz in our application unfortunately.

In somewhat related news, I wrote IOV endpoints for get_mic and verify_mic. Heimdal only has them for wrap and unwrap, but MIT has them for the MIC methods as well. The problem is I tested them in our framework rather than the Heimdal framework so I think pull would be nacked with no tests. Any interest in these patches?

[lhoward]

Please, send them along! I can write tests. Did you implement for all mechglue and mechanisms?

[sthaber]

Sorry for the delay, been doing 1000 things. I made a pull request here:

#1081

You're welcome to close it if not helpful. Thanks!