Tweak lab insecure-deserialization (#668)

* Tweak lab insecure-deserialization Signed-off-by: David A. Wheeler <[email protected]> * Fix XSS vulnerabilities in lab code res.send must *NOT* be used with untrusted user code, as this leads to XSS vulnerabilities. Use res.render instead. See: * https://expressjs.com/en/api.html * https://www.geeksforgeeks.org/express-js-res-render-function/ * https://semgrep.dev/docs/cheat-sheets/express-xss Signed-off-by: David A. Wheeler <[email protected]> * Use textarea for long lab inputs Signed-off-by: David A. Wheeler <[email protected]> * Shorten lab name Signed-off-by: David A. Wheeler <[email protected]> * Fix name in lab Signed-off-by: David A. Wheeler <[email protected]> * Lab README - add new deserialization lab Signed-off-by: David A. Wheeler <[email protected]> * Improve hints in lab deserialization Signed-off-by: David A. Wheeler <[email protected]> * Minor tweaks of lab deserialization Signed-off-by: David A. Wheeler <[email protected]> * Lab deserialization: Allow more variation Allow sub-conditions to be surrounded by parentheses. Signed-off-by: David A. Wheeler <[email protected]> --------- Signed-off-by: David A. Wheeler <[email protected]>
ossf · Oct 17, 2024 · fd01531 · fd01531
1 parent 89db94d
commit fd01531
Show file tree

Hide file tree

Showing 2 changed files with 113 additions and 23 deletions.
diff --git a/docs/labs/README.md b/docs/labs/README.md
@@ -79,7 +79,7 @@ work on.
     * Using Regular Expressions for Text Input Validation - DONE-0 [regex1](regex1.html), [input2](input2.html)
     * [Countering ReDoS Attacks on Regular Expressions](https://github.com/ossf/secure-sw-dev-fundamentals/blob/main/secure_software_development_fundamentals.md#countering-redos-attacks-on-regular-expressions) - DONE-2 (Camila Vilarinho, 2026-07-19) [redos](redos.html)
   * Input Validation: Beyond Numbers and Text
-    * [Insecure Deserialization](https://github.com/ossf/secure-sw-dev-fundamentals/blob/main/secure_software_development_fundamentals.md#insecure-deserialization) - PLANNED-2 (Camila Vilarinho)
+    * [Insecure Deserialization](https://github.com/ossf/secure-sw-dev-fundamentals/blob/main/secure_software_development_fundamentals.md#insecure-deserialization) - PLANNED-2 (Camila Vilarinho) [deserialization](deserialization.html)
     * [Input Validation: Beyond Numbers and Text](https://github.com/ossf/secure-sw-dev-fundamentals/blob/main/secure_software_development_fundamentals.md#input-validation-beyond-numbers-and-text) - PLANNED-2 UNASSIGNED
     * [Minimizing Attack Surface, Identification, Authentication, and Authorization](https://github.com/ossf/secure-sw-dev-fundamentals/blob/main/secure_software_development_fundamentals.md#minimizing-attack-surface-identification-authentication-and-authorization) - PLANNED-2 UNASSIGNED
     * [Search Paths and Environment Variables (including setuid/setgid Programs)](https://github.com/ossf/secure-sw-dev-fundamentals/blob/main/secure_software_development_fundamentals.md#search-paths-and-environment-variables-including-setuidsetgid-programs) - PLANNED-2 UNASSIGNED
@@ -133,6 +133,7 @@ Thanks to the following people who have created or offered to create labs
 (sorted by given/first name):
 
 * Avishay Balter (Microsoft)
+* Camila Vilarinho
 * David A. Wheeler (Linux Foundation)
 * Dhananjay Arunesh
 * Elijah Everett

diff --git a/docs/labs/insecure-deserialization.html → docs/labs/deserialization.html b/docs/labs/insecure-deserialization.html → docs/labs/deserialization.html
@@ -23,18 +23,72 @@
 
 <!-- Full pattern of correct answer -->
 <script id="correct0" type="plain/text">
- const\s+data = JSON \. parse \( base64Decoded \) \; \s*
+\s* const data = JSON \. parse \( base64Decoded \) \; \s*
 </script>
 <script id="correct1" type="plain/text">
- if \( data \. username && typeof\s+data \. username == ('string'|"string"|`string`) && data \. username \. length < 20 \) \{ \s*
+\s* if \( CONDALL \) \{ \s*
 </script>
 
 <script id="info" type="application/yaml">
 ---
+# Allow condition subexpressions to be optionally surrounded by parentheses
+# and allow the order to vary. This allows more real-world answers to be
+# considered acceptable.
+# Doing this is more easily done by buildigg up definitions,
+# which is annoying to write but general.
+definitions:
+- term: COND0
+  value: |-
+    data \. username
+- term: COND0
+  value: |-
+    (COND0|\( COND0 \))
+- term: COND1
+  value: |-
+    typeof\s+data \. username == ('string'|"string"|`string`)
+- term: COND1
+  value: |-
+    (COND1|\( COND1 \))
+- term: COND2
+  value: |-
+    data \. username \. length < 20
+- term: COND2
+  value: |-
+    (COND2|\( COND2 \))
+# Only the first one is likely, but we may as well allow both possibilities.
+# The first condition MUST be first because it checks if the value exists.
+- term: CONDALL
+  value: |-
+    (COND0 && (COND1 && COND2|COND2 && COND1))
 hints:
+- absent: |
+    ^ const data =
+  text: The first section should begin with `const data =`
 - present: "json"
   text: the JSON built-in global object is witten in uppercase.
-- absent: data.username
+- absent: |
+    JSON \. parse
+  text: Make a call to `JSON.parse` with the data retrieved, e.g.,
+    `JSON.parse(base64Decoded)` should be stored in `data`.
+- present: |
+    \+
+  text: You should not have any concatenation (`+`) in the first section.
+- absent: |
+    ; $
+  text: JavaScript does not require semicolons at the end of a
+    statement, but since the other statements terminate with semicolons,
+    you should also terminate your statement with a semicolon to be consistent.
+- absent: |-
+    ^ if \(
+  index: 1
+  text: The second section should start with `if (` followed by a condition.
+  examples:
+  -
+    - const data = JSON.parse(base64Decoded);
+    - |
+        if data.username {
+- absent: |
+    data \. username
   index: 1
   text: Check if the data object has a property called username. You can do this by referencing data.username.
 - absent: \&\&
@@ -43,20 +97,52 @@
 - absent: typeof
   index: 1
   text: Use typeof to check the type of the operand's value.
-- present: typeof data.username == 'String'
+    You should have `typeof data.username == 'string'`
+    or similar.
+- present: |
+    typeof data \. username == 'String'
   index: 1
   text: When using typeof, JavaScript expects "string" all lowercase.
 - absent: length
   index: 1
   text: check if the length of the string is smaller than 20 characters.
-
+    Use the expression `data.username.length &lt; 20` to determine this.
+- present: |-
+    ^ if \(
+  absent: |-
+    ^ if \( data \. username &&
+  index: 1
+  text: Begin the second section with `if ( data.username && ... `
+    because you must check if data is even present before you can check
+    various attributes of that data.
+#   examples:
+#   -
+#     - "const data = JSON.parse(base64Decoded);"
+#     - "if (typeof data.username == 'string' && data.username.length < 20 && data.username) {"
+successes:
+  -
+    - const data = JSON.parse(base64Decoded);
+    - if (data.username && typeof data.username == 'string' && data.username.length < 20) {
+  -
+    - const data = JSON . parse ( base64Decoded ) ;
+    - if ( data . username && typeof data . username == 'string' && data . username.length < 20) {
+  -
+    - const data = JSON.parse(base64Decoded);
+    - if (data.username && (typeof data.username == 'string') && (data.username.length < 20)) {
+  -
+    - const data = JSON.parse(base64Decoded);
+    - if (data.username && typeof data.username == 'string' && (data.username.length < 20)) {
+failures:
+  -
+    - const data = JSON.parse(base64Decoded);
+    - if (data.username && (typeof data.username == 'string')) && (data.username.length < 20)) {
 # debug: true
 </script>
 </head>
 <body>
 <!-- For GitHub Pages formatting: -->
 <div class="container-lg px-3 my-5 markdown-body">
-<h1>Lab Exercise Insecure Deserialization</h1>
+<h1>Lab Exercise deserialization</h1>
 <p>
 This is a lab exercise on developing secure software.
 For more information, see the <a href="introduction.html" target="_blank">introduction to
@@ -79,19 +165,18 @@ <h2>Task Information</h2>
 <p>
 
 <p>
-The code below is called after an application login page. After login, a cookie is set up with the user profile, then in the homepage the cookie is deserialized and uses the username in a greeting message. 
+The code below is called after an application login page. After login, a cookie is set up with the user profile, then in the homepage the cookie is deserialized and uses the username in a greeting message.
 <p>
-If you take a closer look at this code, you’ll see that it’s using <tt>eval()</tt> to deserialize the data from the cookie. This can be very dangerous as <tt>eval()</tt> evaluates a string as JavaScript code, which means any code inside that string will be executed, opening the possibility of Remote Code Execution (RCE) and Code Injection attacks.  
+If you take a closer look at this code, you’ll see that it’s using <tt>eval()</tt> to deserialize the data from the cookie. This can be very dangerous as <tt>eval()</tt> evaluates a string as JavaScript code, which means any code inside that string will be executed, opening the possibility of Remote Code Execution (RCE) and Code Injection attacks.
 <p>
-For this lab we want to fix this by using an approach that prevents code execution, we will also add input validation to make sure the data we receive is what we are expecting and nothing more than that. 
+For this lab we want to fix this by using an approach that prevents executing code from the attacker. We will also add some simple input validation to make sure the data we receive from inside the JSON data is what we are expecting.
 <ol>
   <li>
-    Replace <tt>eval()</tt> with <tt>JSON.parse()</tt>. JSON.parse( ) does not execute any JavaScript code like functions or methods, making it safer than some other serialization libraries. 
+    Replace <tt>eval()</tt> with <tt>JSON.parse()</tt>. JSON.parse( ) does not execute any JavaScript code like functions or methods, making it a much more secure approach for deserialization.
   </li>
-  <li>Besides checking if <tt>data.username</tt> exists, also check that the username is a string and not bigger than 20 characters.</li>
+  <li>Besides checking if <tt>data.username</tt> exists, perform simple validations of its value. Ensure it is a string (<tt>typeof data.username == 'string'</tt>) and that it's less than 20 characters long (<tt>data.username.length &lt; 20</tt>).</li>
 </ol>
 
-
 <p>
 Use the “hint” and “give up” buttons if necessary.
 
@@ -101,13 +186,13 @@ <h2>Interactive Lab (<span id="grade"></span>)</h2>
 <p>
 Change the code below, adding the mitigation steps to prevent Insecure Deserialization:
 <ol>
-  <li>Use a deserialization approach that prevents code execution.</li>
-  <li>Validate the username making sure it is used only if it's a <b>string</b> and no longer than <b>20 characters</b>.</li>
+  <li>Use a deserialization approach that prevents code execution of untrusted code.</li>
+  <li>Validate the username making sure a reply is only sent if it's a <b>string</b> and less than <b>20 characters</b>.</li>
 </ol>
 <form id="lab">
 
-<pre><code>
-const express = require('express');
+<pre><code
+>const express = require('express');
 const cookieParser = require('cookie-parser');
 
 const app = express();
@@ -118,15 +203,19 @@ <h2>Interactive Lab (<span id="grade"></span>)</h2>
 app.get('/', (req, res) => {
   if (req.cookies.profile) {
     try {
-      const base64Decoded = Buffer.from(req.cookies.profile, 'base64').toString('utf8');
-      <input id="attempt0" type="text" size="60" spellcheck="false" value="const data = eval('(' + base64Decoded + ')');">
-
-      <input id="attempt1" type="text" size="60" spellcheck="false" value="if (data.username) {">
-        res.send(`Hello ${data.username}`);
+      const base64Decoded = Buffer.from(
+        req.cookies.profile, 'base64').toString('utf8');
+      <input id="attempt0" type="text" size="60" spellcheck="false"
+             value="const data = eval('(' + base64Decoded + ')');">
+
+      <textarea id="attempt1" rows="3" cols="60" spellcheck="false"
+>if (data.username) {</textarea>
+            // To prevent XSS, avoid res.send with untrusted data
+            res.render('index', {username: data.username});
       }
 
     } catch (err) {
-      res.send('An error occurred: ' + err.message);
+      res.send('An error occurred.');
     }
   } else {
     res.send("Please Login");