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

[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]

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]

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]

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 :)