CVE-2020-26236: Improper Authentication (CWE-287)
value: In ScratchVerifier before commit a603769, an attacker can hijack the verification process to log into someone else's account on any site that uses ScratchVerifier for logins. A possible exploitation would follow these steps: 1. User starts login process. 2. Attacker attempts login for user, and is given the same verification code. 3. User comments code as part of their normal login. 4. Before user can, attacker completes the login process now that the code is commented. 5. User gets a failed login and attacker now has control of the account. Since commit a603769 starting a login twice will generate different verification codes, causing both user and attacker login to fail. For clients that rely on a clone of ScratchVerifier not hosted by the developers, their users may attempt to finish the login process as soon as possible after commenting the code. There is no reliable way for the attacker to know before the user can finish the process that the user has commented the code, so this vulnerability only really affects those who comment the code and then take several seconds before finishing the login.
CWE description: When an actor claims to have a given identity, the product does not prove or insufficiently proves that the claim is correct.
CVSS v3 base score: 7.5
Published: 2020-11-20T18:15Z
Repository: https://github.com/ScratchVerifier/ScratchVerifier
Language: Python
Files changed: backend/db.py, docs/docs.json, docs/reference.html
Commit date: 2020-11-20 03:07:38 +0800
Commit message: Change behavior of PUT /verify/{}

* Instead of returning the same code, which someone else could hijack,
  generate a new code on repeated PUT to preserve integrity.
* Update docs to reflect this new behavior

Diff: diff --git a/backend/db.py b/backend/db.py
index 0b4086a..4cf7031 100644
--- a/backend/db.py
+++ b/backend/db.py
@@ -160,22 +160,23 @@ WHERE username=?', (username,))
     async def start_verification(self, client_id, username):
         async with self.lock:
             await self.db.execute('SELECT code FROM scratchverifier_usage WHERE \
 client_id=? AND username=?', (client_id, username))
             row = await self.db.fetchone()
-        if row is not None:
-            await self.db.execute('UPDATE scratchverifier_usage SET expiry=? \
-WHERE client_id=? AND username=? AND code=?', (int(time.time()) + VERIFY_EXPIRY,
-                                               client_id, username, row[0]))
-            return row[0]
         code = sha256(
             str(client_id).encode()
             + str(time.time()).encode()
             + username.encode()
             + token_bytes()
         # 0->A, 1->B, etc, to avoid Scratch's phone number censor
         ).hexdigest().translate({ord('0') + i: ord('A') + i for i in range(10)})
+        if row is not None:
+            await self.db.execute(
+                'UPDATE scratchverifier_usage SET expiry=?, code=? \
+WHERE client_id=? AND username=?', (int(time.time()) + VERIFY_EXPIRY,
+                                    code, client_id, username))
+            return code
         await self.db.execute('INSERT INTO scratchverifier_usage (client_id, \
 code, username, expiry) VALUES (?, ?, ?, ?)', (client_id, code, username,
                                int(time.time() + VERIFY_EXPIRY)))
         await self.db.execute('INSERT INTO scratchverifier_logs (client_id, \
 username, log_time, log_type) VALUES (?, ?, ?, ?)', (client_id, username,
diff --git a/docs/docs.json b/docs/docs.json
index 55edd31..446b37f 100644
--- a/docs/docs.json
+++ b/docs/docs.json
@@ -57,11 +57,11 @@
       ]
     },
     {
       "type": "endpoint",
       "name": "Start/Renew Verification",
-      "desc": "Request a new verification code for a user. Only one code per user per client - if this endpoint is used again before the <a href=\"#finish-verification-endpoint\">Finish Verification</a> endpoint is used, this will instead renew the 30-minute expiry on the code and return the original code.",
+      "desc": "Request a new verification code for a user. Only one code per user per client - if this endpoint is used again before the <a href=\"#finish-verification-endpoint\">Finish Verification</a> endpoint is used, this will generate a new code and reset the 30-minute expiry, returning the new code instead.",
       "method": "PUT",
       "path": "/verify/{username}",
       "params": {
         "username": {
           "type": "string",
diff --git a/docs/reference.html b/docs/reference.html
index 9808f5d..8c2ce16 100644
--- a/docs/reference.html
+++ b/docs/reference.html
@@ -19,11 +19,11 @@
 <tr><td class="pre">Authorization: Basic MTAxMTQ3NjQ6NDY5MDI1YzYxM2RhNDMwYmEzMTE0NzIwY...==</td></tr>
 </table>
 <h2 id="api-endpoints">API Endpoints</h2>
 <p>The simplicity of this API is such that there are only three total endpoints for its ultimate purpose.</p>
 <h3 id="start/renew-verification-endpoint">Start/Renew Verification Endpoint</h3>
-<p>Request a new verification code for a user. Only one code per user per client - if this endpoint is used again before the <a href="#finish-verification-endpoint">Finish Verification</a> endpoint is used, this will instead renew the 30-minute expiry on the code and return the original code.</p>
+<p>Request a new verification code for a user. Only one code per user per client - if this endpoint is used again before the <a href="#finish-verification-endpoint">Finish Verification</a> endpoint is used, this will generate a new code and reset the 30-minute expiry, returning the new code instead.</p>
 <div class="endpoint">
 <div class="method-path auth-needed" onclick="showOrHide(this)"><span class="method">PUT</span> <code>/verify/<span class="param">{username}</span></code> <a href="#authorization"><img src="https://image.flaticon.com/icons/png/512/61/61457.png" title="Authorization necessary" /></a></div>
 <div style="display: none">
 <table><caption>URL Params</caption>
 <tr><th>Field</th><th>Type</th><th>Description</th>

Vulnerable code:         if row is not None:
            await self.db.execute('UPDATE scratchverifier_usage SET expiry=? \
WHERE client_id=? AND username=? AND code=?', (int(time.time()) + VERIFY_EXPIRY,
                                               client_id, username, row[0]))
            return row[0]
      "desc": "Request a new verification code for a user. Only one code per user per client - if this endpoint is used again before the <a href=\"#finish-verification-endpoint\">Finish Verification</a> endpoint is used, this will instead renew the 30-minute expiry on the code and return the original code.",
<p>Request a new verification code for a user. Only one code per user per client - if this endpoint is used again before the <a href="#finish-verification-endpoint">Finish Verification</a> endpoint is used, this will instead renew the 30-minute expiry on the code and return the original code.</p>
Fixed code:         if row is not None:
            await self.db.execute(
                'UPDATE scratchverifier_usage SET expiry=?, code=? \
WHERE client_id=? AND username=?', (int(time.time()) + VERIFY_EXPIRY,
                                    code, client_id, username))
            return code
      "desc": "Request a new verification code for a user. Only one code per user per client - if this endpoint is used again before the <a href=\"#finish-verification-endpoint\">Finish Verification</a> endpoint is used, this will generate a new code and reset the 30-minute expiry, returning the new code instead.",
<p>Request a new verification code for a user. Only one code per user per client - if this endpoint is used again before the <a href="#finish-verification-endpoint">Finish Verification</a> endpoint is used, this will generate a new code and reset the 30-minute expiry, returning the new code instead.</p>