Skip to content

Commit

Permalink
Adds more references to the model solution and explains the decouplin…
Browse files Browse the repository at this point in the history 
…g of level 2 API
  • Loading branch information
@jkcso
jkcso committed Jan 5, 2024
1 parent 9e51dd6 commit 5622371
Show file tree
Hide file tree
Showing 6 changed files with 76 additions and 56 deletions.
6 changes: 3 additions & 3 deletions Season-1/Level-1/hack.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
35 changes: 17 additions & 18 deletions Season-1/Level-2/code.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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;
Expand All @@ -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");
Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion Season-1/Level-2/hack.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
7 changes: 4 additions & 3 deletions Season-1/Level-2/hint-1.txt
View file
Original file line number Diff line number Diff line change
@@ -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 ;)
6 changes: 5 additions & 1 deletion Season-1/Level-2/hint-2.txt
View file
Original file line number Diff line number Diff line change
@@ -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
76 changes: 46 additions & 30 deletions Season-1/Level-2/solution.c
View file
Original file line number Diff line number Diff line change
@@ -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 ///
/// ///
///////////////////////////////////////////////////

Expand All @@ -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");
Expand All @@ -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;
Expand All @@ -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");
Expand All @@ -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;
Expand All @@ -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
*/

0 comments on commit 5622371

Please sign in to comment.