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

VERIFIED FIXED in mozilla0.9.1



17 years ago
15 years ago


(Reporter: bbaetz, Assigned: dougt)


(Blocks: 1 bug, {relnote})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: MUST HAVE. Security Hole. patch under review, URL)


(5 attachments)



17 years ago
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.


17 years ago
Keywords: mozilla0.9.1, nsbeta1


16 years ago
Whiteboard: must have for beta.
Target Milestone: --- → mozilla0.9.2

Comment 1

16 years ago
does http have similar problems?

Comment 2

16 years ago
related to the nsStdURL stuff that we spoke about.

Comment 3

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


Comment 4

16 years ago
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

16 years ago
The text that netlib presented read 

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

Comment 6

16 years ago
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

16 years ago
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

16 years ago
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

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

Comment 10

16 years ago
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

16 years ago
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

16 years ago
Created attachment 36944 [details] [diff] [review]
First cut of interface changes

Comment 13

16 years ago
->m.9.1 since we want this for beta.
Whiteboard: must have for beta. → MUST HAVE for beta.
Target Milestone: mozilla0.9.2 → mozilla0.9.1

Comment 14

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

Comment 15

16 years ago
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

Comma delimited list - Appends to global port blacklist

Comma delimited list - removes from global port blacklist

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

16 years ago
Created attachment 37034 [details] [diff] [review]
Port Security Patch v1.

Comment 17

16 years ago
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

16 years ago
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

16 years ago
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

16 years ago
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

16 years ago
>> 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

16 years ago
-> 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

16 years ago
Created attachment 37048 [details] [diff] [review]
New HTTP patch with darin's suggestion


16 years ago
Whiteboard: MUST HAVE for beta. → patch under review


16 years ago
Whiteboard: patch under review → MUST HAVE. Security Hole. patch under review

Comment 24

16 years ago
Created attachment 37061 [details] [diff] [review]
Cleaner patch for http

Comment 25

16 years ago
this change include new text to the necko.properties file.
Keywords: l12y

Comment 26

16 years ago
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
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
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 =
to the now prefered:
+        nsCOMPtr<nsIStringBundleService>
2) NS_ConvertASCIItoUCS2("DeniedPortAccess").GetUnicode() should be
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".

Keywords: l12y

Comment 27

16 years ago
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

16 years ago
SMTP can run over ports 25 or 587.  Port 587 is expected to require 

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

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

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

Comment 30

16 years ago
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

16 years ago
Created attachment 37132 [details] [diff] [review]
Whole patch - includes recent suggestions.

Comment 32

16 years ago
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

16 years ago
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

16 years ago
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

16 years ago
I am allowing smtp on any port:

+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.  


16 years ago
Blocks: 83989

Comment 36

16 years ago
Something like:

NS_IMETHODIMP nsImapService::AllowPort(PRInt32 port, const char *scheme, PRBool 
    // allow imap to run on any port
    *_retval = PR_TRUE;
    return NS_OK;
I tunnel IMAP/S through ports in the 7000 range.  This will break.

Comment 38

16 years ago
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 40

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

Comment 41

16 years ago
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

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

Comment 43

16 years ago
fix checked into branch and trunk.
Last Resolved: 16 years ago
Resolution: --- → FIXED
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 

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

16 years ago
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.
QA Contact: tever → bbaetz

Comment 46

16 years ago
Thanks, Bradley!  For a bit more testing: 

huang@netscape.com will test areas in commercial branch builds which may be
affected by this fix (webmail, aol imap)
stephend@netscape.com will double check news and secnews
scalkins@netscape.com will double check IM chat in commercial branch builds
hong@netscape.com's team will cover LDAP ports

Comment 47

16 years ago
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 
LDAP URLs are not turned on in the default builds; no need to verify for those..

Comment 49

16 years ago
Lots of discussion on the newsgroup about side-effects of this fix, starting in 
<news:9fkbaj$93r1@secnews.netscape.com>. Can we get a response out there?

Comment 50

16 years ago
We need to have this well documented for release.
Keywords: relnote

Comment 51

16 years ago
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

16 years ago
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

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?
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

16 years ago
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
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

16 years ago
Can this bug be VERIFIED/CLOSED?

Comment 57

16 years ago
It appears that everything has been verified now.

blizzard: Yeah, this bug doesn't.

Comment 58

16 years ago
-nsconf (after discussion with mitch) so that this can be mentioned in a relnote
Group: netscapeconfidential?


16 years ago
Blocks: 85601

Comment 59

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

Comment 60

16 years ago
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

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

Comment 62

15 years ago
-> networking - this morphed to cover all major protocols.
Component: Networking: FTP → Networking
You need to log in before you can comment on or make changes to this bug.