code to handle FasterXML/jackson-databind/issues/2141 #84

abracadv8 · 2018-10-02T00:42:18Z

Guards against numbers causing CPU or OOM issues when deserializing
large numbers into Instant or Duration (see FasterXML/jackson-databind#2141
) by either:

Scientific notation too large (eg 10000e100000)
Raw string repesenting a number of length too long

toddjonker · 2018-10-02T18:00:36Z

datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/DurationDeserializer.java

+                    NumberFormat formatter = new DecimalFormat("0.0E0");
+                    throw new JsonParseException(context.getParser(),
+                            String.format("Value of BigDecimal (%s) not within range to be converted to Duration",
+                                    formatter.format(value)));


Could this use string rather than reformatting the parsed value?

If the string is a 1,000,000 characters long, I would assume printing it might be more trouble than what its worth, -- eg the test case where I append "1000000000" a thousand times would end up with a 1mb long exception message.

I can just not print the string and just say its not within the expected range.

I think it'd be better to just include length of String as presumably this should only be hit for attack, or some other use case in which input is not legitimate. So printing that value does not make sense.

Makes sense.

The DoS is not coupled to the length of the string, in fact the problem is that short input strings can cause the failure, by representing very large numbers.

toddjonker · 2018-10-02T18:07:16Z

Is there a potential for this same problem -- BigDecimal.longValue() taking unbounded time -- to affect other conversions? For example, what if the serialized decimal is being mapped to an Integer or long instead of an Instant or Duration?

yishaigalatzer · 2018-10-02T18:49:32Z

datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/DurationDeserializer.java

@@ -48,22 +58,35 @@ private DurationDeserializer()
    @Override
    public Duration deserialize(JsonParser parser, DeserializationContext context) throws IOException
    {
+        String string = parser.getText().trim();


this will allocate twice as much than before. I would keep the string inside the switch statement, or alternatively see if there is a way to get the length from the parser without allocating a new string

I can try getTextLength() -- I don't know what the implementation of that is however (eg, if it allocates memory or not). I'll have to look around later today.

I've removed this code entirely.

yishaigalatzer · 2018-10-02T18:51:21Z

datetime/src/test/java/com/fasterxml/jackson/datatype/jsr310/TestInstantSerialization.java

+    public void testDeserializationWithTypeInfoAndStringTooLarge03() throws Exception
+    {
+        // Build a big string by hand
+        String number = "0000000000000000000000000";


would using an actual number rather than 0 be more resilient to future code changes?

I can, but how many zeroes would be reasonable 100?
I believe the current java Instant has a max length of ~30ish digits.

It is an actual number, however, its a number "1" followed by 1_000 x "00000000000" appended.

It definitely causes the JVM to hang without the parser.text.length() check above.

abracadv8 · 2018-10-02T20:40:01Z

@toddjonker -- Yes, I would assume the same thing would happen for other areas.

cowtowncoder · 2018-10-03T05:43:54Z

datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/DurationDeserializer.java

                long seconds = value.longValue();
                int nanoseconds = DecimalUtils.extractNanosecondDecimal(value, seconds);
                return Duration.ofSeconds(seconds, nanoseconds);

            case JsonTokenId.ID_NUMBER_INT:
+                if(string.length() > INSTANT_MAX_STRING_LEN) {


Is the attack also possible via BigInteger, or just BigDecimal? I am thinking that (as per earlier comments by others) for common case there would be unnecessary allocation of another String, for the common case of input being relatively short.
And for special case of binary formats there would also be conversion from binary number into String.

So the extra checks would mostly make sense if use of BigInteger is also similarly dangerous.

It's just BigDecimal, because BigDecimal.longValue() (and intValue() and scale() and some others) will consume unbounded resources even given short string representations of the decimal value. It's the conversion to an integer that's the problem.

The input-string length checks in this PR are unrelated to the reported DoS vector. IMO they should be submitted and considered separately, because they are muddying the analysis here by conflating independent concerns.

I have removed all the string length checking entirely and it only contains the code to handle exponential numbers.

abracadv8 · 2018-10-03T12:40:53Z

QQ @cowtowncoder - Is there a specific form / CLA to fill out for this contribution?

Guards against numbers causing CPU or OOM issues when deserializing large exponential numbers into Instant or Duration by either: - Scientific notation too large (eg 10000e100000)

raganhan · 2018-10-03T23:16:02Z

Please take a look at my comment on FasterXML/jackson-databind#2141 (comment)

Bound checking is necessary but not sufficient as the problem also happens for large negative exponents, e.g. new BigDecimal("1e-100000000")

cowtowncoder · 2018-10-04T04:48:04Z

@abracadv8 Basic Jackson CLA from https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf works, if you haven't sent one (and are not covered by one of existing Corporate CLAs we have received)

arteam · 2018-10-04T10:06:23Z

There is a test for 1e-100000000000, but there is no test for 1e-1000000000, which is high enough not to be considered as zero.

arteam · 2018-10-04T10:28:24Z

I think it should possible to guard against small numbers by checking the scale. _fromDecimal will be invoked only in case the client passes a decimal value where the fraction part represents nanoseconds (like 1538326357.298509112). Only the first 9 digits are used for that, so, practically speaking, we can reject all numbers with the scale more than 9.

abracadv8 · 2018-10-04T13:13:19Z

@abracadv8 Basic Jackson CLA from https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf works, if you haven't sent one (and are not covered by one of existing Corporate CLAs we have received)

Sent.

I'm contributing this under my personal time/resources, but I'm going to submit it to my company and fill out a second if they approve.

cowtowncoder · 2018-10-05T05:21:47Z

@arteam That's what I am thinking -- magnitude of scale could be limited to, say, max 50 (that is, -50 <= scale <= 50) and that would cover ridiculously big/small values already, yet pose no significant DoS capability.

Also, on related note, similar problem affects DecimalNode (when reading JSON floating-point numbers as BigDecimal for tree model), its longValue() (and intValue()). And possibly JsonParser.getLongValue() similarly. Would probably make sense to introduce a method in NumberUtil (or one there for streaming API, and similar util class in databind, so as not to add new tighter version dependency).

cowtowncoder · 2018-10-05T05:23:41Z

One other question: since we should be able to detect "bigger than max" and "smaller than min" values for conversion cases, should we:

Throw exception indicating overflow for "too big"
Silently coerce it into applicable MAX_VALUE

For underflow ("too small"), I assume we could just coerce it to 0.0? I can see argument supporting both (1) and (2).

yishaigalatzer · 2018-10-08T16:49:12Z

@cowtowncoder just my take:

I would write a test that validates the behavior of inputs just above "too big", and just below "too small" prior to the change. Run these tests with the existing code prior to the change, and then make the fix to keep the same behavior, thus preventing unexpected behavior changes, regardless what is the more correct behavior you come up with in a clean design.

Why? This library is already super popular, and it is hard to tell how existing code depends on that edge case. Making a change in behavior in a minor version is not justified.

toddjonker · 2018-10-08T17:56:19Z

datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/DurationDeserializer.java

@@ -52,6 +58,13 @@ public Duration deserialize(JsonParser parser, DeserializationContext context) t
        {
            case JsonTokenId.ID_NUMBER_FLOAT:
                BigDecimal value = parser.getDecimalValue();
+                // If the decimal isnt within the bounds of a float, bail out


Shouldn't "float" be "Instant"? Also, misspelling of "isn't".

~~Also might be worth pointing out in comment that Duration and Instant share the same range.~~ UPDATE: they do not.

Changed to // If the decimal isn't within the bounds of a duration, bail out since a duration has different bounds than an instant.

toddjonker · 2018-10-09T01:01:12Z

datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/InstantDeserializer.java

+        // If the decimal isnt within the bounds of an Instant, bail out
+        if(value.compareTo(INSTANT_MAX) > 0 ||
+                value.compareTo(INSTANT_MIN) < 0) {
+            throw new JsonParseException(context.getParser(),


Today, this method throws DateTimeException when the given value is out of bounds, so we should throw that here, else this is a backwards-incompatible change.

This test case:

@Test // (expected = DateTimeException.class) // Disabled to provoke failure public void testDeserializationAsFloat04() throws Exception { Instant date = Instant.MAX; String customInstant = (date.getEpochSecond() + 1) + ".0"; MAPPER.readValue(customInstant, Instant.class); }

results in:

java.time.DateTimeException: Instant exceeds minimum or maximum instant at java.time.Instant.create(Instant.java:411) at java.time.Instant.ofEpochSecond(Instant.java:330) at com.fasterxml.jackson.datatype.jsr310.deser.InstantDeserializer.lambda$static$1(InstantDeserializer.java:66) at com.fasterxml.jackson.datatype.jsr310.deser.InstantDeserializer._fromDecimal(InstantDeserializer.java:284) at com.fasterxml.jackson.datatype.jsr310.deser.InstantDeserializer.deserialize(InstantDeserializer.java:171) at com.fasterxml.jackson.datatype.jsr310.deser.InstantDeserializer.deserialize(InstantDeserializer.java:50) at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4013) at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3004) at com.fasterxml.jackson.datatype.jsr310.TestInstantSerialization.testDeserializationAsFloat04(TestInstantSerialization.java:244)

I swapped the checks to throw throw new DateTimeException("Instant exceeds minimum or maximum instant");
and throw new DateTimeException("Instant exceeds minimum or maximum Duration"); respectively.

I updated the bounds for duration to be the following, although I am not 100% certain it is correct with regards to minimum:

private static final BigDecimal DURATION_MAX = new BigDecimal( Long.MAX_VALUE + "." + java.time.Instant.MAX.getNano()); private static final BigDecimal DURATION_MIN = new BigDecimal( Long.MIN_VALUE + "." + java.time.Instant.MIN.getNano());

toddjonker · 2018-10-09T01:19:42Z

datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/DurationDeserializer.java

+                if(value.compareTo(INSTANT_MAX) > 0 ||
+                        value.compareTo(INSTANT_MIN) < 0) {
+                    throw new JsonParseException(context.getParser(),
+                            "Value of Float too large to be converted to Duration");


Turns out this isn't the correct boundary: Duration can hold a number-of-seconds up to Long.MAX_VALUE, which is larger than Instant.MAX.getEpochSecond().

For larger values, Jackson currently has the troubling behavior of BigDecimal.longValue(). This test passes:

@Test public void testDeserializationAsFloat05() throws Exception { String customInstant = new BigInteger(Long.toString(Long.MAX_VALUE)).add(BigInteger.ONE) + ".0"; Duration value = READER.without(DeserializationFeature.READ_DATE_TIMESTAMPS_AS_NANOSECONDS) .readValue(customInstant); assertEquals(Long.MIN_VALUE, value.getSeconds()); // The Duration is negative! }

For reference on behavior of similar overflow, Duration.of(Long.MAX_VALUE, ChronoUnit.MINUTES) throws:

java.lang.ArithmeticException: long overflow

while Duration.parse("PT" + Long.MAX_VALUE + "0S") -- note the added 0 to force overflow -- throws:

java.time.format.DateTimeParseException: Text cannot be parsed to a Duration: seconds

Swapped for the following check, although I am not 100% sure about the correct min value:

private static final BigDecimal DURATION_MAX = new BigDecimal( Long.MAX_VALUE + "." + java.time.Instant.MAX.getNano()); private static final BigDecimal DURATION_MIN = new BigDecimal( Long.MIN_VALUE + "." + java.time.Instant.MIN.getNano());

All the test cases, including yours above now throw new DateTimeException( "Instant exceeds minimum or maximum Duration")

…ing exception messages - Changed JsonParseException to DateTimeException where needed - Extended bounds of min and max Duration to be Long.MAX and Long.MIN - Adjusting DateTimeException message to match original for Instant - Adjusting DurationDeserializer DateTimeException message to something similar as Instance

toddjonker

I really like where this is going, but I think we should first add new unit tests proving the extant behavior in these edge cases (ignoring the performance problem). Then fix the performance problem. That makes it much easier to prove that we aren't changing the failure modes, just improving the performance.

toddjonker · 2018-10-09T22:24:54Z

datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/DurationDeserializer.java

@@ -40,6 +40,11 @@

    public static final DurationDeserializer INSTANCE = new DurationDeserializer();

+    private static final BigDecimal DURATION_MAX = new BigDecimal(
+            Long.MAX_VALUE + "." + java.time.Instant.MAX.getNano());


I'd write these bounds as (in effect) MAX+1 and MIN-1 which have the same safety-net effect to guard againstBigDecimal.longValue while letting Duration worry about the specific sub-second boundary.

(I'd rather not have to worry about, for example, how Duration deals with fractions with more decimal places than nanoseconds.)

toddjonker · 2018-10-09T22:36:45Z

datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/DurationDeserializer.java

+                if(value.compareTo(DURATION_MAX) > 0 ||
+                        value.compareTo(DURATION_MIN) < 0) {
+                    throw new DateTimeException(
+                            "Instant exceeds minimum or maximum Duration");


This changes the behavior, and I agree with @yishaigalatzer that we shouldn't change behavior while patching this problem.

It's safer simply guard against the unbounded-execution time by catching the cases where the long will be chopped to zero. I think that looks more or less like:

// Quickly handle zero || no low-order bits || < 1.0 if (value.signum() == 0 || value.scale() < -63 || value.precision() - value.scale() <= 0) { seconds = 0; } else { seconds = value.longValue(); }

That preserves the existing (though undesirable) behavior for large values with random low-order bits, and doesn't throw exceptions in new cases. As @cowtowncoder implied, that change requires careful design consideration, and shouldn't block this fix.

toddjonker · 2018-10-09T22:46:56Z

datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/InstantDeserializer.java

@@ -17,6 +17,7 @@
 package com.fasterxml.jackson.datatype.jsr310.deser;

 import com.fasterxml.jackson.annotation.JsonFormat;
+import com.fasterxml.jackson.core.JsonParseException;


Is this still needed? I think all three added imports are leftover from a prior revision.

toddjonker · 2018-10-09T23:01:01Z

datetime/src/test/java/com/fasterxml/jackson/datatype/jsr310/TestDurationDeserialization.java

@@ -53,28 +54,36 @@ public void testDeserializationAsFloat03() throws Exception
        assertEquals("The value is not correct.", Duration.ofSeconds(13498L, 8374), value);
    }

+    @Test
+    public void testDeserializationAsFloat04() throws Exception


This looks the same as the previous test case. Am I missing something?

toddjonker · 2018-10-10T02:18:03Z

I'm just submitted PR #85 adding a bunch of test cases around the boundary edges. I believe it's a superset of what's tested here.

I suggest we get that reviewed and pulled and then this change can layered on top of it. The fix in that case wouldn't need any test changes, expect to add some low timeouts to the (8) currently-slow test cases.

cowtowncoder · 2018-10-24T04:59:17Z

Ok, I merged @toddjonker's alternative, which I think truncates all out-of-bounds cases to zero value (that is, won't bother with upper-bound check for truncation or exception), which seems like
one way to deal with that.
Also created actual issue for this repo, #90, to use for release notes.

Will close this PR but we can continue discussion either on #90 comments, new issue, mailing list or gitter. I think there are still remaining potential issues that could be tackled for 2.9.8.

1Jesper1 · 2018-11-07T12:31:50Z

Is there already a CVE issued for this problem? When will 2.9.8 be released?

cowtowncoder · 2018-11-12T23:51:59Z

@1Jesper1 I haven't requested CVE, but if someone has or is aware of one, I can add that.
As to 2.9.8, as soon as I have time to work on couple of serialization gadgets so they can be included.
Lately I haven't had much time to work on Jackson (does not pay the bills), so this may take a while.
But hopefully before end of November.

abracadv8 mentioned this pull request Oct 2, 2018

Denial of service when parsing a big JSON number as Instant/ZonedDateTime/OffsetDateTime FasterXML/jackson-databind#2141

Closed

toddjonker reviewed Oct 2, 2018

View reviewed changes

yishaigalatzer reviewed Oct 2, 2018

View reviewed changes

cowtowncoder reviewed Oct 3, 2018

View reviewed changes

code to handle FasterXML/jackson-databind/issues/2141

cab47f5

Guards against numbers causing CPU or OOM issues when deserializing large exponential numbers into Instant or Duration by either: - Scientific notation too large (eg 10000e100000)

abracadv8 force-pushed the 2.9 branch from dd31cf3 to cab47f5 Compare October 3, 2018 22:39

instant deserialization ofr 1e-100000 test

6ac19f6

toddjonker reviewed Oct 8, 2018

View reviewed changes

toddjonker reviewed Oct 9, 2018

View reviewed changes

abracadv8 force-pushed the 2.9 branch from 49c7b6e to e58de98 Compare October 9, 2018 13:22

toddjonker reviewed Oct 9, 2018

View reviewed changes

toddjonker mentioned this pull request Oct 10, 2018

Test behavior of reading Instant and Duration from float. #85

Merged

cowtowncoder closed this Oct 24, 2018

code to handle FasterXML/jackson-databind/issues/2141 #84

code to handle FasterXML/jackson-databind/issues/2141 #84

Conversation

abracadv8 commented Oct 2, 2018 • edited by cowtowncoder Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddjonker commented Oct 2, 2018

Choose a reason for hiding this comment

abracadv8 Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abracadv8 Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

abracadv8 commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abracadv8 commented Oct 3, 2018

raganhan commented Oct 3, 2018

cowtowncoder commented Oct 4, 2018

arteam commented Oct 4, 2018

arteam commented Oct 4, 2018 • edited Loading

abracadv8 commented Oct 4, 2018

cowtowncoder commented Oct 5, 2018

cowtowncoder commented Oct 5, 2018

yishaigalatzer commented Oct 8, 2018

toddjonker Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddjonker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddjonker commented Oct 10, 2018

cowtowncoder commented Oct 24, 2018

1Jesper1 commented Nov 7, 2018

cowtowncoder commented Nov 12, 2018

abracadv8 commented Oct 2, 2018 •

edited by cowtowncoder

Loading

abracadv8 Oct 2, 2018 •

edited

Loading

abracadv8 Oct 2, 2018 •

edited

Loading

arteam commented Oct 4, 2018 •

edited

Loading

toddjonker Oct 8, 2018 •

edited

Loading