83401 - %0a and %0d are unescaped -> malicious link can send mail

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Bradley Baetz (:bbaetz)]

It turns out that ftp unescapes %0a and %0d in the username, replacing them with
new line characters. This means that you can construct a url like the above, and
sent arbitary commands to any server which will ignore the intial USER command.

You'll obviously have to change the recipent and the email server to reproduce
this. A mail server of localhost doesn't work, probably because of the low
latency - moz gets the error code before the message, and stops sending data, I
suppose. Using judge.mcom.com as the mailserver worked.

As well, an error dialog with the server's responses now appears when you do
this (it didn't when I first tried this a couple of months ago), so the user
knows that something has happened, slthough it just looks like any other really
cryptic dialog (and so its probably not scriptable).

The simple solution to this is to just not unescape the resevered characters.
Thats not the best solution, but its the simplest.

There may be other places in the codebase which are exploitable to this type of
attack - see bugscape bug 5574.

Comment 1

[Doug Turner (:dougt)]

does http have similar problems?

Comment 2

[Doug Turner (:dougt)]

related to the nsStdURL stuff that we spoke about.

Comment 3

[Doug Turner (:dougt)]

netscape employee's, to try this out, change XXXXX to your user name in the
below link and open it in mozilla/n6.  

ftp://%0ahelo%20localhost%0amail%20from%3A%20%3Ctest%40example.com%3E%0arcpt%20to%3A%20%3CXXXXX%40netscape.com%3E%0adata%0atest%20test%20test%0a.%0aquit:[e-mail]:25/

Comment 4

[Doug Turner (:dougt)]

I don't think that the solution is to block certain encoded chars in the URL.  I
mean, there are going to be other implemenations of url parsers that might not
remember to block these encoded chars.

The more correct solution is to block bad ports as we did in netlib.

Here are the ports that we use to block:

1, 7, 9, 11, 13, 15, 17, 19, 20, 21, 23, 25, 37, 42, 43, 53, 77, 79, 87, 95,
101, 102, 103, 104, 109, 110, 111, 113, 115, 117, 119, 135, 143, 389, 512, 513,
514, 515, 526, 530, 531, 532, 540, 556, 601, 6000, 0 

We should add similar checks in necko.

I think that this will fix all port related bugs that Bradley has written up.

Comment 5

[Doug Turner (:dougt)]

The text that netlib presented read 

"Sorry, access to the port number given has been disabled for security reasons"

Comment 6

[Scott MacGregor]

hey doug, couple questions.....
1) where does this port blocking occur? I just noticed your list includes
blocking ports used for imap, smtp and news. I'm assuming that we aren't talking
about blocking these ports at the socket service level but at the http level right? 

Comment 7

[Bradley Baetz (:bbaetz)]

The problem with that solution is that its not flexable, which is why ns4 is
exploitable with the other problem. (see my comments in the bugscape bug)

Also, ISTR certain escapes which are invalid in urls. I can't remember which RFC
its in though. Those ports don't block smb or the netbios stuff, either.

If there are other urlparsers, then they need to be fixed. (We have more than
one way of unescaping urls, and I think someone (layout? dom?) has their own
version as well, which is slightly different). some people also run mailservers
on other ports (not 25)

ns4 prints up some message about invalid chars in the url with %0a and %0d. I
looked through the mozillaClassic code and found out where it did that.

I don't think http is vulnerable directly, because it doesn't unescape stuff.
finger and gopher are locked to their respective ports, datetime doesn't send
anything (and it may be port locked as well - I can't recall), and a website
can't force the mail client to send something to a generic server, I hope.

What does that leave?

mscott - the classic code had an array of protocol/port pairs which would be
allowed anyway, from what I recall from looking at LXR. That won't work with
pluggable protocols though. They have to be blocked at the socket level, or from
within the CAPS stuff, because the bug is not http only.

Comment 8

[Doug Turner (:dougt)]

Scott, I was thinking that every protocol handler would have to have some kind
of check.  Maybe some/most could share the same implementation to avoid bloat.

Comment 9

[Doug Turner (:dougt)]

*** Bug 72149 has been marked as a duplicate of this bug. ***

Comment 10

[Jim Roskind]

A few generic motherhood and apple pie comments (SIAK):

From a security perspective, it is a Bad Thing to list illegal ports, and a Good 
Thing to list allowed ports.  With security, the goal is that when you have an 
error, it can't induce a security problem, "just" a correctness problem (i.e., 
ommission in the "illegal ports" list causes a security breach, ommission in a 
"legal ports" list simply blocks a safe service).

