159987 - 10.2: clicking on link with bogus href results in crash

Status: VERIFIED DUPLICATE of bug 166835

(Camino Graveyard :: General, defect)

Description

[sairuh (rarely reading bugmail)]

might be related to bug 159975 (mozilla), so tentatively assigning to conrad.
however, pls reassign as needed.

found using 2002.07.29.05 chimera bits.

1. download the file at this link to your machine:
http://hopey.mcom.com/tests/security/buffer-overflow/a-value.html (this issue
depends on loading the file locally).

2. bring up the file picker to open a file (File > Open File, or cmd+O).

3. select the file you downloaded in step #1 and open it in the browser.

4. click on the link called "link".

expected: because the value for the href in this link is bogus, you should get
an alert dlg which says that the file cannot be found.

actual results: chimera crashed. and, for some odd reason i cannot seem to get a
crash report on this. :-/

Comment 1

[sairuh (rarely reading bugmail)]

not sure if this would be the same bug (still cannot get crash report), but i
also crash when just trying to load the following file (after saving it locally):

http://hopey.mcom.com/tests/security/buffer-overflow/img-value.html

Comment 2

[Simon Fraser [no longer active]]

Crash log from opening a copy of img_value.html saved locally:

Date/Time:  2002-07-29 15:47:47 -0700
OS Version: 10.1.5 (Build 5S66)
Host:       h-10-169-102-84.nscp.aoltw.net

Command:    Navigator
PID:        22664

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0x72737474

Thread 0 Crashed:
 #0   0x72737474 in 0x72737474

Thread 1:
 #0   0x700252fc in select
 #1   0x00b30d50 in poll
 #2   0x00b2a690 in _pr_poll_with_poll
 #3   0x00b2aad0 in PR_Poll
 #4   0x05ad1d98 in nsSocketTransportService::Run(void)
 #5   0x01354f34 in nsThread::Main(void *)
 #6   0x00b2c964 in _pt_root
 #7   0x7002054c in _pthread_body

Thread 2:
 #0   0x7003f4c8 in semaphore_wait_signal_trap
 #1   0x7003f2c8 in _pthread_cond_wait
 #2   0x00b234fc in PR_WaitCondVar
 #3   0x05ae7064 in nsDNSService::DequeuePendingQ(void)
 #4   0x05ae6830 in nsDNSService::Run(void)
 #5   0x01354f34 in nsThread::Main(void *)
 #6   0x00b2c964 in _pt_root
 #7   0x7002054c in _pthread_body

Thread 3:
 #0   0x70044cf8 in semaphore_timedwait_signal_trap
 #1   0x70044cd8 in semaphore_timedwait_signal
 #2   0x7003f2b8 in _pthread_cond_wait
 #3   0x00b22e5c in pt_TimedWait
 #4   0x00b23524 in PR_WaitCondVar
 #5   0x0135c5c8 in TimerThread::Run(void)
 #6   0x01354f34 in nsThread::Main(void *)
 #7   0x00b2c964 in _pt_root
 #8   0x7002054c in _pthread_body

Thread 4:
 #0   0x7003f4c8 in semaphore_wait_signal_trap
 #1   0x7003f2c8 in _pthread_cond_wait
 #2   0x7086c34c in -[NSConditionLock lockWhenCondition:]
 #3   0x70ba1358 in -[NSUIHeartBeat _heartBeatThread:]
 #4   0x70842358 in forkThreadForFunction
 #5   0x7002054c in _pthread_body

Thread 5:
 #0   0x70000978 in mach_msg_overwrite_trap
 #1   0x70005a04 in mach_msg
 #2   0x70026a2c in _pthread_become_available
 #3   0x70026724 in pthread_exit
 #4   0x70020550 in _pthread_body

Thread 6:
 #0   0x7003f4c8 in semaphore_wait_signal_trap
 #1   0x7003f2c8 in _pthread_cond_wait
 #2   0x00b234fc in PR_WaitCondVar
 #3   0x01356ed4 in nsThreadPool::GetRequest(nsIThread *)
 #4   0x01357fbc in nsThreadPoolRunnable::Run(void)
 #5   0x01354f34 in nsThread::Main(void *)
 #6   0x00b2c964 in _pt_root
 #7   0x7002054c in _pthread_body

Comment 3

[sairuh (rarely reading bugmail)]

chatted w/simon, who sez that the crash for img-value.html might be different
from this one, so i filed bug 160006 as a separate issue.

Comment 4

[Brian Nesse (gone)]

I see similar results... except I crashed somewhere else in the alphabet. :)

Thread 0 Crashed:
 #0   0x61626364 in 0x61626364

Comment 5

[Simon Fraser [no longer active]]

Yup, we're executing data in the supplied string. This is a classic source of
security attacks. I can hear the alarms ringing, and the dog-handlers running as
I type....

