OIDC_CHILD: a couple of cosmetic fixes#8172
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a couple of cosmetic fixes, mainly to ensure consistent use of sss_erase_mem_securely() for clearing sensitive data and to fix a compilation warning by initializing a variable. The changes are good for code consistency and correctness.
However, I've noticed a minor security issue related to clearing secrets. When clearing the client secret strings, strlen() is used to determine the size, which omits the null-terminator. This means the allocated buffer is not fully cleared, potentially leaking the length of the secret. I've added suggestions to include the null-terminator in the memory clearing operation.
| free(opts->scope); | ||
| if (opts->client_secret != NULL) { | ||
| explicit_bzero(opts->client_secret, strlen(opts->client_secret)); | ||
| sss_erase_mem_securely(opts->client_secret, strlen(opts->client_secret)); |
There was a problem hiding this comment.
When clearing the secret, strlen() is used for the size. This does not include the terminating null byte. As opts->client_secret was allocated using strdup, the allocated size is strlen(opts->client_secret) + 1. By not clearing the null byte, you are not clearing the entire allocated memory, which could potentially leak the length of the secret. It's better to clear the entire buffer, including the null terminator.
sss_erase_mem_securely(opts->client_secret, strlen(opts->client_secret) + 1);
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
I agree with the changes, ACK.
About Gemini's comment, I think it has a point about potentially leaking the size of the secret (this would be true even if we would overwrite the final \0 with a \0). If we are concerned we might be able to fix this by allocating larger buffers, e.g. 1k or 4k and zero the whole buffer after using the secret.
bye,
Sumit
This fixes
```
../src/oidc_child/oidc_child.c: In function ‘main’:
../src/oidc_child/oidc_child.c:682:17: warning: ‘client_secret_tmp’ may be used uninitialized [-Wmaybe-uninitialized]
682 | explicit_bzero(client_secret_tmp, strlen(client_secret_tmp));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/oidc_child/oidc_child.c:578:11: note: ‘client_secret_tmp’ was declared here
578 | char *client_secret_tmp;
| ^~~~~~~~~~~~~~~~~
```
Warning is a false positive but cheap to "fix".
Reviewed-by: Sumit Bose <sbose@redhat.com>
instead of `explicit_bzero()` as the latter might be unavaialble. Reviewed-by: Sumit Bose <sbose@redhat.com>
sss_erase_mem_securely()