131381 - Array bounds read in cookie_P3PUserPref(int,int)

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail] (gone - use [e-mail] instead)]

Windows 2000, latest trunk.

Just starting the browser with a default URL of http://home.netscape.com (with 
no changes to any policies, etc) yields:

[E] ABR: Array bounds read in cookie_P3PUserPref(int,int) {1 occurrence}
    Reading 1 byte from 0x04c8c22f (1 byte at 0x04c8c22f illegal)
    Address 0x04c8c22f is 1 byte before the beginning of a 9 byte block at 
0x04c8c230
    Address 0x04c8c22f points to a malloc'd block in heap 0x02710000
    Thread ID: 0x5a0
    Error location
    cookie_P3PUserPref(int,int) [nsCookies.cpp:896]
        policy = P3P_ExplicitConsent;
      }
      if (cookie_P3P && PL_strlen(cookie_P3P) == 8) {
 =>     return (foreign ? cookie_P3P[policy+1] : cookie_P3P[policy]);
      } else {
        return P3P_Accept;
      }
    cookie_P3PDecision(char *,char *,nsIIOService *,nsIHttpChannel *) 
[nsCookies.cpp:910]
      return cookie_GetStatus(
               cookie_P3PUserPref(
                 P3P_SitePolicy(curURL, aHttpChannel),
 =>              cookie_isForeign(curURL, firstURL, ioService)));
    }
    
    /* returns PR_TRUE if authorization is required
    COOKIE_SetCookieStringFromHttp(char *,char *,nsIPrompt *,char const*,char 
*,nsIIOService *,nsIHttpChannel *) [nsCookies.cpp:1428]
      /* check to see if P3P pref is satisfied */
      nsCookieStatus status = nsICookie::STATUS_UNKNOWN;
      if (cookie_GetBehaviorPref() == PERMISSION_P3P) {
 =>     status = cookie_P3PDecision(curURL, firstURL, ioService, aHttpChannel);
        if (status == nsICookie::STATUS_REJECTED) {
          nsCOMPtr<nsIObserverService> os(do_GetService("@mozilla.org/observer-
service;1"));
          if (os) {
    nsCookieService::SetCookieStringFromHttp(nsIURI *,nsIURI *,nsIPrompt *,char 
const*,char const*,nsIHttpChannel *) [nsCookieService.cpp:214]
      COOKIE_SetCookieStringFromHttp(
        NS_CONST_CAST(char *, spec.get()),
        NS_CONST_CAST(char *, firstSpec.get()),
 =>     aPrompter, aCookie, (char *)aExpires, mIOService, aHttpChannel);
      return NS_OK;
    }
    
    nsCookieHTTPNotify::OnExamineResponse(nsIHttpChannel *) 
[nsCookieHTTPNotify.cpp:248]
        if (NS_FAILED(rv)) return rv;
    
        // Save the cookie
 =>     rv = mCookieService->SetCookieStringFromHttp(pURL, pFirstURL, 
pPrompter, cookieHeader, dateHeader, aHttpChannel);
    
        return rv;
    }
    XPTC_InvokeByIndex [xptcinvoke.cpp:105]
    nsProxyObject::Post(UINT,nsXPTMethodInfo *,nsXPTCMiniVariant 
*,nsIInterfaceInfo *) [nsProxyEvent.cpp:480]
    nsProxyEventObject::CallMethod(WORD,nsXPTMethodInfo 
const*,nsXPTCMiniVariant *) [nsProxyEventObject.cpp:547]
    PrepareAndDispatch [xptcstubs.cpp:115]
    SharedStub     [xptcstubs.cpp:138]
    Allocation location
    malloc         [MSVCRT.DLL]
    PL_strdup      [strdup.c:46]
    PREF_CopyCharPref [prefapi.cpp:787]
                stringVal = pref->userPref.stringVal;
    
            if (stringVal) {
 =>             *return_buffer = PL_strdup(stringVal);
                result = PREF_OK;
            }
        }
    nsPrefBranch::GetCharPref(char const*,char * *) [nsPrefBranch.cpp:214]
    
      rv = getValidatedPrefName(aPrefName, &pref);
      if (NS_SUCCEEDED(rv)) {
 =>     rv = _convertRes(PREF_CopyCharPref(pref, _retval, mIsDefault));
      }
      return rv;
    }
    nsPrefService::GetCharPref(char const*,char * *) [nsPrefService.h:57]
    public:
      NS_DECL_ISUPPORTS
      NS_DECL_NSIPREFSERVICE
 =>   NS_FORWARD_NSIPREFBRANCH(mRootBranch->)
      NS_DECL_NSIPREFBRANCHINTERNAL
      NS_DECL_NSIOBSERVER
    
    nsPref::GetCharPref(char const*,char * *) [nsPref.cpp:214]
    
      nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(mPrefService, &rv);
      if (NS_SUCCEEDED(rv))
 =>     rv = prefBranch->GetCharPref(aPrefName, _retval);
      return rv;
    }
    
    COOKIE_RegisterPrefCallbacks(void) [nsCookies.cpp:557]
          prefs->RegisterCallback(cookie_lifetimeValue, 
cookie_LifetimeLimitPrefChanged, nsnull);
        
          // Initialize for P3P prefs
     =>   if (NS_FAILED(prefs->CopyCharPref(cookie_p3pPref, &cookie_P3P))) {
            cookie_P3P = PL_strdup(cookie_P3P_Default);
          }
          prefs->RegisterCallback(cookie_p3pPref, cookie_P3PPrefChanged, 
nsnull);
    nsCookieService::Init(void) [nsCookieService.cpp:82]
        
        nsresult nsCookieService::Init()
        {
     =>   COOKIE_RegisterPrefCallbacks();
          nsresult rv;
          mIOService = do_GetService(NS_IOSERVICE_CONTRACTID, &rv);
          COOKIE_Read();
    nsCookieServiceConstructor [nsModuleFactory.cpp:59]
        NS_GENERIC_FACTORY_CONSTRUCTOR(nsCookie)
        NS_GENERIC_FACTORY_CONSTRUCTOR(nsPermission)
        NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsCookieManager, Init)
     => NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsCookieService, Init)
        NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsImgManager, Init)
        NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsPermissionManager, Init)
        NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsCookieHTTPNotify, Init)
    nsGenericFactory::CreateInstance(nsISupports *,nsID const&,void * *) 
