C++: make BufferWrite changes backward compatible

Author: redsun82

cpp/ql/lib/semmle/code/cpp/commons/Printf.qll

@@ -10,6 +10,7 @@ private import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
 
 private newtype TBufferWriteEstimationReason =
+  TNoSpecifiedEstimateReason() or
   TTypeBoundsAnalysis() or
   TValueFlowAnalysis()
 
@@ -18,26 +19,50 @@ private newtype TBufferWriteEstimationReason =
  */
 abstract class BufferWriteEstimationReason extends TBufferWriteEstimationReason {
   /**
-   * Returns a human readable representation of this reason
+   * Returns the name of the concrete class
    */
   abstract string toString();
 
+  /**
+   * Returns a human readable representation of this reason
+   */
+  abstract string getDescription();
+
   /**
    * Combine estimate reasons. Used to give a reason for the size of a format string
    * conversion given reasons coming from its individual specifiers
    */
   abstract BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other);
 }
 
+/**
+ * No particular reason given. This is currently used for backward compatibility so that
+ * classes derived from BufferWrite and overriding getMaxData\0 still work with the 
+ * queries as intended
+ */
+class NoSpecifiedEstimateReason extends BufferWriteEstimationReason, TNoSpecifiedEstimateReason {
+  override string toString() { result = "NoSpecifiedEstimateReason" }
+
+  override string getDescription() { result = "no reason specified" }
+
+  override BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other) {
+    // this reason should not be used in format specifiers, so it should not be combined
+    // with other reasons
+    none()
+  }
+}
+
 /**
  * The estimation comes from rough bounds just based on the type (e.g.
  * `0 <= x < 2^32` for an unsigned 32 bit integer)
  */
 class TypeBoundsAnalysis extends BufferWriteEstimationReason, TTypeBoundsAnalysis {
-  override string toString() { result = "based on type bounds" }
+  override string toString() { result = "TypeBoundsAnalysis" }
+
+  override string getDescription() { result = "based on type bounds" }
 
   override BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other) {
-    result = TTypeBoundsAnalysis() and other = other
+    other != TNoSpecifiedEstimateReason() and result = TTypeBoundsAnalysis()
   }
 }
 
