Adds more references to the model solution and explains the decouplin…

…g of level 2 API

Season-1/Level-1/hack.py

@@ -3,23 +3,23 @@
 
 class TestOnlineStore(unittest.TestCase):
 
-    # Tricks the system and walks away with 1 television, despite valid payment & reimbursement.
+    # Tricks the system and walks away with 1 television, despite valid payment & reimbursement
     def test_6(self):
         tv_item = c.Item(type='product', description='tv', amount=1000.00, quantity=1)
         payment = c.Item(type='payment', description='invoice_4', amount=1e19, quantity=1)
         payback = c.Item(type='payment', description='payback_4', amount=-1e19, quantity=1)
         order_4 = c.Order(id='4', items=[payment, tv_item, payback])
         self.assertEqual(c.validorder(order_4), 'Order ID: 4 - Payment imbalance: $-1000.00')
 
-    # Valid payments that should add up correctly, but do not.
+    # Valid payments that should add up correctly, but do not
     def test_7(self):
         small_item = c.Item(type='product', description='accessory', amount=3.3, quantity=1)
         payment_1 = c.Item(type='payment', description='invoice_5_1', amount=1.1, quantity=1)
         payment_2 = c.Item(type='payment', description='invoice_5_2', amount=2.2, quantity=1)
         order_5 = c.Order(id='5', items=[small_item, payment_1, payment_2])
         self.assertEqual(c.validorder(order_5), 'Order ID: 5 - Full payment received!')
 
-    # The total amount payable in an order should be limited.
+    # The total amount payable in an order should be limited
     def test_8(self):
         num_items = 12
         items = [c.Item(type='product', description='tv', amount=99999, quantity=num_items)]

Season-1/Level-2/code.h

@@ -18,32 +18,31 @@
 #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.
-*/
+// For simplicity, both the private (implementation specific) and the public (API) parts 
+// of this application have been combined inside this header file. In the real-world, it
+// is expected for the public (API) parts only to be presented here. Therefore, for the 
+// purpose of this level, please assume that the private (implementation specific) sections 
+// of this file, would not be known to the non-privileged users of this application
 
-// Internal counter of user accounts.
+// 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.
+// The following structure is implementation-speicific and it's supposed to be unknown 
+// to non-privileged users
 typedef struct {
     bool isAdmin;
     long userid;
     char username[MAX_USERNAME_LEN + 1];
     long setting[SETTINGS_COUNT];
 } user_account;
 
-// Simulates an internal store of active user accounts.
+// 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.
+// The signatures of the following four functions together with the previously introduced 
+// constants (see #DEFINEs) constitute the API of this module
 
-// Creates a new user account and returns it's unique identifier.
+// 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");
@@ -68,8 +67,8 @@ int create_user_account(bool isAdmin, const char *username) {
     return userid_next++;
 }
 
-// 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.
+// 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;
@@ -87,7 +86,7 @@ bool update_setting(int user_id, const char *index, const char *value) {
     return true;
 }
 