If you take the tack of listing "Legal" (safe) ports, then a pluggable protocol 
handler would have to register appropriate (and hopefully safe) additions to the 
allowed list.  It is probably the case that a protocol handler should list 
"allowed ports" for a specific protocol (handler).

Comment 11

[Doug Turner (:dougt)]

I agree that in a security sense, the safer thing is to disallow all ports but a
few which are explictly declared.  (This is not what 4x did).  From a client
point of view, I am worried that many sites will we break because the admin set
up their servers to non standard port numbers which will not be in the protocol
handler's 'allow' port list.  

I think that every protocol handler will have seperate instances a list of known
(bad|good) ports.  In this way, pluggable protocols handlers will be responsible
for knowing what ports are allowed and will not easily be able to add illegal
ports to a global "(un)safe port list"

Comment 12

[Doug Turner (:dougt)]

Attachment: First cut of interface changes

Comment 13

[selmer (gone)]

->m.9.1 since we want this for beta.

Comment 14

[Doug Turner (:dougt)]

I should have a patch ready to be reviewed tomorrow sometime.

Comment 15

[Doug Turner (:dougt)]

So, the approach I took is as follows:

First every protocol handler has a function called |allowPort|.  As the
interface shows, it basically will tell the caller if the port for the given
scheme is valid.  

The IOService also implements |allowPort|.  It will check the port against its
known bad ports, and if a match is found, it will ask the protocol handler to
make the call if the port should be allowed.

The IOService has about 50 ports that it blacklists.  This list was complied
based on what classic did plus a few more I added.  Now, this list is going to
grow as more people find holes.  To prevent a huge firedrill/respin, I added two
new prefs so that we/people can easily add/remove ports from the global list. 
(Using signed JS from netcenter, we should be able to just poke these value.....
well getting netcenter to do this might just be harder than a respin :-/ but
anywayz) 

Comma delimited list - Appends to global port blacklist
network.security.ports.banned

Comma delimited list - removes from global port blacklist
network.security.ports.banned.override


So, now lets apply this...

I click on a ftp link with the port 25 as part of the URL.  

When the channel is opened via AsyncOpen(), it will make a call to
|NS_CheckPortSafety|.  This is a helper routine which lives in nsNetUtil.h. 
Passed into this call is the port number and scheme of the URI.  The port number
is check to see if it is in the list of bad port.  Assuming there is no match,
the function returns and the ftp uri is loaded.  Now, if there was a match, we
obtain the corrosonding protocol handler for the scheme.  |allowPort| is called
on the handler and the result return.  This give complete control to the
protocol handler.  

Comment 16

[Doug Turner (:dougt)]

Attachment: Port Security Patch v1.

Comment 17

[David :Bienvenu]

Adding John Myers to cc list. AFAIK, people do use non-standard ports for many
of the mail protocols like nntp and even IMAP. I'm not sure which protocols
"allow" non-standard ports and which require standard ports.

Comment 18

[Darin Fisher]

looks good for the most part.

how would a pluggable protocol handler add to the blacklist?  just by tweaking
the banned prefs?

also, i have some issue with adding an mIOService to every nsHttpChannel.  while
it is a good idea to cache it, it should just be cached in http handler instead.
the same may go for nsFTPChannel, but i'm less familar with the code there.

Comment 19

[Bradley Baetz (:bbaetz)]

You left out 587 - the msa port. (and smtp should override that as well)

smtp/imap/pop/ldap should also allow connections to whatever port is in the
prefs for that server, but thats probably a lot more work. (Although we
shouldn't allow anyone to construct arbitary urls for those schemes, so maybe it
could just allow everything?) Unless theres an easy way of working this out, it
can probably be left until later.

Even though gopher isn't protected, it should probably override the GOPHER_PORT
for consistancy (similarly for http).

This isn't called for ftp data connections, is it? It shouldn't be, I think, in
case the ftp server ends up using one of the reserved ports > 1024

Oh, and you've got a stray */ in nsBadPortList.h

Comment 20

