721307 - Format string vulnerability in javascript shell command line option parsing

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Ricky Zhou]

Attachment: js-formatstring.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111223105406

Steps to reproduce:

js -o "%n%n%n"


Actual results:

js segfaults


Expected results:

An error message should have been printed.

I've attached a patch that corrects this and another instance where a string is used as a format string.  Perhaps it'd be useful to add annotations to JS_ReportError and similar functions so that the compiler can warn when this happens.

Comment 1

[Ricky Zhou]

Sorry, I typoed the command, it's actually:

js -o "%x%x%x%n"

Comment 2

[David Mandelin [:dmandelin]]

Nice find. The patch looks good. (Trivium: we usually use ".diff" as the suffix on patch filenames.) The next step is to request review. jwalden+bmo or :cdleary would be good choices for reviewer.

Comment 3

[Ricky Zhou]

Fun fact: This bug was found and used by a lot of teams at the Mozilla CTF yesterday (http://mozillactf.org/), which is one of the only situations where a bug like this would have interesting uses and consequences :-)

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Add annotation to mfbt and use it.

Adds the suggested annotation. Thanks for the tip, Ricky! We tend to be sloppy in the shell because isn't exposed to the web, so this should be helpful. :-)

Comment 5

[Jeff Walden [:Waldo]]

Note that in gcc, using printf attributes on a C++ member method has weird indexing behavior; that should be clarified somehow in the documentation:

http://www.codemaestro.com/reviews/18

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Will add that to the comment.

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

We seem to use %hs in a bunch of places in the code base, hoping that printf will treat that as a specifier for strings of 16b chars. Unfortunately, local testing in GCC shows that just doesn't work.

man 3 printf shows that you can prefix the s conversion with an l character (i.e. "%ls") but that interprets it as a string of wchar_t, and I have no idea what wchar_t are defined to be in our code base and/or whether we should really rely on it.

Comment 10

[Till Schneidereit [:till]]

Mass-reassigning cdleary's bugs to default. He won't work on any of them, anymore. I guess, at least.

@cdleary: shout if you take issue with this.