redpwn / rctf Goto Github PK

• Make the team code blockquote have user-select: all; and be in a monospace font.
• When a user clicks on the team code blockquote, automatically copy it to clipboard and have a tooltip appear that says "copied to clipboard" or whatever
• Make a button/link below the blockquote to revoke the old team token regenerate a new one. Be sure to ask the user in an 'important' modal "are you sure" first, because some team member who isn't so smart might go clicking around on the page and accidentally lock everyone out of the team account until the account owner wakes up

There is a lot of code in the /challs component. We should refactor this into multiple smaller components.

https://github.com/redpwn/rctf/blob/master/client/src/pages/challenges.js#L34

Because we are using a CSS framework, there is a lot of unused CSS increasing the bundle size. We should look into CSS-Purge as an option for removing unused CSS, decreasing bundle size.

The verification API currently returns responses.badKnownEmail on any error. We should only return this on violation of the uniqueness constraint for email.

https://github.com/redpwn/rctf/blob/master/server/api/auth-verify.js#L45

We should also return a similar error for a violation on name. You can add a response to https://github.com/redpwn/rctf/blob/master/server/responses/index.js#L1 as badKnownName.

For other database errors, you should return responses.errorInternal.

We should use a custom markdown parsing library that returns preact elements, reducing the risk of a XSS vulnerability.

yarn lint should only lint staged files. This prevents the scenario where you run yarn lint --fix and forget to commit the linted files.

At the moment, challenges are always dynamically scored, but organizers can make statically scored challenges by setting the min and max scores to the same value.

It would be better if we allow a more explicit definition for statically scored challenges.

The client build is currently dependent on a shell script, and is running on an older version of preact-cli (the starter projects created by preact-cli all use [email protected], so we should probably also be using the rc versions). Given the refactoring that needs to be done to the webpack configuration to do this upgrade, we should also fix some other issues / annoyances with our client-side build system.

Goals

• no shell script to build - build everything through preact build (with flags set)
• use SVG sprite sheets for our SVG loader to avoid duplicating inline SVG - most likely we will use this loader, and an example is included for generating React components automatically (which is what happens with our current config using preact-svg-loader)
• use preact-cli's builtin API proxying capabilities in dev mode to avoid needing to have the client access the API cross-origin in dev mode
• fix serving of static files on the server to not require physically moving build outputs on disk (technically a sub-point of removing the shell script)

Upgrading preact-cli (and thus webpack) also will solve our issues with portals crashing on unmount, because this was caused by Webpack resolving the import to the main 'preact' module differently from preact's submodules ('preact/compat'), which are incompatible due to Preact's output setup and mangler in their build.

Ideally this would redirect to /registration while passing the token through using history state, however preact-router does not support attaching state (in JS) to a history entry. We can still implement this by implementing a state store using context and invalidate on any route change, which is enough to implement this.

Originally posted by @ethanwu10 in #200

At the moment, we are hardcoding document.title in the JS bundle:

It would be better if we read this value from the RCTF_NAME env var instead.

Describe the bug
On the scoreboard, when the user's team is not on the current page and the "Go to my team" button is clicked, the user's team is not scrolled into view.

To Reproduce
Steps to reproduce the behavior:

1. Open the scoreboard with the user's team not on the current visible page
2. Click "Go to my team" button
3. Observe that the scoreboard page is changed, but the user's team is not scrolled into view

Expected behavior
The user's team should be scrolled into view after the new page is loaded.

We should finalize on a color scheme for the clientside scoreboard.

Currently static/assets/icons is only referenced in client, importing svgs for icons. We should refactor this directory to client/static/assets/icons to make the dependence more declarative.

