Download Spam Protection cannot be overridden to allow legitimate downloads

added Desktop label

marked this issue as related to #41764 (closed)

assigned to @ma1

added Doing UX labels

will do more thorough testing later today but on first test I can trigger this error on 12.0.7 but not 12.5. I believe it's in the many download warning popup flow. Triggered for one pdf from one site, but not another in 12.0.7. Nothing in 12.5, just working

both of my test pages have links to pdfs:

test that fails in 12.0.7: https://oceanofpdf.com/authors/charles-stross/pdf-epub-accelerando-download-81724909238/

test that works in both: https://rifters.com/real/Blindsight.htm

So I can repro this in both stable and the RC that Pier gave me.

Go to a download link on Dropbox.com. Preferably make it a large file that takes at least a few minutes to download.
Download it without signing in.
While the download is ongoing, open the same download link in a new tab.
Download it too.
Download will be blocked.
The Downloads dialog will have a button to "Allow" the download, however clicking it does not result in the download starting, and subsequent download attempts of the file are still blocked.

The file I tried to download was a zip. I don't know if PDF files will behave differently. Since Firefox has a bunch of special-case code for PDF handling, there may be subtle differences.

The Downloads dialog will have a button to "Allow" the download, however clicking it does not result in the download starting, and subsequent download attempts of the file are still blocked.

On developer build, testing with #41854 (comment 2914116), clicking the button produces the error at DownloadCore.jsm:721:

OperationError: Could not parse path (undefined): NS_ERROR_FILE_UNRECOGNIZED_PATH

The problem could be that this.target.partFilePath is undefined. I.e. there is no ".part" file. I would guess it might have something to do with our disk leak prevention.

Forgot to say, I reproed this on Windows 32-bit. No idea if other platforms behave the same.

I can also reproduce this in 12.5 using Cure53's POC here: #41764 (closed)

It depends on how frequently downloads are getting flagged by spam prevention. The existing download dialogs were a known pain point before this change was implement.

!595 (diffs)

Here's the commit I did around this, I

Deleted

pref("extensions.torbutton.launch_warning", true);

And added

pref("browser.download.useDownloadDir", true); pref("browser.download.always_ask_before_handling_new_types", false); pref("browser.download.showTorWarning", true);

so using the above url, on a fresh start with 12.0.7, first one worked, second didn't. It may have something to do with the downloading and loading into external pdf app?

Got the yellow download blocked warning and the button to allow which did not work triggering a developer console error that @henry mentioned above:

OperationError: Could not parse path (undefined): NS_ERROR_FILE_UNRECOGNIZED_PATH DownloadCore.jsm:721

So it seems like this is something we've had in TB for a bit, def not a 12.5 regression

Next up, replecating in 12.5, and then digging into the code to see if I can make the allow button work.

But first, following tor-browser##41764 (closed) and @ma1's work on the fix in !640 (merged), I set browser.download.enable_spam_prevention to false and could not trigger this any more, so I suspect we may have introduced this issue then, and it looks like that was backported to 12.0.

So while I go looking at why the Allow button doesn't work, uxier minds than mine may want to think about trade offs if nothing is quickly forth coming (@donuts)

Thanks @dan, that all makes sense!

So while I go looking at why the Allow button doesn't work, uxier minds than mine may want to think about trade offs if nothing is quickly forth coming (@donuts)

It's hard for me to judge atm because I've not ran into this issue at all during my normal workflow, and it's difficult to estimate the scale of the issue. It looks like everyone in this ticket has had to try pretty hard to reproduce it in 12.5, going as far as to deliberately download the same file multiple times.

I've also checked in with @gaba, and since the issue (#41764 (closed)) flagged in the audit was marked as "low" reversing the change would be acceptable. Or, it could also be tied to a higher than Standard security level.

For sure we introduced and backported the issue in !640 (merged), but the alternative is pretty dire: I was checking whether Firefox behaved better regarding the [Allow download] button by using Cure53's PoC, but I forgot to set the preference in advance and I now I'm coping with the download flood DOSing the browser even on restart through session restore (but the "persistence" of the DOS is probably not an issue on Tor Browser).

