Re: [TrouSerS-users] bad handling in owner_evict_init

[Hal F. <hal...@gm...>]

Hi David - Well, as long as we are patching this code, I'd suggest
doing it a little differently.

IMO the reason your original version didn't crash is because just
above the for loop, the code does:
    free(respData);
and then you dereference respData, which still holds this stale
pointer to freed space. This happened not to hold a 1 value (== TRUE)
- it hasn't been re-used (yet) and what was there before didn't match.
So your code worked OK, but clearly this is not safe.

Rather than clearing respData where you do, I'd suggest clearing it
immediately after this free() statement above the for loop. This way
erroneous dereferences will be more likely to be obvious :).

Then in your patch I'd just do continue on that special error. I can't
help wondering though if that error is specific to your brand of TPM.
In my experience there is a wide range of errors that TPMs report.
They don't follow this aspect of the spec particularly closely. And of
course in this case, we are apparently dealing with data corruption
within the TPM. Leaving aside the discomfort we may feel at a security
component exposing an embarrassing weakness in this way, it certainly
does not give us confidence in what specific error a future TPM may
report in such a scenario.

In short, this patch will fix your TPM, and won't hurt anything, but
it is not particularly general. I guess what I am getting at is the
possibility of always doing a continue on all errors here. From what I
understand, the only thing that can return an error is a failed TPM
(which your TPM arguably is - we are hoping that it is only a small
failure and that a band-aid will set it right). So maybe looping a
bunch of times is not that bad even on a TPM that is returning a
failure each time.

Or maybe not, maybe it is safest to just do your fix - chances are few
TPMs will fail in this way, so this particular error is the only one
worth ignoring. Just something to think about -

Hal Finney

On Sun, Jul 27, 2008 at 7:03 AM, David Smith <dd...@go...> wrote:
> Indeed, good find. I've been using the above patch for months without
> incident and have started distributing it to users at my organization
> who have had the same issue on their laptops and while nobody has had
> trousers segfault on them, you're right, this should be handled better
> to avoid the possible segfault.
>
> Here's the patch clean up to handle this properly:
>
> --- trousers-0.3.1.orig/src/tcs/tcs_key_mem_cache.c
> +++ trousers-0.3.1/src/tcs/tcs_key_mem_cache.c
> @@ -1080,9 +1080,15 @@
>
>                LoadBlob_UINT32(&offset, keyList.handle[i], (BYTE *)&keyHandle);
>                /* get the ownerEvict flag for this key handle */
> -               if ((result = TCSP_GetCapability_Internal(InternalContext, TPM_CAP_KEY_STATUS,
> -                                                         sizeof(UINT32), (BYTE *)&keyHandle,
> -                                                         &respDataSize, &respData))) {
> +               result = TCSP_GetCapability_Internal(InternalContext, TPM_CAP_KEY_STATUS,
> +                                                    sizeof(UINT32), (BYTE *)&keyHandle,
> +                                                    &respDataSize, &respData);
> +               /* skip over invalid keyhandles, they will be evicted */
> +               if (result == TPM_E_INVALID_KEYHANDLE) {
> +                       respData = NULL;
> +                       continue;
> +               }
> +               if (result) {
>                        free(keyList.handle);
>                        return result;
>                }
>
>
> Cheers,
> - dds
>