814247 - HTTP auth cache is shared between apps

Status: RESOLVED FIXED

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

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Jonas and I just realized that it is important that we do not share the HTTP auth cache between apps. We need to have per-app HTTP auth jars just like we have per-app cookie jars, etc. Otherwise, one app can use another apps' user credentials to access web resources.

Comment 1

[Andrew Overholt [:overholt]]

Brian, who's going to do this?

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Andrew Overholt [:overholt] from comment #1)
> Brian, who's going to do this?

Josh would know who is free to work on it, better than me. It would probably be good to get somebody who's done the related work to take it.

Comment 3

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 4

[Andrew Overholt [:overholt]]

Rather than just setting needinfo on Josh, I'm going to assign it to him to find someone to work on this.

Comment 5

[Josh Matthews [:jdm]]

Josh -> Josh handoff.

Comment 6

[Josh Matthews [:jdm]]

Attachment: Add auth cache jars for separate apps.

Untested wip, just backing up here.

Comment 7

[Josh Matthews [:jdm]]

Attachment: Add auth cache jars for separate apps.

Comment 8

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=0c82beb36509

Comment 9

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 688634 [details] [diff] [review]
Add auth cache jars for separate apps.

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

appid, inbrowser and private flags are more and more spread around our public and internal APIs.  I more and more think about some covering class or interface that would carry these 3 properties.  nsILoadContext and nsIPrincipal could return such object to be passed to our APIs, as well as it could be easily to build "by hand".  |private| flag may be just false when created from nsIPrincipal.

Definitely not for this bug.


The patch is simple and looks good.

Reverting to f+ only for now.

::: dom/plugins/base/nsNPAPIPlugin.cpp
@@ +2764,5 @@
>    GetPrivacyFromNPP(instance, &authPrivate);
>  
> +  nsIDocument *doc = GetDocumentFromNPP(instance);
> +  NS_ENSURE_TRUE(doc, NPERR_GENERIC_ERROR);
> +  nsIPrincipal *principal = doc->NodePrincipal();

I cannot review this code.