As far as I can see we've got a double issue here:

the excessive sensitivity of the mitigation, being triggered by "innocent" downloads.
the inability to recover blocked downloads, which might or might not be a Tor Browser-specific issue (that's what I'm trying to assess).

So, let's see if I can fix #1, #2 or both in a reasonable timeframe. If not, we might temporary reverse the change until the "real" fix.

@ma1 yeah I guess I'm chasing #2 now

For the record, it's a Firefox bug: in Fx 115 (NOT Tor Browser) on the [Allow download] button we've got

OperationError: Could not parse path (undefined): NS_ERROR_FILE_UNRECOGNIZED_PATH in DownloadCore.sys.mjs:717

(now it's a standard ES module, but the error is the same).

So I've made some progress but I'm getting the vibe something might be a bit wrong here.

The first error says "file not downloaded" and then you click "allow download" but it calls DownloadsViewUI.jsm unblockAndSave() which then calls into DownloadCore.jsm unblock() which eventually sets a promise to call later which calls await IOUtils.move(this.target.partFilePath, this.target.path); which is where our current error is from.

But it's because this.target.partFilePath is undefined and this.target.path is empty string, and when i protect that call in some if checks, a bunch of other code paths throw errors also trying to access those and associated vars

My gut is that this code is out of sync with other changes, it seems to assume that the files were downloaded to some quarantine (comment reference to [1]), and it just has to move them out, but i don't think that is (any longer?) the case so it's throwing a ton of errors.

I know we are worried about the exploitability of having browser.download.enable_spam_prevention false again, but it's still false in stock firefox and not yet a problem, so i think we'd be ok setting it back false while we dig into this issue which seems to be a broken upstream issue

[1] unblock()'s comment

/**
   * Unblocks a download which had been blocked by reputation.
   *
   * The file will be moved out of quarantine and the download will be
   * marked as succeeded.
   *
   * @return {Promise}
   * @resolves When the Download has been unblocked and succeeded.
   * @rejects  JavaScript exception if any of the operations failed.
   */

The root cause of this Firefox bug is here, not fixed yet in MC because it is actually quite a complicated matter:

// FIXME: In order to escape memory leaks, currently we cancel blocked
// downloads. This is temporary solution, because download data should be
// kept in order to restart the blocked download.

As a work around I was trying, rather than to make the [Allow download] button resume the download immediately (as it should if this old FIXME was... fixed), to make it change the principal's permissions (for the current session only) so that the subsequent (automatic?) attempts from this site do succeed.

@donuts , @richard , your call if this is worth doing now or better just removing the protection in 12.5.

Maybe that's the reason for which that protection is not enabled in Firefox, after all.

I'd be fully okay with removing the protection for 12.5, rather than pushing an untested fix straight to stable (especially a major edition). We can still backport the fix after giving it a spell in Alpha.

@donuts, from IRC backlog (1h and a half ago):

ma1 | oh nice, in Fx 115 the UI for the blocked download has changed place (or is gone?) and nobody bothered to update the callers :( So even if I'm probably fixing #41854 (closed) in 12.5, the patch will need significant changes in 13.0

Eugh, do you want to bail and we can fix this for 13.0 instead then @ma1?

Personally, I'd be okay for postponing to 12.5.1 in case.

We created the deadline for these days, but there's also a rebase in 5 days, with 12.5.1 currently planned for July 5th-ish.

That's fine for me. So, just a fixup to the preference overriding commit? Or a revert commit?

You can git revert --no-commit and then git commit --fixup whatever, too .

Fixups are better because they're easier to rebase later .

In the meanwhile I've made some progress: I did manage to make the [Allow download] button set the permission for this site to trigger further automatic downloads from then on (until the end of the session).

I'm now trying to make it actually complete the last blocked download, but it seems quite more complicated...

changed title from File downloads failing in 12.0.7 to Download Spam Protection cannot be overridden to allow some legitimate downloads

mentioned in merge request !688 (merged)

Download Spam Protection cannot be overridden to allow legitimate downloads

Designs

Child items ...

Activity