Coverity 1522032: use after free

[paulidale]

Fix use after free error.

• documentation is added or updated
• tests are added or updated

[slontis]

I think this fix is not right.
freeing the ctx here seems completely wrong.. The derive did not create the ctx, So it should also not free it.

[slontis]

same on line 1029

[slontis]

I also dislike multiple exit branches.. it should just set a flag and goto exit.

[t8m]

Better fix in #20534

[paulidale]

The context free is existing code. I've no idea why it was done that way, I just preserved the behaviour.

I don't agree think that #20534 is proper fix: it doesn't cater for loading a provider (or having a dynamic provider) which changes the fetched algorithms between calls to derive.

[tom-cosgrove-arm]

Agree this PR clearly fixes the use-after-free of ctx at line 1140 by performing that before freeing ctx itself at line 1038.

This matches the behaviour in the block at line 1028 when ctx->md == NULL.

However, there is the issue that only those two error returns (returning 0) free ctx; the many others afterwards don't. So a caller getting success (1) back could reasonably expect the context to still exist; but if this function returns 0 the caller doesn't know whether the context was freed or not without examining the specific error, so there's something weird going on here.

This PR doesn't address the pre-existing "what happens to ctx on errors" confusion, but it does clearly fix the use-after-free on an error path

[paulidale]

I've added a commit that takes out the ctx frees.

[slontis]

The context free is existing code. I've no idea why it was done that way, I just preserved the behaviour.

Thanks for changing..
The code is clearly wrong and behaves differently to every other kdf that has been written. The ctx should only be freed in the free method. If you don't do this then I would expect checks for ctx being NULL in every single function. This ctx is passed from core, it is not expected that someone can change it to NULL under the hood and then potentially have a floating pointer back in core.

[tom-cosgrove-arm]

This is clearly an improvement over the existing code!

[slontis]

Is line 1125 correct.. ? If these are no longer needed shouldnt they also be set to NULL? (Or is this a downref)..

[paulidale]

That line is correct. I was being zealous in nulling out pointers.

[tom-cosgrove-arm]

LGTM

[slontis]

#20534 seems to cover a bit more than is required for this fix..

[t8m]

I still think #20534 is more correct. And yeah, I can then rebase it on top of this one if you insist on merging it first.

It does not make sense to re-fetch the internal MD and MAC on every derive call (unless the propq is changed). You do not refetch the Argon2 kdf implementation within the ctx so why should the internal details of it be refetched?

There are also other unnecessary things within the current code such as saving the kdf output within the ctx but never read it again. That is simply wrong and should be removed which #20534 does.

[paulidale]

Merged.