386681 - nsIWebProgressListener security: Test for bits, not for absolute values

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert (:KaiE:)]

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);
}

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Browser patch

Comment 2

[Kai Engert (:KaiE:)]

Attachment: Suite patch

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Minimo patch

Comment 4

[Kai Engert (:KaiE:)]

Attachment: gtkmozembed Patch

Comment 5

[Kai Engert (:KaiE:)]

When you review the patch for your module, could you please indicate whether additional superreview will be required? Thanks a lot

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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

[Doug Turner (:dougt)]

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

Comment 8

[[e-mail]]

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.

Comment 9

[Kai Engert (:KaiE:)]

(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.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Browser patch v2

Comment 11

[Kai Engert (:KaiE:)]

Attachment: Suite patch v2

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Minimo patch v2

Comment 13

[Kai Engert (:KaiE:)]

Attachment: gtkmozembed Patch v2

Comment 14

[Kai Engert (:KaiE:)]

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.

Comment 15

[[e-mail]]

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

Comment 16

[Doug Turner (:dougt)]

i like the wpl_security_bits alot better than the test before.  can we simply add this const to the idl?

Comment 17

[Kai Engert (:KaiE:)]

(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

[Doug Turner (:dougt)]

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>.

Comment 19

[Kai Engert (:KaiE:)]

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.

Comment 20

[Kai Engert (:KaiE:)]

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."

Comment 21

[Kai Engert (:KaiE:)]

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...