Last Comment Bug 664693 - All built-in GCLI commands should be documented for security review
: All built-in GCLI commands should be documented for security review
Status: RESOLVED FIXED
[best:1d, likely:2d, worst:2d]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: ---
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
Mentors:
Depends on:
Blocks: 664696 GCLI-SHIP
  Show dependency treegraph
 
Reported: 2011-06-16 04:29 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2011-12-09 16:48 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-06-16 04:29:48 PDT

    
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-06-16 04:30:55 PDT
Marked as blocking [GCLI-HELP] because that's when the commands will start appearing.

This should be implemented (at least as a first cut) using the metadata already built into commands and the canon.
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-06-16 04:36:37 PDT
Also this (as a wiki page) is likely to be useful to users.
Comment 3 Daniel Veditz [:dveditz] 2011-08-25 10:39:33 PDT
Moved "sec-review-needed" to bug 664696 for the actual review. This bug is for the GCLI team to provide documentation so the security team (and mrbkap for wrappers) can do bug 664696.
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-18 09:59:58 PST
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Comment 6 David Chan [:dchan] 2011-12-07 17:20:44 PST
Thanks for filing the documentation Joe. The current set of commands is safe in my opinion. Sorry about the long delay in the review.

With regard to the question on screenshot, there are no OS critical png files from what I know. However this command doesn't expose more risk than already exists. If a user could overwrite a system critical PNG file, they could do it without Firefox. Are filenames restricted to [A-Za-z0-9] ? If not, a user could specify a filename with a \0 character. This may not have any adverse effect on the JS APIs, but the underlying OS APIs may interpret the \0 as end of string and truncate the filename. e.g.
"firefox.exe\0" (user input) -> "firefox.exe\0.png" (gcli adds png) -> "firefox.exe" as seen by the OS.
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-08 10:56:55 PST
(In reply to David Chan [:dchan] from comment #6)
> Are filenames restricted to [A-Za-z0-9] ?

That code is not committed yet. I'll make sure that it is.
Thanks.
Comment 8 David Chan [:dchan] 2011-12-08 11:23:04 PST
(In reply to Joe Walker from comment #7)
> (In reply to David Chan [:dchan] from comment #6)
> > Are filenames restricted to [A-Za-z0-9] ?
> 
> That code is not committed yet. I'll make sure that it is.
> Thanks.

I always seem to forget about i18n... the change becomes more complex if you plan to support non-Latin file names. Let me get back to you offline about that.
Comment 9 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-08 12:01:02 PST
(In reply to David Chan [:dchan] from comment #8)
> (In reply to Joe Walker from comment #7)
> > (In reply to David Chan [:dchan] from comment #6)
> > > Are filenames restricted to [A-Za-z0-9] ?
> > 
> > That code is not committed yet. I'll make sure that it is.
> > Thanks.
> 
> I always seem to forget about i18n... the change becomes more complex if you
> plan to support non-Latin file names. Let me get back to you offline about
> that.

So to be fair, I read that as:

> Are filenames prevented from containing control characters [charCode < 32]?
Comment 10 David Chan [:dchan] 2011-12-09 16:48:03 PST
> So to be fair, I read that as:
> 
> > Are filenames prevented from containing control characters [charCode < 32]?

That addresses my concerns. I'm assuming that screenshots won't be restricted to a specific directory. If so then you have to worry about path traversal type attacks with "../" . However since the screenshot saving won't run with elevated privileges, this isn't a concern.

Note You need to log in before you can comment on or make changes to this bug.