Closed Bug 856243 Opened 7 years ago Closed 7 years ago

do_QueryFrame support is incomplete and thus a potential foot gun

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox22 --- wontfix
firefox23 --- fixed
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: mats, Assigned: mats)

Details

(Keywords: sec-other, sec-want, Whiteboard: [adv-main23-])

Attachments

(5 files, 5 obsolete files)

Attached patch exampleSplinter Review
Consider the attached example and note that:
1. it compiles without a warning
2. any use of the result 'vp' is likely to be exploitable
3. as part of a real patch this sort of code is likely to get r+

The bug is that the ViewportFrame class should declare this:
  NS_DECL_QUERYFRAME_TARGET(ViewportFrame)
before it can be used as the target of a do_QueryFrame() call.
It compiles because ViewportFrame inherits nsContainerFrame which does
declare it.

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsQueryFrame.h

> 10  #define NS_DECL_QUERYFRAME_TARGET(classname)                    \
> 11    static const nsQueryFrame::FrameIID kFrameIID = nsQueryFrame::classname##_id;

> 59  class do_QueryFrame
> 60  {
> 61  public:
> 62    do_QueryFrame(nsQueryFrame *s) : mRawPtr(s) { }
> 63
> 64    template<class Dest>
> 65    operator Dest*() {
> 66      if (!mRawPtr)
> 67        return nullptr;
> 68
> 69      return reinterpret_cast<Dest*>(mRawPtr->QueryFrame(Dest::kFrameIID));
> 70    }
> 71
> 72  private:
> 73    nsQueryFrame *mRawPtr;
> 74  };
> 75

ViewportFrame::kFrameIID is just an alias for nsContainerFrame::kFrameIID
so the QueryFrame() checks if this is a nsContainerFrame and then casts the
result to ViewportFrame.

Is there a way to make the example fail to compile using some C++ trickery?

If not, then I suggest we make kFrameIID mandatory for all frame classes.
Instead of ViewportFrame::kFrameIID, you could have

  template<class C> struct FrameId;
  template<> struct FrameId<nsContainerFrame> { static const nsQueryFrame::FrameIID id = ...; };

and so on, generated with slightly different macro hackery.  Any attempt to get FrameId<ViewportFrame> would hit the undefined case and die.  I don't know enough about frame class ids to know if this would also require having every class have a frame id or not.
Erm.  I meant to CC jcranmer because I wondered if MOZ_MUST_OVERRIDE might have any vague applicability to static consts in classes.  That might also be a fix too.
NS_DECL_QUERYFRAME_TARGET is in class scope, so clang complains over having
a template specialization there.  What we currently have is this:

class nsContainerFrame : ... {
public:
  NS_DECL_QUERYFRAME_TARGET(nsContainerFrame)
...
};

class ViewportFrame: public nsContainerFrame {
 // no NS_DECL_QUERYFRAME_TARGET here!
};

For the above we want:
  nsContainerFrame* x = do_QueryFrame(a); // ok
  ViewportFrame* y = do_QueryFrame(a); // should fail to compile


All classes have a unique constant in an enum, which is the value
NS_DECL_QUERYFRAME_TARGET refers to as nsQueryFrame::classname##_id

It's not a requirement that do_QueryFrame should fail for ViewportFrame
per se, so adding NS_DECL_QUERYFRAME_TARGET everywhere is a solution.

Just wondering if there exist a solution to add them as needed,
without the foot gun :-)
A simple assignment check seems to do what I want, like so:

#define NS_DECL_QUERYFRAME_TARGET(classname)                    \
  static const nsQueryFrame::FrameIID kFrameIID = nsQueryFrame::classname##_id; \
  static classname* do_QueryFrame_helper() {return nullptr;}


class do_QueryFrame
{
public:
  do_QueryFrame(nsQueryFrame *s) : mRawPtr(s) { }

  template<class Dest>
  operator Dest*() {
    Dest* dummy;
    MOZ_STATIC_ASSERT(!(dummy = Dest::do_QueryFrame_helper()), "");
    if (!mRawPtr)
      return nullptr;

    return reinterpret_cast<Dest*>(mRawPtr->QueryFrame(Dest::kFrameIID));
  }

private:
  nsQueryFrame *mRawPtr;
};


