Fix issue #55 (#58)

* Fix issue #55

* Split hints into two separate files

Season-1/Level-2/code.h

@@ -15,42 +15,93 @@
 
 #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;
     char username[MAX_USERNAME_LEN + 1];
     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;
 }

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("<SOS> PRIVILEGE ESCALATION ATTACK DETECTED \n");
-
-    if (ua->isAdmin == 0) 
-        printf("CONGRATULATIONS LEVEL 2 PASSED!");
+    if (is_admin(user1))
+        printf("<SOS> PRIVILEGE ESCALATION ATTACK DETECTED\n");
+    else
+        printf("CONGRATULATIONS LEVEL 2 PASSED!\n");
 
     return 0;
 }

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.

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.

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,52 +13,117 @@
 
 #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;
     char username[MAX_USERNAME_LEN + 1];
     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.
 */

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;