Closed Bug 386681 Opened 14 years ago Closed 14 years ago

nsIWebProgressListener security: Test for bits, not for absolute values

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

Attachments

(4 files, 4 obsolete files)

Note the comment for parameter "state" in the comment below. It clearly allows for future bits, and this is what I plan to do in bug 386654.

All existing code that checks for absolute values is incorrect.

While I could have filed 4 separate bugs for the products, I'm using a single bug and will attach patches for each location I found.


interface nsIWebProgressListener : nsISupports
{
  /**
   * Notification called for security progress.  This method will be called on
   * security transitions (eg HTTP -> HTTPS, HTTPS -> HTTP, FOO -> HTTPS) and
   * after document load completion.  It might also be called if an error
   * occurs during network loading.
   *
   * @param aWebProgress
   *        The nsIWebProgress instance that fired the notification.
   * @param aRequest
   *        The nsIRequest that has new security state.
   * @param aState
   *        A value composed of the Security State Flags and the Security
   *        Strength Flags listed above.  Any undefined bits are reserved for
   *        future use.
   *
   * NOTE: These notifications will only occur if a security package is
   * installed.
   */
  void onSecurityChange(in nsIWebProgress aWebProgress,
                        in nsIRequest aRequest,
                        in unsigned long aState);
}
Blocks: 386654
Attached patch Browser patch (obsolete) — Splinter Review
Attachment #270673 - Flags: review?(mconnor)
Attached patch Suite patch (obsolete) — Splinter Review
Attachment #270674 - Flags: review?(neil)
Attached patch Minimo patch (obsolete) — Splinter Review
Attachment #270675 - Flags: review?(dougt)
Attached patch gtkmozembed Patch (obsolete) — Splinter Review
Attachment #270676 - Flags: review?(benjamin)
When you review the patch for your module, could you please indicate whether additional superreview will be required? Thanks a lot
Kai, An alternative approach to your first patch is to change just one line.
Change:
    switch (aState) {
to
    switch (aState & (wpl.STATE_IS_SECURE | wpl.STATE_SECURE_HIGH | 
                      wpl.STATE_SECURE_LOW | wpl.STATE_IS_BROKEN)) {

This allows you to go ahead and use the additional bits without 
breaking the switch.
what nelson said w/ a big comment about what you are doing.
Comment on attachment 270674 [details] [diff] [review]
Suite patch

>-    switch (aState) {
>-      case wpl.STATE_IS_SECURE | wpl.STATE_SECURE_HIGH:
>+    if (aState & (wpl.STATE_IS_SECURE | wpl.STATE_SECURE_HIGH)) {
This isn't the same test. It only requires one of the bits to be set.
Note that if STATE_SECURE_HIGH guarantees STATE_IS_SECURE then you only need to test the former; the only reason the old code combines the flags is because it's doing the incorrect equality test.
Attachment #270674 - Flags: review?(neil) → review-
(In reply to comment #8)
> This isn't the same test. It only requires one of the bits to be set.

Oops, right, thanks for catching this, new patches coming up.
Attached patch Browser patch v2Splinter Review
Attachment #270673 - Attachment is obsolete: true
Attachment #271323 - Flags: review?(mconnor)
Attachment #270673 - Flags: review?(mconnor)
Attached patch Suite patch v2Splinter Review
Attachment #270674 - Attachment is obsolete: true
Attachment #271324 - Flags: review?(neil)
Attached patch Minimo patch v2Splinter Review
Attachment #270675 - Attachment is obsolete: true
Attachment #271325 - Flags: review?(dougt)
Attachment #270675 - Flags: review?(dougt)
Attachment #271324 - Attachment description: Suite patch → Suite patch v2
Attachment #270676 - Attachment is obsolete: true
Attachment #271326 - Flags: review?(benjamin)
Attachment #270676 - Flags: review?(benjamin)
This new round of patches uses the approach proposed by Nelson in order to fix the mistake found by Neil. I hope I got the bit testing right this time.

I'm now filtering using a bitmask that has the full set of bits defined in nsIWebProgressListener.

I also noticed that some code did not checked for medium security. As of today the backend never sets medium security, only high or low. But as our frozen nsIWebProgressListener defines it, I propose the frontend should handle it. I propose to map it to low security UI, and I'm including that change in the patch.

Status: NEW → ASSIGNED
Comment on attachment 271324 [details] [diff] [review]
Suite patch v2

>+    var wpl_security_bits = (wpl.STATE_IS_SECURE |
Nits: a) const wpl_security_bits perhaps? b) don't need ()s
Attachment #271324 - Flags: review?(neil) → review+
i like the wpl_security_bits alot better than the test before.  can we simply add this const to the idl?
(In reply to comment #16)
> i like the wpl_security_bits alot better than the test before.  can we simply
> add this const to the idl?

Only if it's allowed to make such a change to a frozen interface.

Yeah, not sure if we can officially do that.  It is a typelib change only, you could mark it in the comment <since Mozilla 1.9a5>.
I propose that we keep it simple and do not change the frozen file.

I'm OK to pick up Neil's proposal and add const and remove the brackets at the time I check this in.
Attachment #271326 - Flags: review?(benjamin) → review+
Attachment #271323 - Flags: review?(mconnor) → review+
Comment on attachment 271325 [details] [diff] [review]
Minimo patch v2

Received approval for this change by email. Doug said:

"Go ahead with the original change you made that didn't require any changes to the IDL."
Attachment #271325 - Flags: review?(dougt) → review+
Thanks for the reviews.
I checked in slightly enhanced patches that uses Neil's enhancement proposal to use "const" and remove the unnecessary () brackets.
And a patch to embedding that actually compiles...
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.