... and with that I find two existing bugs in the tree.
One is in a MOZ_ASSERT which seems safe, the other I'm not sure...
so hiding the bug for now.
Group: core-security
http://hg.mozilla.org/mozilla-central/annotate/8693d1d4c86d/layout/generic/nsSimplePageSequence.cpp#l496

nsHTMLCanvasFrame doesn't have a NS_DECL_QUERYFRAME_TARGET so it
inherits nsContainerFrame::kFrameIID.  So if 'child' is a nsContainerFrame
of some sort then 'canvasFrame' will be non-null.
Keywords: sec-want
Attached patch fix (obsolete) — Splinter Review
We can use the 'return nullptr' for the assignment check.
The clang error message for a missing NS_DECL_QUERYFRAME_TARGET is
quite clear I think:

layout/generic/nsQueryFrame.h:68:14: error: cannot initialize return object of type 'nsHTMLCanvasFrame *' with an rvalue of type 'typename nsHTMLCanvasFrame::Has_NS_DECL_QUERYFRAME_TARGET' (aka 'nsContainerFrame *')
      return static_cast<typename Dest::Has_NS_DECL_QUERYFRAME_TARGET>(nullptr);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
layout/generic/nsSimplePageSequence.cpp:496:40: note: in instantiation of function template specialization 'do_QueryFrame::operator class nsHTMLCanvasFrame *<nsHTMLCanvasFrame>' requested here
      nsHTMLCanvasFrame* canvasFrame = do_QueryFrame(child);
                                       ^

=========

https://tbpl.mozilla.org/?tree=Try&rev=941c357ca3f3
Assignee: nobody → matspal
Attachment #731540 - Flags: review?(dholbert)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Erm.  I meant to CC jcranmer because I wondered if MOZ_MUST_OVERRIDE might
> have any vague applicability to static consts in classes.  That might also
> be a fix too.

