Closed
Bug 256595
Opened 20 years ago
Closed 13 years ago
please get rid of 'scanf(..."%s")'
Categories
(Core :: Security, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: guninski, Assigned: mkaply)
Details
(Whiteboard: [sg:low] OS/2 only, others OK.)
Attachments
(2 files)
694 bytes,
patch
|
mkaply
:
review+
|
Details | Diff | Splinter Review |
1.37 KB,
patch
|
jhpedemonte
:
review+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•20 years ago
|
Whiteboard: [sg:fix]
Assignee | ||
Comment 2•20 years ago
|
||
(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•20 years ago
|
||
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•20 years ago
|
||
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.
Assignee | ||
Comment 5•20 years ago
|
||
Comment on attachment 156923 [details] [diff] [review] Simple fix for OS/2 problem r=mkaply for this change
Attachment #156923 -
Flags: review+
Assignee | ||
Comment 6•19 years ago
|
||
I decided I wanted a more thorough fix since this code was hard to read.
Attachment #201855 -
Flags: review?
Assignee | ||
Updated•19 years ago
|
Attachment #201855 -
Flags: review? → review?(jhpedemonte)
Updated•19 years ago
|
Attachment #201855 -
Flags: review?(jhpedemonte) → review+
Updated•19 years ago
|
Assignee: dveditz → mozilla
Whiteboard: [sg:fix] → [sg:low] OS/2 only, others OK.
Comment 7•18 years ago
|
||
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?
Reporter | ||
Comment 8•17 years ago
|
||
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 );
Assignee | ||
Comment 9•13 years ago
|
||
The Os/2 problem here is fixed. nsMimeMapper.cpp is no longer in the tree. Marking fixed.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•