Closed Bug 571034 Opened 14 years ago Closed 12 years ago

Fix incorrect use of -= operator, change to bitwise operator

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 785259

People

(Reporter: mayhemer, Unassigned)

Details

(Whiteboard: [psm-cert-exceptions])

If we have added a certificate error override for errors e.g. "Invalid host", "Certificate expired" but later the certificate state changes to e.g. only "Invalid host" error being detected on it, we still close the connection and show the certificate error page.

This is because PSM IO layer code wants the set of error found on the certificate be exact as the set of error we stored an override for it.  This is because the bit operation is using "-=" operator and not a bit a mask.  See bug 466011 comment 6 for code reference.

Purpose of this bug is to decide if we want this behavior or we should use bit-masking (allow the remembered override set be larger or equal, to let the connection go through) and potentially fix it.
In my opinion, we should keep the current behaviour. If different validation errors show up when we visit a site, then something has changed, and it should get the user's attention.

Regarding the use of the -= operator in the cited bug and comment:

I'm simply worried that using the -= operator was a mistake, and can produce undesired behaviour.

The code we worry about is this:

  if (overrideService)
  {
    PRBool haveOverride;
    PRBool isTemporaryOverride; // we don't care
  
    nsrv = overrideService->HasMatchingOverride(hostString, port,
                                                ix509, 
                                                &overrideBits,
                                                &isTemporaryOverride, 
                                                &haveOverride);
    if (NS_SUCCEEDED(nsrv) && haveOverride) 
    {
      // remove the errors that are already overriden
      remaining_display_errors -= overrideBits;
    }
  }



The "intention" of the code was: when calculating the set of errors that should be reported, don't display the errors that have been previously accepted using an override.

Looking at the -= code, an incorrect assumption has been made. The code assumes, the "new set of errors will always include the errors seen previously".

This is wrong. For example, after creating the exception, the issuer CA cert may have been imported and marked as trusted, but now the cert has expired.

So, when calculating the set of "remaining_display_errors", we must use bitwise removal of the bits that were seen previously (and remembered as the exception).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Override certificate error set must be exact as error set found with the certificate → Fix incorrect use of -= operator, change to bitwise operator
Whiteboard: [psm-cert-exceptions]
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.