Closed Bug 1319423 Opened 7 years ago Closed 7 years ago

Look into securing the print recording files or move back to more secure IPC method.

Categories

(Core :: Security: Process Sandboxing, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: bobowen, Assigned: Alex_Gaynor)

References

(Blocks 1 open bug)

Details

(Whiteboard: sb+)

Attachments

(3 files)

In bug 1279699, we changed to using temporary files to store the print page recordings, because of OOM issues.

This may open up the possibility for other processes to interfere with the recording before processing in the parent.

We should look into securing the files or moving back to a more secure form of IPC.
Whiteboard: sb? → sb+
Priority: -- → P3
Blocks: sb-print
Blocks: 1405088
A concrete simplification we can make here is that right now the child process creates a file in tmp, and passes the path to the parent.

If we change this to the child asking the parent for an FD, writing to it, and then telling the parent to print that FD, we remove the need for the child to generate and create a path -- this let's us further lock down the sandbox policies because content will no longer need to create files in contemt tmp.
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #1)
> A concrete simplification we can make here is that right now the child
> process creates a file in tmp, and passes the path to the parent.
> 
> If we change this to the child asking the parent for an FD, writing to it,
> and then telling the parent to print that FD, we remove the need for the
> child to generate and create a path -- this let's us further lock down the
> sandbox policies because content will no longer need to create files in
> contemt tmp.

Yeah, we've discussed this before.
Creating the FD/Handle in the parent and passing down would be pretty easy to do as part of the existing functions in the protocol.

Using that might be a little trickier, because I don't think there is an easy way to create an ostream from it.
Although the work that Jeff Muizelaar did to make the recording of the events more generic may well help.
That might mean we can open and pass our own (hopefully existing) classes down and create a simple wrapper to write to them.
Assignee: nobody → agaynor
Priority: P3 → P1
Bob, if you're not the right person for print reviews (sorry!) feel free to forward on to someone else :-)
Comment on attachment 8915737 [details]
Bug 1319423 - Part 3 - Change the print IPC to not require the content process to create a temporary file;

https://reviewboard.mozilla.org/r/186938/#review192288

::: layout/printing/ipc/PRemotePrintJob.ipdl:27
(Diff revision 1)
>    async InitializePrint(nsString aDocumentTitle, nsString aPrintToFile,
>                          int32_t aStartPage, int32_t aEndPage);
>  
> -  // Translate the stored page recording and play back the events to the real
> -  // print device.
> -  async ProcessPage(nsCString aPageFileName);
> +  // Starts the process of printing a page, giving the child a FileDescriptor to
> +  // write the page recording into.
> +  sync StartPage() returns (FileDescriptor fd);

I don't think we should add a new sync protocol for this.
Can't we pass the FDs down as part of PrintInitializationResult and PageProcessed.

We should be able to avoid passing one down for the last page because we pass the start and end page to PrintInitialization.
Comment on attachment 8915735 [details]
Bug 1319423 - Part 1 - Introduce DrawEventRecorderPRFileDesc to allow switching away from std::ofstream in printing;

https://reviewboard.mozilla.org/r/186934/#review192134