[nsGenericFactory.cpp:75]
    nsComponentManagerImpl::CreateInstance(nsID const&,nsISupports *,nsID 
const&,void * *) [nsComponentManager.cpp:1800]
    nsComponentManagerImpl::GetService(nsID const&,nsID const&,void * *) 
[nsComponentManager.cpp:1973]
    nsServiceManager::GetService(nsID const&,nsID const&,nsISupports * 
*,nsIShutdownListener *) [nsServiceManagerObsolete.cpp:77]
    nsJSCID::GetService(nsISupports * *) [xpcjsid.cpp:879]
    XPTC_InvokeByIndex [xptcinvoke.cpp:105]
    XPCWrappedNative::CallMethod(XPCCallContext&,CallMode::XPCWrappedNative) 
[xpcwrappednative.cpp:2025]
    XPC_WN_CallMethod(JSContext *,JSObject *,UINT,long *,long *) 
[xpcwrappednativejsops.cpp:1266]
    js_Invoke      [jsinterp.c:788]
    js_Interpret   [jsinterp.c:2745]
    js_Invoke      [jsinterp.c:805]

Comment 1

[Stephen P. Morse]

stephend, I just backed out part of the checkin for bug 129101 -- namely the 
part that sets cookie acceptance to be based on p3p by default.  That should 
clear this up.  Let me know if it does, in which case this one is no longer 
critical.

However there's still a bug here (I'm pulling a tree now so I can try to 
reproduce it) so we should leave this open until even if you can no longer 
reproduce it.

Comment 2

[[e-mail] (gone - use [e-mail] instead)]

Steve, yes it does.  Sorry about jumping the gun there.

Comment 3

[Stephen P. Morse]

Attaching patch.  But first let me explain what the patch does.

The default pref should be to use p3p (see bug 129101).  I temporarily backed 
out that default setting so as to sidestep this bug for the moment.

The reason the array-bound problem was occuring with such a default pref is due 
to the fact that the p3p module is not present in the default build.  So the 
second part of the patch checks for that and does not access the array in that 
case.

But there is a further problem here if the p3p module is not installed.  Namely 
the default pref of p3p cannot be displayed in the cookie pref panel because 
that choice is left out when the p3p module is missing.  So none of the radio 
buttons are checked.  Therefore I added the first part of the patch which 
detects the inconsistency of the pref specifying p3p but p3p module missing, and 
changes the pref to accept-all in that case (the old default value).

Actually both of these changes are not needed -- the changing of the pref to 
accept-all would have taken care of the array-bound problem as well.  But it's 
safer coding to keep in the check for the array bound.

