KCM: Securely erase memory used for secrets #7062
Conversation
aplopez
force-pushed
the
kcm-memory
branch
3 times, most recently
from
dfa5866 to
8ced9e3
Compare
|
Failing tests seem to be failing in all the PRs, not just here. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The structure is not modified so it is logical to receive a `const struct sss_iobuf`.
The macro was defined in a .c module and thus unavailable to be used on any other modules. It was moved to common.h so that it can be used in other tests.
aplopez
force-pushed
the
kcm-memory
branch
2 times, most recently
from
49496c8 to
f358b4d
Compare
|
@alexey-tikhonov I added a flag parameter to tell the |
| /* | ||
| * @brief Returns whether the data is secured | ||
| */ | ||
| bool sss_iobuf_get_secure(const struct sss_iobuf *iobuf); |
There was a problem hiding this comment.
Access to that value. It is a sibling to the other sss_iobuf_get_*() functions. Even if it is not currently used, there is no other way for a caller to access this value.
There was a problem hiding this comment.
This is "what does it do", not "what would be a reason to use it".
What is the reason to store 'sss_iobuf::secure' at all?
When buffer is being created, one opts if he wants a secure cleanup.
But I can't imagine a realistic use case when one wants to know if buffer is secured.
There was a problem hiding this comment.
The reason is explained in the last sentence:
there is no other way for a caller to access this value.
The struct sss_iobuf is defined in the sss_iobuf.h header file, but declared in the sss_iobuf.c module. External users only have access to those fields by using getters. As said before, today it is not used, but we don' t know if it will be need anytime soon. Also it was created for API completeness: all the other fields have a getter, this one too.
The secure field is needed by the sss_iobuf_read_varlen() and sss_iobuf_read_iobuf() functions which create copies of the data and need to know whether to secure or not those new buffers.
There was a problem hiding this comment.
The
securefield is needed by thesss_iobuf_read_varlen()andsss_iobuf_read_iobuf()functions which create copies of the data and need to know whether to secure or not those new buffers.
Could you please elaborate on this?
There was a problem hiding this comment.
Function sss_iobuf_read_varlen() receives an sss_iobuf, read some data from it and puts them on a brand new sss_iobuf. If the old structure were the data was read from was secured (secure is true), then the new one must also be secured. The cleanest way to know whether the original structure was secured is using the secure flag. This function is the reason the flag exists. Until I found this function, there was no flag.
Function sss_iobuf_read_iobuf() internally uses sss_iobuf_read_varlen() and sss_iobuf_init_steal() to create a new buffer, which must be secured in the same way, so the flag stored in the source structure is used to set the flag in the new structure. The flag isn't actually needed for this case, but since it exists for the other case, it must be set to the correct value.
There was a problem hiding this comment.
Function
sss_iobuf_read_varlen()receives ansss_iobuf, read some data from it and puts them on a brand newsss_iobuf.
Ah, this is not 'new sss_iobuf' but 'uint8_t **_out'...
This is a can of worms.
This output buffer might be copied further, and information about it's "security" is already lost anyway.
I would argue it's up to a user of sss_iobuf_read_varlen() to decide if read chunk of data has to be secured.
This is more transparent.
There was a problem hiding this comment.
As to talloc_steal(), I believe it keeps d-tor.
There was a problem hiding this comment.
This output buffer might be copied further, and information about it's "security" is already lost anyway. I would argue it's up to a user of
sss_iobuf_read_varlen()to decide if read chunk of data has to be secured. This is more transparent.
No option is better than the other. I just choose the one that simplifies things by letting the original owner of the data decide about its security. "If it was originally secure, then it must remain secure."
About further copies... Sure. There is nothing we can do about that right now.
As to talloc_steal(), I believe it keeps d-tor.
Yes it does. I confirmed it. That's why I say sss_iobuf_init_steal() doesn't need the flag, but we need to keep it coherent for sss_iobuf_read_varlen().
There was a problem hiding this comment.
Ok, I will not insist further, that's not too important after all (though I still think sss_iobuf_get_secure()/secure isn't really needed).
I would keep it in the API. Imo, it makes this utility more versatile and thus appealing to re-use. |
alexey-tikhonov
added
no-backport
Make sure all the memory blocks allocated dynamically or statically by KCM to store credentials and messages (which might include credentials) are erased by calling sss_erase_mem_securely() or sss_erase_talloc_mem_securely() before being freed.
|
Thanks, ACK. |
Make sure all the memory blocks allocated dynamically or statically by KCM to store credentials and messages (which might include credentials) are erased by calling
sss_erase_mem_securely()orsss_erase_talloc_mem_securely()before being freed.struct sss_iobufis the main structure used to store secrets. It's buffer will be cleaned when freed using a destructor. The destructor is associated to the buffer itself, in case it is stolen from one structure to another.struct kcm_ccachedoesn't need to be explicitly cleaned because, in the end, it usesstruct sss_iobuf.A unit test was added to check this functionality.
Valgrind's magic interfere with this test because the heap disappears. This makes the test incompatible avecValgrind, so the test is skipped under these conditions. This added a new build dependency on the packagevalgrind-devel.