Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uniq filenames #808

Closed
jimmywarting opened this issue Jan 2, 2022 · 3 comments
Closed

uniq filenames #808

jimmywarting opened this issue Jan 2, 2022 · 3 comments
Labels
wontfix This will not be worked on

Comments

@jimmywarting
Copy link
Collaborator

Whenever i encounter a "need to generate a uniq filename" and including dependencies and so forth like hexoid to generate them

"hexoid": "1.0.0",

and seeing stuff like:
// prevent directory traversal attacks

Then i might sometimes link to this article: https://advancedweb.hu/secure-tempfiles-in-nodejs-without-dependencies/


fsPromise.mkdtemp is suppose to help you with this...

how about instead generate one uniq dir with mkdtemp and simple use the filename n+1 instead (without extension)?

const uploadDir = await fs.mkdtemp(await fs.realpath(os.tmpdir()) + path.sep);

my proposal: remove hexoid

@GrosSacASac
Copy link
Contributor

Replace hexoid with crypto module ?

As for directory traversal attacks tentative, they can only occur if options.filename is used, in which case the dev already opted out of formidable name generation.

The uploadDir should not be temp because files are made for multiple reuse, the default however is os.tmpdir() which contradicts , but I think only for tests reason (I think most people don't use the default)

If the dev wants to not store the file he can use options.fileWriteStreamHandler.

@tunnckoCore
Copy link
Member

tunnckoCore commented Jan 7, 2022

Hexoid is used only for filenames I think, and the reason is better naming and safer defaults. Avoiding the bad names and characters, Windows defaults, and what not.

I think using something small and efficient like cuid or hexoid is a lot better than the crypto module. They are optimized, they are small, they does great job exactly for such cases. And it's better for bundling cases.

The defaults are safe and secure. Changing comes with responsibility to handle additional things, and reading more docs, as always. We can't, should not, and won't do everything on user's behalf. If you don't want the files or want to "clean up after", just don't write them to the disk in the first place, that's the options.fileWriteStreamHandler option for.

But thanks for the suggestions 😉

@tunnckoCore
Copy link
Member

User responsibility. We provide sane defaults. And yes, it helps for such attacks, at least to some extent. There's far more stupid shit out there that does NOTHING about such things.

Things will change soon anyway.

Closing.

@tunnckoCore tunnckoCore added the wontfix This will not be worked on label Apr 2, 2022
davidmehren pushed a commit to hedgedoc/hedgedoc that referenced this issue Apr 10, 2022
This patch adds an own filename function for `formidable`, which will
make sure to generate a random file name, using UUIDv4. This should
resolve GHSA-q6vv-2q26-j7rx.

This change is required due to a change in behaviour from version 1 to
version 2 of formidable. Formidable version 2 will generate predictable
filenames by default, which results in potential access to images, that
were uploaded while formidable v2 was used in Hedgedoc. This affects the
versions `1.9.1` and `1.9.2`.

Files generated previous to this commit will look like this:

```
<random string generated on app start><counter>.<file-extension>
38e56506ec2dcab52e9282c00.jpg
38e56506ec2dcab52e9282c01.jpg
38e56506ec2dcab52e9282c02.jpg
```

After this patch it'll look like this:

```
<uuid v4>.<file-extension>
a67f36b8-9afb-43c2-9ef2-a567a77d8628.jpg
56b3d5d0-c586-4679-9ae6-d2044843c2cd.jpg
2af727ac-a2d4-4aad-acb5-73596c2a7eb6.jpg
```

This patch was implemented using `uuid` since we already utilise this
package elsewhere in the project as well as using a secure function to
generate random strings. UUIDv4 is ideal for that. In order to be
consumable by formidable, it was wrapped in a function that makes sure
to keep the file extension.

This vulnerability was reported by Matias from [NCSC-FI](https://www.kyberturvallisuuskeskus.fi/).

References:
https://github.com/node-formidable/formidable/blob/v2-latest/src/Formidable.js#L574
node-formidable/formidable#808 (comment)
https://www.npmjs.com/package/uuid
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants