diff --git a/Season-1/Level-2/code.h b/Season-1/Level-2/code.h index cef6404..6543b7e 100644 --- a/Season-1/Level-2/code.h +++ b/Season-1/Level-2/code.h @@ -15,8 +15,21 @@ #define MAX_USERNAME_LEN 39 #define SETTINGS_COUNT 10 -int userid_next = 1; +#define MAX_USERS 100 +#define INVALID_USER_ID -1 +/* + To keep things simple, both private (implementation specific) and public (API) parts of + the application have been bundled inside this header file. In reality, you would + only keep the API here. That being said, assume that the private sections would not be + known to casual users of this module. +*/ + +// Internal counter of user accounts. +int userid_next = 0; + +// This whole structure is purely an implementation detail and is supposed to be +// unknown for normal users. typedef struct { bool isAdmin; long userid; @@ -24,33 +37,71 @@ typedef struct { long setting[SETTINGS_COUNT]; } user_account; -user_account* create_user_account(bool isAdmin, const char* username) { - user_account* ua; - if (strlen(username) > MAX_USERNAME_LEN) - return NULL; +// Simulates an internal store of active user accounts. +user_account *accounts[MAX_USERS]; + +// The signatures of the next 4 functions together with previously introduced constants (see #DEFINEs) +// constitute the API of this module. + +// Creates a new user account and returns it's unique identifier. +int create_user_account(bool isAdmin, const char *username) { + if (userid_next >= MAX_USERS) { + fprintf(stderr, "the maximum number of users have been exceeded"); + return INVALID_USER_ID; + } + + user_account *ua; + if (strlen(username) > MAX_USERNAME_LEN) { + fprintf(stderr, "the username is too long"); + return INVALID_USER_ID; + } ua = malloc(sizeof (user_account)); - if (NULL == ua) { + if (ua == NULL) { fprintf(stderr, "malloc failed to allocate memory"); - return NULL; + return INVALID_USER_ID; } ua->isAdmin = isAdmin; ua->userid = userid_next++; strcpy(ua->username, username); memset(&ua->setting, 0, sizeof ua->setting); - return ua; + accounts[userid_next] = ua; + return userid_next++; } -bool update_setting(user_account* ua, const char *index, const char *value) { +// Updates the matching setting for the specified user and returns the status of the operation. +// A setting is some arbitrary string associated with an index as a key. +bool update_setting(int user_id, const char *index, const char *value) { + if (user_id < 0 || user_id >= MAX_USERS) + return false; + char *endptr; long i, v; i = strtol(index, &endptr, 10); if (*endptr) return false; - if (i >= SETTINGS_COUNT) - return false; + v = strtol(value, &endptr, 10); - if (*endptr) + if (*endptr || i >= SETTINGS_COUNT) return false; - ua->setting[i] = v; + accounts[user_id]->setting[i] = v; return true; +} + +// Returns whether the specified user is an admin or not. +bool is_admin(int user_id) { + if (user_id < 0 || user_id >= MAX_USERS) { + fprintf(stderr, "invalid user id"); + return false; + } + return accounts[user_id]->isAdmin; +} + +// Returns the username of the specified user. +const char* username(int user_id) { + // A better approach would be to signal an error. + if (user_id < 0 || user_id >= MAX_USERS) { + fprintf(stderr, "invalid user id"); + return NULL; + } + return accounts[user_id]->username; } \ No newline at end of file diff --git a/Season-1/Level-2/hack.c b/Season-1/Level-2/hack.c index dfc92f1..cbd86fd 100644 --- a/Season-1/Level-2/hack.c +++ b/Season-1/Level-2/hack.c @@ -20,19 +20,18 @@ int main() { printf("Level 2 \n\n"); // Creates a non-admin username called "pwned" - user_account* ua = create_user_account(false, "pwned"); - printf("0. Non-admin (admin:%i) username called '%s' has been created \n", ua->isAdmin, ua->username); + int user1 = create_user_account(false, "pwned"); + printf("0. Non-admin (admin:%i) username called '%s' has been created \n", is_admin(user1), username(user1)); // An outsider or an insider managed to supply the following input that originally aimed to change a dummy non-admin setting. - update_setting(ua, "-7", "1"); + update_setting(user1, "-7", "1"); printf("1. A dummy setting has been set to dummy number '1' \n"); - printf("2. Making sure user '%s' is not an admin by performing a check -> [Result] Admin:%i \n\n", ua->username, ua->isAdmin); + printf("2. Making sure user '%s' is not an admin by performing a check -> [Result] Admin:%i \n\n", username(user1), is_admin(user1)); - if (ua->isAdmin == 1) - printf(" PRIVILEGE ESCALATION ATTACK DETECTED \n"); - - if (ua->isAdmin == 0) - printf("CONGRATULATIONS LEVEL 2 PASSED!"); + if (is_admin(user1)) + printf(" PRIVILEGE ESCALATION ATTACK DETECTED\n"); + else + printf("CONGRATULATIONS LEVEL 2 PASSED!\n"); return 0; } \ No newline at end of file diff --git a/Season-1/Level-2/hint-1.txt b/Season-1/Level-2/hint-1.txt new file mode 100644 index 0000000..6eb37f9 --- /dev/null +++ b/Season-1/Level-2/hint-1.txt @@ -0,0 +1,4 @@ +Think about what could happen if an attacker would figure out the +details of the user_account structure (see code.h). + +Try again the exercise without looking further into hint-2.txt. \ No newline at end of file diff --git a/Season-1/Level-2/hint-2.txt b/Season-1/Level-2/hint-2.txt new file mode 100644 index 0000000..067a5c5 --- /dev/null +++ b/Season-1/Level-2/hint-2.txt @@ -0,0 +1,2 @@ +Have a look inside hack.c and look at what the attacker is passing as an argument. +Then think if that value is overwriting something in memory. \ No newline at end of file diff --git a/Season-1/Level-2/hint.txt b/Season-1/Level-2/hint.txt deleted file mode 100644 index 4c6795b..0000000 --- a/Season-1/Level-2/hint.txt +++ /dev/null @@ -1,6 +0,0 @@ -Have a look inside hack.c and look at what the attacker is passing as an argument. -Then think if that value is overwriting something in memory. - - -Contribute new levels to the game in 3 simple steps! -Read our Contribution Guideline at github.com/skills/secure-code-game/blob/main/CONTRIBUTING.md \ No newline at end of file diff --git a/Season-1/Level-2/solution.c b/Season-1/Level-2/solution.c index 530952a..094e962 100644 --- a/Season-1/Level-2/solution.c +++ b/Season-1/Level-2/solution.c @@ -1,7 +1,7 @@ /////////////////////////////////////////////////// /// /// -/// Vulnerability was in line 49 of code.h /// -/// Fix can be found in line 47 below /// +/// Vulnerability was in line 84 of code.h /// +/// Fix can be found in line 83 below /// /// /// /////////////////////////////////////////////////// @@ -13,8 +13,21 @@ #define MAX_USERNAME_LEN 39 #define SETTINGS_COUNT 10 -int userid_next = 1; +#define MAX_USERS 100 +#define INVALID_USER_ID -1 +/* + To keep things simple, both private (implementation specific) and public (API) parts of + the application have been bundled inside this header file. In reality, you would + only keep the API here. That being said, assume that the private sections would not be + known to casual users of this module. +*/ + +// Internal counter of user accounts. +int userid_next = 0; + +// This whole structure is purely an implementation detail and is supposed to be +// unknown for normal users. typedef struct { bool isAdmin; long userid; @@ -22,43 +35,95 @@ typedef struct { long setting[SETTINGS_COUNT]; } user_account; -user_account* create_user_account(bool isAdmin, const char* username) { - user_account* ua; - if (strlen(username) > MAX_USERNAME_LEN) - return NULL; +// Simulates an internal store of active user accounts. +user_account *accounts[MAX_USERS]; + +// The signatures of the next 4 functions together with previously introduced constants (see #DEFINEs) +// constitute the API of this module. + +// Creates a new user account and returns it's unique identifier. +int create_user_account(bool isAdmin, const char *username) { + if (userid_next >= MAX_USERS) { + fprintf(stderr, "the maximum number of users have been exceeded"); + return INVALID_USER_ID; + } + + user_account *ua; + if (strlen(username) > MAX_USERNAME_LEN) { + fprintf(stderr, "the username is too long"); + return INVALID_USER_ID; + } ua = malloc(sizeof (user_account)); - if (NULL == ua) { + if (ua == NULL) { fprintf(stderr, "malloc failed to allocate memory"); - return NULL; + return INVALID_USER_ID; } ua->isAdmin = isAdmin; ua->userid = userid_next++; strcpy(ua->username, username); memset(&ua->setting, 0, sizeof ua->setting); - return ua; + accounts[userid_next] = ua; + return userid_next++; } -bool update_setting(user_account* ua, const char *index, const char *value) { +// Updates the matching setting for the specified user and returns the status of the operation. +// A setting is some arbitrary string associated with an index as a key. +bool update_setting(int user_id, const char *index, const char *value) { + if (user_id < 0 || user_id >= MAX_USERS) + return false; + char *endptr; long i, v; i = strtol(index, &endptr, 10); if (*endptr) return false; - if (i < 0 || i >= SETTINGS_COUNT) // FIX - return false; + v = strtol(value, &endptr, 10); - if (*endptr) + // FIX: Checking for negative index values, too! + if (*endptr || i < 0 || i >= SETTINGS_COUNT) return false; - ua->setting[i] = v; + accounts[user_id]->setting[i] = v; return true; } +// Returns whether the specified user is an admin or not. +bool is_admin(int user_id) { + if (user_id < 0 || user_id >= MAX_USERS) { + fprintf(stderr, "invalid user id"); + return false; + } + return accounts[user_id]->isAdmin; +} + +// Returns the username of the specified user. +const char* username(int user_id) { + // A better approach would be to signal an error. + if (user_id < 0 || user_id >= MAX_USERS) { + fprintf(stderr, "invalid user id"); + return NULL; + } + return accounts[user_id]->username; +} + /* -Buffer Overflow Vulnerability + Security through Obscurity Abuse Vulnerability + -------------------------------------------- + You may read about the concept of security through obscurity here: + https://en.wikipedia.org/wiki/Security_through_obscurity + + In code.h the user_account structure is supposed to be an implementation + detail not handed over to the user. Otherwise, they could easily modify the + structure and change the isAdmin flag to true, thus gaining admin privileges. + + Nonetheless, as this example illustrates, security through obscurity alone is not enough + to secure your system. The attacker can easily reverse engineer the code and + find the vulnerability. This is exposed in hack.c (see below). -In hack.c, an attacker escalated privileges and became an admin by abusing -the fact that the code wasn't checking for negative index values. + Buffer Overflow Vulnerability + ---------------------------- + In hack.c, an attacker escalated privileges and became an admin by abusing + the fact that the code wasn't checking for negative index values. -Negative indexing here caused an unauthorized write to memory and affected a -flag, changing a non-admin user to admin. + Negative indexing here caused an unauthorized write to memory and affected a + flag, changing a non-admin user to admin. */ \ No newline at end of file diff --git a/Season-1/Level-2/tests.c b/Season-1/Level-2/tests.c index b33fa1e..26701a4 100644 --- a/Season-1/Level-2/tests.c +++ b/Season-1/Level-2/tests.c @@ -14,18 +14,18 @@ int main() { printf("Level 2 \n\n"); // Creates a non-admin username called "pwned" - user_account* ua = create_user_account(false, "pwned"); - printf("0. Non-admin (admin:%i) username called '%s' has been created \n\n", ua->isAdmin, ua->username); + int user1 = create_user_account(false, "pwned"); + printf("0. Non-admin (admin:%i) username called '%s' has been created \n\n", is_admin(user1), username(user1)); - printf("1. Non-admin users like '%s' can update some dummy numerical settings \n", ua->username); + printf("1. Non-admin users like '%s' can update some dummy numerical settings \n", username(user1)); printf("2. Non-admin users have no access to settings that can escalate themselves to admins \n\n"); // Updates the setting '1' of the pwned username to the number '10' - update_setting(ua, "1", "10"); - printf("3. Dummy setting '1' has been now set to dummy number '10' for user '%s' \n", ua->username); - printf("4. Making sure user '%s' is not an admin by performing a check -> [Result] Admin:%i \n\n", ua->username, ua->isAdmin); + update_setting(user1, "1", "10"); + printf("3. Dummy setting '1' has been now set to dummy number '10' for user '%s' \n", username(user1)); + printf("4. Making sure user '%s' is not an admin by performing a check -> [Result] Admin:%i \n\n", username(user1), is_admin(user1)); - if (ua->isAdmin == 0) + if (!is_admin(user1)) printf("User is not an admin so the code works as expected... is it though? \n"); return 0;