Skip to content

Commit

Permalink
KCM: Securely erase memory used for secrets
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@aplopez
aplopez committed Dec 1, 2023
1 parent c7d4559 commit f358b4d
Show file tree
Hide file tree
Showing 11 changed files with 363 additions and 33 deletions.
1 change: 1 addition & 0 deletions Makefile.am
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3726,6 +3726,7 @@ test_iobuf_CFLAGS = \
test_iobuf_LDADD = \
$(CMOCKA_LIBS) \
$(SSSD_LIBS) \
$(SSSD_INTERNAL_LTLIBS) \
$(NULL)

test_confdb_SOURCES = \
Expand Down
1 change: 1 addition & 0 deletions contrib/sssd.spec.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ BuildRequires: systemd-devel
BuildRequires: systemtap-sdt-devel
BuildRequires: uid_wrapper
BuildRequires: po4a
BuildRequires: valgrind-devel
%if %{build_subid}
BuildRequires: shadow-utils-subid-devel
%endif
Expand Down
3 changes: 3 additions & 0 deletions src/responder/kcm/kcm_renew.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,9 @@ errno_t kcm_renew_all_tgts(TALLOC_CTX *mem_ctx,

kcm_creds_check_times(tmp_ctx, renew_tgt_ctx, extracted_creds[j],
cc, client_name);

/* Once used, clean the credentials from memory. */
sss_erase_talloc_mem_securely(extracted_creds[j]);
}
}

Expand Down
18 changes: 18 additions & 0 deletions src/responder/kcm/kcmsrv_ccache.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,21 @@ static krb5_creds *kcm_cred_to_krb5(krb5_context kctx, struct kcm_cred *kcm_crd)
}
#endif

static void securely_erase_krb5_data(krb5_data *data)
{
if (data != NULL) {
sss_erase_mem_securely(data->data, data->length);
}
}

static void securely_erase_krb5_creds(krb5_creds *cred)
{
if (cred != NULL) {
securely_erase_krb5_data(&cred->ticket);
securely_erase_krb5_data(&cred->second_ticket);
}
}

static errno_t
kcm_cc_remove_duplicates(struct kcm_ccache *cc,
struct kcm_cred *kcm_crd)
Expand Down Expand Up @@ -308,6 +323,7 @@ kcm_cc_remove_duplicates(struct kcm_ccache *cc,
}

