245727 - PSM could use nsIStreamLoaderObserver

Status: RESOLVED WONTFIX

(Core :: Security: PSM, enhancement, P3)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

PSMContentDownloader (see URL) looks like it could use nsIStreamLoader; that
would avoid having essentially the same code in two places

hmm, would this be a good first bug?

Comment 1

[Boris Zbarsky [:bzbarsky]]

It's a hard first bug, but a reasonable one, in my opinion...

Comment 2

[[e-mail]]

Would attachment 150131 [details] [diff] [review] be of any use to potential hackers?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah that shows basically how to use streamloader...

Comment 4

[Son Le]

I've had a go at this (based on neil's patch) and have a working build. Question
is, how do I test it?

Comment 5

[Son Le]

Attachment: patch v0

here's the patch

Comment 6

[[e-mail]]

I guess you have to browser to a "secure" site using a self-signed certificate.

Comment 7

[Son Le]

Cool! Tested it on localhost with a self-signed certificate, got a certificate
popup warning and then I'm thru. 

Looking for r=?

Comment 8

[[e-mail]]

So were you able to accept the certificate permanently to survive a restart?

Also, I am somewhat guessing as to the purpose of this code; you should use
debugging techniques (breakpoints / printfs) to verify that your code is used.

Comment 9

[Son Le]

Ok, I've tested accepting certificates per session/permanently and used the
Tools | Options | Advance to import/delete/view certificates, but could not get
the code path to execute. I tried both setting breakpoints and PR_LOG-ging and
got no joy.. :(

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

try installing a root certificate, maybe - https://trust.web.de/root.sql for
example (the "Zertifikat installieren" links near the bottom)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, it looks like installation of root certs (by clicking on links to them)
and updates of CRL (whatever that is) are all this is used for.

Comment 12

[Son Le]

Yay and nay.

Installing a root certificate does indeed invoke the PSM code, however I've now
run into a new problem. The current code uses a nsIURIContentListener to peek at
the content before loading, which expects a nsIStreamListener to be returned.

Since this has been changed, I need a different way to get the nsIStreamListener
object. Can it be obtained from nsIStreamLoaderObserver somehow? Or converted to it?

Alternatively, I can convert PSMContentListener to use nsIContentHandler (like
the addressbook), but then I don't know how to register the handler. My guess is
that I need to change nsNSSModule.cpp, but I am not sure how to do this.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

urg. OK. can we make nsIStreamLoader be an nsIStreamListener and have users of
it open the channel itself as needed? (similar to nsIDownloader)
That would also simplify the streamloader code a bit (since it does not need to
handle AsyncOpen failure, which currently makes it post an event)

if we do that, nsIStreamLoader should inherit from nsIStreamListener.

Comment 14

[Darin Fisher]

> urg. OK. can we make nsIStreamLoader be an nsIStreamListener and have users of
> it open the channel itself as needed? (similar to nsIDownloader)

Yes, that is something I've considered doing.  (Feel free to file a bug to
propose that change?)  Perhaps another option for this bug might be to use
nsISimpleStreamListener?

Comment 15

[Son Le]

Attachment: patch v1

Decided to go with bi's suggestion to change nsIStreamLoader to inherit from
nsIStreamListener. After much reading and experimenting, I finally got a
working patch.

The PSM code now works perfectly. The only remaining issue is how to handle the
openning of channels now that it has been removed from nsIStreamLoader. I've
added AsyncOpen calls to the relevant places, but am not sure how to deal with
failed AsyncOpens.

Comment 16

[Darin Fisher]

I don't think you should be including patches to the networking library in this
bug.  File a new bug for those and make this bug be dependent on that one.  Then
come back and write a patch to use the new API.  Also, I think that
nsIStreamLoader and nsIUnicharStreamLoader should have parallel APIs, so
whatever change is made to one should be made to the other, which might bloat
your patch a bit.

Comment 17

[Son Le]

Ok. Bug 281153 created.

Comment 18

[Kai Engert [:KaiE:]]

Assigning bug to Son Le, since he is currently working on it.

Comment 19

[Son Le]

Attachment: patch v2

Patch to make PSMContentDownloader inherit from nsIStreamLoaderObserver.
Pending checkin to bug 281153.