[Darin Fisher]

or what i really meant to ask was: how would a protocol handler say that a
particular set of ports can never be used with it?  would it just intercept
such cases after calling NS_CheckPortSafety()?

also, why does port checking have to be done from AsyncOpen?  don't we bottle
neck everything through the io service already (NewChannelFromURI)?  couldn't
this do the port checking?

Comment 21

[Doug Turner (:dougt)]

>> how would a pluggable protocol handler add to the blacklist?  just by 
tweaking the banned prefs?

Yes, or by getting it added to the TRUNK with a good reason.  

the ioservice - I can move it to the handler and add an accessor.


the msa port has been added.  I also added GOPHER to the list of bad ports and 
made the gopher protocol handler allow this port.  

John, please comment on allowing alternate smtp/imap/pop/ldap ports.

Comment 22

[Doug Turner (:dougt)]

-> or what i really meant to ask was: how would a protocol handler say that a
particular set of ports can never be used with it?  would it just intercept
such cases after calling NS_CheckPortSafety()?

The black list is a general list of protocol that have been known to be 
exploited.  if your new protocol handler doesn't want to talk on port x, y, and 
z, that is your implementation.  


-> also, why does port checking have to be done from AsyncOpen?  don't we bottle
neck everything through the io service already (NewChannelFromURI)?  couldn't
this do the port checking?

Nope.  Two problems, (a) you can create a channel with a bad port.  there is no 
security hole there.  The problem is when you try to open this channel. and (b), 
there are a places where channels are created outside the IO service.  For 
example, if you know what the concrete class is, and a |new| is called on it.

Comment 23

[Doug Turner (:dougt)]

Attachment: New HTTP patch with darin's suggestion

Comment 24

[Doug Turner (:dougt)]

Attachment: Cleaner patch for http

Comment 25

[Doug Turner (:dougt)]

this change include new text to the necko.properties file.

Comment 26

[vidur (gone)]

Required changes
----------------

1) Check return value from CreateBundle() in nsDocShell.
2) nsIProtocolHandler.idl comment says that the call is going to be done at the
socket transport layer which is not correct.
3) The bad port list should really be defined in a cpp file, rather than a
header file to avoid multiple declaration if multiple files include
nsBadPortList.h. The list should probably be called gBadPortList to match
convention.
4) Put a comment next to the last element in nsBadPortList saying that it has to
be the last element (the fact that the last is 0 is used in populating the void
array).
5) In nsIOService::AllowPort, take mBadPortList.Count() out of the for loop.
6) Is it safe for the FTP and HTTP channel/handler implementations to cache the
IO service. There's no possibility of a circular reference, right?

Recommended changes
-------------------
1) In the nsDocShell patch, switch 
+        nsCOMPtr<nsIStringBundleService> sbs =
do_GetService(NS_STRINGBUNDLE_CONTRACTID);
to the now prefered:
+        nsCOMPtr<nsIStringBundleService>
sbs(do_GetService(NS_STRINGBUNDLE_CONTRACTID));
2) NS_ConvertASCIItoUCS2("DeniedPortAccess").GetUnicode() should be
NS_LITERAL_STRING("DeniedPortAccess").get().
3) Tabbing in nsPop3Protocol.cpp seems wrong.
4) Is it really a "bad" port list? How about restricted port list?
5) Why does FTP not override with the FTP port? Is FTP not a bad port?
6) For HTTP, why not get the scheme from the URI if there's some question about
the appropriateness of hardcoding "http".

Comment 27

[Doug Turner (:dougt)]

new patch coming up with all your suggestions.

As I mentioned to you in our AIM session, there should not be any cycles.  The 
IOService sticks around forever, and only holds weak references to the handlers.

For HTTP, that comment should have been removed.  It was a reminder to myself to 
think about how http(s) is handled.  Since it is using a secure port with auth, 
it is not band.

Comment 28

[John G. Myers]

SMTP can run over ports 25 or 587.  Port 587 is expected to require 
authentication.

The NNTP/SSL, POP3/SSL, IMAP/SSL, and LDAP/SSL ports are 995, 593, 993, and 636, 
respectively.

There is an alternate port 532 assigned for netnews reading.

Many ISPs block outgoing connections to port 25 from their dialup pools.  This 
might cause people to run SMTP on an alternate port.  They might not know about 
port 587.

