Format string vulnerability in javascript shell command line option parsing

NEW
Unassigned

Status

()

Core
JavaScript Engine
6 years ago
2 years ago

People

(Reporter: Ricky Zhou, Unassigned)

Tracking

9 Branch
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 591707 [details] [diff] [review]
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.
(Reporter)

Comment 1

6 years ago
Sorry, I typoed the command, it's actually:

js -o "%x%x%x%n"
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 3

6 years ago
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 :-)
Created attachment 591943 [details] [diff] [review]
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. :-)
Assignee: general → christopher.leary
Status: NEW → ASSIGNED
Attachment #591943 - Flags: review?
Attachment #591943 - Flags: review? → review?(dmandelin)
Attachment #591943 - Flags: review?(dmandelin) → review+
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
Will add that to the comment.
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.
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.
Assignee: cdleary → general
Status: ASSIGNED → NEW
(Assignee)

Updated

3 years ago
Assignee: general → nobody
You need to log in before you can comment on or make changes to this bug.