skills / secure-code-game Goto Github PK

Summary

The documentation for codespace users is only partially complete.
Snippet for context (from hack-1.js)

// Please note that if you are inside a codespace, it is not possible to perform step 1. For
// this reason, please create a local copy for the file 'index.html'. You can do so by copying 
// and pasting the contents of 'index.html' in a local file so that you can open it in a browser.
// Then, follow the remaining steps.

The key detail here missing is that the same series of steps is necessary with code.js, as it is referenced in the HTML here. Therefore, only copying index.html to local environment will result in errors like:

ReferenceError: CryptoAPI is not defined

How to reproduce

Copy only index.html to local environment and open in browser. Attempt to replicate steps outlined in the "hack-x.js".
Screenshot:

Additional context

On a side note, I imagine codespace users (such as myself) would like a solution that fits under the Codespace umbrella. Proposing via a PR a solution that worked for me and hopefully for others as well.

The original behavior of the get_prof_picture function was to return None after checking denylist. But, hack.py expects the function to return the path instead of None. Wouldn't it make more sense to return early by checking if the returned value is None instead of just comparing the base directory and the output base directory? It should allow the functions to return None if invalid path is passed.

Summary

Deprecated Node.js version

How to reproduce

1. Go to Actions tab
2. Click on a run from the CodeQL Analysis workflow
3. Scroll down to Annotations
4. See warnings

Node.js 16 actions are deprecated. Please update the following actions to use Node.js 20: actions/checkout@v3, github/codeql-action/init@v2, github/codeql-action/autobuild@v2, github/codeql-action/analyze@v2. For more information see: https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/.

CodeQL Action v2 will be deprecated on December 5th, 2024. Please update all occurrences of the CodeQL Action in your workflow files to v3. For more information, see https://github.blog/changelog/2024-01-12-code-scanning-deprecation-of-codeql-action-v2/

Additional context

Add any other context about the problem here.

Summary

There is a huge blunder in the implementation of Season-2/Level-3/code.py that induced cascading errors in both Season-2/Level-3/hack.py and Season-2/Level-3/solution.txt. The latter is especially troublesome as it contains totally wrong statements about program behavior.

How to reproduce

To exemplify the issue take a look at the following excerpt from Season-2/Level-3/code.py (the same story applies to the index function, too):

@app.route('/getPlanetInfo', methods=['GET'])
def get_planet_info_endpoint():
    planet = request.args.get('planet')
    sanitized_planet = re.sub(r'[<>(){}[\]]', '', planet)

    if 'script' in sanitized_planet.lower() :
        return '<h2>Blocked</h2></p>'

    elif sanitized_planet:
        details = get_planet_info(sanitized_planet)

        if planet:
            return f'<h2>Planet Details:</h2><p>{get_planet_info(planet)}</p>'
        else:
            return '<h2>Please enter a planet name.</h2>'

def get_planet_info(planet):
    if planet in planet_data:
        return planet_data[planet]
    else:
        return f'No information found for {planet}.'

The planet variable is loaded with the content of the same named request parameter. It is sanitized and such a sanitized input is passed to the get_planet_info helper function. The return value is stored in the details variable. But this is not later used anywhere! The content rendered inside the browser is based on the raw planet variable instead, since another call to the get_planet_info function happens using planet as an argument. Honestly, the whole structure of the code starting with elif is wrong, since it cannot happen that a sanitized content is non empty while the raw input is, thus making the inner if redundant. Consequently, passing an empty input would never trigger the message for asking the user to enter a planet name.

As a proof that sanitization does change the original content of the planet variable, here is the dump from a Python 3 interactive session using the payload from Season-2/Level-3/hack.py:

>>> import re
>>>
>>> payload = "<<img src='x' onerror='alert(1)'>>"
>>> sanitized_payload = re.sub(r'[<>(){}[\]]', '', payload)
>>> sanitized_payload
"img src='x' onerror='alert1'"

Apparently, the << and >> were properly removed. The Season-2/Level-3/solution.txt is written under the false assumption that sanitization doesn't happen at all. Thus most of the current content of this file makes no sense.

As a matter of fact, even the simpler payload <img src='x' onerror='alert(1)'> triggers XSS in the current code base.

How to Fix

I would recommend skipping sanitization inside Season-2/Level-3/code.py and just checking whether the script tag is present in the user input. The solution would then go on and explain why sanitization is important to prevent XSS, since besides the script tag img can be used, too. With this approach the current hint.txt would also become meaningful.

Summary

There is another unintended way to win level one's challenge at avoiding the underflow and overflow errors. By converting all number types into the int data type, the bits for the number storage expands to be large enough to handle pretty much any number the machine can handle. However the tests shouldn't allow it to pass. As it is a store, dollars and cents need to be accounted for.

How to reproduce

