Very fishy file-name / hash generation in incoming_form.js #247

Open
x00mario opened this Issue Jul 24, 2013 · 7 comments

Projects

None yet

3 participants

@x00mario

https://github.com/felixge/node-formidable/blob/master/lib/incoming_form.js#L516

The code used to generate the file-name looks quite fishy to me - to say the least:

var name = '';
for (var i = 0; i < 32; i++) {
    name += Math.floor(Math.random() * 16).toString(16);
}

Is there any reason why you don't use the NodeJS crypto API (specifically crypto.createHash(algorithm) or alike) to get an actual hash and not this "Hey let's concat the single-char content from the provably insecure Math.random() function of doom?"

Cheers,
.mario

@felixge
Owner
felixge commented Jul 24, 2013

Is there any reason why you don't use the NodeJS crypto API

Yes, IIRC the node crypto API did not exist when I wrote this code ; ).

Anyway, the only purpose of this function is to generate a unique filename that has a very low chance of collision. IMO the current code does that well enough, but that being said, I'd be happy to accept a patch to use the crypto module as a source of randomness.

@x00mario

"that has a very low chance of collision" < don't let the crypto people hear that ;)

I never write patches, just doing an eval on this library and report spotted issues for an external entity. Writing the patches is out of scope right now.

I do think nevertheless that collisions are possible in case a sufficient amount of POST requests is being sent to the server - and that an attacker might thereby be able to overwrite existing files if the stars are aligned properly. Changing the code to make use of the crypto API would effectively fix that and take the burden off the developer's shoulders to take care of that possibility.

@felixge
Owner
felixge commented Jul 24, 2013

I never write patches

💔

Changing the code to make use of the crypto API would effectively fix that and take the burden off the developer's shoulders to take care of that possibility.

http://felixge.de/2013/03/07/open-source-and-responsibility.html

I'm unfortunately able to do this right now, but again, I'm happy to accept a patch if anybody cares.

@johnwilander

Felix, I think there are two issues here.

Firstly, a hash should be reproducible. You shall always get the same output for the same input. This is a requirement for hashing whether you want cryptographic safety or just something that works in a hash table. Your "hash" code is the opposite of reproducible since it's (pseudo) random. I suggest you rename it to uniqueIdentifier or the like to not confuse people.

Secondly, pseudo randomness is not a good way to achieve uniqueness. A combination of currentTimeInMillis and an add-on of either a short pseudo random number or a per-runtime counter should do it.

@felixge
Owner
felixge commented Jul 24, 2013

Firstly, a hash should be reproducible. You shall always get the same output for the same input. This is a requirement for hashing whether you want cryptographic safety or just something that works in a hash table. Your "hash" code is the opposite of reproducible since it's (pseudo) random. I suggest you rename it to uniqueIdentifier or the like to not confuse people.

I'm afraid Mario and you are confused. I never called this a hash. It's not supposed to be one. The function is named _uploadPath(). That being said, renaming it to _uniqueUploadPath might be a nice touch.

Secondly, pseudo randomness is not a good way to achieve uniqueness. A combination of currentTimeInMillis and an add-on of either a short pseudo random number or a per-runtime counter should do it.

Thanks, but the issue here isn't a lack of a good solution, but the lack of a volunteer to implement it.

@johnwilander

Felix, sorry for the confusion. If it's not supposed to be a hash it obviously doesn't need to be reproducible.

So you're just solving a collusion-free storage, right? There's no risk for enumeration where UserA sees his file is named "budget12345.doc" and starts firing requests for "budget12344.doc", "budget12343.doc", "budget12342.doc", et cetera? Several high-profile file storage services have had this problem before.

@x00mario

To summarize my point: From what I can see, the targeted uniqueness of the file-name is very weak. Also the headline for the ticket I chose was technically wrong - I shouldn't have co-labelled it "hash". My bad.

In the usage scenario I was evaluating the library for, this lack of reliable uniqueness is a problem. A one-line patch will fix this issue. Yet I am not the one who can and/or will submit this patch. I am simply pointing out the issue and that's that - cheers to the person who will at some point submit a patch, kthxbai :)

@Jimbly Jimbly added a commit to Jimbly/node-formidable that referenced this issue Sep 9, 2014
@Jimbly Jimbly Change upload temporary file path generation to use crypto.randomByte…
…s().

This should resolve an issue when using the Node cluster module, and two workers get forked with the same random seed, and the two processes' upload paths collide.
This also resolves the (somewhat confused) complaints of #247.
4cf6b7e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment