Lack of Input Validation and Sanitization in Admin Login Endpoint

cjackett commented 2 months ago

Description

The admin-login.ts endpoint lacks proper input validation and sanitization for the email field, resulting in potential security vulnerabilities. Special characters in the email input are not adequately handled, leading to internal server errors and exposure of detailed error messages.

Steps to Reproduce

XSS Injection Attempt:

curl -X POST http://localhost:3000/api/admin-login \
    -H "Content-Type: application/json" \
    -d '{"email": "admin@example.com<script>alert(\"XSS\")</script>"}'

HTML Injection Attempt:

curl -X POST http://localhost:3000/api/admin-login \
    -H "Content-Type: application/json" \
    -d '{"email": "admin@example.com<b>bold</b>"}'

Path Traversal-like Input:

curl -X POST http://localhost:3000/api/admin-login \
    -H "Content-Type: application/json" \
    -d '{"email": "admin@example.com../../../../etc/passwd"}'

Command Injection-like Input:

curl -X POST http://localhost:3000/api/admin-login \
    -H "Content-Type: application/json" \
    -d '{"email": "admin@example.com; ls -la;"}'

XML Injection Attempt:

curl -X POST http://localhost:3000/api/admin-login \
    -H "Content-Type: application/json" \
    -d '{"email": "admin@example.com<foo>bar</foo>"}'

Embedded URL:

curl -X POST http://localhost:3000/api/admin-login \
    -H "Content-Type: application/json" \
    -d '{"email": "admin@example.comhttp://malicious.com"}'

Malformed Email Address:

curl -X POST http://localhost:3000/api/admin-login \
    -H "Content-Type: application/json" \
    -d '{"email": "admin@@example..com"}'

SQL-like Payload with Union:

curl -X POST http://localhost:3000/api/admin-login \
     -H "Content-Type: application/json" \
     -d '{"email": "admin@example.com\' UNION SELECT null,null,null;--"}'

Observed Response and Error Logs

<!DOCTYPE html><html><head><style data-next-hide-fouc="true">body{display:none}</style><noscript data-next-hide-fouc="true"><style>body{display:block}</style></noscript><meta charSet="utf-8"/><meta name="viewport" content="width=device-width"/><meta name="next-head-count" content="2"/><noscript data-n-css=""></noscript><script defer="" nomodule="" src="/_next/static/chunks/polyfills.js?ts=1722931607612"></script><script src="/_next/static/chunks/webpack.js?ts=1722931607612" defer=""></script><script src="/_next/static/chunks/main.js?ts=1722931607612" defer=""></script><script src="/_next/static/chunks/pages/_app.js?ts=1722931607612" defer=""></script><script src="/_next/static/chunks/pages/_error.js?ts=1722931607612" defer=""></script><script src="/_next/static/development/_buildManifest.js?ts=1722931607612" defer=""></script><script src="/_next/static/development/_ssgManifest.js?ts=1722931607612" defer=""></script><noscript id="__next_css__DO_NOT_USE__"></noscript></head><body><div id="__next"></div><script src="/_next/static/chunks/react-refresh.js?ts=1722931607612"></script><script id="__NEXT_DATA__" type="application/json">{"props":{"pageProps":{"statusCode":500}},"page":"/_error","query":{"__NEXT_PAGE":"/api/admin-login"},"buildId":"development","isFallback":false,"err":{"name":"Error","source":"server","message":"Value for argument \"documentPath\" must point to a document, but was \"admin@example.com\u003cb\u003ebold\u003c/b\u003e\". Your path does not contain an even number of components.","stack":"Error: Value for argument \"documentPath\" must point to a document, but was \"admin@example.com\u003cb\u003ebold\u003c/b\u003e\". Your path does not contain an even number of components.\n    at CollectionReference.doc (siv/node_modules/@google-cloud/firestore/build/src/reference.js:2065:19)\n    at __WEBPACK_DEFAULT_EXPORT__ (webpack-internal:///(api)/./pages/api/admin-login.ts:21:103)\n    at Object.apiResolver (siv/node_modules/next/dist/server/api-utils/node.js:366:15)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at async DevServer.runApi (siv/node_modules/next/dist/server/next-server.js:481:9)\n    at async Object.fn (siv/node_modules/next/dist/server/next-server.js:741:37)\n    at async Router.execute (siv/node_modules/next/dist/server/router.js:252:36)\n    at async DevServer.run (siv/node_modules/next/dist/server/base-server.js:365:29)\n    at async DevServer.run (siv/node_modules/next/dist/server/dev/next-dev-server.js:709:20)\n    at async DevServer.handleRequest (siv/node_modules/next/dist/server/base-server.js:303:20)"},"gip":true,"scriptLoader":[]}</script></body></html>

Impact

Input Validation and Sanitization: Lack of proper input validation and sanitization for the email field allows the injection of special characters and potentially harmful scripts.
Error Handling: Detailed error messages expose internal implementation details, including file paths and stack traces, which can be leveraged by attackers to craft more targeted attacks.

Recommendations

Implement Robust Email Validation: Use a library such as validator to ensure proper email format validation.

