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

RESOLVED FIXED

Status

()

Firefox
Developer Tools: Console
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [best:1d, likely:2d, worst:2d])

Comment hidden (empty)
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.
Blocks: 659059
Also this (as a wiki page) is likely to be useful to users.
Whiteboard: [sg:audit]
Blocks: 675923
No longer blocks: 659059
Whiteboard: [sg:audit] → [sg:audit][minotaur][best:1d, likely:2d, worst:2d]
Assignee: nobody → jwalker
Keywords: sec-review-needed
Blocks: 664696
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.
No longer blocks: 664696
Keywords: sec-review-needed
Whiteboard: [sg:audit][minotaur][best:1d, likely:2d, worst:2d] → [minotaur][best:1d, likely:2d, worst:2d]
Blocks: 664696
Whiteboard: [minotaur][best:1d, likely:2d, worst:2d] → [best:1d, likely:2d, worst:2d]
Blocks: 689605
No longer blocks: 675923
Priority: -- → P1
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Component: Developer Tools → Developer Tools: Console
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 6

6 years ago
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.
(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

6 years ago
(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.
(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

6 years ago
> 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.
You need to log in before you can comment on or make changes to this bug.