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

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Jeff Walden [:Waldo]]

...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.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 3

[Andrew McCreight [:mccr8]]

It sounds like this bug isn't about a particular existing error, so I'm just marking this sec-want.

Comment 4

[Jeff Walden [:Waldo]]

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

Comment 5

[Joshua Cranmer [:jcranmer]]

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.

Comment 6

[(no longer active)]

Attachment: Part 1: Add an analysis that catches calls to certain functions involving arithmetic expressions

Comment 7

[(no longer active)]

Attachment: Part 2: Apply MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT to CheckedInt's constructor

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.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 9

[(no longer active)]

(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.

Comment 10

[(no longer active)]

Attachment: Part 1: Add an analysis that catches calls to certain functions involving arithmetic expressions; r=jrmuizel

Comment 11

[(no longer active)]

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.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 13

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 14

[(no longer active)]

OK, I will land without sec-approval then!  I didn't know sec-approval only applies to sec-high/critical.  Thanks.

Comment 15

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40768f723990
https://hg.mozilla.org/integration/mozilla-inbound/rev/acb4dd16755c

Comment 16

[(no longer active)]

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!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/29c50b824923

https://treeherder.mozilla.org/logviewer.html#?job_id=4771805&repo=mozilla-inbound

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 19

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a07116d74024
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b0bbb1d4f9

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a07116d74024
https://hg.mozilla.org/mozilla-central/rev/c9b0bbb1d4f9