::: netwerk/protocol/http/nsHttpAuthCache.cpp
@@ +15,2 @@
>  {
> +    key.AssignLiteral("");

Truncate()

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +25,5 @@
> +static void
> +GetAppIdAndBrowserStatus(nsIChannel* aChan, uint32_t* aAppId, bool* aInBrowserElem)
> +{
> +    nsCOMPtr<nsILoadContext> loadContext;
> +    NS_QueryNotificationCallbacks(aChan, loadContext);

Add null check for aChan before this call please.

::: netwerk/protocol/http/nsIHttpAuthManager.idl
@@ +53,2 @@
>       */
> +    void getAuthIdentityWithDefaultPrincipal(

Why exactly this way and not to leave getAuthIdentity/setAuthIdentity as are and have new set/getAuthIdentityForPrincipal ?

Just for curiosity since that would be my way to do it.  Better compatibility and less changes to tests (I didn't check you correctly changed all places, I leave this up to you to double check).

Also why not to just make principal another optional arg?

If you have strong arguments for the way you did it, then I can quickly give r+ to an updated patch.

Comment 11

[Jason Duell]

> appid, inbrowser and private flags are more and more spread around our public and 
> internal APIs.  I more and more think about some covering class or interface that 
> would carry these 3 properties. 

+1 

> Definitely not for this bug.

+1

Comment 12

[Josh Matthews [:jdm]]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> ::: netwerk/protocol/http/nsIHttpAuthManager.idl
> @@ +53,2 @@
> >       */
> > +    void getAuthIdentityWithDefaultPrincipal(
> 
> Why exactly this way and not to leave getAuthIdentity/setAuthIdentity as are
> and have new set/getAuthIdentityForPrincipal ?
> 
> Just for curiosity since that would be my way to do it.  Better
> compatibility and less changes to tests (I didn't check you correctly
> changed all places, I leave this up to you to double check).
> 
> Also why not to just make principal another optional arg?
> 
> If you have strong arguments for the way you did it, then I can quickly give
> r+ to an updated patch.

I made these API choices because it feels like an important thing to be intentional about (being explicit about using the default principal vs. providing one). I'll abide by whatever you decide is best.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You should also hook this code up to the webapps-clear-data notification, so that the auth data for an app is cleared when the app is uninstalled. That will automatically also hook it up to the "clear private data" feature.

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Josh Matthews [:jdm] from comment #12)
> I made these API choices because it feels like an important thing to be
> intentional about (being explicit about using the default principal vs.
> providing one). I'll abide by whatever you decide is best.

So, when there is a JS extension or any code using the old signatures of getAuthIdentity then it may put password to the argument of where username has to be.  There will be a JS warning but no error I believe.  That could be a potential security risk.

From point of simplicity and compatibility I'd rather put the principal as a last optional argument.  A new code will use this, so new consumers will add principal when using this new signature.

This will make the patch smaller and less error prone and save us also from extension compatibility issues.

Comment 15

[Josh Matthews [:jdm]]

I agree with your reasoning. I'll make the changes you recommend.

Comment 16

[Josh Matthews [:jdm]]

Attachment: Add auth cache jars for separate apps.

Comment 17

[Josh Matthews [:jdm]]

Comment on attachment 692377 [details] [diff] [review]
Add auth cache jars for separate apps.

Benjamin, could you look at the plugin code changes?

Comment 18

[Josh Matthews [:jdm]]

Comment on attachment 692377 [details] [diff] [review]
Add auth cache jars for separate apps.

Justin, do you want to look over the addition to the browser element auth test?

Comment 19

[Justin Lebar (not reading bugmail)]

Comment on attachment 692377 [details] [diff] [review]
Add auth cache jars for separate apps.

lgtm!

Comment 20

[Josh Matthews [:jdm]]

Attachment: Clear app auth data on uninstall.

It occurred to me that I should really write the clearing code as well.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that the webapps-clear-data notification has somewhat unintuitive behavior for the aBrowserOnly flag:

If aBrowserOnly is *false*, we should clear all data for the provided appid
If aBrowserOnly is *true*, we should clear only the data for the appid where the browser flag is set to true.


We use aBrowserOnly=false to uninstall an app, which means that we want to get rid of all data associated with that app.

We use aBrowserOnly=true to clear private data for browser content associated with an app.


This can be implemented by calling ClearAppData twice when aBrowserOnly is false as performance isn't a huge deal when uninstalling an app.

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 692377 [details] [diff] [review]
Add auth cache jars for separate apps.

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

r=honzab

Thanks.

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 693152 [details] [diff] [review]
Clear app auth data on uninstall.

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

r=honzab but with comments addressed.

::: netwerk/protocol/http/nsHttpAuthCache.cpp
@@ +9,5 @@
>  #include "nsString.h"
>  #include "nsCRT.h"
>  #include "prprf.h"
> +#include "mozIApplicationClearPrivateDataParams.h"
> +#include "nsIScriptSecurityManager.h"

Don't include this header.  Use NECKO_UNKNOWN_APP_ID.

@@ +49,5 @@
>  //-----------------------------------------------------------------------------
>  
>  nsHttpAuthCache::nsHttpAuthCache()
>      : mDB(nullptr)
> +    , mObserver(new AppDataClearObserver(this))

You may need to disable the specific warning about |this| being used in an initiator.

@@ +63,5 @@
>      if (mDB)
>          ClearAll();
> +    nsCOMPtr<nsIObserverService> obsSvc = mozilla::services::GetObserverService();
> +    if (obsSvc) {
> +        obsSvc->RemoveObserver(mObserver, "webapps-clear-data");

Please also set mObserver->mOwner to null and add a null check on its access.  I know that it is very paranoid, but it is not good to let mObserver potentially living longer then this object keep ref to a dead memory.  This could happen only when some of "webapps-clear-data" observers would spin the main thread loop.  It is still unfortunately possible in our platform, e.g. by an extension.

I think I have seen this observer/owner construction in some of you other patches too.  Maybe worth fixing it as well.

@@ +290,5 @@
> +static int
> +RemoveEntriesForApp(PLHashEntry *entry, int32_t number, void *arg)
> +{
> +    nsDependentCString key(static_cast<const char*>(entry->key));
> +    nsACString* prefix = static_cast<nsACString*>(arg);

Why not nsAutoCString* ?

@@ +291,5 @@
> +RemoveEntriesForApp(PLHashEntry *entry, int32_t number, void *arg)
> +{
> +    nsDependentCString key(static_cast<const char*>(entry->key));
> +    nsACString* prefix = static_cast<nsACString*>(arg);
> +    if (prefix->Equals(Substring(key, 0, prefix->Length()))) {

StringBeginsWith(key, prefix)

Comment 24

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4faa85ebbbf7

Comment 25

[Ed Morley [:emorley]]

Backed out for build bustage:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4faa85ebbbf7

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c794d4722f7

Comment 26

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bcd9a5a33a2

Comment 27

[Ed Morley [:emorley]]

Backed out for crashes:
eg M2: https://tbpl.mozilla.org/php/getParsedLog.php?id=18107300&tree=Mozilla-Inbound
m-oth: https://tbpl.mozilla.org/php/getParsedLog.php?id=18107333&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1a10ed19a32

A full try run before this relands for it's third time please :-)

Comment 28

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=720e3a73cc16

Sincere apologies. There have been a string of these recently, and I'll try to improve my practices.

Comment 29

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=24a2dbe468ed

Fixed the test to align with sicking's comment.

Comment 30

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b432b50d61

Comment 31

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b6b432b50d61

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7c7b8b5f804a

This didn't apply cleanly enough to b2g18 for me to uplift. Please provide a branch-specific patch or nominate dependent bugs for uplift.

Comment 33

[Josh Matthews [:jdm]]

Attachment: Patch for mozilla-b2g18

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Thank you good sir!

https://hg.mozilla.org/releases/mozilla-b2g18/rev/a698f13091ae

Comment 35

[Paul Silaghi, QA [:pauly]]

Is there something QA can do here ?

Comment 36

[Josh Matthews [:jdm]]

You can verify that a site requiring HTTP authentication brings up the prompt with no cached auth data in both the browser and a bookmarked copy. Log in to one, then check that the other still triggers a prompt.

Comment 37

[Paul Silaghi, QA [:pauly]]

(In reply to Josh Matthews [:jdm] from comment #36)
> Log in to one, then check that the other still triggers a prompt.
No prompt for credentials for the second one.
http://browserspy.dk/password-ok.php
login: test/test

Comment 38

[Josh Matthews [:jdm]]

(In reply to Paul Silaghi [QA] from comment #37)
> (In reply to Josh Matthews [:jdm] from comment #36)
> > Log in to one, then check that the other still triggers a prompt.
> No prompt for credentials for the second one.
> http://browserspy.dk/password-ok.php
> login: test/test

I just tested desktop b2g. The default, in-process version has a bug that causes creating the auth dialog to fail, as far as I can tell. However, when I enabled multiple processes, I saw one prompt for the browser, and one for the homescreen bookmark. How did you perform your test, Paul?

Comment 39

[Paul Silaghi, QA [:pauly]]

I hope I understood correctly what you meant in comment 36.
1. Bookmark http://browserspy.dk/password-ok.php
2. Open http://browserspy.dk/password-ok.php in a new tab and login
3. Open the bookmark from step 1 in another tab
AR: No prompt for credentials on the step 3.
FF 20b1, Win 7 x64

Comment 40

[Josh Matthews [:jdm]]

Sorry, the in-browser bookmark is not enough. You need to add the page to the homescreen as a separate app.

Comment 41

[Paul Silaghi, QA [:pauly]]

This is b2g related only, as Josh explained me.