Make Account Provisioner XML handler only request the XML once.

RESOLVED FIXED in Thunderbird 12.0

Status

Thunderbird
Mail Window Front End
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

11 Branch
Thunderbird 12.0
x86
All
Bug Flags:
sec-review +

Thunderbird Tracking Flags

(thunderbird11 fixed)

Details

(Whiteboard: [secr:dchan])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
This is a pretty serious bug that made itself known once we started live-testing against some of our account providers.

For one thing, the old code only considered XML where the contentType exactly matched "text/xml".

Next, it looks like we requested the XML twice - once to determine the content type, and a second time in order to invisibly parse / deal with the XML content.

That doesn't work if our providers dynamically generate the XML, and don't allow us to re-request it.
(Assignee)

Updated

5 years ago
Assignee: nobody → mconley
(Assignee)

Comment 1

5 years ago
Created attachment 588950 [details] [diff] [review]
Patch v1

Here's my first pass at a solution.  This is quite extensive for something we'd likely want to land on Aurora, but unfortunately, I don't think this can be helped.

Instead of stopping the request as soon as we detect the contentType, and re-requesting, I use the following approach:

1.  Once the order form tab opens, wire up an observer to watch for any and all HTTP requests going in and out of Thunderbird (this observer is automatically unregistered once the order form tab is closed)
2.  For each HTTP request, watch for one where the contentType *starts* with the string "text/xml".  When we see one, ensure that the request is associated with the order form tab, and then attach an nsITraceableChannel listener to the request
3.  The nsITraceableChannel listener does the job of assembling the XML and trying to parse it.

I have to stress that this is the first time I've ever had to work with nsITraceableChannel, and the input/output streams/buffers in this code.  Most of this work is based upon examples found in http://www.softwareishard.com/blog/firebug/nsitraceablechannel-intercept-http-traffic/, Firebug, and this file:  http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/test/unit/test_traceable_channel.js
(Assignee)

Comment 2

5 years ago
Created attachment 588953 [details] [diff] [review]
Patch v2

Fixing a typo, putting back a log message that I accidentally chopped out.
Attachment #588950 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
I should also point out that with this patch, I can successfully get an account from Hover.
Will this need security review ?
(Assignee)

Comment 5

5 years ago
Comment on attachment 588953 [details] [diff] [review]
Patch v2

Hey Blake - let me know if I should redirect this review request elsewhere.
Attachment #588953 - Flags: review?(bwinton)
(Assignee)

Updated

5 years ago
tracking-thunderbird11: --- → ?
(Assignee)

Comment 6

5 years ago
Ludo:

I'm unsure, but I think I'm leaning towards "yes".

-Mike
Keywords: sec-review-needed
(Assignee)

Updated

5 years ago
Attachment #588953 - Flags: review?(bwinton) → review?(dbienvenu)

Comment 7

5 years ago
Comment on attachment 588953 [details] [diff] [review]
Patch v2

I can't test this, but it looks OK in general - but the scoping issues with the services includes are new and surprising to me. Are there other examples in the code of that happening?
Attachment #588953 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 8

5 years ago
(In reply to David :Bienvenu from comment #7)
> Comment on attachment 588953 [details] [diff] [review]
> Patch v2
> 
> I can't test this, but it looks OK in general - but the scoping issues with
> the services includes are new and surprising to me. Are there other examples
> in the code of that happening?

They were a surprise to me too.  I'm not sure why they happen, I just know that they do.  :/  It might have to do with the Account Provisioner running within a modal dialog, and therefore possibly having a scope / event loop completely separated from the main one.

Ludo marked this bug as sec-review-needed...should I just hold on committing this until we receive a security review?
(Assignee)

Updated

5 years ago
Attachment #588953 - Flags: approval-comm-aurora?
(In reply to Mike Conley (:mconley) from comment #8)

> Ludo marked this bug as sec-review-needed...should I just hold on committing
> this until we receive a security review?

We should probably talk to some of the security guys to get a faster review adding Curtis to the bug list.
(Assignee)

Comment 10

5 years ago
Is there anything else I need to do in order to receive a security review on this?

Comment 11

5 years ago
Comment on attachment 588953 [details] [diff] [review]
Patch v2

Review of attachment 588953 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/newmailaccount/content/uriListener.js
@@ +105,2 @@
>  
> +    if (contentType.indexOf("text/xml") != 0)

nsIHttpChannel doesn't do case-normalization of header values from what I know. This may not be an issue in practice if the current service providers return a lowercase Content-Type.
Whiteboard: [secr:dchan]

Comment 12

5 years ago
The security review will be for bug #686347 . There isn't any further security work that needs to be done for this particular bug.

Mike: Can you address comment 11 ?

It may be a potential functionality bug but shouldn't have security implications.
Keywords: sec-review-needed
See Also: → bug 686347
(Assignee)

Comment 13

5 years ago
Created attachment 591631 [details] [diff] [review]
Patch v3 (r+'d by bienvenu)

Thanks for pointing that out, dchan.  I'm running the content type through toLowerCase() first now, before checking for text/xml at index 0.
Attachment #588953 - Attachment is obsolete: true
Attachment #588953 - Flags: approval-comm-aurora?
(Assignee)

Comment 14

5 years ago
Created attachment 592263 [details] [diff] [review]
Patch v4 (r+'d by bienvenu)

I've taken the security review comments from bug 686347, and made the corrections in this patch.

I think this is ready to land on Aurora now.
Attachment #591631 - Attachment is obsolete: true
Attachment #592263 - Flags: approval-comm-aurora?
(Assignee)

Comment 15

5 years ago
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/5608d36000c9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #592263 - Flags: approval-comm-aurora? → approval-comm-aurora+
(Assignee)

Comment 16

5 years ago
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/070572a7da3d
status-thunderbird11: --- → fixed
tracking-thunderbird11: ? → ---
Target Milestone: --- → Thunderbird 12.0

Updated

5 years ago
Keywords: sec-review-complete
Flags: sec-review+
You need to log in before you can comment on or make changes to this bug.