MOZ_MUST_OVERRIDE is defined for methods, and is tested to work for non-virtual methods. Given its design, it may even work for static methods (I hadn't considered that possibility before)... As it stands, it doesn't work for fields, though.
Comment on attachment 731540 [details] [diff] [review]
fix

Review of attachment 731540 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsQueryFrame.h
@@ +64,5 @@
>  
>    template<class Dest>
>    operator Dest*() {
>      if (!mRawPtr)
> +      return static_cast<typename Dest::Has_NS_DECL_QUERYFRAME_TARGET>(nullptr);

This is sufficiently magic that I think a comment of some sort is in order.  Perhaps just 

 // Ensure that Dest declared itself as a queryframe target.
Attachment #731540 - Flags: feedback+
Attached patch fix, v2 (obsolete) — Splinter Review
With the comment Zack suggested.
Attachment #731540 - Attachment is obsolete: true
Attachment #731540 - Flags: review?(dholbert)
Attachment #731897 - Flags: review?(dholbert)
Attachment #731897 - Flags: review?(dholbert) → review+
GetPrintCanvasElementsInFrame is recursive and the only other caller is
nsSimplePageSequenceFrame::PrePrintNextPage
http://dxr.mozilla.org/mozilla-central/layout/generic/nsSimplePageSequence.cpp#l605
calling it 'mCurrentPageFrame', so it seems there's no risk it can be
affected by unprivileged content.  'canvasFrame->GetContent()' is the only
use of 'canvasFrame' anyway and that's safe regardless of frame type.
Keywords: sec-other
That all appears to be true. (and the nsHTMLFramesetBlankFrame do_QueryFrame call isn't exploitable in release builds, because it's not compiled into those builds -- it's part of a debug-only assertion.)

So I don't think this is exploitable (and hence, this patch isn't going to zero-day our branch releases when it's landed).
Backed it out because of:
Assertion failure: blank = do_QueryFrame(child) (unexpected child frame type), at layout/generic/nsFrameSetFrame.cpp:1052

Odd because it appears to be correct:

1052  MOZ_ASSERT(blank = do_QueryFrame(child), "unexpected child frame type");
(gdb) p *child
$1 = (nsHTMLFramesetBlankFrame) {
  <nsLeafFrame> = {
    <nsFrame> = {
(In reply to Mats Palmgren [:mats] from comment #12)
> Backed it out because of:
> Assertion failure: blank = do_QueryFrame(child) (unexpected child frame
> type), at layout/generic/nsFrameSetFrame.cpp:1052
> 
> Odd because it appears to be correct:
> 
> 1052  MOZ_ASSERT(blank = do_QueryFrame(child), "unexpected child frame
> type");

I don't see nsHTMLFramesetBlankFrame declaring NS_DECL_QUERYFRAME_TARGET?
You're correct, it doesn't (and mats' patch fixes that).
(In reply to Mats Palmgren [:mats] from comment #12)
> Assertion failure: blank = do_QueryFrame(child) (unexpected child frame
> type), at layout/generic/nsFrameSetFrame.cpp:1052
> 
> 1052  MOZ_ASSERT(blank = do_QueryFrame(child), "unexpected child frame
> type");
> (gdb) p *child
> $1 = (nsHTMLFramesetBlankFrame) {
>   <nsLeafFrame> = {
>     <nsFrame> = {

Uh, it's surely not a good idea to have |=| there, even if those are the semantics you want!
Hmm, so an assignment there *does* work.  It just reads horribly wrong to see a presumptively-side-effecting assignment in an assertion condition.  :-\  Guh.
I blame DebugOnly<> that evaluates also in non-debug builds... ;-P
It seems I misunderstood what NS_DECL_QUERYFRAME_TARGET is for.
Here's an example to illustrate, nsIStatefulFrame:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIStatefulFrame.h
which is then added by the concrete class:
http://hg.mozilla.org/mozilla-central/annotate/0b7c27024048/layout/generic/nsGfxScrollFrame.cpp#l868

So it's just a way to add additional targets for a concrete class that MUST
implement QueryFrame.  NS_DECL_QUERYFRAME_TARGET doesn't do anything by itself.
Attached patch fix, v3 (obsolete) — Splinter Review
So we also need NS_DECL_QUERYFRAME + implementation.

I also added an assertion in NS_QUERYFRAME_TAIL_INHERITANCE_ROOT
which is only implemented in the top QueryFrame (in nsFrame).
So if the target ID did not match any of the supported QF IDs,
then it must not match the real ID of the instance either.
If it does then you added NS_DECL_QUERYFRAME_TARGET without
actually implementing it.  This asserts for the v2 patch.

This patch pass reftest/crashtests locally (Linux64).
Let's see what Try says:
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=952c8f1a3eeb
Attachment #731897 - Attachment is obsolete: true
Attachment #732103 - Flags: review?(dholbert)
Comment on attachment 732103 [details] [diff] [review]
fix, v3

> #define NS_QUERYFRAME_TAIL_INHERITANCE_ROOT                     \
>   default: break;                                               \
>   }                                                             \
>-  return nullptr;                                                \
>+  MOZ_ASSERT(id != GetFrameId(),                                \
>+    "NS_DECL_QUERYFRAME_TARGET without NS_DECL_QUERYFRAME ?");  \
>+  return nullptr;                                               \

The assertion-message could stand to be clarified a bit.

Maybe replace the assertion with:
  MOZ_ASSERT(id != GetFrameId(),                                 \
    "A frame failed to QueryFrame to its *own type*. It may be " \
    "missing NS_DECL_QUERYFRAME, or a NS_QUERYFRAME_ENTRY() "    \
    "line with its own type name");                              \

(or, add a comment to that effect)

r=me with that.

Also, can we un-hide this bug, given that comment 10 appears to address your security concerns at the end of comment 4?
Attachment #732103 - Flags: review?(dholbert) → review+
I want to ask you to change this comment again...

    if (!mRawPtr) {
      // Ensure that Dest declared itself as a queryframe target.
      return static_cast<typename Dest::Has_NS_DECL_QUERYFRAME_TARGET>(nullptr);
    }

Looking at the larger context, it reads as if we're only doing the "Ensure ..." when !mRawPtr.  This would be clearer:

  // This expression doubles as a compile-time assertion that
  // Dest declared itself as a queryframe target.

Tweak wording if you like; the important thing is "compile time".
I will improve both comments as you suggested, thanks.

(In reply to Daniel Holbert [:dholbert] from comment #20)
> Also, can we un-hide this bug, given that comment 10 appears to address your
> security concerns at the end of comment 4?

Maybe.  I'm still a little worried over how error prone this code is
so there could be additional problems we haven't found yet...

So I ran a few scripts over the tree to gather some statistics:

We have 60 unique NS_QUERYFRAME_HEAD
We have 54 unique NS_DECL_QUERYFRAME_TARGET
We have 55 unique NS_QUERYFRAME_ENTRY  (huh? one more??)
We have 0 NS_QUERYFRAME_ENTRY_CONDITIONAL

The ENTRY that does not have a TARGET is:
nsMathMLFrame

http://hg.mozilla.org/mozilla-central/annotate/aae004a3c5d9/layout/mathml/nsMathMLContainerFrame.cpp#l41

Is that ENTRY supposed to be nsIMathMLFrame ?

There are ~17 or so do_QueryFrame calls with nsIMathMLFrame as target.
That doesn't seem right.  Are all those safe?
Attached patch experiment 2Splinter Review
So I did this experiment and looked at the actual frames when
I got an assertion in gdb.  Here's a few:

(gdb) p *this
$1 = (nsMathMLmrowFrame) {
  <nsMathMLContainerFrame> = {
    <nsContainerFrame> = {
      <nsSplittableFrame> = {
        <nsFrame> = {
          <nsBox> = {
            <nsIFrame> = {
              <nsQueryFrame> = {
                _vptr$nsQueryFrame = 0x7ffff635f710
              }, 

(gdb) p *this
$2 = (nsMathMLmstyleFrame) {
  <nsMathMLContainerFrame> = {
    <nsContainerFrame> = {
      <nsSplittableFrame> = {
        <nsFrame> = {
          <nsBox> = {
            <nsIFrame> = {
              <nsQueryFrame> = {
                _vptr$nsQueryFrame = 0x7ffff6364d80
              }, 

(gdb) p *this
$3 = (nsMathMLTokenFrame) {
  <nsMathMLContainerFrame> = {
    <nsContainerFrame> = {
      <nsSplittableFrame> = {
        <nsFrame> = {
          <nsBox> = {
            <nsIFrame> = {
              <nsQueryFrame> = {
                _vptr$nsQueryFrame = 0x7ffff635ec10
              }, 


I get the impression most MathML frame classes don't QueryFrame
to nsIMathMLFrame.  This seems wrong.
This is the same compile-time check that we did in do_QueryFrame.
This makes the "NS_QUERYFRAME_ENTRY(nsMathMLFrame)" fail to compile
since the nsMathMLFrame class does not have a TARGET.
Attachment #732170 - Flags: review?(dholbert)
So we should either remove this entry, since it now won't compile,
or change it to nsIMathMLFrame to make nsMathMLContainerFrame and
derived classes QueryFrame to nsIMathMLFrame.  This seems reasonable
since nsMathMLContainerFrame inherits nsMathMLFrame which inherits
nsIMathMLFrame.  (I'll submit a Try run in a bit...)
Attachment #732172 - Flags: review?(dholbert)
For the record, here are some additional statistics:

TARGETs with no corresponding HEAD:

nsISVGSVGFrame
nsISVGGlyphFragmentNode
nsISVGChildFrame
nsIAnonymousContentCreator
nsIPageSequenceFrame
nsIStatefulFrame
nsIObjectFrame
nsIScrollableFrame
nsIFrame
nsIMathMLFrame
nsIPercentHeightObserver
nsIFormControlFrame
nsITextControlFrame
nsISelectControlFrame
nsIListControlFrame
nsIComboboxControlFrame
nsIScrollbarMediator
nsIRootBox
nsITableCellLayout

(all abstract classes so that seems reasonable)

HEADs with no corresponding TARGET:

nsSVGOuterSVGFrame
nsSVGForeignObjectFrame
nsSVGUseFrame
nsSVGTSpanFrame
nsSVGGlyphFrame
nsFrame
nsSimplePageSequenceFrame
nsVideoFrame
nsHTMLScrollFrame
nsXULScrollFrame
nsMathMLContainerFrame
nsMathMLmtableOuterFrame
nsMathMLmtdInnerFrame
nsListControlFrame
nsComboboxControlFrame
nsFormControlFrame
nsHTMLButtonControlFrame
nsFileControlFrame
nsImageControlFrame
nsGfxButtonControlFrame
nsTextControlFrame
nsRootBoxFrame
nsDocElementBoxFrame
nsTableFrame

We should probably audit these for mistakes.
For example, nsTableFrame is empty which seems harmless but useless:
http://hg.mozilla.org/mozilla-central/annotate/aae004a3c5d9/layout/tables/nsTableFrame.cpp#l163
# grep -r 'public nsQueryFrame' .
./layout/svg/nsISVGGlyphFragmentNode.h:class nsISVGGlyphFragmentNode : public nsQueryFrame
./layout/svg/nsISVGChildFrame.h:class nsISVGChildFrame : public nsQueryFrame
./layout/generic/nsIPageSequenceFrame.h:class nsIPageSequenceFrame : public nsQueryFrame
./layout/generic/nsIObjectFrame.h:class nsIObjectFrame : public nsQueryFrame
./layout/generic/nsIScrollableFrame.h:class nsIScrollableFrame : public nsQueryFrame {
./layout/generic/nsIFrame.h:class nsIFrame : public nsQueryFrame
./layout/forms/nsIFormControlFrame.h:class nsIFormControlFrame : public nsQueryFrame
./layout/forms/nsISelectControlFrame.h:class nsISelectControlFrame : public nsQueryFrame
./layout/forms/nsIListControlFrame.h:class nsIListControlFrame : public nsQueryFrame
./layout/forms/nsIComboboxControlFrame.h:class nsIComboboxControlFrame : public nsQueryFrame

which means all frame classes that inherits those will have an
additional vtable pointer for nsQueryFrame per interface.  sigh.

class nsListControlFrame : public nsHTMLScrollFrame,
                           public nsIFormControlFrame, 
                           public nsIListControlFrame,
                           public nsISelectControlFrame

Poor nsListControlFrame, it will have 5!

$1 = (nsHTMLScrollFrame) {
  <nsHTMLScrollFrame> = {
    <nsContainerFrame> = {
      <nsSplittableFrame> = {
        <nsFrame> = {
          <nsBox> = {
            <nsIFrame> = {
              <nsQueryFrame> = {
                _vptr$nsQueryFrame = 0x7ffff6340ad0
              }, 
...
    <nsIScrollableFrame> = {
      <nsQueryFrame> = {
        _vptr$nsQueryFrame = 0x7ffff6341088
      }, <No data fields>}, 
...
  <nsIFormControlFrame> = {
    <nsQueryFrame> = {
      _vptr$nsQueryFrame = 0x7ffff65461c0
    }, <No data fields>}, 
  <nsIListControlFrame> = {
    <nsQueryFrame> = {
      _vptr$nsQueryFrame = 0x7ffff65462b0
    }, <No data fields>}, 
  <nsISelectControlFrame> = {
    <nsQueryFrame> = {
      _vptr$nsQueryFrame = 0x0
    }, <No data fields>}, 
...
Old version didn't compile on Windows due to the 'typename', let's try without
https://tbpl.mozilla.org/?tree=Try&rev=a63f7377ac75
Attachment #732170 - Attachment is obsolete: true
Attachment #732170 - Flags: review?(dholbert)
Attachment #732199 - Flags: review?(dholbert)
If you're doing a compile-time assertion, you might as well make it a static assertion, so that when compiling as C++11 (which Gecko does, I believe) you'll get an even better error message.  Like so, say:

  MOZ_STATIC_ASSERT((mozilla::IsSame<Dest, typename Dest::Has_NS_DECL_QUERYFRAME_TARGET>::value),
                    "Dest must declare itself as a queryframe target");

This'll turn into a static_assert(..., "Dest must...") in those builds and report a compile error with exactly that string as the error output.  Hard to beat that!

IsSame (mozilla/TypeTraits.h) looks like what you wanted for the original patch.  I think you might want IsBaseOf in some of the newer patches, but there's enough churn/traffic here I'm no longer certain what's actually wanted here.  :-)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #30)
> IsSame (mozilla/TypeTraits.h) looks like what you wanted for the original
> patch.  I think you might want IsBaseOf in some of the newer patches

I'm actually pretty sure we want to use IsSame() in part 2, as well. (Not IsBaseOf.)  There *is* converting to a base class going on, but the assertion is about "Does this particular base class 'class' explicitly declare itself as a QueryFrame target?" which involves checking whether 'class' and 'class:::Has_NS_DECL_QUERYFRAME_TARGET' are the same type.

So -- mats, the new patches look good, but could you look into using MOZ_STATIC_ASSERT / IsSame as Waldo suggests in comment 30, for parts 1 and 2? If that works, I'd prefer it, because it's much more human-readable to actually use assertions to assert stuff, and it also sounds like it'll produce saner output when it fails.
Status: NEW → ASSIGNED
Couple notes here:

1) http://mxr.mozilla.org/mozilla-central/source/layout/generic/frame-verify.js was designed to do static verification of this, except of course that we never got dehydra-based static analysis working. Oh well!

2) This is very similar to bug 514280. I think that we could use template specialization instead of an accessor method to do this, e.g.

template<class Target> struct FrameTypeInfo;

// specialize for each class
template<>
struct FrameTypeInfo<nsIMathMLFrame>
{
  static const nsQueryFrame::FrameIID kIID = nsQueryFrame::nsIMathMLFrame;
};

I'm not sure whether you could even do this all in nsQueryFrame.h with forward-declared classes... I'm going to try it.
Attached patch fix, v4Splinter Review
Changed the typecasts to MOZ_STATIC_ASSERT of mozilla::IsSame, as requested.
And yes, the error messages looks nicer.

I folded part 2 into the first patch so you can see all nsQueryFrame.h
changes in the same patch.  (only nsQueryFrame.h needs a new review)
Attachment #732103 - Attachment is obsolete: true
Attachment #732199 - Attachment is obsolete: true
Attachment #732199 - Flags: review?(dholbert)
Attachment #732516 - Flags: review?(dholbert)
Comment on attachment 732516 [details] [diff] [review]
fix, v4

Looks good!

When landing, you should either:
 a) fold "part 3" (the nsIMathMLFrame tweak) into the main patch
...OR...
 b) land "part 3" first (*before* the main patch)

Otherwise, you'll be introducing an intermediate changeset that doesn't build successfully (due to the failing MOZ_STATIC_ASSERT), which is undesirable from the perspective of anyone who uses 'hg bisect' to track down some unrelated issue and happens to hit your intermediate changeset and can't build.

I'd lean towards just folding it all together, since you've already got the other random-frameclass-fixups as being part of the main fix, and the nsIMathMLFrame tweak isn't fundamentally any different from those.
Attachment #732516 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c94dce47533
Flags: in-testsuite-
Target Milestone: --- → mozilla23
Attachment #732172 - Flags: review?(dholbert)
(In reply to Benjamin Smedberg from comment #32)
> 2) This is very similar to bug 514280. I think that we could use template
> specialization instead of an accessor method to do this
Just for completeness, this is how the patch would have looked.

Obviously this approach found the same compile errors.
(In reply to Mats Palmgren [:mats] from comment #35)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0c94dce47533

https://hg.mozilla.org/mozilla-central/rev/0c94dce47533
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [adv-main23-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.