1216882 - When "security.nocertdb" pref is true, HTTP Auth Dialog fails (Tor 14716)

Status: NEW

(Core :: Networking: HTTP, defect, P5)

Description

[Arthur Edelstein [:arthur]]

In bug 629558, we introduced a boolean pref, "security.nocertdb", that when enabled ensures the intermediate certificate store is memory only. Unfortunately, this caused an unexpected bug, where the username/password prompt for HTTP Basic Authentication is not displayed a second time. Here's how to reproduce it (copied from https://trac.torproject.org/14716):

0. In about:config, add a boolean pref "security.nocertdb" and set to true.
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.

Comment 1

[Arthur Edelstein [:arthur]]

The latest version of Tor Browser's fix for this issue is here:
https://torpat.ch/14716

Comment 2

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1216882-Fix-bug-in-HTTP-Auth-prompt-when-securit.patch

Here's Tor Browser's patch, rebased to the latest version of mozilla-central.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8677747 [details] [diff] [review]
0001-Bug-1216882-Fix-bug-in-HTTP-Auth-prompt-when-securit.patch

Review of attachment 8677747 [details] [diff] [review]:
-----------------------------------------------------------------

FWIW, I think this is the wrong level to fix this at. Something in PSM (probably nsSDR or one of the PKCS11 interfaces) is unexpectedly failing because of the lack of a permanent database, and it would be better to handle the problem there.

Comment 4

[Arthur Edelstein [:arthur]]

Comment on attachment 8677747 [details] [diff] [review]
0001-Bug-1216882-Fix-bug-in-HTTP-Auth-prompt-when-securit.patch

