go-openapi / validate

openapi toolkit validation helpers
Apache License 2.0
115 stars 53 forks source link

validate/values.go:272 Division by zero #150

Closed stasos24 closed 1 year ago

stasos24 commented 1 year ago

https://github.com/go-openapi/validate/blob/ae5ca9780532343cff7dc51f653621dbd9523f66/values.go#L272

if func MultipleOfInt(path, in string, data int64, factor int64) is called with factor = 0 You'll get division by zero at 272

liggitt commented 1 year ago

should all numbers return true for "multiple of 0", or should all numbers return false for "multiple of 0", or should MultipleOf==0 be considered an error?

stasos24 commented 1 year ago

should all numbers return true for "multiple of 0", or should all numbers return false for "multiple of 0", or should MultipleOf==0 be considered an error?

Return zero

liggitt commented 1 year ago

I'm not sure what you mean by that... this would protect against division by zero by treating all numbers as divisible by zero:

diff --git a/values.go b/values.go
index c88d35d..72afe80 100644
--- a/values.go
+++ b/values.go
@@ -251,6 +251,9 @@ func MultipleOf(path, in string, data, factor float64) *errors.Validation {
    if factor < 0 {
        return errors.MultipleOfMustBePositive(path, in, factor)
    }
+   if factor == 0 {
+       return nil
+   }
    var mult float64
    if factor < 1 {
        mult = 1 / factor * data
@@ -269,6 +272,9 @@ func MultipleOfInt(path, in string, data int64, factor int64) *errors.Validation
    if factor < 0 {
        return errors.MultipleOfMustBePositive(path, in, factor)
    }
+   if factor == 0 {
+       return nil
+   }
    mult := data / factor
    if mult*factor != data {
        return errors.NotMultipleOf(path, in, factor, data)
@@ -278,6 +284,9 @@ func MultipleOfInt(path, in string, data int64, factor int64) *errors.Validation

 // MultipleOfUint validates if the provided unsigned integer is a multiple of the factor
 func MultipleOfUint(path, in string, data, factor uint64) *errors.Validation {
+   if factor == 0 {
+       return nil
+   }
    mult := data / factor
    if mult*factor != data {
        return errors.NotMultipleOf(path, in, factor, data)

this would protect against it by requiring multipleOf to be positive:

diff --git a/values.go b/values.go
index c88d35d..e7ad8c1 100644
--- a/values.go
+++ b/values.go
@@ -248,7 +248,7 @@ func MinimumUint(path, in string, data, min uint64, exclusive bool) *errors.Vali
 // MultipleOf validates if the provided number is a multiple of the factor
 func MultipleOf(path, in string, data, factor float64) *errors.Validation {
    // multipleOf factor must be positive
-   if factor < 0 {
+   if factor <= 0 {
        return errors.MultipleOfMustBePositive(path, in, factor)
    }
    var mult float64
@@ -266,7 +266,7 @@ func MultipleOf(path, in string, data, factor float64) *errors.Validation {
 // MultipleOfInt validates if the provided integer is a multiple of the factor
 func MultipleOfInt(path, in string, data int64, factor int64) *errors.Validation {
    // multipleOf factor must be positive
-   if factor < 0 {
+   if factor <= 0 {
        return errors.MultipleOfMustBePositive(path, in, factor)
    }
    mult := data / factor
@@ -278,6 +278,10 @@ func MultipleOfInt(path, in string, data int64, factor int64) *errors.Validation

 // MultipleOfUint validates if the provided unsigned integer is a multiple of the factor
 func MultipleOfUint(path, in string, data, factor uint64) *errors.Validation {
+   // multipleOf factor must be positive
+   if factor == 0 {
+       return errors.MultipleOfMustBePositive(path, in, factor)
+   }
    mult := data / factor
    if mult*factor != data {
        return errors.NotMultipleOf(path, in, factor, data)
stasos24 commented 1 year ago

I am thinking first suggestion is more accurate

stasos24 commented 1 year ago

It's look like a question of math. But in terms of code security both variants are valid

liggitt commented 1 year ago

Opened https://github.com/go-openapi/validate/pull/151 to fix this

I made all versions return errors, since MultipleOf already returned an error when evaluating division by 0.0