Skip to content

Commit a5fcc23

Browse files
akopachovakopachov
authored
Fixing SONAR issues (#13)
* Secure cleanup of FuriString holding token secret in CLI * Few refactoring * Working on SONAR issues
1 parent 8d7167c commit a5fcc23

File tree

18 files changed

+154
-72
lines changed

18 files changed

+154
-72
lines changed

‎scenes/add_new_token/totp_input_text.c‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,16 @@
22
#include <gui/view_i.h>
33
#include "../../types/common.h"
44

5+
size_t strnlen(const char* s, size_t maxlen) {
6+
size_t len;
7+
8+
for(len = 0; len < maxlen; len++, s++) {
9+
if(!*s) break;
10+
}
11+
12+
return len;
13+
}
14+
515
void view_draw(View* view, Canvas* canvas) {
616
furi_assert(view);
717
if(view->draw_callback) {
@@ -32,10 +42,14 @@ static void commit_text_input_callback(void* context) {
3242
InputTextSceneState* text_input_state = (InputTextSceneState*)context;
3343
if(text_input_state->callback != 0) {
3444
InputTextSceneCallbackResult* result = malloc(sizeof(InputTextSceneCallbackResult));
35-
result->user_input_length = strlen(text_input_state->text_input_buffer);
45+
result->user_input_length =
46+
strnlen(text_input_state->text_input_buffer, INPUT_BUFFER_SIZE);
3647
result->user_input = malloc(result->user_input_length + 1);
3748
result->callback_data = text_input_state->callback_data;
38-
strcpy(result->user_input, text_input_state->text_input_buffer);
49+
strlcpy(
50+
result->user_input,
51+
text_input_state->text_input_buffer,
52+
result->user_input_length + 1);
3953
text_input_state->callback(result);
4054
}
4155
}

‎scenes/add_new_token/totp_input_text.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
typedef struct {
1212
char* user_input;
13-
uint8_t user_input_length;
13+
size_t user_input_length;
1414
void* callback_data;
1515
} InputTextSceneCallbackResult;
1616

‎scenes/add_new_token/totp_scene_add_new_token.c‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,9 @@ typedef enum {
2525

2626
typedef struct {
2727
char* token_name;
28-
uint8_t token_name_length;
28+
size_t token_name_length;
2929
char* token_secret;
30-
uint8_t token_secret_length;
30+
size_t token_secret_length;
3131
bool saved;
3232
Control selected_control;
3333
InputTextSceneContext* token_name_input_context;
@@ -235,7 +235,10 @@ bool totp_scene_add_new_token_handle_event(PluginEvent* const event, PluginState
235235

236236
if(token_secret_set) {
237237
tokenInfo->name = malloc(scene_state->token_name_length + 1);
238-
strcpy(tokenInfo->name, scene_state->token_name);
238+
strlcpy(
239+
tokenInfo->name,
240+
scene_state->token_name,
241+
scene_state->token_name_length + 1);
239242
tokenInfo->algo = scene_state->algo;
240243
tokenInfo->digits = scene_state->digits_count;
241244

‎scenes/generate_token/totp_scene_generate_token.c‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "../../services/totp/totp.h"
1010
#include "../../services/config/config.h"
1111
#include "../../services/crypto/crypto.h"
12+
#include "../../services/crypto/memset_s.h"
1213
#include "../scene_director.h"
1314
#include "../token_menu/totp_scene_token_menu.h"
1415

@@ -95,7 +96,7 @@ void update_totp_params(PluginState* const plugin_state) {
9596
}
9697
}
9798

98-
void totp_scene_generate_token_init(PluginState* plugin_state) {
99+
void totp_scene_generate_token_init(const PluginState* plugin_state) {
99100
UNUSED(plugin_state);
100101
}
101102

@@ -180,7 +181,7 @@ void totp_scene_generate_token_render(Canvas* const canvas, PluginState* plugin_
180181
->data);
181182

182183
if(tokenInfo->token != NULL && tokenInfo->token_length > 0) {
183-
uint8_t key_length;
184+
size_t key_length;
184185
uint8_t* key = totp_crypto_decrypt(
185186
tokenInfo->token, tokenInfo->token_length, &plugin_state->iv[0], &key_length);
186187

@@ -195,7 +196,7 @@ void totp_scene_generate_token_render(Canvas* const canvas, PluginState* plugin_
195196
TOKEN_LIFETIME),
196197
scene_state->last_code,
197198
tokenInfo->digits);
198-
memset(key, 0, key_length);
199+
memset_s(key, sizeof(key), 0, key_length);
199200
free(key);
200201
} else {
201202
i_token_to_str(0, scene_state->last_code, tokenInfo->digits);
@@ -265,7 +266,9 @@ void totp_scene_generate_token_render(Canvas* const canvas, PluginState* plugin_
265266
}
266267
}
267268

268-
bool totp_scene_generate_token_handle_event(PluginEvent* const event, PluginState* plugin_state) {
269+
bool totp_scene_generate_token_handle_event(
270+
const PluginEvent* const event,
271+
PluginState* plugin_state) {
269272
if(event->type == EventTypeKey) {
270273
if(event->input.type == InputTypeLong && event->input.key == InputKeyBack) {
271274
return false;
@@ -314,11 +317,10 @@ void totp_scene_generate_token_deactivate(PluginState* plugin_state) {
314317
if(plugin_state->current_scene_state == NULL) return;
315318
SceneState* scene_state = (SceneState*)plugin_state->current_scene_state;
316319

317-
free(scene_state->last_code);
318320
free(scene_state);
319321
plugin_state->current_scene_state = NULL;
320322
}
321323

322-
void totp_scene_generate_token_free(PluginState* plugin_state) {
324+
void totp_scene_generate_token_free(const PluginState* plugin_state) {
323325
UNUSED(plugin_state);
324326
}

‎scenes/generate_token/totp_scene_generate_token.h‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,13 @@ typedef struct {
1010
uint8_t current_token_index;
1111
} GenerateTokenSceneContext;
1212

13-
void totp_scene_generate_token_init(PluginState* plugin_state);
13+
void totp_scene_generate_token_init(const PluginState* plugin_state);
1414
void totp_scene_generate_token_activate(
1515
PluginState* plugin_state,
1616
const GenerateTokenSceneContext* context);
1717
void totp_scene_generate_token_render(Canvas* const canvas, PluginState* plugin_state);
18-
bool totp_scene_generate_token_handle_event(PluginEvent* const event, PluginState* plugin_state);
18+
bool totp_scene_generate_token_handle_event(
19+
const PluginEvent* const event,
20+
PluginState* plugin_state);
1921
void totp_scene_generate_token_deactivate(PluginState* plugin_state);
20-
void totp_scene_generate_token_free(PluginState* plugin_state);
22+
void totp_scene_generate_token_free(const PluginState* plugin_state);

‎services/cli/cli_helpers.c‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ bool totp_cli_ensure_authenticated(PluginState* plugin_state, Cli* cli) {
1111
}
1212

1313
TOTP_CLI_DELETE_LAST_LINE();
14-
fflush(stdout);
1514

1615
if(plugin_state->current_scene == TotpSceneAuthentication) {
1716
return false;

‎services/cli/cli_helpers.h‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,26 @@
1313
#define DOCOPT_OPTIONS "[options]"
1414
#define DOCOPT_DEFAULT(val) "[default: " val "]"
1515

16-
#define TOTP_CLI_PRINTF(format, ...) \
17-
_Pragma(STRINGIFY(GCC diagnostic push)); \
18-
_Pragma(STRINGIFY(GCC diagnostic ignored "-Wdouble-promotion")); \
19-
printf(format, ##__VA_ARGS__); \
20-
_Pragma(STRINGIFY(GCC diagnostic pop));
21-
22-
#define TOTP_CLI_DELETE_LAST_LINE() TOTP_CLI_PRINTF("\033[A\33[2K\r")
23-
#define TOTP_CLI_DELETE_CURRENT_LINE() TOTP_CLI_PRINTF("\33[2K\r")
16+
#define TOTP_CLI_PRINTF(format, ...) \
17+
do { \
18+
_Pragma(STRINGIFY(GCC diagnostic push)) \
19+
_Pragma(STRINGIFY(GCC diagnostic ignored "-Wdouble-promotion")) \
20+
printf(format, ##__VA_ARGS__); \
21+
_Pragma(STRINGIFY(GCC diagnostic pop)) \
22+
} while(false)
23+
24+
#define TOTP_CLI_DELETE_LAST_LINE() \
25+
TOTP_CLI_PRINTF("\033[A\33[2K\r"); \
26+
fflush(stdout)
27+
28+
#define TOTP_CLI_DELETE_CURRENT_LINE() \
29+
TOTP_CLI_PRINTF("\33[2K\r"); \
30+
fflush(stdout)
31+
32+
#define TOTP_CLI_DELETE_LAST_CHAR() \
33+
TOTP_CLI_PRINTF("\b \b"); \
34+
fflush(stdout)
35+
2436
#define TOTP_CLI_PRINT_INVALID_ARGUMENTS() \
2537
TOTP_CLI_PRINTF( \
2638
"Invalid command arguments. use \"help\" command to get list of available commands")

‎services/cli/commands/add/add.c‎

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -79,10 +79,16 @@ void totp_cli_command_add_docopt_options() {
7979
TOTP_CLI_COMMAND_ADD_ARG_UNSECURE_PREFIX) " Show console user input as-is without masking\r\n");
8080
}
8181

82+
static void furi_string_secure_free(FuriString* str) {
83+
for(long i = furi_string_size(str) - 1; i >= 0; i--) {
84+
furi_string_set_char(str, i, '\0');
85+
}
86+
87+
furi_string_free(str);
88+
}
89+
8290
void totp_cli_command_add_handle(PluginState* plugin_state, FuriString* args, Cli* cli) {
8391
FuriString* temp_str = furi_string_alloc();
84-
const char* temp_cstr;
85-
8692
TokenInfo* token_info = token_info_alloc();
8793

8894
// Reading token name
@@ -93,9 +99,9 @@ void totp_cli_command_add_handle(PluginState* plugin_state, FuriString* args, Cl
9399
return;
94100
}
95101

96-
temp_cstr = furi_string_get_cstr(temp_str);
97-
token_info->name = malloc(strlen(temp_cstr) + 1);
98-
strcpy(token_info->name, temp_cstr);
102+
size_t temp_cstr_len = furi_string_size(temp_str);
103+
token_info->name = malloc(temp_cstr_len + 1);
104+
strlcpy(token_info->name, furi_string_get_cstr(temp_str), temp_cstr_len + 1);
99105

100106
// Read optional arguments
101107
bool mask_user_input = true;
@@ -146,13 +152,15 @@ void totp_cli_command_add_handle(PluginState* plugin_state, FuriString* args, Cl
146152
uint8_t c;
147153
while(cli_read(cli, &c, 1) == 1) {
148154
if(c == CliSymbolAsciiEsc) {
155+
// Some keys generating escape-sequences
156+
// We need to ignore them as we case about alpha-numerics only
149157
uint8_t c2;
150158
cli_read_timeout(cli, &c2, 1, 0);
151159
cli_read_timeout(cli, &c2, 1, 0);
152160
} else if(c == CliSymbolAsciiETX) {
153161
TOTP_CLI_DELETE_CURRENT_LINE();
154-
TOTP_CLI_PRINTF("Cancelled by user");
155-
furi_string_free(temp_str);
162+
TOTP_CLI_PRINTF("Cancelled by user\r\n");
163+
furi_string_secure_free(temp_str);
156164
token_info_free(token_info);
157165
return;
158166
} else if((c >= '0' && c <= '9') || (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')) {
@@ -166,8 +174,7 @@ void totp_cli_command_add_handle(PluginState* plugin_state, FuriString* args, Cl
166174
} else if(c == CliSymbolAsciiBackspace || c == CliSymbolAsciiDel) {
167175
size_t temp_str_size = furi_string_size(temp_str);
168176
if(temp_str_size > 0) {
169-
TOTP_CLI_PRINTF("\b \b");
170-
fflush(stdout);
177+
TOTP_CLI_DELETE_LAST_CHAR();
171178
furi_string_left(temp_str, temp_str_size - 1);
172179
}
173180
} else if(c == CliSymbolAsciiCR) {
@@ -176,25 +183,26 @@ void totp_cli_command_add_handle(PluginState* plugin_state, FuriString* args, Cl
176183
}
177184
}
178185

179-
temp_cstr = furi_string_get_cstr(temp_str);
180-
181186
TOTP_CLI_DELETE_LAST_LINE();
182187

183188
if(!totp_cli_ensure_authenticated(plugin_state, cli)) {
184-
furi_string_free(temp_str);
189+
furi_string_secure_free(temp_str);
185190
token_info_free(token_info);
186191
return;
187192
}
188193

189-
if(!token_info_set_secret(token_info, temp_cstr, strlen(temp_cstr), plugin_state->iv)) {
194+
if(!token_info_set_secret(
195+
token_info,
196+
furi_string_get_cstr(temp_str),
197+
furi_string_size(temp_str),
198+
plugin_state->iv)) {
190199
TOTP_CLI_PRINTF("Token secret seems to be invalid and can not be parsed\r\n");
191-
furi_string_free(temp_str);
200+
furi_string_secure_free(temp_str);
192201
token_info_free(token_info);
193202
return;
194203
}
195204

196-
furi_string_reset(temp_str);
197-
furi_string_free(temp_str);
205+
furi_string_secure_free(temp_str);
198206

199207
bool load_generate_token_scene = false;
200208
if(plugin_state->current_scene == TotpSceneGenerateToken) {

‎services/config/config.c‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -343,9 +343,9 @@ TokenLoadingResult totp_config_file_load_tokens(PluginState* const plugin_state)
343343

344344
TokenInfo* tokenInfo = token_info_alloc();
345345

346-
const char* temp_cstr = furi_string_get_cstr(temp_str);
347-
tokenInfo->name = (char*)malloc(strlen(temp_cstr) + 1);
348-
strcpy(tokenInfo->name, temp_cstr);
346+
size_t temp_cstr_len = furi_string_size(temp_str);
347+
tokenInfo->name = (char*)malloc(temp_cstr_len + 1);
348+
strlcpy(tokenInfo->name, furi_string_get_cstr(temp_str), temp_cstr_len + 1);
349349

350350
uint32_t secret_bytes_count;
351351
if(!flipper_format_get_value_count(
@@ -355,9 +355,11 @@ TokenLoadingResult totp_config_file_load_tokens(PluginState* const plugin_state)
355355

356356
if(secret_bytes_count == 1) { // Plain secret key
357357
if(flipper_format_read_string(fff_data_file, TOTP_CONFIG_KEY_TOKEN_SECRET, temp_str)) {
358-
temp_cstr = furi_string_get_cstr(temp_str);
359358
if(token_info_set_secret(
360-
tokenInfo, temp_cstr, strlen(temp_cstr), &plugin_state->iv[0])) {
359+
tokenInfo,
360+
furi_string_get_cstr(temp_str),
361+
furi_string_size(temp_str),
362+
&plugin_state->iv[0])) {
361363
FURI_LOG_W(LOGGING_TAG, "Token \"%s\" has plain secret", tokenInfo->name);
362364
} else {
363365
tokenInfo->token = NULL;

‎services/crypto/crypto.c‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#include <furi_hal.h>
44
#include "../config/config.h"
55
#include "../../types/common.h"
6+
#include "memset_s.h"
67

78
#define CRYPTO_KEY_SLOT 2
89
#define CRYPTO_VERIFY_KEY "FFF_Crypto_pass"
@@ -11,13 +12,13 @@
1112

1213
uint8_t* totp_crypto_encrypt(
1314
const uint8_t* plain_data,
14-
const uint8_t plain_data_length,
15+
const size_t plain_data_length,
1516
const uint8_t* iv,
16-
uint8_t* encrypted_data_length) {
17+
size_t* encrypted_data_length) {
1718
uint8_t* encrypted_data;
1819
size_t remain = plain_data_length % CRYPTO_ALIGNMENT_FACTOR;
1920
if(remain) {
20-
uint8_t plain_data_aligned_length = plain_data_length - remain + CRYPTO_ALIGNMENT_FACTOR;
21+
size_t plain_data_aligned_length = plain_data_length - remain + CRYPTO_ALIGNMENT_FACTOR;
2122
uint8_t* plain_data_aligned = malloc(plain_data_aligned_length);
2223
memset(plain_data_aligned, 0, plain_data_aligned_length);
2324
memcpy(plain_data_aligned, plain_data, plain_data_length);
@@ -29,7 +30,7 @@ uint8_t* totp_crypto_encrypt(
2930
furi_hal_crypto_encrypt(plain_data_aligned, encrypted_data, plain_data_aligned_length);
3031
furi_hal_crypto_store_unload_key(CRYPTO_KEY_SLOT);
3132

32-
memset(plain_data_aligned, 0, plain_data_aligned_length);
33+
memset_s(plain_data_aligned, sizeof(plain_data_aligned), 0, plain_data_aligned_length);
3334
free(plain_data_aligned);
3435
} else {
3536
encrypted_data = malloc(plain_data_length);
@@ -45,9 +46,9 @@ uint8_t* totp_crypto_encrypt(
4546

4647
uint8_t* totp_crypto_decrypt(
4748
const uint8_t* encrypted_data,
48-
const uint8_t encrypted_data_length,
49+
const size_t encrypted_data_length,
4950
const uint8_t* iv,
50-
uint8_t* decrypted_data_length) {
51+
size_t* decrypted_data_length) {
5152
*decrypted_data_length = encrypted_data_length;
5253
uint8_t* decrypted_data = malloc(*decrypted_data_length);
5354
furi_hal_crypto_store_load_key(CRYPTO_KEY_SLOT, iv);
@@ -118,7 +119,7 @@ void totp_crypto_seed_iv(PluginState* plugin_state, uint8_t* pin, uint8_t pin_le
118119
}
119120

120121
bool totp_crypto_verify_key(const PluginState* plugin_state) {
121-
uint8_t decrypted_key_length;
122+
size_t decrypted_key_length;
122123
uint8_t* decrypted_key = totp_crypto_decrypt(
123124
plugin_state->crypto_verify_data,
124125
plugin_state->crypto_verify_data_length,

0 commit comments

Comments
 (0)