nsIWebProgressListener security: Test for bits, not for absolute values

RESOLVED FIXED

Status

()

Core
Security: PSM
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

10 years ago
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);
}
(Assignee)

Updated

10 years ago
Blocks: 386654
(Assignee)

Comment 1

10 years ago
Created attachment 270673 [details] [diff] [review]
Browser patch
Attachment #270673 - Flags: review?(mconnor)
(Assignee)

Comment 2

10 years ago
Created attachment 270674 [details] [diff] [review]
Suite patch
Attachment #270674 - Flags: review?(neil)
(Assignee)

Comment 3

10 years ago
Created attachment 270675 [details] [diff] [review]
Minimo patch
Attachment #270675 - Flags: review?(dougt)
(Assignee)

Comment 4

10 years ago
Created attachment 270676 [details] [diff] [review]
gtkmozembed Patch
Attachment #270676 - Flags: review?(benjamin)
(Assignee)

Comment 5

10 years ago
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.

Comment 7

10 years ago
what nelson said w/ a big comment about what you are doing.

Comment 8

10 years ago
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-
(Assignee)

Comment 9

10 years ago
(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.
(Assignee)

Comment 10

10 years ago
Created attachment 271323 [details] [diff] [review]
Browser patch v2
Attachment #270673 - Attachment is obsolete: true
Attachment #271323 - Flags: review?(mconnor)
Attachment #270673 - Flags: review?(mconnor)
(Assignee)

Comment 11

10 years ago
Created attachment 271324 [details] [diff] [review]
Suite patch v2
Attachment #270674 - Attachment is obsolete: true
Attachment #271324 - Flags: review?(neil)
(Assignee)

Comment 12

10 years ago
Created attachment 271325 [details] [diff] [review]
Minimo patch v2
Attachment #270675 - Attachment is obsolete: true
Attachment #271325 - Flags: review?(dougt)
Attachment #270675 - Flags: review?(dougt)
(Assignee)

Updated

10 years ago
Attachment #271324 - Attachment description: Suite patch → Suite patch v2
(Assignee)

Comment 13

10 years ago
Created attachment 271326 [details] [diff] [review]
gtkmozembed Patch v2
Attachment #270676 - Attachment is obsolete: true
Attachment #271326 - Flags: review?(benjamin)
Attachment #270676 - Flags: review?(benjamin)
(Assignee)

Comment 14

10 years ago
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 15

10 years ago
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+

Comment 16

10 years ago
i like the wpl_security_bits alot better than the test before.  can we simply add this const to the idl?
(Assignee)

Comment 17

10 years ago
(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.

Comment 18

10 years ago
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>.
(Assignee)

Comment 19

10 years ago
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.

Updated

10 years ago
Attachment #271326 - Flags: review?(benjamin) → review+

Updated

10 years ago
Attachment #271323 - Flags: review?(mconnor) → review+
(Assignee)

Comment 20

10 years ago
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+
(Assignee)

Comment 21

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Depends on: 408238
You need to log in before you can comment on or make changes to this bug.