Need ability to disable stack in _data error response to limit attackers identifying and exploiting dependencies

[ngbrown]

What version of Remix are you using?

1.5.1

Steps to Reproduce

Pass an invalid parameter to the /?_data=routes%2Fsomething_invalid endpoint. Observe the results. Or pass an invalid POST body to delve deeper into the application stack.

Expected Behavior

Need a way to disable the stack return data in production and have a more generic error returned to the client.

Actual Behavior

During testing by our security analyst, a stack trace was returned that identified the logging system in use and the open-source libraries of the application code running. This could be used by an attacker to exploit dependencies in the application the most serious of which may have zero day implications.

This line unconditionally returns the stack:

remix/packages/remix-server-runtime/errors.ts

Line 68 in 252ea6c

stack: error.stack,

[pkalisiewicz]

I'm happy to work on this bug, the question is, do we want to just remove the stack trace when in production mode, or do we want to filter out paths, etc?

[p-m-p]

This appears to be quite a big security issue if there is no way to suppress the stack trace in responses with uncaught errors relying on use of ErrorBoundary.

I need to solve this myself either through a change to the framework or by other means; perhaps adding a middleware in express to redact the stack traces.

I had a quick look and I think serializeError could possibly take a second parameter to optionally add the stack trace if serverMode is not set to ServerMode.Production in the handler functions?

[kasprownik]

Hey! If you want to quick fix the above error, this is a snippet from the express instance I use to strip stack from the response and return 400 on incorrect _data param:

app.all(
  '*',
  createRequestHandler({ build: require('../build'), mode: 'production' }),
  (err, req, res, next) => {
    if (
      err.message === "Cannot read properties of undefined (reading 'params')"
    ) {
      res.status(400).send({ error: 'bad request' });
    } else {
      res.status(500).send({ error: 'unexpected error occured' });
    }
  }
);

[p-m-p]

Hey! If you want to quick fix the above error, this is a snippet from the express instance I use to strip stack from the response and return 400 on incorrect _data param:

app.all(
  '*',
  createRequestHandler({ build: require('../build'), mode: 'production' }),
  (err, req, res, next) => {
    if (
      err.message === "Cannot read properties of undefined (reading 'params')"
    ) {
      res.status(400).send({ error: 'bad request' });
    } else {
      res.status(500).send({ error: 'unexpected error occured' });
    }
  }
);

Probably not going to work so well if you want to maintain use of CatchBoundary for things like 404 page.

It's probably possible to remove stack from data and page requests (it's in appState) with a middleware and a bit of hackery. I've not looked into it yet but imagine there will be some trade offs in being able to use server components/streams.

[machour]

Converting this to a Proposal discussion, as per our Development Process.

[p-m-p]

I had raised this proposal previously #4564.

[ngbrown]

Recent versions of Remix now include a serializeErrors() function that also includes this problem.

I noticed that react-router-dom server.js's serializeErrors() now includes a line:

// Do not serialize stack traces from SSR for security reasons

Other than that, it is the exact same function. So the solution looks to be to copy from react-router-dom to @remix-run/server-runtime and also remove stack from serializeError.

[brophdawg11]

This should be resolved as of 1.15.0 via #5541. There is also a new handleError function where you can capture and log/report the error on the server prior to the stack trace being stripped.