@@ -52,12 +77,12 @@ class TypeBoundsAnalysis extends BufferWriteEstimationReason, TTypeBoundsAnalysi
  * ```
  */
 class ValueFlowAnalysis extends BufferWriteEstimationReason, TValueFlowAnalysis {
-  override string toString() { result = "based on flow analysis of value bounds" }
+  override string toString() { result = "ValueFlowAnalysis" }
+
+  override string getDescription() { result = "based on flow analysis of value bounds" }
 
   override BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other) {
-    other = TTypeBoundsAnalysis() and result = TTypeBoundsAnalysis()
-    or
-    other = TValueFlowAnalysis() and result = TValueFlowAnalysis()
+    other != TNoSpecifiedEstimateReason() and result = other
   }
 }
 

cpp/ql/lib/semmle/code/cpp/security/BufferWrite.qll

@@ -68,22 +68,26 @@ abstract class BufferWrite extends Expr {
   /**
    * Gets an upper bound to the amount of data that's being written (if one
    * can be found).
+   * DEPRECATED: getMaxData\1 should be used and overridden instead
    */
-  int getMaxData() { result = max(getMaxData(_)) }
+  deprecated int getMaxData() { none() }
 
   /**
    * Gets an upper bound to the amount of data that's being written (if one
-   * can be found), except that float to string conversions are assumed to be
-   * much smaller (8 bytes) than their true maximum length.  This can be
-   * helpful in determining the cause of a buffer overflow issue.
+   * can be found), specifying the reason for the estimation
    */
-  int getMaxDataLimited() { result = max(getMaxDataLimited(_)) }
+  int getMaxData(BufferWriteEstimationReason reason) {
+    reason instanceof NoSpecifiedEstimateReason and result = getMaxData()
+  }
 
   /**
    * Gets an upper bound to the amount of data that's being written (if one
-   * can be found), specifying the reason for the estimation
+   * can be found), except that float to string conversions are assumed to be
+   * much smaller (8 bytes) than their true maximum length.  This can be
+   * helpful in determining the cause of a buffer overflow issue.
+   * DEPRECATED: getMaxDataLimited\1 should be used and overridden instead
    */
-  int getMaxData(BufferWriteEstimationReason reason) { none() }
+  deprecated int getMaxDataLimited() { result = getMaxData() }
 
   /**
    * Gets an upper bound to the amount of data that's being written (if one
@@ -92,7 +96,9 @@ abstract class BufferWrite extends Expr {
    * than their true maximum length.  This can be helpful in determining the
    * cause of a buffer overflow issue.
    */
-  int getMaxDataLimited(BufferWriteEstimationReason reason) { result = getMaxData(reason) }
+  int getMaxDataLimited(BufferWriteEstimationReason reason) {
+    result = getMaxData(reason)
+  }
 
   /**
    * Gets the size of a single character of the type this
@@ -150,12 +156,16 @@ class StrCopyBW extends BufferWriteCall {
     result = this.getArgument(this.getParamSize()).getValue().toInt() * this.getCharSize()
   }
 
-  override int getMaxData(BufferWriteEstimationReason reason) {
+  private int getMaxDataImpl(BufferWriteEstimationReason reason) {
     // when result exists, it is an exact flow analysis
     reason instanceof ValueFlowAnalysis and
     result =
       this.getArgument(this.getParamSrc()).(AnalysedString).getMaxLength() * this.getCharSize()
   }
+
+  override int getMaxData(BufferWriteEstimationReason reason) { result = getMaxDataImpl(reason) }
+
+  deprecated override int getMaxData() { result = max(getMaxDataImpl(_)) }
 }
 
 /**
@@ -190,12 +200,20 @@ class StrCatBW extends BufferWriteCall {
     result = this.getArgument(this.getParamSize()).getValue().toInt() * this.getCharSize()
   }
 
-  override int getMaxData(BufferWriteEstimationReason reason) {
+  private int getMaxDataImpl(BufferWriteEstimationReason reason) {
     // when result exists, it is an exact flow analysis
     reason instanceof ValueFlowAnalysis and
     result =
       this.getArgument(this.getParamSrc()).(AnalysedString).getMaxLength() * this.getCharSize()
   }
+
+  override int getMaxData(BufferWriteEstimationReason reason) {
+    result = getMaxDataImpl(reason)
+  }
+
+  deprecated override int getMaxData() {
+    result = max(getMaxDataImpl(_))
+  }
 }
 
 /**
@@ -252,19 +270,27 @@ class SprintfBW extends BufferWriteCall {
 
   override Expr getDest() { result = this.getArgument(f.getOutputParameterIndex(false)) }
 
-  override int getMaxData(BufferWriteEstimationReason reason) {
+  private int getMaxDataImpl(BufferWriteEstimationReason reason) {
     exists(FormatLiteral fl |
       fl = this.(FormattingFunctionCall).getFormat() and
       result = fl.getMaxConvertedLengthWithReason(reason) * this.getCharSize()
     )
   }
 
-  override int getMaxDataLimited(BufferWriteEstimationReason reason) {
+  override int getMaxData(BufferWriteEstimationReason reason) { result = getMaxDataImpl(reason) }
+
+  deprecated override int getMaxData() { result = max(getMaxDataImpl(_)) }
+
+  private int getMaxDataLimitedImpl(BufferWriteEstimationReason reason) {
     exists(FormatLiteral fl |
       fl = this.(FormattingFunctionCall).getFormat() and
       result = fl.getMaxConvertedLengthLimitedWithReason(reason) * this.getCharSize()
     )
   }
+
+  override int getMaxDataLimited(BufferWriteEstimationReason reason) { result = getMaxDataLimitedImpl(reason) }
+
+  deprecated override int getMaxDataLimited() { result = max(getMaxDataLimitedImpl(_)) }
 }
 
 /**
@@ -355,19 +381,27 @@ class SnprintfBW extends BufferWriteCall {
     result = this.getArgument(this.getParamSize()).getValue().toInt() * this.getCharSize()
   }
 
-  override int getMaxData(BufferWriteEstimationReason reason) {
+  private int getMaxDataImpl(BufferWriteEstimationReason reason) {
     exists(FormatLiteral fl |
       fl = this.(FormattingFunctionCall).getFormat() and
       result = fl.getMaxConvertedLengthWithReason(reason) * this.getCharSize()
     )
   }
 
-  override int getMaxDataLimited(BufferWriteEstimationReason reason) {
+  override int getMaxData(BufferWriteEstimationReason reason) { result = getMaxDataImpl(reason) }
+
+  deprecated override int getMaxData() { result = max(getMaxDataImpl(_)) }
+
+  private int getMaxDataLimitedImpl(BufferWriteEstimationReason reason) {
     exists(FormatLiteral fl |
       fl = this.(FormattingFunctionCall).getFormat() and
       result = fl.getMaxConvertedLengthLimitedWithReason(reason) * this.getCharSize()
     )
   }
+
+  override int getMaxDataLimited(BufferWriteEstimationReason reason) { result = getMaxDataLimitedImpl(reason) }
+
+  deprecated override int getMaxDataLimited() { result = max(getMaxDataLimitedImpl(_)) }
 }
 
 /**
@@ -455,7 +489,7 @@ class ScanfBW extends BufferWrite {
 
   override Expr getDest() { result = this }
 
-  override int getMaxData(BufferWriteEstimationReason reason) {
+  private int getMaxDataImpl(BufferWriteEstimationReason reason) {
     // when this returns, it is based on exact flow analysis
     reason instanceof ValueFlowAnalysis and
     exists(ScanfFunctionCall fc, ScanfFormatLiteral fl, int arg |
@@ -465,6 +499,12 @@ class ScanfBW extends BufferWrite {
     )
   }
 
+  override int getMaxData(BufferWriteEstimationReason reason) {
+    result = getMaxDataImpl(reason)
+  }
+
+  deprecated override int getMaxData() { result = max(getMaxDataImpl(_)) }
+
   override string getBWDesc() {
     exists(FunctionCall fc |
       this = fc.getArgument(_) and
@@ -495,10 +535,14 @@ class RealpathBW extends BufferWriteCall {
 
   override Expr getASource() { result = this.getArgument(0) }
 
-  override int getMaxData(BufferWriteEstimationReason reason) {
+  private int getMaxDataImpl(BufferWriteEstimationReason reason) {
     // although there may be some unknown invariants guaranteeing that a real path is shorter than PATH_MAX, we can consider providing less than PATH_MAX a problem with high precision
     reason instanceof ValueFlowAnalysis and
     result = path_max() and
     this = this // Suppress a compiler warning
   }
+
+  override int getMaxData(BufferWriteEstimationReason reason) { result = getMaxDataImpl(reason) }
+
+  deprecated override int getMaxData() { result = max(getMaxDataImpl(_)) }
 }

cpp/ql/src/Security/CWE/CWE-120/OverrunWrite.ql

@@ -21,14 +21,15 @@ import semmle.code.cpp.commons.Alloc
  * See CWE-120/UnboundedWrite.ql for a summary of CWE-120 alert cases.
  */
 
-from BufferWrite bw, Expr dest, int destSize, BufferWriteEstimationReason reason
+from BufferWrite bw, Expr dest, int destSize, int estimated, BufferWriteEstimationReason reason
 where
   not bw.hasExplicitLimit() and // has no explicit size limit
   dest = bw.getDest() and
   destSize = getBufferSize(dest, _) and
+  estimated = bw.getMaxDataLimited(reason) and
   // we can deduce that too much data may be copied (even without
   // long '%f' conversions)
-  bw.getMaxDataLimited(reason) > destSize
+  estimated > destSize
 select bw,
-  "This '" + bw.getBWDesc() + "' operation requires " + bw.getMaxData() +
-    " bytes but the destination is only " + destSize + " bytes (" + reason.toString() + ")."
+  "This '" + bw.getBWDesc() + "' operation requires " + estimated +
+    " bytes but the destination is only " + destSize + " bytes (" + reason.getDescription() + ")."

cpp/ql/src/Security/CWE/CWE-120/OverrunWriteFloat.ql

@@ -21,14 +21,15 @@ import semmle.code.cpp.security.BufferWrite
  * See CWE-120/UnboundedWrite.ql for a summary of CWE-120 alert cases.
  */
 
-from BufferWrite bw, int destSize
+from BufferWrite bw, int destSize, int estimated, BufferWriteEstimationReason reason
 where
   not bw.hasExplicitLimit() and
   // has no explicit size limit
   destSize = getBufferSize(bw.getDest(), _) and
-  bw.getMaxData() > destSize and
+  estimated = bw.getMaxData(reason) and
+  estimated > destSize and
   // and we can deduce that too much data may be copied
-  bw.getMaxDataLimited() <= destSize // but it would fit without long '%f' conversions
+  bw.getMaxDataLimited(reason) <= destSize // but it would fit without long '%f' conversions
 select bw,
-  "This '" + bw.getBWDesc() + "' operation may require " + bw.getMaxData() +
+  "This '" + bw.getBWDesc() + "' operation may require " + estimated +
     " bytes because of float conversions, but the target is only " + destSize + " bytes."

cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql

@@ -44,7 +44,7 @@ import TaintedWithPath
 
 predicate isUnboundedWrite(BufferWrite bw) {
   not bw.hasExplicitLimit() and // has no explicit size limit
-  not exists(bw.getMaxData()) // and we can't deduce an upper bound to the amount copied
+  not exists(bw.getMaxData(_)) // and we can't deduce an upper bound to the amount copied
 }
 
 /*