Add a static analysis that makes CheckedInt<...>(expr BINOP expr) an error

RESOLVED FIXED in Firefox 37

Status

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: Waldo, Assigned: Ehsan)

Tracking

({sec-want})

Trunk
mozilla37
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox-esr31 wontfix)

Details

(Whiteboard: [adv-main37-])

Attachments

(2 attachments, 1 obsolete attachment)

I noticed in firebot checkin-spew that someone was misusing CheckedInt, passing in the result of an inline operation and thinking that that would be safe:

http://hg.mozilla.org/integration/mozilla-inbound/rev/fa4d8053055d

diff --git a/gfx/2d/Blur.cpp b/gfx/2d/Blur.cpp
--- a/gfx/2d/Blur.cpp
+++ b/gfx/2d/Blur.cpp
@@ -396,17 +396,17 @@ AlphaBoxBlur::AlphaBoxBlur(const Rect& a
                            float aSigma)
   : mRect(int32_t(aRect.x), int32_t(aRect.y),
           int32_t(aRect.width), int32_t(aRect.height)),
     mSpreadRadius(),
     mBlurRadius(CalculateBlurRadius(Point(aSigma, aSigma))),
     mStride(aStride),
     mSurfaceAllocationSize(-1)
 {
-  CheckedInt<int32_t> minDataSize = CheckedInt<int32_t>(aRect.width*aRect.height);
+  CheckedInt<int32_t> minDataSize = CheckedInt<int32_t>(aRect.width)*aRect.height;
   if (minDataSize.isValid()) {
     mSurfaceAllocationSize = minDataSize.value();
   }
 }
 
 
 AlphaBoxBlur::~AlphaBoxBlur()
 {

I can't particularly think of a good reason to ever allow CheckedInt's constructor to be called passing the result of a binary operation.  This seems like it'd be super-easy for static analysis to flag and reject.

I'm marking this security-sensitive for now as that bug's still in the process of being fixed.  We should open this up when that opens up -- or after writing this patch and determining that there are no similar mistakes of this sort elsewhere in the tree.
...the result of a binop where neither argument is a CheckedInt of the exact same type as the CheckedInt<...> being called, that is.  In other words, this shouldn't be flagged:

  return CheckedInt<int32_t>(CheckedInt<int32_t>(...) * 7);

Dunno if anyone does something like that, but it's not entirely inconceivable it might make sense in some places.
Notice that by itself we /can/ do such a check in plain C++, in the implementation of CheckedInt, by taking advantage of a quirk of C++: one cannot take a non-const reference to a temporary. So if CheckedInt constructors took T& instead of const T&, they would reject that. The problem is that they would then also reject a lot of valid usage patterns, so we can't actually do that.

So yes, doing this in static analysis would be the only feasible way --- and yes, that would be very useful.
Keywords: sec-want
It sounds like this bug isn't about a particular existing error, so I'm just marking this sec-want.
Interestingly, the Microsoft SafeInt developers flagged this (as it relates to the assignment operator, but the idea generalizes) as something they have some sort of static analysis to handle.

http://channel9.msdn.com/Shows/Going+Deep/David-LeBlanc-Inside-SafeInt
I know ehsan was asking me about static analyses I wanted to see, so I'm CC'ing him in case he has more time than I to implement this.
Note that the analysis currently just looks at the AST subtree of the
function call site and is therefore unable to correctly deal with cases
such as the last two hunks of the change to OggCodecState.cpp.  Fixing
the analysis to deal with that would be very difficult, so we currently
adjust the code so that it compiles.  The first hunk in that file though
is a real bug that this analysis found.
Attachment #8536299 - Flags: review?(jmuizelaar)
Assignee: nobody → ehsan.akhgari
Comment on attachment 8536298 [details] [diff] [review]
Part 1: Add an analysis that catches calls to certain functions involving arithmetic expressions

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

I'd prefer semantics that only looked at the direct descedented and rejected something like CheckedInt(static_cast<int>(x+y)) but Ehsan doesn't like that, and it probably doesn't matter that much in practice....

::: build/clang-plugin/clang-plugin.cpp
@@ +369,5 @@
> +}
> +
> +/// This matcher will match all arithmetic binary operators.
> +AST_MATCHER(BinaryOperator, binaryArithmeticOperator) {
> +  StringRef name = Node.getOpcodeStr();

Can't use getOpcode?

@@ +481,5 @@
> +                                        anyOf(hasDescendant(declRefExpr()), declRefExpr())))
> +              )).bind("node"))
> +          )
> +      )).bind("call"),
> +    &arithmeticArgChecker);

