Last Comment Bug 519252 - consider builder newsgroup link in a safer way
: consider builder newsgroup link in a safer way
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: 3.0
: All All
: -- normal (vote)
: Thunderbird 22.0
Assigned To: Magnus Melin
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-28 11:58 PDT by Magnus Melin
Modified: 2013-03-25 12:31 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (1.96 KB, patch)
2013-03-23 04:57 PDT, Magnus Melin
standard8: review+
Details | Diff | Splinter Review

Description Magnus Melin 2009-09-28 11:58:09 PDT
+++ This bug was initially created as a clone of Bug #519118 comment 2 +++

In general, this looks good.  However, from a security standpoint, Newsgroups
originally comes from an untrusted source (the message).  Now, if there are no
bugs in the rest of our code, we'll be fine constructing the URL using strings
like this.  However, it's better to have defense in depth.  Ideally, we'd build
up the URL object using nsIURI attribute setters on an nsStandardURL object so
that it enforces all the right escaping.  Assuming nsStandardURL supports that
mode of use, please do that, and request review from again.  If not, please
file a bug on nsStandardURL and we'll live with the string construction for
now, so r+ing now for that case.
Comment 1 Magnus Melin 2013-03-23 04:57:42 PDT
Created attachment 728608 [details] [diff] [review]
proposed fix

I guess this would be the standard way to make sure the url is ok.
Comment 2 Mark Banner (:standard8, limited time in Dec) 2013-03-25 08:03:44 PDT
Comment on attachment 728608 [details] [diff] [review]
proposed fix

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

::: mail/base/content/msgHdrViewOverlay.js
@@ +1725,3 @@
>    let url;
>    if (server.socketType != Components.interfaces.nsMsgSocketType.SSL) {
>      url = "news://" + server.hostName;

This lot feels like it is begging for a utility function, unfortunately I don't think we have one, and we wouldn't be able to share it easily anyway (due to historical mailnews issues). Oh well.
Comment 3 Magnus Melin 2013-03-25 12:31:52 PDT
https://hg.mozilla.org/comm-central/rev/b411f5facb9b -> FIXED

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