Comment 4

[Stephen P. Morse]

Attachment: fix array-bound problem, fix initial-pref setting

Comment 5

[Darin Fisher]

Comment on attachment 74527 [details] [diff] [review]
fix array-bound problem, fix initial-pref setting

sr=darin, looks good to me.

Comment 6

[Bill Law]

Comment on attachment 74527 [details] [diff] [review]
fix array-bound problem, fix initial-pref setting

r=law

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 74527 [details] [diff] [review]
fix array-bound problem, fix initial-pref setting

Eek.  Actually changing the pref seems to me like the wrong thing to do.  What
if someone has two installs of Mozilla, one with P3P and one without?  Couldn't
greying out in the UI be handled in the UI code?  This will mean that if p3p
becomes enabled by default in the future, and someone enables this pref, and
then they test something using an "old" (meaning between now and when p3p is
enabled, or one in which they didn't install the p3p extension) build, they'll
have the pref turned off for them.  That really doesn't seem like the way prefs
should work.  Or am I missing something?

Comment 8

[Stephen P. Morse]

I think that it would be a very unusual person that would have two installs, one 
with and one without p3p.  Certainly not your typical user.

The feedback that I've gotten is that we should not do any greying out -- if the 
p3p extensions directory is not present, then there should be no mention of p3p 
in the ui.

Given that constraint, I decided to turn off the pref if the user doesn't have 
the p3p module in his build.  The other solution is to do some pref juggling.  
That is, effectively change the pref to accept-all for this session but not for 
future sessions, but if he explicitly changes the pref during this session then 
we permanently remember that.  That way we remember the fact that the user 
wanted to use p3p and if ever he goes back to a p3p-enabled build in the future 
he will get the p3p pref again.  But I didn't think that that was the way to go.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It may not affect the typical user, but it very likely will affect a number of
the best QA people working on the Mozilla project and prevent the p3p code from
being tested well.

Also, you don't have this check in |cookie_BehaviorPrefChanged|, so you're
missing some cases.  Maybe it would be better to have the check in
|cookie_SetBehaviorPref|?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I guess it seems to me like the basic problem is that the p3p extension should
essentially have a separate pref rather than a value for the same one.  It could
even be implemented using the current UI, so that, in a build with p3p, you have
4 options, that set the pair of prefs to (0,0), (1,0), (<previous value>,1),
(2,0), and in a build without p3p, the radio buttons set the prefs to (0,
<previous value>), (1, <previous value>), <missing radio button>, and <2,
<previous value>).  However, if you want to leave things the way they are I
guess I won't object, although I still think the pref-changed callback issue
should be fixed (consider turbo with multiple profiles, where the first user had
the cookie pref changed to a non-default value and the second user had the
p3p-enabled default, but the build didn't have p3p).

Comment 11

[Stephen P. Morse]

It's impossible to change the pref to p3p if the p3p module is not installed 
(that choice does not appear in the cookie pref panel).  So there is no need for 
this test in cookie_BehaviorPrefChanged.

Yes, it could equally well have been put in cookie_SetBehaviorPref.  But, based 
on what I said in the preceding paragraph, doing so would not catch any 
additional cases that the current placement doesn't catch.

Comment 12

[Stephen P. Morse]

My comment 11 above was in response to dbaron's comment 9.  I hadn't yet seen 
his comment 10.

Yes, with turbo mode, there may be a problem.  Since it certainly doesn't hur to 
move the test to cookie_SetBehaviorPref, and it might be better for turbo mode, 
I'll make that change.

Attaching new patch shortly.

The two-prefs idea is also intriguing.  However it doesn't fit in nicely with 
the var _elementIDs usage in our pref panels, whereby a change in a radio button 
automatically updates a single pref without the need for any explicit coding to 
do so.

Comment 13

[Stephen P. Morse]

Attachment: Same patch but with added code moved to a different routine

Comment 14

[Stephen P. Morse]

No substantial code-change occured between attachment 72547 [details] [diff] [review] and attachment 
74748 [details] [diff] [review] (code was moved but not changed), so the reviews on the first should still 
be valid.

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 74748 [details] [diff] [review]
Same patch but with added code moved to a different routine

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 16

[Stephen P. Morse]

Fix checked in.

Comment 17

[Tom Everingham]

stephend, can you verify this one please.  Thanks.

Comment 18

[[e-mail] (gone - use [e-mail] instead)]

This has been fixed for quite some time.  Verified.