664693 - All built-in GCLI commands should be documented for security review

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Comment 1

[Joe Walker [:jwalker]]

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]]

Also this (as a wiki page) is likely to be useful to users.

Comment 3

[Daniel Veditz [:dveditz]]

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]]

Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.

Comment 5

[Joe Walker [:jwalker]]

Docs done:
  https://etherpad.mozilla.org/gcli-sec-review-2011
Or:
  https://github.com/joewalker/gcli/blob/secreview-664696/docs/review/security-review-2011.md

Comment 6

[David Chan [:dchan]]

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]]

(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]]

(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]]

(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]]

> 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.