bret = sss_krb5_creds_compare(kctx, kcrd, kcrd_cc);
securely_erase_krb5_creds(kcrd_cc);
krb5_free_creds(kctx, kcrd_cc);
if (!bret) {
continue;
Expand All @@ -320,6 +336,7 @@ kcm_cc_remove_duplicates(struct kcm_ccache *cc,
ret = EOK;

done:
securely_erase_krb5_creds(kcrd);
krb5_free_creds(kctx, kcrd);
krb5_free_context(kctx);

Expand Down Expand Up @@ -380,6 +397,7 @@ static int kcm_cc_unmarshal_destructor(krb5_creds **creds)
}

for (i = 0; creds[i] != NULL; i++) {
securely_erase_krb5_creds(creds[i]);
krb5_free_creds(krb_ctx, creds[i]);
}

Expand Down
2 changes: 1 addition & 1 deletion src/responder/kcm/kcmsrv_ccache_binary.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ errno_t kcm_ccache_to_sec_input_binary(TALLOC_CTX *mem_ctx,
struct sss_iobuf *buf;
errno_t ret;

buf = sss_iobuf_init_empty(mem_ctx, sizeof(krb5_principal_data), 0);
buf = sss_iobuf_init_empty(mem_ctx, sizeof(krb5_principal_data), 0, true);
if (buf == NULL) {
return ENOMEM;
}
Expand Down
5 changes: 3 additions & 2 deletions src/responder/kcm/kcmsrv_ccache_secdb.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ static errno_t sec_get(TALLOC_CTX *mem_ctx,
goto done;
}

buf = sss_iobuf_init_steal(tmp_ctx, data, len);
buf = sss_iobuf_init_steal(tmp_ctx, data, len, true);
if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Cannot init the iobuf\n");
ret = EIO;
Expand Down Expand Up @@ -683,7 +683,8 @@ static struct tevent_req *ccdb_secdb_set_default_send(TALLOC_CTX *mem_ctx,

iobuf = sss_iobuf_init_readonly(state,
(const uint8_t *) uuid_str,
UUID_STR_SIZE);
UUID_STR_SIZE,
false);
if (iobuf == NULL) {
ret = ENOMEM;
goto immediate;
Expand Down
12 changes: 8 additions & 4 deletions src/responder/kcm/kcmsrv_ops.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ struct tevent_req *kcm_cmd_send(TALLOC_CTX *mem_ctx,

state->reply = sss_iobuf_init_empty(state,
KCM_PACKET_INITIAL_SIZE,
KCM_PACKET_MAX_SIZE);
KCM_PACKET_MAX_SIZE,
true);
if (state->reply == NULL) {
ret = ENOMEM;
goto immediate;
Expand Down Expand Up @@ -138,7 +139,8 @@ struct tevent_req *kcm_cmd_send(TALLOC_CTX *mem_ctx,

state->op_ctx->input = sss_iobuf_init_readonly(state->op_ctx,
input->data,
input->length);
input->length,
true);
if (state->op_ctx->input == NULL) {
ret = ENOMEM;
goto immediate;
Expand All @@ -155,7 +157,8 @@ struct tevent_req *kcm_cmd_send(TALLOC_CTX *mem_ctx,
state->op_ctx->reply = sss_iobuf_init_empty(
state,
KCM_PACKET_INITIAL_SIZE,
KCM_PACKET_MAX_SIZE - 2*sizeof(uint32_t));
KCM_PACKET_MAX_SIZE - 2*sizeof(uint32_t),
true);
if (state->op_ctx->reply == NULL) {
ret = ENOMEM;
goto immediate;
Expand Down Expand Up @@ -882,7 +885,8 @@ static struct tevent_req *kcm_op_store_send(TALLOC_CTX *mem_ctx,

state->cred_blob = sss_iobuf_init_empty(state,
creds_len,
creds_len);
creds_len,
true);
if (state->cred_blob == NULL) {
ret = ENOMEM;
goto immediate;
Expand Down
75 changes: 66 additions & 9 deletions src/responder/kcm/secrets/secrets.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,33 @@ static char *local_dn_to_path(TALLOC_CTX *mem_ctx,
struct ldb_dn *basedn,
struct ldb_dn *dn);

static void db_result_erase_message_securely(struct ldb_message *msg, const char *attr)
{
int i;
struct ldb_message_element *element;
struct ldb_val *value;

element = ldb_msg_find_element(msg, attr);
if (element != NULL) {
/* If the element exists, overwrite every single value */
for (i = 0; i < element->num_values; i++) {
value = &element->values[i];

sss_erase_mem_securely(value->data, value->length);
value->length = 0;
}
}
}

static void db_result_erase_securely(struct ldb_result *res, const char *attr)
{
int i;

for (i = 0; i < res->count; i++) {
db_result_erase_message_securely(res->msgs[i], attr);
}
}

static int local_db_check_containers(TALLOC_CTX *mem_ctx,
struct sss_sec_ctx *sec_ctx,
struct ldb_dn *leaf_dn)
Expand Down Expand Up @@ -198,7 +225,8 @@ static errno_t get_secret_expiration_time(uint8_t *key, size_t key_length,
struct cli_creds client = {};
struct kcm_ccache *cc;
struct sss_iobuf *iobuf;
krb5_creds **cred_list, **cred;
krb5_creds **cred_list = NULL;
krb5_creds **cred;
const char *key_str;

if (_expiration == NULL) {
Expand All @@ -216,7 +244,7 @@ static errno_t get_secret_expiration_time(uint8_t *key, size_t key_length,
goto done;
}

iobuf = sss_iobuf_init_readonly(tmp_ctx, sec, sec_length);
iobuf = sss_iobuf_init_readonly(tmp_ctx, sec, sec_length, true);
if (iobuf == NULL) {
ret = ENOMEM;
goto done;
Expand All @@ -233,11 +261,14 @@ static errno_t get_secret_expiration_time(uint8_t *key, size_t key_length,
goto done;
}

/* Look for the (first) expiration time while securely deleting the
* unmarshalled credentials. */
for (cred = cred_list; *cred != NULL; cred++) {
if ((*cred)->times.endtime != 0) {
if (expiration == 0 && (*cred)->times.endtime != 0) {
expiration = (time_t) (*cred)->times.endtime;
break;
}
sss_erase_talloc_mem_securely(*cred);
*cred = NULL;
}

*_expiration = expiration;
Expand Down Expand Up @@ -395,6 +426,9 @@ static int local_db_check_peruid_number_of_secrets(TALLOC_CTX *mem_ctx,

ret = EOK;
done:
if (res != NULL) {
db_result_erase_securely(res, SEC_ATTR_SECRET);
}
talloc_free(tmp_ctx);
return ret;
}
Expand Down Expand Up @@ -820,7 +854,7 @@ errno_t sss_sec_list(TALLOC_CTX *mem_ctx,
size_t *_num_keys)
{
TALLOC_CTX *tmp_ctx;
static const char *attrs[] = { SEC_ATTR_SECRET, NULL };
static const char *attrs[] = { NULL };
struct ldb_result *res;
char **keys;
int ret;
Expand Down Expand Up @@ -884,8 +918,8 @@ errno_t sss_sec_get(TALLOC_CTX *mem_ctx,
{
TALLOC_CTX *tmp_ctx;
static const char *attrs[] = { SEC_ATTR_SECRET, NULL };
struct ldb_result *res;
const struct ldb_val *attr_secret;
struct ldb_result *res = NULL;
const struct ldb_val *attr_secret = NULL;
int ret;

if (req == NULL || _secret == NULL) {
Expand Down Expand Up @@ -936,6 +970,7 @@ errno_t sss_sec_get(TALLOC_CTX *mem_ctx,
ret = ENOMEM;
goto done;
}
talloc_set_destructor((void *) *_secret, sss_erase_talloc_mem_securely);

if (_secret_len) {
*_secret_len = attr_secret->length;
Expand All @@ -944,6 +979,12 @@ errno_t sss_sec_get(TALLOC_CTX *mem_ctx,
ret = EOK;

done:
if (attr_secret != NULL) {
sss_erase_mem_securely(attr_secret->data, attr_secret->length);
}
if (res != NULL) {
db_result_erase_securely(res, SEC_ATTR_SECRET);
}
talloc_free(tmp_ctx);
return ret;
}
Expand All @@ -953,7 +994,8 @@ errno_t sss_sec_put(struct sss_sec_req *req,
size_t secret_len)
{
struct ldb_message *msg;
struct ldb_val secret_val;
struct ldb_val secret_val = { .data = NULL };
bool erase_msg = false;
int ret;

if (req == NULL || secret == NULL) {
Expand Down Expand Up @@ -1016,6 +1058,7 @@ errno_t sss_sec_put(struct sss_sec_req *req,
ret, sss_strerror(ret));
goto done;
}
erase_msg = true;

ret = ldb_msg_add_fmt(msg, SEC_ATTR_CTIME, "%lu", time(NULL));
if (ret != EOK) {
Expand All @@ -1041,6 +1084,12 @@ errno_t sss_sec_put(struct sss_sec_req *req,

ret = EOK;
done:
if (secret_val.data != NULL) {
sss_erase_mem_securely(secret_val.data, secret_val.length);
}
if (erase_msg) {
db_result_erase_message_securely(msg, SEC_ATTR_SECRET);
}
talloc_free(msg);
return ret;
}
Expand All @@ -1050,7 +1099,8 @@ errno_t sss_sec_update(struct sss_sec_req *req,
size_t secret_len)
{
struct ldb_message *msg;
struct ldb_val secret_val;
struct ldb_val secret_val = { .data = NULL };
bool erase_msg = false;
int ret;

if (req == NULL || secret == NULL) {
Expand Down Expand Up @@ -1122,6 +1172,7 @@ errno_t sss_sec_update(struct sss_sec_req *req,
ret = EIO;
goto done;
}
erase_msg = true;

ret = ldb_modify(req->sctx->ldb, msg);
if (ret == LDB_ERR_NO_SUCH_OBJECT) {
Expand All @@ -1138,6 +1189,12 @@ errno_t sss_sec_update(struct sss_sec_req *req,

ret = EOK;
done:
if (secret_val.data != NULL) {
sss_erase_mem_securely(secret_val.data, secret_val.length);
}
if (erase_msg) {
db_result_erase_message_securely(msg, SEC_ATTR_SECRET);
}
talloc_free(msg);
return ret;
}
Expand Down
Loading

0 comments on commit f358b4d

Please sign in to comment.