256595 - please get rid of 'scanf(..."%s")'

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[georgi - hopefully not receiving bugspam]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040115
Build Identifier: Mozilla/5.0

widget/src/mac/nsMimeMapper.cpp:220:    sscanf ( currPosition, "%ld %s ",
&flavor, mimeType );

may be exploitable if this code is hit. don't have a mac to check.

at least width format mask shoud be used.

another bad usages of sscanf:
-------------------------------------
directory/c-sdk/ldap/libraries/liblber/decode.c:439:            sprintf( msg,
"ber_scanf fmt (%s) ber:\n", fmt );
gc/boehm/dyn_load.c:312:          result = sscanf(next, "%s\n%n", path, &count);
gfx/src/xprintutil/xprintutil.c:721:    num_input_items = sscanf(d, "%s %f %f %f
%f", boolbuf, ma1, ma2, ma3, ma4);
netwerk/test/TestPageLoad.cpp:117:          sscanf(loc, "=%s", parseBuf);         
netwerk/test/TestProtocols.cpp:722:    scanf("%s", buffer);
other-licenses/libical/src/libical/icaltimezone.c:1431: if (sscanf (buf,
"%4d%2d%2d %4d%2d%2d %s",
security/nss/cmd/crmftest/testcrmf.c:121:    scanf ("%s", passWord);
security/nss/cmd/crmftest/testcrmf.c:153:    scanf  ("%s", userPin);
widget/src/mac/nsMimeMapper.cpp:220:    sscanf ( currPosition, "%ld %s ",
&flavor, mimeType );
widget/src/os2/nsWindow.cpp:1580:      if( 2 == sscanf( buf[0], "%d.%s",
&ptSize, buf[1])) // mmm, scanf()...
-------------------

not sure what the comment "mmm" means.




Reproducible: Didn't try
Steps to Reproduce:

Comment 1

[Daniel Veditz [:dveditz]]

To overrun the buffer with the mac nsMimeMapper one you'd have to have another
app running on your machine putting mimetype data into the clipboard using the
Mozilla app code. Not all that likely, and in that case you've got a hostile app
already running on your system. But adding a width specifier wouldn't be a bad idea.

widget/src/os2/nsWindow.cpp might need a width also. I guess WinQueryPresParam()
is an os2 API, that's the only reference in our codebase to that call. Can we
trust that a font name won't exceed the structure size given? If you've got a
hostile OS you're /really/ in trouble :-)

I assume "mmmm, scanf()..." is a reference to Homer Simpson's "Mmmm,
doughnuts..." catch-phrase, in this case meaning scanf is NOT a tasty delicacy.

The ldap one is OK, ber_scanf() allocates memory for strings. The line listed in
comment 0 is actually a sprintf() in a debug block.

I don't think we use boehm gc in final builds. The section in question is
reading from /proc/self/maps that it created.

The xprint one is OK, it reads part of a string into a buffer that's allocated
to be twice the length of the whole string (ugly but safe).

libical uses equal size buffers for the original string and the partial it scans
out of it. The *real* problem is the fgets (!!) into the original 1024 char
buffer from /Projects/libical/zoneinfo/zones.tab (by default, presumably movable
via config settings of some kind).

I'm not too worried about the scanfs in test programs.

Comment 2

[Mike Kaply [:mkaply]]

(In reply to comment #1)
=> widget/src/os2/nsWindow.cpp might need a width also. I guess WinQueryPresParam()
> is an os2 API, that's the only reference in our codebase to that call. Can we
> trust that a font name won't exceed the structure size given? If you've got a
> hostile OS you're /really/ in trouble :-)

Good question - I'll check into this.

> I assume "mmmm, scanf()..." is a reference to Homer Simpson's "Mmmm,
> doughnuts..." catch-phrase, in this case meaning scanf is NOT a tasty delicacy.

Definitely not something we wanted to do there.

Comment 3

[Peter Weilbacher]

About the OS/2 case:
The fifth parameter of WinQueryPresParam gives the length of the buffer in the
sixth parameter. Although I don't find anything in the docs about how it is
used, I would guess it restricts the number of characters written to the buffer,
buf[0] here... In that case a maximum of 126 characters would be written to
buf[1]. But I guess a %128s won't hurt.
At the moment I am not sure where the 128 comes from in the first place, maybe I
need to have a closer look at the toolkit...

Comment 4

[Peter Weilbacher]

Attachment: Simple fix for OS/2 problem

The longest font name I have is around 30 characters (I didn't get OS/2 to
actually install any fonts that I hacked to have longer names, but that could
be a different problem), so the 128 char buffer is definitely long enough.
But just to be sure, here is the minimal patch for widget/src/os2/nsWindow.cpp.

Comment 5

[Mike Kaply [:mkaply]]

Comment on attachment 156923 [details] [diff] [review]
Simple fix for OS/2 problem

r=mkaply for this change

Comment 6

[Mike Kaply [:mkaply]]

Attachment: More through fix

I decided I wanted a more thorough fix since this code was hard to read.

Comment 7

[Peter Weilbacher]

The fix to the OS/2 file was apparently checked in with the patch from bug 323254, already on 2006-01-13 09:56. So that part is fixed.

Can someone alerted the Calendar of the problem in libical, by CCing them (who?). Or should I mark this fixed and open a new bug for that part?

Comment 8

[georgi - hopefully not receiving bugspam]

this doesn't seem safe
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/mac/nsMimeMapper.cpp&rev=1.33&mark=217-220#217

217                     char mimeType[100];
218                     ResType flavor = nsnull;
219                 
220                     sscanf ( currPosition, "%ld %s ", &flavor, mimeType );

Comment 9

[Mike Kaply [:mkaply]]

The Os/2 problem here is fixed. nsMimeMapper.cpp is no longer in the tree.

Marking fixed.