-// Returns whether the specified user is an admin or not.
+// Returns whether the specified user is an admin
 bool is_admin(int user_id) {
     if (user_id < 0 || user_id >= MAX_USERS) {
         fprintf(stderr, "invalid user id");
@@ -96,9 +95,9 @@ bool is_admin(int user_id) {
     return accounts[user_id]->isAdmin;
 }
 
-// Returns the username of the specified user.
+// Returns the username of the specified user
 const char* username(int user_id) {
-    // A better approach would be to signal an error.
+    // Returns an error for invalid user ids
     if (user_id < 0 || user_id >= MAX_USERS) {
         fprintf(stderr, "invalid user id");
         return NULL;

Season-1/Level-2/hack.c

@@ -23,7 +23,7 @@ int main() {
     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.
+    // An outsider or an insider managed to supply the following input that originally aimed to change a dummy non-admin setting
     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", username(user1), is_admin(user1));

Season-1/Level-2/hint-1.txt

@@ -1,4 +1,5 @@
-Think about what could happen if an attacker would figure out the
-details of the user_account structure (see code.h).
+Consider what can happen if an attacker figures out the private,
+implementation-specific details of the user_account structure 
+inside code.h. What can the attacker do with this information? 
 
-Try again the exercise without looking further into hint-2.txt.
+Go back and try the exercise without looking hint-2.txt ;)

Season-1/Level-2/hint-2.txt

@@ -1,2 +1,6 @@
 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.
+Is that input able to overwrite something important somewhere?
+
+
+# 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

Season-1/Level-2/solution.c

@@ -1,7 +1,7 @@
 ///////////////////////////////////////////////////
 ///                                             ///
-///   Vulnerability was in line 84 of code.h    ///
-///   Fix can be found in line 83 below         ///
+///   Vulnerability was in line 83 of code.h    ///
+///   Fix can be found in line 82 below         ///
 ///                                             ///
 ///////////////////////////////////////////////////
 
@@ -16,32 +16,31 @@
 #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.
-*/
+// For simplicity, both the private (implementation specific) and the public (API) parts 
+// of this application have been combined inside this header file. In the real-world, it
+// is expected for the public (API) parts only to be presented here. Therefore, for the 
+// purpose of this level, please assume that the private (implementation specific) sections 
+// of this file, would not be known to the non-privileged users of this application
 
-// Internal counter of user accounts.
+// 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.
+// The following structure is implementation-speicific and it's supposed to be unknown 
+// to non-privileged users
 typedef struct {
     bool isAdmin;
     long userid;
     char username[MAX_USERNAME_LEN + 1];
     long setting[SETTINGS_COUNT];
 } user_account;
 
-// Simulates an internal store of active user accounts.
+// 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.
+// The signatures of the following four functions together with the previously introduced 
+// constants (see #DEFINEs) constitute the API of this module
 
-// Creates a new user account and returns it's unique identifier.
+// 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");
@@ -66,8 +65,8 @@ int create_user_account(bool isAdmin, const char *username) {
     return userid_next++;
 }
 
-// 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.
+// 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;
@@ -79,14 +78,14 @@ bool update_setting(int user_id, const char *index, const char *value) {
         return false;
 
     v = strtol(value, &endptr, 10);
-    // FIX: Checking for negative index values, too!
+    // FIX: We should check for negative index values too! Scroll for the full solution
     if (*endptr || i < 0 || i >= SETTINGS_COUNT)
         return false;
     accounts[user_id]->setting[i] = v;
     return true;
 }
 
-// Returns whether the specified user is an admin or not.
+// Returns whether the specified user is an admin
 bool is_admin(int user_id) {
     if (user_id < 0 || user_id >= MAX_USERS) {
         fprintf(stderr, "invalid user id");
@@ -95,9 +94,9 @@ bool is_admin(int user_id) {
     return accounts[user_id]->isAdmin;
 }
 
-// Returns the username of the specified user.
+// Returns the username of the specified user
 const char* username(int user_id) {
-    // A better approach would be to signal an error.
+    // Returns an error for invalid user ids
     if (user_id < 0 || user_id >= MAX_USERS) {
         fprintf(stderr, "invalid user id");
         return NULL;
@@ -106,24 +105,41 @@ const char* username(int user_id) {
 }
 
 /*
-    Security through Obscurity Abuse Vulnerability
+    There are two vulnerabilities in this code:
+  
+    (1) Security through Obscurity Abuse Vulnerability
     --------------------------------------------
-    You may read about the concept of security through obscurity here:
-    https://en.wikipedia.org/wiki/Security_through_obscurity
+
+    The concept of security through obscurity (STO) relies on the idea that a 
+    system can remain secure if something (even a vulnerability!) is secret or 
+    hidden. If an attacker doesn't know what the weaknesses are, they cannot 
+    exploit them. The flip side is that once that vulnerability is exposed, 
+    it's no longer secure. It's widely believed that security through obscurity 
+    is an ineffective security measure on its own, and should be avoided due to
+    a potential single point of failure and a fall sense of security.
 
     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.
+    detail that is not visible to the user. Otherwise, attackers could easily 
+    modify the structure and change the 'isAdmin' flag to 'true', to gain admin 
+    privileges.
+
+    Therefore, as this example illustrates, security through obscurity alone is 
+    not enough to secure a system. Attackers are in position toreverse engineer 
+    the code and find the vulnerability. This is exposed in hack.c (see below).
 
-    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).
+    You can read more about the concept of security through obscurity here:
+    https://securitytrails.com/blog/security-through-obscurity
 
-    Buffer Overflow Vulnerability
+
+    (2) 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.
+
+    You can read more about buffer overflow vulnerabilities here:
+    https://owasp.org/www-community/vulnerabilities/Buffer_Overflow
 */