[security] files outside of ./static are served publicly in dev mode

[AlbertMarashi]

Describe the bug

Unexpected behaviour / Security vunerabilty. Files inside the root directory (outside of the static directory) are exposed to anyone via URLs.

Users may have secrets, & keys in their root directory, which should NOT be visible to anyone viewing from the web

Reproduction

1. Create a fresh sveltekit install
2. run npm run dev
3. goto localhost:3000/svelte.config.js -> This file should not be accessible.

Logs

No response

System Info

System:
    OS: macOS 11.5.2
    CPU: (8) arm64 Apple M1
    Memory: 169.05 MB / 8.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.3.0 - ~/.nvm/versions/node/v16.3.0/bin/node
    npm: 7.15.1 - ~/.nvm/versions/node/v16.3.0/bin/npm
  Browsers:
    Chrome: 94.0.4606.81
    Safari: 14.1.2
  npmPackages:
    @sveltejs/adapter-node: ^1.0.0-next.55 => 1.0.0-next.55 
    @sveltejs/kit: next => 1.0.0-next.184 
    svelte: ^3.34.0 => 3.43.2

Severity

blocking all usage of SvelteKit

Additional Information

No response

[benmccann]

I think the bug is on this line: https://github.com/vitejs/vite/blob/d1c85d1c053bf65ab691e697063f9796109120de/packages/vite/src/node/server/index.ts#L528

I believe it is serving the root directory regardless of what fs.allow is

[benmccann]

PR to fix: vitejs/vite#5361

[benmccann]

I've upgraded SvelteKit to Vite 2.6.11, but it looks like there's another issue preventing this from being fixed yet: vitejs/vite#5416

[AlbertMarashi]

Great, think it's been merged now.

[Prinzhorn]

@benmccann I did dig a little into the whole Vite serveStaticMiddleware / serveRawFsMiddleware / isFileServingAllowed / ensureServingAccess code. I find this a little too complex for security relevant code. The whole things can be bypassed via URL encoded path traversal:

cd /tmp/
npm init svelte@next my-app
cd my-app
npm install
npm run dev

# In other terminal:
curl http://localhost:3000/@fs/tmp/my-app/%2E%2E/%2E%2E/etc/passwd

In contrast to serveStaticMiddleware (https://github.com/vitejs/vite/blob/d1c85d1c053bf65ab691e697063f9796109120de/packages/vite/src/node/server/middlewares/static.ts#L65) the serveRawFsMiddleware does not url decode the path. This creates an asymmetry (a different understanding of the same thing) with sirv which can lead to security issues. Until two month ago the path traversal was also possible with serveStaticMiddleware (I think) but this was accidentally fixed (vitejs/vite#4728).

To me this whole things does not belong into Vite, it is the responsibility of sirv. It already has a dir argument. It doesn't make much sense to set it to / and then trying to limit the things again.

Couldn't this entire code be deleted from Vite and replaced with something like this?

fs.allow.forEach((dir) => {
  middlewares.use(sirv(dir))
});

To be fair I've never used Vite outside of SvelteKit and only played with SvelteKit a little. But I do follow all security related issues.

Edit: fwiw, because of req.url = url even url decoding on vite's end wouldn't help. I'd just double encode it and vite still thinks it's in the allowed directory but the second decode in sirv will cause the traversal. First source I found https://owasp.org/www-community/Double_Encoding

[benmccann]

Good find @Prinzhorn ! I'd recommend filing an issue in the Vite repo about this

[chanced]

As an aside, you shouldn't keep sensitive files in a project folder structure anyway, especially if it is under source control (which it should be).

If you need to, consider using SOPS or some other form of source-control safe encryption mechanism. There are a few alternative offerings out there.

[Conduitry]

As another aside, this whole issue only affects projects served in dev mode, and if you're worried about what people accessing your app served in dev mode might see, you have a whole additional set of largely unsolvable problems.

[benmccann]

I think an even more important point is that the dev mode server is only accessible from outside your machine if you explicitly pass the --host flag to enable that. If you want to share your server you can also use preview mode where this wouldn't come into play

[AlbertMarashi]

To me this whole things does not belong into Vite, it is the responsibility of sirv. It already has a dir argument. It doesn't make much sense to set it to / and then trying to limit the things again.

Couldn't this entire code be deleted from Vite and replaced with something like this?

I had a similar idea, but didn't wanna overstep other people and tell them how to run their libraries. Oh well.