Most of the times I've seen POP, IMAP, or SMTP run on port other than mentioned 
above was for testing.

Comment 29

[Gagan]

I'd consider caching IOService for the channel. With that r=gagan

Comment 30

[Doug Turner (:dougt)]

John, your comments regarding ISPs block outgoing connections to port 25 worries 
me.  With my patch, I basically restricts smtp connections to port 25 and 587.  
It sounds like I should be less restrictive or will be busting alot of dialup 
users.  How correct is this?

Comment 31

[Doug Turner (:dougt)]

Attachment: Whole patch - includes recent suggestions.

Comment 32

[vidur (gone)]

Doug - are there protocol handlers in the commercial tree that need to be
patched as well? If so, please make sure you deal with them.

Comment 33

[Doug Turner (:dougt)]

Vidur, it was done a while ago, but I just didn't think it would be a good 
idea to attach them here.  If you want be to open a bugscape bug, I will.  The 
changes are quite trival.

Comment 34

[Scott MacGregor]

doug this is going to cause some serious regressions to the commercial tree
because web mail makes connections for imap and smtp on non standard ports in
the 4 and 5 thousand range. We can't block these and these port numbers are
generated dynamically on the server side and frequently change. So there's no
way we can have a pre-populated list to block.

Comment 35

[Doug Turner (:dougt)]

I am allowing smtp on any port:




+NS_IMETHODIMP
+nsSmtpService::AllowPort(PRInt32 port, const char *scheme, PRBool *_retval)
+{
+    // allow smtp to run on any port
+    *_retval = PR_TRUE;
+    return NS_OK;
+}
+

I can do the same for IMAP.  

Comment 36

[Doug Turner (:dougt)]

Something like:

NS_IMETHODIMP nsImapService::AllowPort(PRInt32 port, const char *scheme, PRBool 
*_retval)
{
    // allow imap to run on any port
    *_retval = PR_TRUE;
    return NS_OK;
}

Comment 37

[Christopher Blizzard (:blizzard)]

I tunnel IMAP/S through ports in the 7000 range.  This will break.

Comment 38

[Bradley Baetz (:bbaetz)]

blizzard: No. This patch only restricts access to a certain number of ports
(those listed in the header file). The individual protocol stuff in there in
order to allow protocols to override this blocking. So you would have problems
putting imap on port 25, or 6000, but not 7000 (because its not in the list of
blocked ports), even if IMAP wasn't allowed through unconditionally.

Comment 39

[Christopher Blizzard (:blizzard)]

OK.

Comment 40

[Scott MacGregor]

doug answered my concerns over the phone. looks good to me. 

Comment 41

[Bradley Baetz (:bbaetz)]

dougt - I just realised that you didn't update chatzilla to add the dummy
routine. I presume that it will stop working when the missing js method is called.

Comment 42

[Doug Turner (:dougt)]

yeah.  it will never be called, so I didn't implement it.  File a seperate bug 
if you want and cc rginda.  

Comment 43

[Doug Turner (:dougt)]

fix checked into branch and trunk.

Comment 44

[(not reading, please use [e-mail] instead)]

hold up, looking at what got checked in, there are some problems.

it looks like for news, pop and ldap, only the "standard" ports will work.
if so,pop and ldap on non-standard ports will not work anymore?  that would be 
bad.

also, the defaults for NNTP / SSL and POP / SSL are wrong.

> if (port == NEWS_PORT || port == 995 || port == 532) // port 995 is NNTP/SSL, 
532 is netnews

563 is NNTP/SSL (not 995)

> if (port == POP3_PORT || port == 593)  // 593 is POP3/SSL

pop over ssl is 995, (not 593)

Comment 45

[Bradley Baetz (:bbaetz)]

VERIFIED on both trunk (CVS opt build from just after the patches were checked
in) and branch (mozilla buildid 2001060606).

I can read (imap) mail, send mail to myself, read news from news.mozilla.org,
and browse the net using http, ftp, and gopher.

On a branch commercial build, its possible to log into AIM (although I didn't do
exhaustive testing), and in a mozilla branch build I can use chatzilla.

None of my exploits work any more.

The pop breakage is a separate bug (bug 84283), and I'm concerned that that
didn't put up a large warning dialog like my exploits now do for the other
protocols, but I'll go comment in that bug and maybe file a new one.

