Opened 4 years ago

Closed 3 years ago

#14716 closed defect (fixed)

HTTP Basic Authentication prompt only displayed once

Reported by: mcs Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-usability-stoppoint-navigation, TorBrowserTeam201504R
Cc: brade, gk, mikeperry, popupbug Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The username/password prompt for HTTP Basic Authentication is not displayed a second time. This seems to be related to various errors that are logged to the Browser Console (see attached log).

To reproduce this problem:

  1. Open https://www.httpwatch.com/httpgallery/authentication/
  2. Click the "Display Image" button within Example 10 on that page.
  3. When prompted, enter httpwatch for the username and any password.
  4. Click the "Display Image" button a second time.

Expected result: a new username / password prompt.
Actual result: no prompt.

Child Tickets

Attachments (1)

auth-log.txt (2.5 KB) - added by mcs 4 years ago.
Browser Console errors

Download all attachments as: .zip

Change History (17)

Changed 4 years ago by mcs

Attachment: auth-log.txt added

Browser Console errors

comment:1 Changed 3 years ago by gk

Cc: gk added

comment:2 Changed 3 years ago by gk

This seems to be another fallout of fixing #12998. #13254 has similar symptoms. I wonder whether real world basic auth systems are working at all. I was not able to get a response from a server. Maybe all the users have disk history enabled and this is the reason no one stumbles over it?

comment:3 Changed 3 years ago by cypherpunks

I can reproduce this bug. My community uses HTTP basic auth as a protection against automated attacks, and it's quite annoying to misspell my username or password. I have to close the TBB and start it up again.

Maybe also of note: choosing a New Identity from the onion menu doesn't resolve the problem, so this might be fingerprinting the browser instance as well.

comment:4 Changed 3 years ago by gk

Cc: popupbug added

#15387 is a duplicate.

comment:5 Changed 3 years ago by mikeperry

Keywords: tbb-usability-stoppoint-navigation added

comment:6 Changed 3 years ago by mcs

Another site that may be useful for testing: http://browserspy.dk/password.php

Last edited 3 years ago by mcs (previous) (diff)

comment:7 in reply to:  3 Changed 3 years ago by mcs

Replying to cypherpunks:

Maybe also of note: choosing a New Identity from the onion menu doesn't resolve the problem, so this might be fingerprinting the browser instance as well.

I am not 100% sure, but I think the reason failures continue even after New Identity is because the login manager code ends up in a bad state and stays that way until you restart the browser.

The very first failure occurs inside toolkit/components/passwordmgr/nsLoginManager.js in the _storage getter. The root cause is deep inside NSS due to the lack of a key DB (due to #12998). Kathy and I have experimented with two possible fixes:

  1. Add null checks for _storage in several places inside nsLoginManager.js.
  2. Put a hack inside NSC_InitPIN() (inside security/nss/lib/softoken/pkcs11.c) that returns CKR_OK instead of an error if there is no key DB and the password/pin has length zero. This fixes the problem because the fallback code uses a zero-length password to initialize an in-memory security DB. And I think (but am not certain) that NSC_InitPIN() is trying to set a new password, which is an uninteresting thing to do in this case.

The first approach is fairly straightforward but involves more changes. The second approach is more of an unknown but may possibly fix other "fallout" from #12998 (probably we would need to hold off until our next test release).

Feedback welcome.

comment:8 Changed 3 years ago by mcs

Cc: mikeperry added

We decide to make the fixes available (it is almost always better to look at code).

Approach 1 is on the bug14716-loginmgr-01 branch in the brade tor-browser repo (https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug14716-loginmgr-01&id=f0fe46bcec5946e94fd66792983b1f081913a74f).

Note that many of the functions to which we added null checks are not called (probably because with these changes getLoginSavingEnabled() will return false and countLogins() will return zero, which prevents other code from being executed).

Approach 2 is on the bug14716-loginmgr-01 branch (https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug14716-softoken-01&id=27ff6a1f17a006620b092ddc79ed37d9c3e74c35).

Short and sweet -- but Kathy and I do not understand the surrounding code well.

comment:9 Changed 3 years ago by mcs

There is one more thing that I forgot to mention: with the nsLoginManager.js fix (approach 1), messages similar to the following still appear on the Browser Console:

[Exception... "Initialization failed'Initialization failed' when calling method: [nsILoginManagerStorage::init]" nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)" location: "JS frame :: jar:file:///Users/.../tb-14716.app/Contents/MacOS/omni.ja!/components/nsLoginManager.js :: LoginManager.prototype._storage :: line 98" data: no]

I am sure we could suppress these messages if necessary.

comment:10 Changed 3 years ago by gk

I think we should go with approach 1 which I sort of had in mind for #13254 (which somehow dropped from my ToDo list, or better, got pushed down on it). Two reasons for this:

1) I'd guess this has much better chances to get upstreamed. I mean, why not checking for _storage first before using it? Compared to the NSS related fix this might be much less controversial and fixes the issue where it occurs.
2) IMO this would allow us to sneak the patch still into 4.5. While we may not know exactly what these checks fix I guess they help a lot and there is not much that can go wrong compared to our current state where we simply throw and the functionality is broken. Usability ftw!

To minimize the patch we should only add checks where really needed. I am not sure about suppressing the strings yet. I think in the end we want what Mozilla is comfortable to merge with but ideally I'd like to avoid spamming the error console.

comment:11 Changed 3 years ago by brade

If we are going to go with the first patch, I'd rather include all of the checks (not just the code paths we happen to cross with the scenario presented here). If the logic is valid to add the checks in some of the code paths it seems like we should do so in the other cases as well. Also, I would guess that it would be easier to land a patch in Mozilla if it covers every case.

Mark and I are looking into a way to suppress the error console message that we saw.

comment:12 Changed 3 years ago by mikeperry

Keywords: tbb-4.5-alpha added

comment:13 in reply to:  11 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201504R added
Status: newneeds_review

Replying to brade:

Mark and I are looking into a way to suppress the error console message that we saw.

Unfortunately, it seems that suppressing the Browser Console message that I mentioned in ticket:14716#comment:9 will be difficult. Minus a few rare cases, whenever an exception is throw across an XPConnect boundary it is logged to the Browser Console. See:
http://mxr.mozilla.org/mozilla-esr31/source/js/xpconnect/src/XPCWrappedJSClass.cpp#891

The good news is that the fix we already have on the bug14716-loginmgr-01 branch within the brade repo appears to fix this ticket as well as #13254 and most of the tickets that were marked as duplicates of that ticket. So it is a huge improvement, as Georg predicted!

However, I still see the symptoms described in #13926, so that must be caused by something else or some aspect of nsLoginManager.js that we did not patch. It takes 5 or 6 cycles of opening and closing the Page Info window and certificate viewer to produce the problem, which makes it more difficult to debug.

For TB 4.5, I think we should go ahead and merge the fix we already have, i.e.,
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug14716-loginmgr-01&id=f0fe46bcec5946e94fd66792983b1f081913a74f
We can pursue an improved fix as part of our ff38-esr efforts.

comment:14 Changed 3 years ago by mikeperry

Ok, sounds good. This fix seems simple enough. I've merged it for 4.5. If you want to close this ticket and file another one for the remaining issues, feel free to do that.

comment:15 Changed 3 years ago by mikeperry

Keywords: tbb-4.5-alpha removed

comment:16 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

I think we can close this ticket as its issue is solved. I've reopened #13926 and opened #15770 for tests around security.nocertdb.

Note: See TracTickets for help on using tickets.