Comment 6

[Winnie Lam]

Attachment: crash log when opening "link" from locally saved a-value.html

my stack is slightly different from sfraser's.

Comment 7

[Conrad Carlen (not reading bugmail)]

Looking.

Comment 8

[Conrad Carlen (not reading bugmail)]

It crashes on the call to ::FSPathMakeRef() in nsLocalFile::InitWithPath().
Certainly an Apple bug but, if we want to avoid this crash, we'll have to parse
the string ourselves first in order to reject strings which would crash
::FSPathMakeRef(). Another option I'll investigate is using CFURL routines to
see if they're any better bullet-proofed.

Comment 9

[Simon Fraser [no longer active]]

Is there a max file/folder name length that we can truncate to?

Comment 10

[Conrad Carlen (not reading bugmail)]

Yeah, but we wouldn't want to truncate on InitWithPath(). With HFS+, I don't
want to truncate at all - its pathname limits are pretty generous (unlimited
path len, 255 max component len). Instead, it should just return an error on
truly pathological cases like this. My Windows build is in progress so I can't
verify that's what happens there, but I bet it is.

I'm working on a path which, instead of calling FSPathMakeRef, uses CFURL
routines. Hopefully that will catch this case without crashing without a perf hit.

Comment 11

[Simon Fraser [no longer active]]

My thoughts were to scan for directory separators, and truncate each component
of the path if necessary. Or, come to think of it, just throw an error if any
component is too long; after all, if that's the case then it has to be a bogus path.

Comment 12

[Mike Pinkerton (not reading bugmail)]

why are we in file code trying to load this url? is it because we think it's a
relative path from the webpage loaded as a file: url?

the other thing i want to know is how on earth did you track down where it was
crashing? ;)

Comment 13

[Simon Fraser [no longer active]]

pink: we're trying to load the file because the URL is relative to the source
page, which is being loaded from disk in the test.

Comment 14

[Conrad Carlen (not reading bugmail)]

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

Comment 15

[Conrad Carlen (not reading bugmail)]

Attachment: patch

This fixes it. It does not call FSPathMakeFSRef directly on the whole path,
which is what crashes in this case. It only calls FSPathMakeFSRef on the
portion of the path which may begin in "/Volumes/XXX" The volume name is
assumed to be safe to pass to FSPathMakeFSRef because relative locations in
content are relative to their base which has to be on an existing volume.
 This also another bug: The old code, if FSPathMakeFSRef failed, started with
an FSRef of "/" and then appended each node. I found that this does not work
for a path in which the first node is "/Volumes" :-/

Looking for review.

Comment 16

[Conrad Carlen (not reading bugmail)]

Attachment: better patch

This uses CFURL to digest the path, doesn't crash, doesn't error on
InitWithNativePath so we get an actual error msg when attempting to load the
file which is consistent with other platforms.

Comment 17

[Conrad Carlen (not reading bugmail)]

Attachment: performance of old vs. new InitWithPath

Using CFURL, in addition to not crashing, also performs better than the old
way. I ran 3 tests and a path which (1) existed, (2) had one non-extant node,
and (3) had 2 non-extant nodes.

In case 1, new and old are about the same. In (2), which is very common, the
new way kicks ass.

Comment 18

[Conrad Carlen (not reading bugmail)]

This patch also fixes bug 160608.
Looking for review.

Comment 19

[Simon Fraser [no longer active]]

Comment on attachment 94644 [details]
performance of old vs. new InitWithPath

Nice! Thanks for taking the time to do a second round.

Small comment on StBuffer (we so need that to be exported from XPCOM) -- can
you rename the various 'capacity' methods/member vars to make it clear that
they measure bytes, not characters. It's too easy to get them confused. I
suggest 'EnsureCapacityBytes' and 'GetCapacityBytes' or somesuch.

Comment 20

[Steve Dagley]

Comment on attachment 94642 [details] [diff] [review]
better patch

r=sdagley (and xfering sfraser's sr= from the perf test he attached it to)

Comment 21

[Conrad Carlen (not reading bugmail)]

Fixed.

Comment 22

[sairuh (rarely reading bugmail)]

this is fixed using 2002.08.26.05, but bug 160006 still causes a crash.

Comment 23

[sairuh (rarely reading bugmail)]

as with bug 160006, using 2002.09.03.05, this no longer crashes on 10.1.5.
however, on 10.2 this results in a crash.

reopening. again, if you'd rather i file a new bug for this, let me know.

Comment 24

[sairuh (rarely reading bugmail)]

Attachment: crash report from 2002090305 build on 10.2

Comment 25

[Conrad Carlen (not reading bugmail)]

Since both this and bug 160006 are the same problem, made one bug.



*** This bug has been marked as a duplicate of 166835 ***

Comment 26

[sairuh (rarely reading bugmail)]

v