(pulled out of #131)

Is your feature request related to a problem? Please describe.
Currently, each icon inlines the entire SVG for the icon, resulting in duplication when the same icon is used multiple times

Describe the solution you'd like
Use SVG sprite sheets for our SVG loader - most likely we will use this loader, and an example is included for generating React components automatically (which is what happens with our current config using preact-svg-loader)

Describe the bug
The HTTP status code 429 Too Many Requests is used to indicate to a client that it is ratelimited. We should use this

To Reproduce
Steps to reproduce the behavior:

1. Make a ratelimited request too quickly (e.g. try to update team info)
2. Observe that the API replies with 401 Unauthorized

Expected behavior
Response code should be 429

Why?

At the moment, we are embedding the client config inside the JS bundle. This means that we cannot distribute rctf Docker images, and that people have to build their own.

How?

We can move all the client config into a JSON blob rendered into the HTML by the server, allowing us to remove all config from the Docker images. The JSON blob can be derived from the server config.

We are currently loading the point value for each challenge individually. This results in calls to the Redis database proportional to the number of solves a user has. For CTFs with a large number of problems, this could be a bottleneck.

We should refactor this with either a server-side cache or hmget.

https://github.com/redpwn/rctf/blob/master/server/api/users/util.js#L30

We should have an endpoint, /api/v1/integrations/ctftime/leaderboard, to return data according to the json scoreboard feed as specified in https://ctftime.org/json-scoreboard-feed.

Note we already have an endpoint for leaderboard data, so this is merely a task of changing the data format.

Description

There is a session fixation vulnerability in rCTF exploitable through the #token=$ssid hash when making a request to the /verify endpoint.

Vulnerable code

 document.title = 'Verify' + config.ctfTitle

    const prefix = '#token='
    if (document.location.hash.startsWith(prefix)) {
      route('/verify', true)

      const verifyToken = document.location.hash.substring(prefix.length)

      verify({ verifyToken })
        .then(errors => {
          this.setState({
            errors
          })
        })
    }

Exploitation Scenario

An attacker team could potentially steal flags by, for example, exploiting a stored XSS payload in a CTF challenge so that victim teams who solve the challenge are unknowingly (and against their will) signed into the attacker team's account. Then, the attacker can gain points / value off the backs of the victims.

Reproduction Steps

1. Create two teams: an attacker, and a victim. Sign into the victim's account.
2. Make an HTTP request to /verify#hash=$ssid where $ssid is the attacker's team code.
3. Observe that you have been logged in as the attacker.

Extra Details

Commit that introduced the vulnerability

1f91230#diff-95a87eb07806dffb6d81c2ffdd27f8f5R16-R32

Potential solution

Instead of having the verification email link immediately sign users in, have it be purely for confirmation purposes. After opening the verification link and verifying the email address, the original registration page--which is polling the server for updates--would receive word that the email is verified. It would then log in without requiring a session ID from user input.

Describe the bug
We currently include polyfills manually in client/src/index.js because our build setup (before #133) removed the polyfills from preact-cli. With the newest version of preact-cli, the polyfills are loaded using nomodule, which should work without an inline script (however an inline script is still being generated). We should drop our polyfills and use the ones that are generated for us to avoid loading the polyfills when they are not needed.

We should use husky to add a pre-push hook for yarn test to ensure that tests don't break on master.

Requirements:

• support Debian and RHEL derivatives
• curl ... | sh-able
• minimal configuration to use

Describe the bug
The installation script and rCTF cli tool both rely on the user being root. However, this is not necessary. We should be able to detect if the current user is in the docker group, and only require root if not.

We are currently importing the entire font-awesome library when we only, in reality, use a few icons. This increases the bundle size by a lot, which would be worth fixing.

https://github.com/redpwn/rctf/blob/master/client/src/index.js#L10

• make a data directory that is runtime-generated and specific to the instance. This allows the user to rm -rf the directory to delete everything, or make a backup more easily. Keep all instance-specific files in it including:
  • the postgresql data directory
  • .rdeploy directory
• change permissions of the postgresql data directory (chown to uid 999)
• if .env doesn't exist at runtime, copy .env.example to .env since .env has sane defaults for docker-compose. This allows it to work on first run.
• make .env work by default for docker-compose (fixed here)

Logging is very ad-hoc on the backend, we should use a structured logging system which tags entries by components and includes relevant data. Also consider setting up config options for logging to cloud services like Google Cloud Logging or CloudWatch (though that should probably be tracked by a separate issue after a logging framework is implemented).

Most likely we should use winston since it's (comparatively) active and well-supported by things like Google Cloud Logging

As mentioned in #73, we should remove the use of prompt which blocks the event loop.

The best solution would probably a custom popup box.

It would be a nice feature to be able to add multiple email addresses to a single account. A single email address would be required to register the account, but after registering, users can add their email addresses and remove them at will (as long as there is always one verified email address tied to the account).

There should be no limits on the number of email addresses you can add, so long as you can verify them all.

The benefits of this feature are:

• if a team member who isn't so smart goes and clicks the "regenerate team token button" and locks everyone out of the account, the team doesn't have to wait for the account owner to wake up to recover the account or anything. There are lots of convenience benefits for this for users.
• if a team wins prizes but is hard to contact at a single address, we now have the email addresses of everyone on the team; that is, we know who is on a team if users opt to give us that information.
• we could possibly collect more email addresses if we use them for notifying about the next RedpwnCTF

Describe the bug
When the client has bad / invalid tokens in localstorage, the UI assumes that the user is logged in, however all data fetching will fail since the tokens are not valid. We should have something that tests the tokens for validity on page load (probably a dummy component that's always mounted that runs the checks on mount - useEffect(()=>{}, [])) and deletes the tokens from localstorage if they are invalid. We would probably need to trigger some sort of a reload (maybe re-mounting all routes is good enough?), since currently most frontend code assumes localstorage tokens don't change except when the route changes.

To Reproduce
Steps to reproduce the behavior:

1. Enter garbage data in localstorage under the keys teamToken and token
2. Observe that UI is in logged-in state, with the profile page completely non-functional.

Expected behavior
The UI should show as the user being logged out. If the current route needs authentication, the user should be prompted to log back in or redirected to the home page.

Screenshots
Broken profile page

We removed the /api/users/me/solves route on accident in this commit.

We should restore this file and the corresponding API route in server/api/index.js.

We should optimize the fonts we are importing, removing all non-ascii chars to decrease bundle size.

We might also want to make font loading configurable, with sane fallback system fonts.

Describe the bug
When a user logs in, they are redirected to /challenges, however the correct route is /challs, resulting in a 404.

To Reproduce
Steps to reproduce the behavior:

1. Log in
2. Get 404

Expected behavior
Challenges page is loaded correctly

At the moment, anyone on the team is allowed to delete the entire team. This can pose some problems with larger teams.

It would be better if we only allow the owner of the email address associated with the team to delete the team.

Is your feature request related to a problem? Please describe.
When viewing the scoreboards, the row with your own team should be highlighted to make it easier to see how you're doing.

In addition, when clicking the "go to my team" button, your team should be scrolled into view if it is not already visible.

Describe the bug
PurgeCSS is removing styles from Cirrus that are legitimately being used (namely many selectors like [class*=" btn-"], [class^="btn-"] for selecting class prefixes), breaking the styling of various pages.

To Reproduce
Steps to reproduce the behavior:

1. Run a development build (yarn dev) and a production build (yarn build:client and connect to localhost:3000 started by yarn dev)
2. Compare how pages are styled (notably anything with buttons - /register, /login

Expected behavior
Dev build (with purgecss disabled) and prod build should look identical

Screenshots
PurgeCSS on:

PurgeCSS off:

We should use a Map for the toasts so that we don't have to do this O(n) removal?

Originally posted by @ginkoid in #125

In the challs listing page, only the maximum number of points is shown. We should fetch the current point value from redis and show that instead.

https://github.com/redpwn/rCTF/blob/1b6011dfd97556524c555388572e526393389fed/client/src/routes/challenges.js#L118

Could we add an option to the Form component instead of overriding the padding using !important?

Originally posted by @ginkoid in #125

Make it so that admins can specify which brackets they want to support in the config. Also, if none are specified, do not show UI elements involving brackets.

Describe the bug
When the server is started without any solves in the database, new solves do not have points associated with them. It appears that anything interacting with redis fails.

To Reproduce
Steps to reproduce the behavior:

1. Clear solves table in postgres and clear redis (flushall)
2. Start server
3. Solve challenge
4. Observe the solve has no points associated with it
5. Observe that there are no keys in redis

Expected behavior
Solve should have points

Logs

UnhandledPromiseRejectionWarning: ReplyError: ERR Error running script (call to  f_7092fe0ac2782e5291c8781f97c50700a3458c4b): @user_script:2: @user_script: 2: Wrong number of args calling Redis command From Lua script

Describe the bug
The current client builds are generating a bundle.esm.js.LICENSE.txt, and it appears the generation is broken as well (it seems to be attempting to put the header for the actual bundle and the contents of the LICENSE comments into the LICENSE.txt file). My best guess is that this is being produced by the Terser plugin, however it should not attempt to output to the file twice, we don't have any license comments in our source for it to keep, and preact-cli appears to have this behavior disabled.

To Reproduce
Steps to reproduce the behavior:

1. Build the client

Expected behavior
No webpack warnings are emitted

Currently, error handling for the register and login components is inconsistent. We need to decide on a standard way of handling and displaying the information on the client.

A PUT request should update the resource to the exact contents of its payload; currently, a PUT request to /admin/challs/:id will do a shallow merge with the existing challenge data. Merging functionality should only happen with the PATCH verb.

We should add a config option to allow people to submit flags after the CTF ends, but with additional code to filter those solves out of the leaderboard and score calculations.

Describe the bug
When the scoreboard page is resized, there is poor performance due to long layouts caused by updating the viewbox of the graph every frame of the resize.

To Reproduce
Steps to reproduce the behavior:

1. Go to /scores
2. Resize the window
3. Observe frame timings and layout timings in devtools performance profiler

Expected behavior
The page should redraw at 60fps while it is being resized.

Potential solutions

• Do not use viewbox to render the graph - this appears to yield about a 2x improvement on layout times (however somehow this also resulted in a ~10x speedup on one of my machines that seems to do particularly bad at updating the viewbox)
• Throttle updates to the graph due to resize events, and scale it in between updates with viewbox. This may still appear janky, but should allow the rest of the page to render at a consistent 60fps.

We should be able to filter challenges by category in /challs on frontend

Cirrus success styling on toasts looks really bad, but we should pass it in as toast({ type: 'success' }) to retain the semantic meaning.

To alleviate the styling issue, we should override the styles on the success Cirrus toast class with something that is not neon green.

We may need to refactor other parts of the codebase to pass in success for what is semantically a "success" too.

• For the sake of simplicity, I think it's best to move the dynamically generated sponsors info page to the bottom of the index/home page. The sponsors info currently disappears when you login, but...
• ...we should make the home page always accessible, even after logging in. It should be on the left side of the navbar. This page is where the CTF description, rules, etc would go. IMO is makes sense to keep this meta together.