Fix: Clip lat and lng to prevent DoS attacks using very large values

[lastzero]

Unclear why longitude should not be clipped, although existing test cases use values that are out of range (+-180). Normalize gets stuck for me with large numbers... 100% CPU load, not enough time to test for how long. My Go version is 1.13.4 (OS X and Linux on a MacBook).

Signed-off-by: Michael Mayer michael@liquidbytes.net

[lastzero]

Failing tests seem to be Ruby issues, not related to this fix - let me / us know if we should take any action. Thank you and Merry Christmas! 🎄

[zongweil]

Thanks for the PR @lastzero. This looks like you're actually changing the algorithmic behavior. Previously, a longitude of 380 would be normalized to a longitude of 20. Now it would simply be clipped to 180.

I do see the issue though - very large longitude values can take a long time to normalize.

@drinckes: Do we care about properly normalizing longitude values < -360 or >360? Any particular reason we clip the latitude and not the longitude?

[bocops]

Regarding clipping latitude vs. normalizing longitude, this makes some amount of sense.

I can start at a location at 170°E and then move 20° further to the east with the resulting location still being legal - and I probably would want the underlying math to be easy by just normalizing the result to fall into a [-180;180] range. I can not start at a location at 80°N and then move 20° further to the north, so clipping needs to happen in that case.

[lastzero]

I agree, but does this happen in practice with longitudes > 360? Business value of this feature is zero if it results in a vulnerable service.

[zongweil]

I think the right thing to do is to optimize the normalize method to calculate the new longitude in one step instead of a while loop. In psuedocode, something like:

if longitude < -180
    divisor = (int) longitude / -180
    longitude = longitude + (divisor * 360)
else if longitude > 180
    divisor = (int) longitude / 180
    longitude = longitude - (divisor * 360)
if longitude == 180
    longitude = -180

This preserves proper normalization behavior while handling large doubles quickly. @lastzero, would this address your concerns?

[lastzero]

Sure, if the test passes and doesn't hang :)

[lastzero]

Note that in practice it really makes sense to return "" (or an error) for suspiciously high input values because it's likely a mistake. The decimal separator can easily get lost when working with floats, that's how you would trigger this "bug" at the moment without bad intentions.

[lastzero]

This is my wrapper that also fixes the vulnerability for us:

func OlcEncode(lat, lng float64) string {
    if lat < -90 || lat > 90 {
        log.Warnf("olc: latitude out of range (%f)", lat)
        return ""
    }

    if lng < -180 || lng > 180 {
        log.Warnf("olc: longitude out of range (%f)", lng)
        return ""
    }

    return olc.Encode(lat, lng, OlcLength)
}

[drinckes]

We can't truncate longitude at -180/180 because it's common in geo systems to extend the range, for example, when defining a polygon that cross the antimeridian. Truncating at some other value (360 or 720) is arbitrary and unpredictable, so I think @zongweil's suggestion above is a good one.

[lastzero]

@drinckes Makes sense, in that case it must be handled on the application level exactly like we did. I'll close this as it seems you already have a solution. Might lead to a DoS vulnerability, so it was important for me to report. We switched to S2 in the meantime as it's a better fit for our specific use case.