Are these two matchers different?
Attachment #8536298 - Flags: review?(jmuizelaar) → review+
Attachment #8536299 - Flags: review?(jmuizelaar) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> @@ +481,5 @@
> > +                                        anyOf(hasDescendant(declRefExpr()), declRefExpr())))
> > +              )).bind("node"))
> > +          )
> > +      )).bind("call"),
> > +    &arithmeticArgChecker);
> 
> Are these two matchers different?

Yes, one uses constructExpr and the other uses callExpr.
Attachment #8536298 - Attachment is obsolete: true
Comment on attachment 8537210 [details] [diff] [review]
Part 1: Add an analysis that catches calls to certain functions involving arithmetic expressions; r=jrmuizel

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not very easily, this analysis only found only one bad usage of CheckedInt on mozilla-central.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? If you know what you are looking for, yes.

Which older supported branches are affected by this flaw?  All, I think.

If not all supported branches, which bug introduced the flaw? Bug 674225 (adding opus support for the first time)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No, very easy.  But I don't think it's worth backporting the analysis itself.

How likely is this patch to cause regressions; how much testing does it need? It should be extremely safe.
Attachment #8537210 - Flags: sec-approval?
Comment on attachment 8536298 [details] [diff] [review]
Part 1: Add an analysis that catches calls to certain functions involving arithmetic expressions

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

I'd prefer semantics that only looked at the direct descedented and rejected something like CheckedInt(static_cast<int>(x+y)) but Ehsan doesn't like that, and it probably doesn't matter that much in practice....

::: build/clang-plugin/clang-plugin.cpp
@@ +369,5 @@
> +}
> +
> +/// This matcher will match all arithmetic binary operators.
> +AST_MATCHER(BinaryOperator, binaryArithmeticOperator) {
> +  StringRef name = Node.getOpcodeStr();

Can't use getOpcode?

@@ +481,5 @@
> +                                        anyOf(hasDescendant(declRefExpr()), declRefExpr())))
> +              )).bind("node"))
> +          )
> +      )).bind("call"),
> +    &arithmeticArgChecker);

Are these two matchers different?
This is marked as a "sec-want" for security. Is this mis-rated? If it really is just a "nice to have" thing, you don't need sec-approval to go in. That only applies to sec-high and sec-critical bugs.
Flags: needinfo?(ehsan.akhgari)
OK, I will land without sec-approval then!  I didn't know sec-approval only applies to sec-high/critical.  Thanks.
Flags: needinfo?(ehsan.akhgari)
Attachment #8537210 - Flags: sec-approval?
Comment on attachment 8536299 [details] [diff] [review]
Part 2: Apply MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT to CheckedInt's constructor

Chris, can you please look at the first hunk in OggCodecState.cpp and see if this analysis uncovered a real bug?  Thanks!
Attachment #8536299 - Flags: feedback?(cpearce)
Comment on attachment 8536299 [details] [diff] [review]
Part 2: Apply MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT to CheckedInt's constructor

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

This is a real bug; the addition could overflow. Both of the values we're doing arithmetic are read from a media file. However the return value of this function is only used in one benign place [1], and there's no way that we can be owned due to this, so I'm happy for this patch to ride the trains.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/ogg/OggReader.cpp#1941
Attachment #8536299 - Flags: feedback?(cpearce) → feedback+
https://hg.mozilla.org/mozilla-central/rev/a07116d74024
https://hg.mozilla.org/mozilla-central/rev/c9b0bbb1d4f9
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Whiteboard: [adv-main37-]
Group: core-security → core-security-release
Group: core-security-release
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.