Closed Bug 58774 Opened 24 years ago Closed 24 years ago

We should "salt" the temp file name we store external content to

Categories

(Core Graveyard :: File Handling, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mscott, Assigned: mscott)

References

Details

(Whiteboard: [rtm++] r=sspitzer, sr=alecf)

Attachments

(1 file)

When you click on a link that requies an external handler (i.e. requires us to open a helper app or save to disk), we immediately start downloading the content into a temp file while the user chooses their final destination for the content. This temp file name is currently generated based on the file name in the url we are trying to run. This could pose a security hazard as we are placing content in a predictable place with a predictable file name. The fix is to do what the profile folks are doing with salting profile names. Use a salted name for the name of the temporary file. I'll attach the diffs.
adding rtm keywords since the fix is low risk with a high gain (from a security stand point).
Keywords: rtm
Whiteboard: [rtm need info]
Here's the proposed fix. Instead of using the file name in the url for the temp file name, generate a random name for the temp file. This code was copied directly from the code the profile team is using to salt profile names. Seth and I are trying to figure out where to find a good home for this code on the tip so it wouldn't need to be copied. seeking an r=sspitzer, sr=alecf??
Status: NEW → ASSIGNED
Target Milestone: --- → M18
nominating for 0.9 should this not make rtm
Keywords: mozilla0.9
r=sspitzer. looks good. I'll go log a bug to make sure we combine the duplicated code.
Whiteboard: [rtm need info] → [rtm need info] r=sspitzer, sr=?
As an added bonus, this fix also fixes another rtm bug I have: 56296 --> nsLocalFileMac chokes when given files > 31 characters, this causes us to fail to download content on the Mac where the file name is > 31 characters.
my only concern here is that certain external applications might be expecting a file to have a specific extension... i.e. RealAudio MIGHT be expecting the file to be called foo.ram if we salt it, do we loose the extension (looks like it from the patch)? I believe 4.x retained the extension, or at the very least tacked on the default extension from the MIME types for that type. If I'm imagining 4.x behavior, then never mind :) the patch itself looks fine, sr=alecf - leaving in the need info state so that my question about filename extensions can be addressed.
Whiteboard: [rtm need info] r=sspitzer, sr=? → [rtm need info] r=sspitzer, sr=alecf
actually the patch applies the file extension from the url onto the salted name. So we do indeed preserve the file extension and every thing is kosher. Moving to rtm+ now that everything is lined up.
Whiteboard: [rtm need info] r=sspitzer, sr=alecf → [rtm+] r=sspitzer, sr=alecf
Component: Browser-General → Networking
Keywords: patch
OS: Windows NT → All
QA Contact: doronr → sairuh
Hardware: PC → All
This bug is in candidate limbo
This has been checked into the tip.
PDT marking [rtm-]. Medium benefit, medium risk, not enough at this point.
Whiteboard: [rtm+] r=sspitzer, sr=alecf → [rtm-] r=sspitzer, sr=alecf
Blocks: 59114
QA Contact: sairuh → tever
rtm++, taking on respin.
Whiteboard: [rtm-] r=sspitzer, sr=alecf → [rtm++] r=sspitzer, sr=alecf
checked into the branch and the tip.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Scott - can you give suggestions on verification? This extends beyond mail attachments, doesn't it?
My apologies I made a big post right after Lisa asked for verification instructions but apparently I wasn't logged into bugzilla so my changes didn't get submitted and I never noticed the login screen. Let's try this again a day later: Browse to www.mozilla.org. Click on one of the nightly build links. When the helper app dialog comes up, don't do anything. =) Go to your temp directory. Search for *.exe, *.bin, *.gz (depending on the nightly you downloaded). You should see an obscured name in the temp directory like x584lkz.exe. This is the temp file we are downloading content in the background into. Now go ahead and go back to the helper app dialog and click save to disk. Pick a file location (for kicks use the same directory the salted file is in). When the download is complete, go back to the temp directory and make sure the salted name is no longer there. Verify that the file name you chose is there. An added bonus --> try launching with a helper app. i.e. click on a mp3 link or something.
vrfied using opt comm branch bits [using mscott's steps]: linux rh 6.2 2000.11.08.07-n6 winNT 2000.11.08.01-n6 mac os 9.0 2000.11.08.00-n6
Keywords: vtrunk
Is this really a security issue? It causes lots of confusion with downloaded filenames. See bug 60203.
Simon, As Scott describes, this is a security issue as it could allow the placing of foreign content at a known location. I see from 60203 that people are confused, so we need to educate people that this is now the expected behavior.
this is similar to 63225
Status: RESOLVED → VERIFIED
QA Contact: tever → benc
qa to me. VERIFIED, per Sarah.
Blocks: advocacybugs
No longer blocks: advocacybugs
This security fix was reversed in bug 60203.
not quite - during the download, the salted file name is still used. only once the download is finished is the file renamed (in the case of a helper application).
So? In the case of a helper application that might be switched to automatic by the user, like pdf files perhaps, the files are still renamed to a predictable name without a user interaction. It should not be hard to use a delay and then invoking that file from JS to execute something with local permissions. The root of the problem is that the salting is very similar to security by obscurity. In my opinion it would be safer to declare all download locations like the caches and temp directories equivalent to internet zone because that's where the content in these directories comes from. This would amount to something similar as tainting these locations, as perl does with internal dataflow.
Depends on: 369428
Component: Networking → File Handling
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: