Closed Bug 664693 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jwalker, Assigned: jwalker)

References

Details

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

No description provided.
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: GCLI-HELP
Also this (as a wiki page) is likely to be useful to users.
Whiteboard: [sg:audit]
Blocks: GCLI-JS
No longer blocks: GCLI-HELP
Whiteboard: [sg:audit] → [sg:audit][minotaur][best:1d, likely:2d, worst:2d]
Assignee: nobody → jwalker
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
Whiteboard: [sg:audit][minotaur][best:1d, likely:2d, worst:2d] → [minotaur][best:1d, likely:2d, worst:2d]
Whiteboard: [minotaur][best:1d, likely:2d, worst:2d] → [best:1d, likely:2d, worst:2d]
Blocks: GCLI-SHIP
No longer blocks: GCLI-JS
Priority: -- → P1
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Component: Developer Tools → Developer Tools: Console
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.
(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]?
> 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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.