Comment 20

[Son Le]

Attachment: patch v3.0 - basic changes

Basic changes to make PSM use IStreamLoaderObserver.

Comment 21

[Son Le]

Attachment: patch v3.1 - basic changes + OOM checks + other goodness

Uber patch to make PSM use IStreamLoaderObserver as well as OOM checks, and some code cleanup.

Tested CRL import using the "Browser Import" at http://www.grid-support.ac.uk/content/view/182/98 and updates using Options|Advanced|Revocation Lists|Update.

Made sure that certificates survived firefox browser sessions and PSM was properly destroyed. Didn't test the silent update functionality.

All seems to work correctly.

Comment 22

[Kai Engert [:KaiE:]]

Thanks for your patch.
In addition to the "real" change that is requested in this bug, you are doing a lot of additional cleanup.
I am ok with this in general and appreciate it, but I do have some comments.

---
Please undo your cleanup around entropy collector allocation.

With your code, if allocation of ec fails, we will no longer get the assertion. We'll only get it if ec succeeded, but bec fails.

We should get the assertion if ec fails or if both fail.

---
Please note the purpose of nsPSMUITracker and that it triggers some actions in its constructor and destructor. We should limit the lifetime of this object as much as possible to the real UI execution.

You removed the {} around this object, we should add it back. Also, the call to NSS function PK11_GetTokenName should be executed outside of this scope.

    PRBool canceled;
    NS_ConvertUTF8toUTF16 tokenName(PK11_GetTokenName(slot));

    {
      nsPSMUITracker tracker;
      if (tracker.isUIForbidden()) {
        rv = NS_ERROR_NOT_AVAILABLE;
      }
      else {
        rv = dialogs->SetPassword(ctx, tokenName.get(), &canceled);
      }
    }

---
In PSMContentDownloader::OnStreamComplete, is there a chance that aLoader will be null? I don't know. If it's possible, we should use NS_ENSURE_ARG.


If you address these requests, you'll get my r+

Comment 23

[Son Le]

Attachment: patch 3.2

New patch addressing review comments.

> In PSMContentDownloader::OnStreamComplete, is there a chance that aLoader will
> be null? I don't know. If it's possible, we should use NS_ENSURE_ARG.

No. The aLoader will actually call this OnStreamComplete function.

Comment 24

[Jason Duell]

It looks like we got close to a fix here, but the latest patch has been sitting unreviewed for two year.  Son, are you still interested in working on this bug?  Otherwise I might grab it if we still feel it's worth it (it's still listed as one of our top "first bugs" for newbies to try fixing).

Comment 25

[Son Le]

Jason, feel free to grab it.

Comment 26

[Jason Duell]

OK, I've assigned this to myself.  Son, thanks for all the work you've done on this bug already--I'll try to finally get your code into the tree!  (I bet you didn't expect it to take 5 years for your code to make it in when you started working on this!)

Comment 27

[Steve Harper]

Is this change going to make it into the final build anytime soon? If more work needs to be done on this let me take a look.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Someone probably needs to prod Kai with a sharp stick over e-mail to review this.

Comment 29

[Kai Engert [:KaiE:]]

Attachment: Patch v4

I've merged the patch to the current trunk.

Comment 30

[Kai Engert [:KaiE:]]

Comment on attachment 451606 [details] [diff] [review]
Patch v4

Wonderful patch, great cleanup, thank a lot for working on it.

r=kaie

Comment 31

[Josh Matthews [:jdm]]

Is this checkin-needed material? Or (more likely) does this need rebasing again?

Comment 32

[Tiziana Sellitto [:tiziana]]

I'm going through old "good first bugs" that have been inactive for a while. 
It seems that something is blocking this; As for comment 31 is it a checkin-needed problem?

Kai can you do something to unlock this?

Comment 33

[Kai Engert [:KaiE:]]

Yes, someone should have reminded me three years ago that checkin is needed... By now, the underlying code has changed a lot, portions have been removed. Someone must merge the old patch to the most recent code, prior to proceeding with this.

Comment 34

[Srujana Peddinti [:srujana121]]

Hi, I would like to work on the bug. Is this active?

Comment 35

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I think given how the architecture has changed due to process separation, it doesn't make sense to do this any longer.