::: gfx/2d/DrawEventRecorder.cpp:54
(Diff revision 1)
>    WriteHeader(mOutputStream);
>  }
>  
>  DrawEventRecorderFile::~DrawEventRecorderFile()
>  {
> -  mOutputStream.close();
> +  Close();

Is there any chance we might hit the MOZ_ASSERT in Close by doing this?

::: gfx/2d/RecordedEvent.h:10
(Diff revision 1)
>  
>  #ifndef MOZILLA_GFX_RECORDEDEVENT_H_
>  #define MOZILLA_GFX_RECORDEDEVENT_H_
>  
>  #include "2D.h"
> +#include "prio.h"

Not sure if the gfx guys will want Moz2D to depend on NSPR.
We need to get someone from gfx to look at this as well.
Jeff Muizelaar might be a good choice as he changed the stream stuff or he can suggest someone.
Comment on attachment 8915735 [details]
Bug 1319423 - Part 1 - Introduce DrawEventRecorderPRFileDesc to allow switching away from std::ofstream in printing;

https://reviewboard.mozilla.org/r/186934/#review192134

> Is there any chance we might hit the MOZ_ASSERT in Close by doing this?

I don't believe so, no.

> Not sure if the gfx guys will want Moz2D to depend on NSPR.

Using `std::ofstraem` wasn't possible because there's no standard way to go from a platform file handle to one (which is required by patch 3).
Comment on attachment 8915737 [details]
Bug 1319423 - Part 3 - Change the print IPC to not require the content process to create a temporary file;

https://reviewboard.mozilla.org/r/186938/#review192288

> I don't think we should add a new sync protocol for this.
> Can't we pass the FDs down as part of PrintInitializationResult and PageProcessed.
> 
> We should be able to avoid passing one down for the last page because we pass the start and end page to PrintInitialization.

Good call!
Comment on attachment 8915735 [details]
Bug 1319423 - Part 1 - Introduce DrawEventRecorderPRFileDesc to allow switching away from std::ofstream in printing;

https://reviewboard.mozilla.org/r/186934/#review192368

::: gfx/2d/RecordedEvent.h:10
(Diff revision 2)
>  
>  #ifndef MOZILLA_GFX_RECORDEDEVENT_H_
>  #define MOZILLA_GFX_RECORDEDEVENT_H_
>  
>  #include "2D.h"
> +#include "prio.h"

We indeed don't want to depend on nspr in Moz2D. However you should be able to implement a recorder outside of Moz2D

::: gfx/2d/RecordedEventImpl.h:539
(Diff revision 2)
>  
>    virtual bool PlayEvent(Translator *aTranslator) const;
>  
>    template<class S> void Record(S &aStream) const;
>    virtual void OutputSimpleEventInfo(std::stringstream &aStringStream) const;
> -  
> +

There's a bunch of whitespace changes too. If you want them, please separate them out into a separate commit.
Attachment #8915735 - Flags: review?(jmuizelaar) → review-
Thanks for the quick feedback Jeff; I'll move the NSPR based recorder out to live with the print code and leave the existing recorders alone.
Comment on attachment 8915737 [details]
Bug 1319423 - Part 3 - Change the print IPC to not require the content process to create a temporary file;

https://reviewboard.mozilla.org/r/186938/#review192372


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/printing/ipc/RemotePrintJobParent.cpp:99
(Diff revision 2)
> +nsresult RemotePrintJobParent::PrepareNextPageFD(FileDescriptor* aFd) {
> +  PRFileDesc *prFd = nullptr;
> +  nsresult rv = NS_OpenAnonymousTemporaryFile(&prFd);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8915735 [details]
Bug 1319423 - Part 1 - Introduce DrawEventRecorderPRFileDesc to allow switching away from std::ofstream in printing;

https://reviewboard.mozilla.org/r/186934/#review192368

> We indeed don't want to depend on nspr in Moz2D. However you should be able to implement a recorder outside of Moz2D

For my own edification, what's the reason to avoid depending on NSPR?
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #18)
> Comment on attachment 8915735 [details]
> Bug 1319423 - Part 1 - Switch away from std::ofstream to PRFileDesc in
> DrawEventRecorderFile;
> 
> https://reviewboard.mozilla.org/r/186934/#review192368
> 
> > We indeed don't want to depend on nspr in Moz2D. However you should be able to implement a recorder outside of Moz2D
> 
> For my own edification, what's the reason to avoid depending on NSPR?

Moz2D is used in projects other than Gecko (e.g. Servo)
Interesting, thanks.

Do you have an opinion on the right way to handle |RecordToStream| in |gfx/2d/RecordedEvent.h| which, as written, needs a concrete type? I suspect it could just be templated over |S|, but I want to make sure that's in keeping with the overall design.

Same thing with |LoadEventFromStream|.
Flags: needinfo?(jmuizelaar)
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #20)
> Interesting, thanks.
> 
> Do you have an opinion on the right way to handle |RecordToStream| in
> |gfx/2d/RecordedEvent.h| which, as written, needs a concrete type? I suspect
> it could just be templated over |S|, but I want to make sure that's in
> keeping with the overall design.
> 
> Same thing with |LoadEventFromStream|.

Ah yes. There was some nuance to that. Let me see if I can recall it.
So what I'd suggest is that you make an implementation of RecordToStream which takes an interface that has a virtual stream method. Then you can implement this interface on top of PRFileDesc. This adds an extra virtual call, but I don't think it can be done another way.
Flags: needinfo?(jmuizelaar)
Comment on attachment 8915736 [details]
Bug 1319423 - Part 2 - Switch away from std::ifstream to PRFileDesc in PrintTranslator;

https://reviewboard.mozilla.org/r/186936/#review192460
Attachment #8915736 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8915737 [details]
Bug 1319423 - Part 3 - Change the print IPC to not require the content process to create a temporary file;

https://reviewboard.mozilla.org/r/186938/#review192528

Gah, I just realised that what I said on IRC isn't correct, sorry.
The IPC/Quantum DOM part was, it was the:
"as we wait for each page to be printed in the parent, before continuing"

This isn't always true, I'd forgotten that we call |mPrintEngine->PrePrintPage()| before the parent has finished processing the page and that can call BeginPage in some circumstances.
(Did I mention that I hate printing.)

We would either need to wait in nsPagePrintTimer::Notify for |!mWaitingForRemotePrint| before calling PrePrintPage, which might slow down printing a bit.
Alternatively we could add a new message that sends down the FD and call it at the start of RecvProcessPage (assuming it's not the last), add a similar mechanism to wait for that as well in nsPagePrintTimer and call PrePrintPage once it arrives.
Attachment #8915737 - Flags: review?(bobowencode)
Comment on attachment 8915737 [details]
Bug 1319423 - Part 3 - Change the print IPC to not require the content process to create a temporary file;

https://reviewboard.mozilla.org/r/186938/#review193148

See comment 24, apologies again that I misled you here.
Attachment #8915737 - Flags: review?(bobowencode)
Comment on attachment 8915735 [details]
Bug 1319423 - Part 1 - Introduce DrawEventRecorderPRFileDesc to allow switching away from std::ofstream in printing;

https://reviewboard.mozilla.org/r/186934/#review193766
Attachment #8915735 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8915736 [details]
Bug 1319423 - Part 2 - Switch away from std::ifstream to PRFileDesc in PrintTranslator;

https://reviewboard.mozilla.org/r/186936/#review193768
Attachment #8915736 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8915737 [details]
Bug 1319423 - Part 3 - Change the print IPC to not require the content process to create a temporary file;

https://reviewboard.mozilla.org/r/186938/#review193974

Few nits and the thing over mWaitingForRemotePrint, which I'm slightly concerned will slow things down a bit anyway.

I lost track a bit of the automated tests someone was working on, but I think we have some in-tree now, but I suspect the coverage is still pretty poor.
So, you'll probably want to do a fair bit of manual testing.

I tended to use things like wikipedia.org (with the languages revealed) and bostonglobe.com for heavy font/webfont usage.
Also, the entire one page html spec for a bit of stress testing.
All to PDF of course. :-)
I assume the temp files are getting cleaned up OK.

::: commit-message-b4cf2:1
(Diff revision 4)
> +Bug 1319423 - Part 3 - Change the print IPC to not require teh content process to create a temporary file; r?bobowen

nit: s/teh/the/

::: layout/printing/DrawEventRecorder.h:6
(Diff revision 4)
>  /* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef MOZILLA_PRINTING_DRAWEVENTRECORDER_H_

nit: as this is new, the style guide specifies mozilla_layout_printing_DrawEventRecorder_h for this

::: layout/printing/DrawEventRecorder.h:20
(Diff revision 4)
>  
>  class PRFileDescStream : public mozilla::gfx::EventStream {
>  public:
>    PRFileDescStream() : mFd(nullptr), mGood(true) {}
>  
> -  void Open(const char *aFilename) {
> +  void OpenFD(PRFileDesc *aFd) {

nit: * with type, same in other places, also for &

::: layout/printing/ipc/RemotePrintJobParent.cpp:53
(Diff revision 4)
>    mPrintTranslator.reset(new PrintTranslator(mPrintDeviceContext));
> -  Unused << SendPrintInitializationResult(NS_OK);
> +  FileDescriptor fd;
> +  rv = PrepareNextPageFD(&fd);
> +  if (NS_FAILED(rv)) {
> +    Unused << SendPrintInitializationResult(rv, FileDescriptor());
> +    Unused << Send__delete__(this);

nit: I think return IPC_OK() and no else is better for these types of checks.

::: layout/printing/ipc/RemotePrintJobParent.cpp:119
(Diff revision 4)
> +  nsresult rv = PrintPage(mCurrentPageStream);
> +  mCurrentPageStream.Close();
>  
>    if (NS_FAILED(rv)) {
>      Unused << SendAbortPrint(rv);
>    } else {

nit: same here over else (which it looks like I clearly didn't do originally :-) ), it prevents the sort of nesting below.

::: layout/printing/nsPagePrintTimer.cpp:161
(Diff revision 4)
> +      if (mWaitingForRemotePrint) {
> +        return NS_OK;
> +      }

hmm, doing it this way with the return means we don't start the WatchDogTimer, which is another change in behaviour.

::: widget/nsDeviceContextSpecProxy.cpp:152
(Diff revision 4)
>  NS_IMETHODIMP
>  nsDeviceContextSpecProxy::BeginDocument(const nsAString& aTitle,
>                                          const nsAString& aPrintToFileName,
>                                          int32_t aStartPage, int32_t aEndPage)
>  {
> -  nsAutoCString recordingPath;
> +  mRecorder = new mozilla::layout::DrawEventRecorderPRFileDesc();

I had a valid recorder from this point originally because we used to get things to record before the first page, but I'm pretty sure jwatt changed things to make sure this can't happen now, so we should be OK.
Attachment #8915737 - Flags: review?(bobowencode) → review+
Comment on attachment 8915737 [details]
Bug 1319423 - Part 3 - Change the print IPC to not require the content process to create a temporary file;

https://reviewboard.mozilla.org/r/186938/#review193974

Tested with each of these, all seem to print ok.

> hmm, doing it this way with the return means we don't start the WatchDogTimer, which is another change in behaviour.

Hmm, is this a problem do you think? I wasn't sure.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/62081d87bcc9
Part 1 - Introduce DrawEventRecorderPRFileDesc to allow switching away from std::ofstream in printing; r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/f3f5b04640ad
Part 2 - Switch away from std::ifstream to PRFileDesc in PrintTranslator; r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/b186fddce27f
Part 3 - Change the print IPC to not require the content process to create a temporary file; r=bobowen
Keywords: checkin-needed
Depends on: 1412643
Depends on: 1417939
Depends on: 1427012
You need to log in before you can comment on or make changes to this bug.