Fix issue #62

[evarga]

Summary

This is a rework of the code base to fix issue #62. This new version is even leaner than the original, which also improves the learning experience.

Changes

All files have been altered except the hint.txt. Here are the changes broken up by files:

• code.py
  • The programmatic endpoint was removed since it had no real purpose and only went against the DRY principle.
  • The index function is restructured and doesn't leverage sanitization. This introduces the XSS vulnerability, as explained in the solution text.
  • The index function now correctly handles the case of a missing or empty planet parameter.
• tests.py
  • It has been expanded with tests to check for a missing or empty planet parameter.
  • It now contains a test with a payload having script inside it.
  • The unused import of Flask was removed.
  • It demonstrates how to send a form parameter in a HTTP POST request inside a unit test.
• hack.py
  • Deleted.
• hack.txt
  • A new text file with instructions how to insert an active content into the application to trigger an XSS attack. The initial implementation will be susceptible to XSS attack, whilst passes with a fixed version according to the solution.
• solution.txt
  • It is now fully correct and aligned with a program behavior.
  • It instructs the reader how to use the escape function in a non-deprecated fashion.

Closes:

• [Bug] Season 2/Level 3 - Cascading Errors in the Code Base and Solution Text #62

Task list

• For workflow changes, I have verified the Actions workflows function as expected.
• For content changes, I have reviewed the style guide.

[jkcso]

@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?

[evarga]

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.

[jkcso]

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

[evarga]

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.

[jkcso]

Thank you!!!

[jkcso]

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!

[evarga]

See my comment above.

[jkcso]

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

[evarga]

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.

[jkcso]

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.

[evarga]

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

[jkcso]

Thanks! I want to also ask the original creator of the level who is @viralvaghela.

[evarga]

@jkcso I have reworked the game as we've agreed. It is now showcasing some advanced features of working with dynamic HTML pages (see the new details.html template page) as well as ways to circumvent proprietary sanitization approaches. The solution.txt explains all the details. The new tainted payload that triggers the XSS attack is &ltimg src='x' onerror='alert(1)'&gt, which nicely illustrates that attackers can always came up with creative payloads. 😄

I hope that this version is our desired win-win situation.

[jkcso]

Pushed manually to make some minor changes on the language (not code!) and resolve conflicts! Thanks a lot legend @evarga!