import validator from 'validator';

if (!validator.isEmail(email)) {
 return res.status(400).send('Invalid email');
}

Sanitize Input: Sanitize the email input to remove or escape potentially harmful characters.
```
import sanitize from 'sanitize-html';

email = sanitize(email);
```
Improve Error Handling: Avoid exposing detailed error messages to users. Log detailed errors internally and provide generic error messages to users.
```
try {
 // Existing code
} catch (error) {
 console.error(error); // Log detailed error
 res.status(500).send('Internal server error');
}
```
Consistent Input Handling: Apply consistent input validation and sanitization across all inputs and contexts within the application.

dsernst commented 2 months ago

Thank you for the report!

Lot going on here, so going to try to take it one by one.

The admin-login.ts endpoint

Permalink to file for reference: https://github.com/siv-org/siv/blob/9011544bfd1752f1d056e7700af8b08658fe23e1/pages/api/admin-login.ts (snapshot from when this issue was opened)

lacks proper input validation and sanitization for the email field, resulting in potential security vulnerabilities.

Relevant lines: https://github.com/siv-org/siv/blob/9011544bfd1752f1d056e7700af8b08658fe23e1/pages/api/admin-login.ts#L8-L16

Agree the email validation there could be improved.

Special characters in the email input are not adequately handled

Sure.

leading to internal server errors

That's not so bad. Just crashes the temporary endpoint, created only for you. When deployed, all endpoints are independent serverless functions from each other, so you can't harm other users this way.

and exposure of detailed error messages.

Might be unsightly or intimidating, but not a security issue.

Especially since all the underlying backend code is public already.
Even if it wasn't public, hiding this would only be Security through Obscurity anyway.

Ok, that covers the intro. Now moving onto the next sections...

dsernst commented 2 months ago

Just tried running Reproduction Example 1:

curl -X POST http://localhost:3000/api/admin-login \
     -H "Content-Type: application/json" \
     -d '{"email": "admin@example.com<script>alert(\"XSS\")</script>"}'

I'm seeing the html for an error page like:

<!DOCTYPE html>
<html>

<head>
    <style data-next-hide-fouc="true">
        body{display:none}
    </style><noscript data-next-hide-fouc="true"><style>body{display:block}</style></noscript>
    <meta charSet="utf-8" />
    <meta name="viewport" content="width=device-width" />
    <meta name="next-head-count" content="2" /><noscript data-n-css=""></noscript>
    <script defer="" nomodule="" src="/_next/static/chunks/polyfills.js?ts=1723019276418"></script>
    <script src="/_next/static/chunks/webpack.js?ts=1723019276418" defer=""></script>
    <script src="/_next/static/chunks/main.js?ts=1723019276418" defer=""></script>
    <script src="/_next/static/chunks/pages/_app.js?ts=1723019276418" defer=""></script>
    <script src="/_next/static/chunks/pages/_error.js?ts=1723019276418" defer=""></script>
    <script src="/_next/static/development/_buildManifest.js?ts=1723019276418" defer=""></script>
    <script src="/_next/static/development/_ssgManifest.js?ts=1723019276418" defer=""></script><noscript id="__next_css__DO_NOT_USE__"></noscript></head>

<body>
    <div id="__next"></div>
    <script src="/_next/static/chunks/react-refresh.js?ts=1723019276418"></script>
    <script id="__NEXT_DATA__" type="application/json">
        {"props":{"pageProps":{"statusCode":500}},"page":"/_error","query":{"__NEXT_PAGE":"/api/admin-login"},"buildId":"development","isFallback":false,"err":{"name":"Error","source":"server","message":"Value for argument \"documentPath\" must point to a document, but was \"admin@example.com\u003cscript\u003ealert(\"xss\")\u003c/script\u003e\". Your path does not contain an even number of components.","stack":"Error: Value for argument \"documentPath\" must point to a document, but was \"admin@example.com\u003cscript\u003ealert(\"xss\")\u003c/script\u003e\". Your path does not contain an even number of components.\n    at CollectionReference.doc (/Users/dsernst/Documents/siv/node_modules/@google-cloud/firestore/build/src/reference.js:2065:19)\n    at __WEBPACK_DEFAULT_EXPORT__ (webpack-internal:///(api)/./pages/api/admin-login.ts:21:103)\n    at Object.apiResolver (/Users/dsernst/Documents/siv/node_modules/next/dist/server/api-utils/node.js:366:15)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at async DevServer.runApi (/Users/dsernst/Documents/siv/node_modules/next/dist/server/next-server.js:481:9)\n    at async Object.fn (/Users/dsernst/Documents/siv/node_modules/next/dist/server/next-server.js:741:37)\n    at async Router.execute (/Users/dsernst/Documents/siv/node_modules/next/dist/server/router.js:252:36)\n    at async DevServer.run (/Users/dsernst/Documents/siv/node_modules/next/dist/server/base-server.js:365:29)\n    at async DevServer.run (/Users/dsernst/Documents/siv/node_modules/next/dist/server/dev/next-dev-server.js:709:20)\n    at async DevServer.handleRequest (/Users/dsernst/Documents/siv/node_modules/next/dist/server/base-server.js:303:20)"},"gip":true,"scriptLoader":[]}
    </script>