(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> Comment on attachment 8677747 [details] [diff] [review]
> 0001-Bug-1216882-Fix-bug-in-HTTP-Auth-prompt-when-securit.patch
> 
> Review of attachment 8677747 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> FWIW, I think this is the wrong level to fix this at. Something in PSM
> (probably nsSDR or one of the PKCS11 interfaces) is unexpectedly failing
> because of the lack of a permanent database, and it would be better to
> handle the problem there.

Thanks for pointing this out. I'm removing the review request while we look at problem again.

Comment 5

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1216882-Fix-incorrect-handling-of-key-DB-when-se.patch

(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> Comment on attachment 8677747 [details] [diff] [review]
> 0001-Bug-1216882-Fix-bug-in-HTTP-Auth-prompt-when-securit.patch
> 
> Review of attachment 8677747 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> FWIW, I think this is the wrong level to fix this at. Something in PSM
> (probably nsSDR or one of the PKCS11 interfaces) is unexpectedly failing
> because of the lack of a permanent database, and it would be better to
> handle the problem there.

I think this is correct. When Firefox starts up, I see the following error message:

JavaScript error: ...dist/NightlyDebug.app/Contents/Resources/components/crypto-SDR.js, line 66: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIPK11Token.initPassword]

I tracked the reason for this error to the following lines in pkcs11.c:

https://dxr.mozilla.org/mozilla-central/rev/9a8f2342fb3116d23989087e026448d38a3768c5/security/nss/lib/softoken/pkcs11.c#3541

It turns out that sftk_getKeyDB(slot) returns NULL, so NSC_InitPIN returns the error code CKR_PIN_LEN_RANGE.

The reason sftk_getKeyDB(slot) is returning NULL is that earlier we set slot->certDB to NULL and never initialized it because the following `if` statement returned false:
https://dxr.mozilla.org/mozilla-central/rev/9a8f2342fb3116d23989087e026448d38a3768c5/security/nss/lib/softoken/pkcs11.c#2426

That's to be expected, as "security.nocertdb" is set to true.

So here's a patch that appears to gracefully handle sftk_getKeyDB(slot) == NULL. It gets rid of the error messages, and also fixes the original HTTP Auth dialog bug.

(I don't really understand the design of the PKCS11 library, so any advice here would be appreciated.)

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8679748 [details] [diff] [review]
0001-Bug-1216882-Fix-incorrect-handling-of-key-DB-when-se.patch

Review of attachment 8679748 [details] [diff] [review]:
-----------------------------------------------------------------

This is NSS code, so even if I were familiar with it, I wouldn't be appropriate to review it. In any case, I think I've come up with a more compartmentalized solution that (hopefully) is less likely to have unintended side-effects.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: 1216882-catch-initPassword-failure.diff

Comment 8

[Justin Dolske [:Dolske]]

I'm probably ok with this as a workaround in password manager... But does it actually work? In other words, what happens if the user saves a password and tries to later use it? In a new session?

Seems like this is really a flaw in NSS/PSM, and it's not clear to me that bug 629558 should inherently have had any effect here (other than by somehow screwing up softtoken initialization?). In particular, I'm confused because the password manager really just uses this via SecretDecoderRing, which basically just uses a key from key3.db to do the crypto. No certs involved at all.

Forcing all of NSS/PSM into "nodb" mode is a pretty big hammer for just avoiding intermediate certs. I didn't read through all the code, but it sounds like this entirely prevents creation of cert8.db and key3.db? And I wouldn't be surprised if that mode doesn't work well with Firefox or had never been tested...

Comment 9

[Arthur Edelstein [:arthur]]

Comment on attachment 8679748 [details] [diff] [review]
0001-Bug-1216882-Fix-incorrect-handling-of-key-DB-when-se.patch

Review of attachment 8679748 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Justin Dolske [:Dolske] from comment #8)
> I'm probably ok with this as a workaround in password manager... But does it
> actually work? In other words, what happens if the user saves a password and
> tries to later use it? In a new session?

We have been using "security.nocertdb" -> true in Tor Browser for a while, and it does work, in the sense that HTTP username/password are remembered within a session. However, in a new Firefox session, the credentials must be entered anew (which is the desired behavior).

> Seems like this is really a flaw in NSS/PSM, and it's not clear to me that
> bug 629558 should inherently have had any effect here (other than by somehow
> screwing up softtoken initialization?).

Yes, I agree. Does my patch make any sense at all? Unfortunately I have little understanding NSS/PSM, so I don't know what side effects there might be. If this isn't safe, then probably David's patch is a better approach.

Comment 10

[Justin Dolske [:Dolske]]

(In reply to Arthur Edelstein from comment #9)

> > I'm probably ok with this as a workaround in password manager... But does it
> > actually work? In other words, what happens if the user saves a password and
> > tries to later use it? In a new session?
> 
> We have been using "security.nocertdb" -> true in Tor Browser for a while,
> and it does work, in the sense that HTTP username/password are remembered
> within a session. However, in a new Firefox session, the credentials must be
> entered anew (which is the desired behavior).

No, I mean are the credentials being saved in the login manager database, and are those credentials readable in the next session?

I ask because it _sounds_ like this isn't initializing any of the NSS databases, including key3.db, which would mean users with this enabled are saving bad entries in the password manager that can't be retrieved later.

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 8680289 [details] [diff] [review]
1216882-catch-initPassword-failure.diff

Yeah, this might make HTTP auth work, but it leaves password manager broken.

If I try to same a password from a web page (say, Gmail) with this, upon clicking "Remember" I get prompt to set a password on the software security device. Which doesn't actually work, attempting to do so results in an error and all the user can do is click cancel.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Justin Dolske [:Dolske] from comment #11)
> Comment on attachment 8680289 [details] [diff] [review]
> 1216882-catch-initPassword-failure.diff
> 
> Yeah, this might make HTTP auth work, but it leaves password manager broken.
> 
> If I try to same a password from a web page (say, Gmail) with this, upon
> clicking "Remember" I get prompt to set a password on the software security
> device. Which doesn't actually work, attempting to do so results in an error
> and all the user can do is click cancel.

That's what I'm seeing too. In any case, I imagine if a user sets the "security.nocertdb" pref, they also wouldn't want to save any logins, so perhaps that whole subsystem should be disabled there.

Going back to the big picture, "security.nocertdb" is not a first-class feature. It isn't tested and it isn't guaranteed to not break things. My understanding was our support of it would be limited to occasional, small-scope fixes to get specific things working (like http auth in this case).

Now that I think about it, though, I'm not even sure what it's for. Presumably anyone who sets this pref doesn't want to save anything to disk (and perhaps blowing away the profile directory on exit isn't considered good enough). If that's the case, have they also disabled memory swapping? What about coldboot attacks? I realize that's a bit of a "prefect being the enemy of good" argument, but as we've seen, "security.nocertdb" isn't actually even that good.

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 8679748 [details] [diff] [review]
0001-Bug-1216882-Fix-incorrect-handling-of-key-DB-when-se.patch

...so I think fixing this down in NSS/PSM is probably the correct route. Although my experience has been that this code is really obtuse and fragile, so I'm not going to be able to spend enough time on it in the near future to plausibly review. How about Tim, who I hear has newly found enthusiasm for NSS/PSM?! :)

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8679748 [details] [diff] [review]
0001-Bug-1216882-Fix-incorrect-handling-of-key-DB-when-se.patch

Sorry, I'll have to refer back to comment #6. David would probably be a way better fit to review this as I certainly know less than he does about NSS. But both of us can't review NSS patches, or know a lot about that part of the code as it seems.

Comment 15

[Wan-Teh Chang]

Comment on attachment 8679748 [details] [diff] [review]
0001-Bug-1216882-Fix-incorrect-handling-of-key-DB-when-se.patch

Review of attachment 8679748 [details] [diff] [review]:
-----------------------------------------------------------------

Bob: could you please review this patch? Thanks.

Arthur: although I am not reviewing the patch, I took a quick
look and have some suggestions and questions for you.

::: security/nss/lib/softoken/pkcs11.c
@@ +3540,4 @@
>  
>      handle = sftk_getKeyDB(slot);
>      if (handle == NULL) {
> +	return CKR_OK;

Although I can't evaluate whether this change is correct, I think
this should at least be accompanied with a comment, because it is
not clear why this function should return successfully when the
sftk_getKeyDB() fails.

Also, I wonder if we should only return the sftk_getKeyDB() failure
after checking the error code. It seems wrong to ignore the failure
blindly. Perhaps we should only ignore the failure when we know the
failure is caused by the security.nocertdb setting.

Comment 16

[Robert Relyea]

Comment on attachment 8679748 [details] [diff] [review]
0001-Bug-1216882-Fix-incorrect-handling-of-key-DB-when-se.patch

Review of attachment 8679748 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/nss/lib/softoken/pkcs11.c
@@ +3540,4 @@
>  
>      handle = sftk_getKeyDB(slot);
>      if (handle == NULL) {
> +	return CKR_OK;

So the reason we return the error on NSC_InitPIN() if there is not database is because you can't actually set the pin if there is not database.

Why returning this error here is a problem is another matter. If the upper level code cares that the pin was set, then it should get an error here (we shouldn't silently succeed). If it doesn't care then it shouldn't blow up if we give it an error.

I could be talked into a different error code (like function not supported or something), but return CRK_OK in this case is definately wrong.

Comment 17

[Robert Relyea]

We should look up the stack and see what is calling PK11_InitPin() and decide if we care if the pin was initialized or not. (It's usually used at database creation time, but we don't have a database, it may be the case it's being improperly called).

There are a set of flags that tell you if the database is initialized or not. maybe those aren't being checked, or aren't being set correctly.

bob

Comment 18

[Mark Smith [:mcs]]

For what it's worth, the goal of the patch in attachment 8677747 [details] [diff] [review] is to disable login manager storage when security.nocertdb = true (if there is no secure storage available, the login manager cannot store anything). This is more consistent with Wan-Teh and Bob's desire to handle the error checking at a higher level, although technically the code in attachment 8677747 [details] [diff] [review] entirely skips initialization of the underlying systems when security.nocertdb = true.

Comment 19

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 20

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 21

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, untouched DOM/Storage bug's priority.

If you have reason to believe, this is wrong, please write a comment and ni :jstutte.