Seth's concerns were discussed by Seth and dougt in bug 84283.

I'll leave this unverified until the respins from the pop bug come through
(inclusing the other fixes), and other people have checked things like LDAP, and
so on.

This bug and its dupe, which gives a different way to reproduce this, should
probably stay 'secure' (whether that means nsconf, mozilla.org conf, or the
security group) for the time being, for various reasons which I'm being
deliberatly obscure about in this bug in case people disagree.

Taking QA after discussion with tever.

Comment 46

[lchiang]

Thanks, Bradley!  For a bit more testing: 

[e-mail] will test areas in commercial branch builds which may be
affected by this fix (webmail, aol imap)
[e-mail] will double check news and secnews
[e-mail] will double check IM chat in commercial branch builds
[e-mail]'s team will cover LDAP ports

Comment 47

[Karen Huang]

Verified on 06-06-06-0.9.1 build on IMAP, IMAP/SSL, AOL/IMAP & WebMail/IMAP -> 
all pass. Leave it open until QAs verify on POP and LDAP, AIM based on previous 
comment.

Comment 48

[Dan Mosedale (:dmosedale, :dmose)]

LDAP URLs are not turned on in the default builds; no need to verify for those..

Comment 49

[Simon Fraser [no longer active]]

Lots of discussion on the newsgroup about side-effects of this fix, starting in 
<news:9fkbaj$[e-mail]>. Can we get a response out there?

Comment 50

[benc]

We need to have this well documented for release.

Comment 51

[scalkins]

For the AIM piece of this :
Checked IM Chat functionality in Linux 2001-06-06-11 0.9.1 branch.
Verifying it works ok in IM sending and receiving invites..and sending and
receiving messages within chat sessions.
 (Also works fine on Win branch from 6am tho this bug was for linux)

Comment 52

[Bradley Baetz (:bbaetz)]

Note that mozillaclassic blocked exactly the same ports - The example given in
the newsgroup of https://foo.bar.com:143/ is blocked in ns4 (I just tested now
on linux 4.77), so there is no issue of breaking things that previously worked.
I don't think it needs to be relnoted, but I'll leave the keyword in there for
consideration.

See http://lxr.mozilla.org/classic/source/network/main/mkconect.c#622 for the
code which did so (or at least, thats where the error message is printed)

Posts from the newsgroups seem to indicate that people have either worked this
particular exploit out on their own, or gotten access to this bug anyway (and
I'm guessing the second, since I can't see any comments referencing %0a in ftp
usernames in urls in the source...), so we could open this bug up now.

dougt, mstoltz, comments?

Comment 53

[Mitchell Stoltz (not reading bugmail)]

Which newsgroups? We could open it up, but I feel we've mishandled something
here. How did this information get out? Did people just see the code changes and
extrapolate to the bug? How did they read the status whiteboard of the bug?

Comment 54

[benc]

Well, for one thing, if we documented this for Communicator 4, we might just
file a bug that says "get parity for Mozilla" and then mark it fixed...

People are going to figure out what we allow anyhow once they get the first
error message, then someone will just poke throught every port to see what comes
back...

Comment 55

[Christopher Blizzard (:blizzard)]

I think that we're going to open this up soon.  We're still discussing this on
staff and we'll reach a conclusion shortly.

I don't think that it contains any Netscape confidential info so you guys should
be OK.  I suspect that people have just looked at the status whiteboard.

Comment 56

[Doug Turner (:dougt)]

Can this bug be VERIFIED/CLOSED?

Comment 57

[Bradley Baetz (:bbaetz)]

It appears that everything has been verified now.

blizzard: Yeah, this bug doesn't.

Comment 58

[Bradley Baetz (:bbaetz)]

-nsconf (after discussion with mitch) so that this can be mentioned in a relnote

Comment 59

[benc]

bbaetz; can you suggest some relnote text for this bug?

Comment 60

[Bradley Baetz (:bbaetz)]

I think the relnote is part of another bug, somewhere. Check the 6.1 notes (its
in the wrong section there, and misformatted, but it is there - just search for
port, IIRC

Comment 61

[benc]

I looked, but nothing promising seemed to match this issue.

Comment 62

[benc]

-> networking - this morphed to cover all major protocols.