</body>

</html>

Some things I notice about this:

The alert('XSS') is not running.
Firebase is rejecting the input as invalid, with error message:

"Value for argument \"documentPath\" must point to a document, but was \"admin@example.com\u003cscript\u003ealert(\"xss\")\u003c/script\u003e\"
No one other than the user who launched this would ever see any of their malicious payload code.
- Potentially this could (accidentally?) change in the future, if we started passing this data around, like to some error dashboard.
- But for now, hard to see who or what is really being attacked here.

So this first Reproduction Example seems invalid. (For now, at least...)

Please correct me if I appear mistaken about any of this.

dsernst commented 2 months ago

2 "HTML Injection Attempt":

curl -X POST http://localhost:3000/api/admin-login \
     -H "Content-Type: application/json" \
     -d '{"email": "admin@example.com<b>bold</b>"}'

and 5 "XML Injection Attempt"::

curl -X POST http://localhost:3000/api/admin-login \
     -H "Content-Type: application/json" \
     -d '{"email": "admin@example.com<foo>bar</foo>"}'

are also nearly identical to 1.

1 would seem the most dangerous of the 3, since its trying to run js, but all 3 are a very similar vector of trying to smuggle in arbitrary <tags>.

So if 1 isn't an issue (per comment above), it seems safe to assume 2 and 5 wouldn't be either.

cjackett commented 2 months ago

Yes, all the examples produced a similar response from the backend. While they are not true security issues, they highlight areas where improved validation and sanitization on the front end would be beneficial. The timing of my attention to this codebase was coincidental with the hacking comp. Additionally, I have gathered some UI/UX observations that might interest you, which are also unrelated to the DEF CON HACK.

dsernst commented 2 months ago

Path Traversal:

curl -X POST http://localhost:3000/api/admin-login \
     -H "Content-Type: application/json" \
     -d '{"email": "admin@example.com../../../../etc/passwd"}'

When I run this, I'm again getting a firebase error:

Value for argument \"documentPath\" must point to a document, but was \"admin@example.com../../../../etc/passwd\". Your path does not contain an even number of components.

I think of all of these, this one is the one I was the most concerned might work, because Firebase does have a shorthand for specifying document paths. But now researching the issue, everything I'm seeing is saying Firebase does not support .. as a special character in documents, and just treats it as a plain string.

dsernst commented 2 months ago

4 Command Injection-like Input:

curl -X POST http://localhost:3000/api/admin-login \
     -H "Content-Type: application/json" \
     -d '{"email": "admin@example.com; ls -la;"}'

Runs at least without erroring, but just gives me an innocuous:

'admin@example.com; ls -la;' is not approved election manager

The ls unix command is not running. Firebase is just treating the whole thing as a plain string.

Seems also like not an issue?

dsernst commented 2 months ago

Ok, I'm going to skip running the rest of these. No reason to think they would be any worse than the ones I did try to reproduce.

The main valid thing here is that we can and should totally just run a proper email validator function over the input. Would be shorter than our double .includes('@') .includes(.) checks. Doesn't appear there's actually a live security issue here, given Firebase is doing its own sanitization + hardening, and the other mitigations mentioned in my 1st comment reply. But would be good to handle the validation better anyway, in case we want to switch out Firebase later, for example.

(IIRC, that double includes() check was from very early on, before we started using an email validator elsewhere. It was just meant to be quick to warn people who accidentally try to enter a username, or forget to include .com, etc)

dsernst commented 2 months ago

Cleaned up the email validation here: https://github.com/siv-org/siv/commit/12253ce847e394ff1d6bf92f188242085ee32361

dsernst commented 2 months ago

Yes, all the examples produced a similar response from the backend. While they are not true security issues, they highlight areas where improved validation and sanitization on the front end would be beneficial. The timing of my attention to this codebase was coincidental with the hacking comp. Additionally, I have gathered some UI/UX observations that might interest you, which are also unrelated to the DEF CON HACK.

Well it was still useful to think through all these malicious payload possibilities. And review how the system handles against them. And the provided curl examples, ready to copy-and-paste right in, were especially nice to make testing quick and easy. 👍👍

dsernst commented 2 months ago

Ok, closing this issue for now. Still valid for HACK SIV prizes though!

arianabuilds commented 1 month ago

Thanks again for participating!

HACK SIV Competition Submission

DEF CON 2024, August 6-11 hack.siv.org

SIV Evaluation

Type: Implementation

In Favor:

If it was exploitable, it could lead to taking over the backend server
- Brought this lack of validation to our attention
- Provided straightforward fix: validateEmail()

Against:

It wasn't exploitable.
The report of exposure of error logs is irrelvant, because the code is all public already

Prize Allocation

Total: $281.79

SIV: $136.05
Public: $145.74

Payment Status

Track process on https://github.com/siv-org/hack.siv.org/issues/10

siv-org / siv