Skip to content

Commit

Permalink
dynsec: Fix modifyClient and modifyGroup commands
Browse files Browse the repository at this point in the history 
They will now not modify the client/group if a new group/client being
added is not valid, or on other failures.

Closes #2598. Thanks to Sebastian Szczepański.
  • Loading branch information
@ralight
ralight committed Aug 15, 2022
1 parent b22df51 commit 436f0b9
Show file tree
Hide file tree
Showing 3 changed files with 172 additions and 66 deletions.
3 changes: 3 additions & 0 deletions ChangeLog.txt
View file
Expand Up @@ -24,6 +24,9 @@ Broker:
directory. Closes #2520.
- Fix bridge queued messages not being persisted when local_cleansession is
set to false and cleansession is set to true. Closes #2604.
- Dynamic security: Fix modifyClient and modifyGroup commands to not modify
the client/group if a new group/client being added is not valid.
Closes #2598.

Client library:
- Fix threads library detection on Windows under cmake. Bumps the minimum
Expand Down
136 changes: 94 additions & 42 deletions plugins/dynamic-security/clients.c
View file
Expand Up @@ -720,10 +720,12 @@ static void client__remove_all_roles(struct dynsec__client *client)
int dynsec_clients__process_modify(cJSON *j_responses, struct mosquitto *context, cJSON *command, char *correlation_data)
{
char *username;
char *clientid;
char *password;
char *text_name, *text_description;
char *clientid = NULL;
char *password = NULL;
char *text_name = NULL, *text_description = NULL;
bool have_clientid = false, have_text_name = false, have_text_description = false, have_rolelist = false, have_password = false;
struct dynsec__client *client;
struct dynsec__group *group;
struct dynsec__rolelist *rolelist = NULL;
char *str;
int rc;
Expand All @@ -746,81 +748,87 @@ int dynsec_clients__process_modify(cJSON *j_responses, struct mosquitto *context
return MOSQ_ERR_INVAL;
}

if(json_get_string(command, "clientid", &clientid, false) == MOSQ_ERR_SUCCESS){
if(clientid && strlen(clientid) > 0){
str = mosquitto_strdup(clientid);
if(str == NULL){
if(json_get_string(command, "clientid", &str, false) == MOSQ_ERR_SUCCESS){
have_clientid = true;
if(str && strlen(str) > 0){
clientid = mosquitto_strdup(str);
if(clientid == NULL){
dynsec__command_reply(j_responses, context, "modifyClient", "Internal error", correlation_data);
return MOSQ_ERR_NOMEM;
rc = MOSQ_ERR_NOMEM;
goto error;
}
}else{
str = NULL;
clientid = NULL;
}
mosquitto_free(client->clientid);
client->clientid = str;
}

if(json_get_string(command, "password", &password, false) == MOSQ_ERR_SUCCESS){
if(strlen(password) > 0){
/* If password == "", we just ignore it */
rc = client__set_password(client, password);
if(rc != MOSQ_ERR_SUCCESS){
dynsec__command_reply(j_responses, context, "modifyClient", "Internal error", correlation_data);
mosquitto_kick_client_by_username(username, false);
return MOSQ_ERR_NOMEM;
}
have_password = true;
}
}

if(json_get_string(command, "textname", &text_name, false) == MOSQ_ERR_SUCCESS){
str = mosquitto_strdup(text_name);
if(str == NULL){
if(json_get_string(command, "textname", &str, false) == MOSQ_ERR_SUCCESS){
have_text_name = true;
text_name = mosquitto_strdup(str);
if(text_name == NULL){
dynsec__command_reply(j_responses, context, "modifyClient", "Internal error", correlation_data);
mosquitto_kick_client_by_username(username, false);
return MOSQ_ERR_NOMEM;
rc = MOSQ_ERR_NOMEM;
goto error;
}
mosquitto_free(client->text_name);
client->text_name = str;
}

if(json_get_string(command, "textdescription", &text_description, false) == MOSQ_ERR_SUCCESS){
str = mosquitto_strdup(text_description);
if(str == NULL){
if(json_get_string(command, "textdescription", &str, false) == MOSQ_ERR_SUCCESS){
have_text_description = true;
text_description = mosquitto_strdup(str);
if(text_description == NULL){
dynsec__command_reply(j_responses, context, "modifyClient", "Internal error", correlation_data);
mosquitto_kick_client_by_username(username, false);
return MOSQ_ERR_NOMEM;
rc = MOSQ_ERR_NOMEM;
goto error;
}
mosquitto_free(client->text_description);
client->text_description = str;
}

rc = dynsec_rolelist__load_from_json(command, &rolelist);
if(rc == MOSQ_ERR_SUCCESS){
client__remove_all_roles(client);
client__add_new_roles(client, rolelist);
dynsec_rolelist__cleanup(&rolelist);
have_rolelist = true;
}else if(rc == ERR_LIST_NOT_FOUND){
/* There was no list in the JSON, so no modification */
}else if(rc == MOSQ_ERR_NOT_FOUND){
dynsec__command_reply(j_responses, context, "modifyClient", "Role not found", correlation_data);
dynsec_rolelist__cleanup(&rolelist);
mosquitto_kick_client_by_username(username, false);
return MOSQ_ERR_INVAL;
rc = MOSQ_ERR_INVAL;
goto error;
}else{
if(rc == MOSQ_ERR_INVAL){
dynsec__command_reply(j_responses, context, "modifyClient", "'roles' not an array or missing/invalid rolename", correlation_data);
}else{
dynsec__command_reply(j_responses, context, "modifyClient", "Internal error", correlation_data);
}
dynsec_rolelist__cleanup(&rolelist);
mosquitto_kick_client_by_username(username, false);
return MOSQ_ERR_INVAL;
rc = MOSQ_ERR_INVAL;
goto error;
}

j_groups = cJSON_GetObjectItem(command, "groups");
if(j_groups && cJSON_IsArray(j_groups)){
dynsec__remove_client_from_all_groups(username);
/* Iterate through list to check all groups are valid */
cJSON_ArrayForEach(j_group, j_groups){
if(cJSON_IsObject(j_group)){
jtmp = cJSON_GetObjectItem(j_group, "groupname");
if(jtmp && cJSON_IsString(jtmp)){
group = dynsec_groups__find(jtmp->valuestring);
if(group == NULL){
dynsec__command_reply(j_responses, context, "modifyClient", "'groups' contains an object with a 'groupname' that does not exist", correlation_data);
rc = MOSQ_ERR_INVAL;
goto error;
}
}else{
dynsec__command_reply(j_responses, context, "modifyClient", "'groups' contains an object with an invalid 'groupname'", correlation_data);
rc = MOSQ_ERR_INVAL;
goto error;
}
}
}

dynsec__remove_client_from_all_groups(username);
cJSON_ArrayForEach(j_group, j_groups){
if(cJSON_IsObject(j_group)){
jtmp = cJSON_GetObjectItem(j_group, "groupname");
Expand All @@ -832,6 +840,44 @@ int dynsec_clients__process_modify(cJSON *j_responses, struct mosquitto *context
}
}

if(have_password){
/* FIXME - This is the one call that will result in modification on internal error - note that groups have already been modified */
rc = client__set_password(client, password);
if(rc != MOSQ_ERR_SUCCESS){
dynsec__command_reply(j_responses, context, "modifyClient", "Internal error", correlation_data);
mosquitto_kick_client_by_username(username, false);
/* If this fails we have the situation that the password is set as
* invalid, but the config isn't saved, so restarting the broker
* *now* will mean the client can log in again. This might be
* "good", but is inconsistent, so save the config to be
* consistent. */
dynsec__config_save();
rc = MOSQ_ERR_NOMEM;
goto error;
}
}

if(have_clientid){
mosquitto_free(client->clientid);
client->clientid = clientid;
}

if(have_text_name){
mosquitto_free(client->text_name);
client->text_name = text_name;
}

if(have_text_description){
mosquitto_free(client->text_description);
client->text_description = text_description;
}

if(have_rolelist){
client__remove_all_roles(client);
client__add_new_roles(client, rolelist);
dynsec_rolelist__cleanup(&rolelist);
}

dynsec__config_save();
dynsec__command_reply(j_responses, context, "modifyClient", NULL, correlation_data);

Expand All @@ -843,6 +889,12 @@ int dynsec_clients__process_modify(cJSON *j_responses, struct mosquitto *context
mosquitto_log_printf(MOSQ_LOG_INFO, "dynsec: %s/%s | modifyClient | username=%s",
admin_clientid, admin_username, username);
return MOSQ_ERR_SUCCESS;
error:
mosquitto_free(clientid);
mosquitto_free(text_name);
mosquitto_free(text_description);
dynsec_rolelist__cleanup(&rolelist);
return rc;
}


Expand Down
99 changes: 75 additions & 24 deletions plugins/dynamic-security/groups.c
View file
Expand Up @@ -911,10 +911,12 @@ int dynsec_groups__process_remove_role(cJSON *j_responses, struct mosquitto *con

int dynsec_groups__process_modify(cJSON *j_responses, struct mosquitto *context, cJSON *command, char *correlation_data)
{
char *groupname;
char *text_name, *text_description;
struct dynsec__group *group;
char *groupname = NULL;
char *text_name = NULL, *text_description = NULL;
struct dynsec__client *client = NULL;
struct dynsec__group *group = NULL;
struct dynsec__rolelist *rolelist = NULL;
bool have_text_name = false, have_text_description = false, have_rolelist = false;
char *str;
int rc;
int priority;
Expand All @@ -936,52 +938,73 @@ int dynsec_groups__process_modify(cJSON *j_responses, struct mosquitto *context,
return MOSQ_ERR_INVAL;
}

if(json_get_string(command, "textname", &text_name, false) == MOSQ_ERR_SUCCESS){
str = mosquitto_strdup(text_name);
if(str == NULL){
if(json_get_string(command, "textname", &str, false) == MOSQ_ERR_SUCCESS){
have_text_name = true;
text_name = mosquitto_strdup(str);
if(text_name == NULL){
dynsec__command_reply(j_responses, context, "modifyGroup", "Internal error", correlation_data);
return MOSQ_ERR_NOMEM;
rc = MOSQ_ERR_NOMEM;
goto error;
}
mosquitto_free(group->text_name);
group->text_name = str;
}

if(json_get_string(command, "textdescription", &text_description, false) == MOSQ_ERR_SUCCESS){
str = mosquitto_strdup(text_description);
if(str == NULL){
if(json_get_string(command, "textdescription", &str, false) == MOSQ_ERR_SUCCESS){
have_text_description = true;
text_description = mosquitto_strdup(str);
if(text_description == NULL){
dynsec__command_reply(j_responses, context, "modifyGroup", "Internal error", correlation_data);
return MOSQ_ERR_NOMEM;
rc = MOSQ_ERR_NOMEM;
goto error;
}
mosquitto_free(group->text_description);
group->text_description = str;
}

rc = dynsec_rolelist__load_from_json(command, &rolelist);
if(rc == MOSQ_ERR_SUCCESS){
dynsec_rolelist__cleanup(&group->rolelist);
group->rolelist = rolelist;
/* Apply changes below */
have_rolelist = true;
}else if(rc == ERR_LIST_NOT_FOUND){
/* There was no list in the JSON, so no modification */
rolelist = NULL;
}else if(rc == MOSQ_ERR_NOT_FOUND){
dynsec__command_reply(j_responses, context, "modifyGroup", "Role not found", correlation_data);
dynsec_rolelist__cleanup(&rolelist);
group__kick_all(group);
return MOSQ_ERR_INVAL;
rc = MOSQ_ERR_INVAL;
goto error;
}else{
if(rc == MOSQ_ERR_INVAL){
dynsec__command_reply(j_responses, context, "modifyGroup", "'roles' not an array or missing/invalid rolename", correlation_data);
}else{
dynsec__command_reply(j_responses, context, "modifyGroup", "Internal error", correlation_data);
}
dynsec_rolelist__cleanup(&rolelist);
group__kick_all(group);
return MOSQ_ERR_INVAL;
rc = MOSQ_ERR_INVAL;
goto error;
}

j_clients = cJSON_GetObjectItem(command, "clients");
if(j_clients && cJSON_IsArray(j_clients)){
/* Iterate over array to check clients are valid before proceeding */
cJSON_ArrayForEach(j_client, j_clients){
if(cJSON_IsObject(j_client)){
jtmp = cJSON_GetObjectItem(j_client, "username");
if(jtmp && cJSON_IsString(jtmp)){
client = dynsec_clients__find(jtmp->valuestring);
if(client == NULL){
dynsec__command_reply(j_responses, context, "modifyGroup", "'clients' contains an object with a 'username' that does not exist", correlation_data);
rc = MOSQ_ERR_INVAL;
goto error;
}
}else{
dynsec__command_reply(j_responses, context, "modifyGroup", "'clients' contains an object with an invalid 'username'", correlation_data);
rc = MOSQ_ERR_INVAL;
goto error;
}
}
}

/* Kick all clients in the *current* group */
group__kick_all(group);
dynsec__remove_all_clients_from_group(group);

/* Now we can add the new clients to the group */
cJSON_ArrayForEach(j_client, j_clients){
if(cJSON_IsObject(j_client)){
jtmp = cJSON_GetObjectItem(j_client, "username");
Expand All @@ -993,11 +1016,28 @@ int dynsec_groups__process_modify(cJSON *j_responses, struct mosquitto *context,
}
}

/* Apply remaining changes to group, note that user changes are already applied */
if(have_text_name){
mosquitto_free(group->text_name);
group->text_name = text_name;
}

if(have_text_description){
mosquitto_free(group->text_description);
group->text_description = text_description;
}

if(have_rolelist){
dynsec_rolelist__cleanup(&group->rolelist);
group->rolelist = rolelist;
}

/* And save */
dynsec__config_save();

dynsec__command_reply(j_responses, context, "modifyGroup", NULL, correlation_data);

/* Enforce any changes */
/* Enforce any changes - kick any clients in the *new* group */
group__kick_all(group);

admin_clientid = mosquitto_client_id(context);
Expand All @@ -1006,6 +1046,17 @@ int dynsec_groups__process_modify(cJSON *j_responses, struct mosquitto *context,
admin_clientid, admin_username, groupname);

return MOSQ_ERR_SUCCESS;
error:
mosquitto_free(text_name);
mosquitto_free(text_description);
dynsec_rolelist__cleanup(&rolelist);

admin_clientid = mosquitto_client_id(context);
admin_username = mosquitto_client_username(context);
mosquitto_log_printf(MOSQ_LOG_INFO, "dynsec: %s/%s | modifyGroup | groupname=%s",
admin_clientid, admin_username, groupname);

return rc;
}


Expand Down

0 comments on commit 436f0b9

Please sign in to comment.