1. Go to the level one challenge
2. Open code.py
3. Cast every number to an integer like the attached code.
4. Run the tests
5. See error

Please include screenshots.

Additional context

Here's the full code that breaks the challenge:

from collections import namedtuple

Order = namedtuple('Order', 'id, items')
Item = namedtuple('Item', 'type, description, amount, quantity')

def validorder(order: Order):
    net = 0
    
    for item in order.items:
        if item.type == 'payment':
            net += int(item.amount)
        elif item.type == 'product':
            net -= int(item.amount * item.qu
![Capture](https://github.com/skills/secure-code-game/assets/9373245/4faab16a-02cb-440b-ab09-e5318aa135d7)
antity)
        else:
            return("Invalid item type: %s" % item.type)
    
    if net != 0:
        return("Order ID: %s - Payment imbalance: $%0.2f" % (order.id, net))
    else:
        return("Order ID: %s - Full payment received!" % order.id)

Hi guys, thanks for this course and also the other available in the skills section.
I was trying to follow the instruction, so I have create the new repo and waited for the readme file to be updated, as decribed in the initial readme file.

But unfortunately, even after the "Initial commit" action has completed without errors, the readme file is not updated. Any suggestions?
Here some extra info:

• New repo

Thanks

Discussed in #97

Summary

The provided solution.py contains a dangerous bug that allows attackers to easily blow up a system and potentially make it unavailable.

How to reproduce

Add the following test case to hack.py and change line number 2 to import solution instead of code (notice that the quantity is a float number):

# Invalid input should not blow up the system
def test_6(self):
    tv = c.Item(type='product', description='tv', amount=1000, quantity=1.5)
    order_1 = c.Order(id='1', items=[tv])
    try:
        c.validorder(order_1)
    except:
        self.fail("Unhandled exception occured in the target system!")

How to Fix this Bug

Input validation should be expanded to also check data types besides testing allowed range of values. This specific bug, caused by using a non-integer quantity, is due to insufficient attention to requirements engineering. In some systems it is OK to buy half of something, but here it was assumed to be a positive integer.

Describe the bug
Database file name refers to Level 3 but it's on Level 4

To Reproduce
Steps to reproduce the behavior:

1. Play Level 4

Expected behavior
Confusion for players and mostly beginners

Device information

• All

Additional context
Pull request opened to fix it

Summary

There are two minor problems as explained in the next section.

How to reproduce

In the hint-1.txtthere are two examples given as follows:

Example 1:
$ var s = { toString: function() { alert('Exploit 1'); } };
$ CryptoAPI.sha1.hash(s)

Example 2:
$ var s = { toString: function() { alert('Exploit 1'); } };
$ CryptoAPI.sha1.hash("abc")

Nonetheless, the second example doesn't demonstrate anything and should be removed.

In solution.js the line 62 should be changed from:

if ((i & 63) == 63) CryptoAPI.sha1._round(H, w);

to:

if ((i & 63) == 63) internalRound(H, w);

This is explained in the text of the solution, but the code base is not synced with that description.

Is your feature request related to a problem? Please describe.
As a security reviewer, seeing sensitive information being logged or HTTP error responses being too verbose that allows attackers to perform user enumeration gives me nightmares.

The goal of this level would be to prone awareness and be extra cautious about what information is being logged and also the HTTP error response messages that can give hints to attackers.

Describe the solution you'd like
Removing every log.Printf() that contains sensitive informations.
Fixing the typo in one http.Error()function that allows user enumeration.
Fixing the hack_test.go to replace the log.Printf()message with the correct one

Describe alternatives you've considered
N/A

Additional context
N/A

Summary

A clear and concise description of what the problem is, and what you solution you want.

Changes

What changes this feature would introduce. Any risks these changes involve. Any alternative solutions or features you've considered.

Additional context

Add any other context or screenshots about the feature request here.

Summary

A clear and concise description of what the bug is, and what you expected to happen.

How to reproduce

1. Go to '...'
2. Click on '...'
3. Scroll down to '...'
4. See error

Please include screenshots.

Additional context

Add any other context about the problem here.

Summary

A clear and concise description of what the bug is, and what you expected to happen.

How to reproduce

1. Go to '...'
2. Click on '...'
3. Scroll down to '...'
4. See error

Please include screenshots.

Additional context

Add any other context about the problem here.

Summary

At the time of this writing, the whole level 2 game is misguided. The fix is not only in that extra checking whether the index is negative, like it is suggested in solution.c. The fix is more about the redesign of the API and concomitant code to apply the information hiding principle by restricting access to the user account structure. An end user should never work directly with low level data. They should only receive a minimum amount of data by which they can be adequately authenticated and authorized by a target system.

How to reproduce

Change line number 1 in hack.c to reference solution.c instead of code.h. Insert the following line in hack.c before the final tests whether admin rights have been acquired:

ua->isAdmin = true;

How to Fix the Bug

Any attacker would seek the path of least resistance to achieve their goals. Why would anyone bother passing negative indexes to the update procedure, when all they need to perform is simply set the matching field to true?

Therefore, the function create_user_account should return a user id whilst update_setting should receive this id. A separate function should be available to read the user's status based upon their id. At any moment in time, all what a user would hold is their unique id and never work with implementation level structures.

Of course, besides making the above changes all inputs must be properly validated, including the suggested change for checking against a negative index value.

Is your feature request related to a problem? Please describe.
As a security reviewer, seeing third party unofficial actions being used in critical repositories gives me nightmares. In case the GitHub organization you work for didn't limit GitHub Actions, this can quickly become a vicious attack vector. Specially if you run your GitHub Action in a self-hosted runner.

The goal of this level would be to prone awareness over what publicly available actions to use and what not to use with a hands on exercise.

Describe the solution you'd like
Replacing the unofficial GitHub Action that I created dduzgun-security/[email protected] with a simple curl command that doesn't contain any "malicious" thing. The malicious thing in question is just a console.log of the token.

Describe alternatives you've considered
Using an official GitHub Action created by GitHub itself that gets the status page

Additional context
N/A

Describe the bug
SHA256_hasher.password_hash fails the tests because Random_generator.generate_salt() aren't generating valid salts to the bcrypt.hashpw

To Reproduce
Steps to reproduce the behavior:

1. Run python3 tests.py

Expected behavior
The tests should pass.

Screenshots

Device information

• Type: MacBook
• OS: Ventura 13.2.1 (22D68)
• Python: 3.9.6
• Clang: 14.0.0 (clang-1400.0.29.202)

Additional context
Analyzing how bcrypt.gensalt() generates base64 encoding I see that it doesn't match any other base64 encodings:


So, even generating valid base64 bytes the hashpw aren't working

Summary

There is a typo in the code comment in the file Season-2/Level-2/solution/solution_test.go.

How to reproduce

Line number 5 should be changed from:

This file is a copy of main_test.go and solution_test.go

to read as:

This file is a copy of code_test.go and hack_test.go

To install dependecies on ArchLinux you should run commands bellow

sudo pacman -S libldap libsasl

Please add them in README

Is your feature request related to a problem? Please describe.
The validOrder function given in code.py returns a string in the event of success and a string in the event of an error but the successful solution needed is one that actually ignores an invalid amount or quantity rather than return an error string when encountering it because of the specific output hack.py is looking for. solution.py returns a error string in other cases, just not there.

Describe the solution you'd like

• hack.py could be testing using assetNotEqual to check that the attempt to purchase the items fraudulently simply did not succeed in some way
• Unrelated to this but hack.py should be attempting to exploit both the amount and quantity properties

Describe alternatives you've considered
Other options include:

• pydoc for validOrder indicating the response strings expected in different scenarios ("An unexpected error occurred" for an error case, for example)
• hack.py could expect the same error string as returned in line 28 (though that message will need tweaking, again "An unexpected error occurred" or "something went wrong" could work)
• validOrder could be a function that calculates the balance given the items and raises an exception when an invalid state is reached, that way hack.py could check for an exception being thrown and the output is more regular

Describe the bug
Level one has an additional bug/vulnerability in some flows it does not account for the quantity

To Reproduce
Steps to reproduce the behavior:

1. Update Test to test with >1 quantity
2. Run test with solution
3. See Failure

Expected behavior
As a user when running the solution, I should expect it to return Success with any valid test values (>1 quantity)

          You should generate a new repository from the template repository (this repository skills/review-pull-requests) to start the course.

Originally posted by @sinsukehlab in skills/review-pull-requests#66 (comment)

Summary

The provided solution.py wrongly implements order validation. Currently, the outcome depends on how items are arranged in an order.

How to reproduce

Just add the following test case to the tests.py file and change line number 2 to import solution instead of code. Order 1 will be properly detected as exceeding the allowed maximum amount, but Order 2 will be wrongly accepted.

# Example 4 - order validation shouldn't depend on ordering of items
def test_4(self):
    num_items = 12
    items = [c.Item(type='product', description='tv', amount=99999, quantity=num_items)]
    for i in range(num_items):
        items.append(c.Item(type='payment', description='invoice_' + str(i), amount=99999, quantity=1))
    order_1 = c.Order(id='1', items=items)
    self.assertEqual(c.validorder(order_1), 'Total amount exceeded')

    # Put payments before products
    items = items[1:] + [items[0]]
    order_2 = c.Order(id='2', items=items)
    self.assertEqual(c.validorder(order_2), 'Total amount exceeded')

How to Fix this Bug

There are couple of possibilities as enrolled below:

1. Use separate variables for total payments and expenses; use the latter for checking whether the total maximum amount is exceeded or not.
2. Handle products before payments.