Last Comment Bug 386681 - nsIWebProgressListener security: Test for bits, not for absolute values
: nsIWebProgressListener security: Test for bits, not for absolute values
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Kai Engert (:kaie)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on: 408238
Blocks: 386654
  Show dependency treegraph
 
Reported: 2007-07-02 19:26 PDT by Kai Engert (:kaie)
Modified: 2007-12-13 12:33 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Browser patch (2.33 KB, patch)
2007-07-02 19:46 PDT, Kai Engert (:kaie)
no flags Details | Diff | Splinter Review
Suite patch (1.94 KB, patch)
2007-07-02 19:47 PDT, Kai Engert (:kaie)
neil: review-
Details | Diff | Splinter Review
Minimo patch (3.05 KB, patch)
2007-07-02 19:48 PDT, Kai Engert (:kaie)
no flags Details | Diff | Splinter Review
gtkmozembed Patch (2.61 KB, patch)
2007-07-02 19:49 PDT, Kai Engert (:kaie)
no flags Details | Diff | Splinter Review
Browser patch v2 (1.95 KB, patch)
2007-07-06 19:37 PDT, Kai Engert (:kaie)
mconnor: review+
Details | Diff | Splinter Review
Suite patch v2 (1.86 KB, patch)
2007-07-06 19:38 PDT, Kai Engert (:kaie)
neil: review+
Details | Diff | Splinter Review
Minimo patch v2 (2.33 KB, patch)
2007-07-06 19:39 PDT, Kai Engert (:kaie)
kaie: review+
Details | Diff | Splinter Review
gtkmozembed Patch v2 (1.76 KB, patch)
2007-07-06 19:41 PDT, Kai Engert (:kaie)
benjamin: review+
Details | Diff | Splinter Review

Description Kai Engert (:kaie) 2007-07-02 19:26:22 PDT
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) 2007-07-02 19:46:21 PDT
Created attachment 270673 [details] [diff] [review]
Browser patch
Comment 2 Kai Engert (:kaie) 2007-07-02 19:47:04 PDT
Created attachment 270674 [details] [diff] [review]
Suite patch
Comment 3 Kai Engert (:kaie) 2007-07-02 19:48:06 PDT
Created attachment 270675 [details] [diff] [review]
Minimo patch
Comment 4 Kai Engert (:kaie) 2007-07-02 19:49:29 PDT
Created attachment 270676 [details] [diff] [review]
gtkmozembed Patch
Comment 5 Kai Engert (:kaie) 2007-07-02 19:50:34 PDT
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) 2007-07-02 19:51:58 PDT
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) 2007-07-02 20:36:41 PDT
what nelson said w/ a big comment about what you are doing.
Comment 8 neil@parkwaycc.co.uk 2007-07-04 03:54:28 PDT
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) 2007-07-06 18:53:50 PDT
(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) 2007-07-06 19:37:19 PDT
Created attachment 271323 [details] [diff] [review]
Browser patch v2
Comment 11 Kai Engert (:kaie) 2007-07-06 19:38:35 PDT
Created attachment 271324 [details] [diff] [review]
Suite patch v2
Comment 12 Kai Engert (:kaie) 2007-07-06 19:39:34 PDT
Created attachment 271325 [details] [diff] [review]
Minimo patch v2
Comment 13 Kai Engert (:kaie) 2007-07-06 19:41:03 PDT
Created attachment 271326 [details] [diff] [review]
gtkmozembed Patch v2
Comment 14 Kai Engert (:kaie) 2007-07-06 19:48:17 PDT
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 neil@parkwaycc.co.uk 2007-07-07 07:39:25 PDT
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) 2007-07-08 17:30:24 PDT
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) 2007-07-08 17:37:18 PDT
(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) 2007-07-08 17:46:58 PDT
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) 2007-07-11 18:07:22 PDT
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) 2007-07-20 07:41:51 PDT
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) 2007-07-23 04:05:58 PDT
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...

Note You need to log in before you can comment on or make changes to this bug.