Skip to content

fix range checks bypassed by wraparound in number converters #439

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rootvector2 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix range checks bypassed by wraparound in number converters #439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 57 additions & 31 deletions src/main/java/org/apache/commons/beanutils2/converters/NumberConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,59 @@ public void setUseLocaleFormat(final boolean useLocaleFormat) {
this.useLocaleFormat = useLocaleFormat;
}

/**
* Converts a {@code Number} to a {@code long}, validating that its whole part is within the specified range.
* <p>
* The range test must not be performed on a narrowed copy of the value: {@code longValue()} of a {@link BigInteger} or {@link BigDecimal} keeps only the
* low-order 64 bits and a {@code double} cannot represent every {@code long}, so an out-of-range value can wrap or round into range before a {@code long}
* or {@code double} based bounds check sees it. These types are therefore compared as {@link BigDecimal}.
*
* @param sourceType The type being converted from
* @param targetType The Number type to convert to
* @param value The Number to convert.
* @param min The smallest value of the target type
* @param max The largest value of the target type
* @return The value as a {@code long}, with any fractional part discarded.
* @throws ConversionException if the value is outside the specified range.
*/
private long toLong(final Class<?> sourceType, final Class<?> targetType, final Number value, final long min, final long max) {
BigDecimal decimalValue = null;
if (value instanceof BigDecimal) {
decimalValue = (BigDecimal) value;
} else if (value instanceof BigInteger) {
decimalValue = new BigDecimal((BigInteger) value);
} else if (value instanceof Float || value instanceof Double) {
final double doubleValue = value.doubleValue();
if (doubleValue == Double.POSITIVE_INFINITY) {
throw ConversionException.format("%s value '%s' is too large for %s", toString(sourceType), value, toString(targetType));
}
if (doubleValue == Double.NEGATIVE_INFINITY) {
throw ConversionException.format("%s value '%s' is too small %s", toString(sourceType), value, toString(targetType));
}
if (!Double.isNaN(doubleValue)) {
decimalValue = new BigDecimal(doubleValue);
}
}
if (decimalValue != null) {
// Values whose whole part truncates into range stay accepted, so compare against min - 1 and max + 1.
if (decimalValue.compareTo(BigDecimal.valueOf(max).add(BigDecimal.ONE)) >= 0) {
throw ConversionException.format("%s value '%s' is too large for %s", toString(sourceType), value, toString(targetType));
}
if (decimalValue.compareTo(BigDecimal.valueOf(min).subtract(BigDecimal.ONE)) <= 0) {
throw ConversionException.format("%s value '%s' is too small %s", toString(sourceType), value, toString(targetType));
}
return decimalValue.longValue();
}
final long longValue = value.longValue();
if (longValue > max) {
throw ConversionException.format("%s value '%s' is too large for %s", toString(sourceType), value, toString(targetType));
}
if (longValue < min) {
throw ConversionException.format("%s value '%s' is too small %s", toString(sourceType), value, toString(targetType));
}
return longValue;
}

/**
* Default String to Number conversion.
* <p>
Expand Down Expand Up @@ -413,49 +466,22 @@ private <T> T toNumber(final Class<?> sourceType, final Class<T> targetType, fin

// Byte
if (targetType.equals(Byte.class)) {
final long longValue = value.longValue();
if (longValue > Byte.MAX_VALUE) {
throw ConversionException.format("%s value '%s' is too large for %s", toString(sourceType), value, toString(targetType));
}
if (longValue < Byte.MIN_VALUE) {
throw ConversionException.format("%s value '%s' is too small %s", toString(sourceType), value, toString(targetType));
}
return targetType.cast(Byte.valueOf(value.byteValue()));
return targetType.cast(Byte.valueOf((byte) toLong(sourceType, targetType, value, Byte.MIN_VALUE, Byte.MAX_VALUE)));
}

// Short
if (targetType.equals(Short.class)) {
final long longValue = value.longValue();
if (longValue > Short.MAX_VALUE) {
throw ConversionException.format("%s value '%s' is too large for %s", toString(sourceType), value, toString(targetType));
}
if (longValue < Short.MIN_VALUE) {
throw ConversionException.format("%s value '%s' is too small %s", toString(sourceType), value, toString(targetType));
}
return targetType.cast(Short.valueOf(value.shortValue()));
return targetType.cast(Short.valueOf((short) toLong(sourceType, targetType, value, Short.MIN_VALUE, Short.MAX_VALUE)));
}

// Integer
if (targetType.equals(Integer.class)) {
final long longValue = value.longValue();
if (longValue > Integer.MAX_VALUE) {
throw ConversionException.format("%s value '%s' is too large for %s", toString(sourceType), value, toString(targetType));
}
if (longValue < Integer.MIN_VALUE) {
throw ConversionException.format("%s value '%s' is too small %s", toString(sourceType), value, toString(targetType));
}
return targetType.cast(Integer.valueOf(value.intValue()));
return targetType.cast(Integer.valueOf((int) toLong(sourceType, targetType, value, Integer.MIN_VALUE, Integer.MAX_VALUE)));
}

// Long
if (targetType.equals(Long.class)) {
if (value.doubleValue() > Long.MAX_VALUE) {
throw ConversionException.format("%s value '%s' is too large for %s", toString(sourceType), value, toString(targetType));
}
if (value.doubleValue() < Long.MIN_VALUE) {
throw ConversionException.format("%s value '%s' is too small %s", toString(sourceType), value, toString(targetType));
}
return targetType.cast(Long.valueOf(value.longValue()));
return targetType.cast(Long.valueOf(toLong(sourceType, targetType, value, Long.MIN_VALUE, Long.MAX_VALUE)));
}

// Float
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/apache/commons/beanutils2/locale/converters/ByteLocaleConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ private ByteLocaleConverter(final Byte defaultValue, final Locale locale, final
@Override
protected Byte parse(final Object value, final String pattern) throws ParseException {
final Number parsed = super.parse(value, pattern);
if (parsed.longValue() != parsed.byteValue()) {
throw new ConversionException("Supplied number is not of type Byte: " + parsed.longValue());
if (parsed.longValue() != parsed.byteValue() || !inRange(parsed, Byte.MIN_VALUE, Byte.MAX_VALUE)) {
throw new ConversionException("Supplied number is not of type Byte: " + parsed);
}
// now returns property Byte
return Byte.valueOf(checkInteger(parsed).byteValue());
Expand Down
32 changes: 32 additions & 0 deletions src/main/java/org/apache/commons/beanutils2/locale/converters/DecimalLocaleConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

package org.apache.commons.beanutils2.locale.converters;

import java.math.BigDecimal;
import java.math.BigInteger;
import java.text.DecimalFormat;
import java.text.NumberFormat;
import java.text.ParseException;
Expand Down Expand Up @@ -104,6 +106,36 @@ Number checkInteger(final Number number) {
return number;
}

/**
* Tests whether the whole part of the given number is within the specified range.
* <p>
* The test must not be performed on a narrowed copy of the value: {@code longValue()} of a {@link BigInteger} or {@link BigDecimal} keeps only the
* low-order 64 bits and a {@code double} cannot represent every {@code long}, so an out-of-range value can wrap or round into range before a narrowed
* bounds check sees it. These types are therefore compared as {@link BigDecimal}; other types compare their {@code longValue()}.
*
* @param number The number to test.
* @param min The smallest value of the target type.
* @param max The largest value of the target type.
* @return {@code true} if the whole part of the number is within the range.
*/
boolean inRange(final Number number, final long min, final long max) {
BigDecimal decimalValue = null;
if (number instanceof BigDecimal) {
decimalValue = (BigDecimal) number;
} else if (number instanceof BigInteger) {
decimalValue = new BigDecimal((BigInteger) number);
} else if ((number instanceof Float || number instanceof Double) && Double.isFinite(number.doubleValue())) {
decimalValue = new BigDecimal(number.doubleValue());
}
if (decimalValue != null) {
// Values whose whole part truncates into range stay accepted, so compare against min - 1 and max + 1.
return decimalValue.compareTo(BigDecimal.valueOf(max).add(BigDecimal.ONE)) < 0
&& decimalValue.compareTo(BigDecimal.valueOf(min).subtract(BigDecimal.ONE)) > 0;
}
final long longValue = number.longValue();
return longValue <= max && longValue >= min;
}

/**
* Tests whether the underlying {@link DecimalFormat} should parse into a {@link java.math.BigDecimal} so that magnitude and precision are preserved.
* Subclasses that build {@link java.math.BigInteger} or {@link java.math.BigDecimal} values override this to return {@code true}; the narrowing converters
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/apache/commons/beanutils2/locale/converters/IntegerLocaleConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ private IntegerLocaleConverter(final Integer defaultValue, final Locale locale,
@Override
protected Integer parse(final Object value, final String pattern) throws ParseException {
final Number parsed = super.parse(value, pattern);
if (parsed.longValue() != parsed.intValue()) {
throw new ConversionException("Supplied number is not of type Integer: " + parsed.longValue());
if (parsed.longValue() != parsed.intValue() || !inRange(parsed, Integer.MIN_VALUE, Integer.MAX_VALUE)) {
throw new ConversionException("Supplied number is not of type Integer: " + parsed);
}
return Integer.valueOf(checkInteger(parsed).intValue()); // unlike superclass it will return proper Integer
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/apache/commons/beanutils2/locale/converters/LongLocaleConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ protected Long parse(final Object value, final String pattern) throws ParseExcep
return (Long) result;
}
final double doubleValue = result.doubleValue();
if (doubleValue < Long.MIN_VALUE || doubleValue > Long.MAX_VALUE) {
if (doubleValue < Long.MIN_VALUE || doubleValue > Long.MAX_VALUE || !inRange(result, Long.MIN_VALUE, Long.MAX_VALUE)) {
throw new ConversionException("Supplied number is not of type Long: " + result);
}
return Long.valueOf(checkInteger(result).longValue());
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/apache/commons/beanutils2/locale/converters/ShortLocaleConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ protected Short parse(final Object value, final String pattern) throws ParseExce
return (Short) result;
}
final Number parsed = (Number) result;
if (parsed.longValue() != parsed.shortValue()) {
throw new ConversionException("Supplied number is not of type Short: " + parsed.longValue());
if (parsed.longValue() != parsed.shortValue() || !inRange(parsed, Short.MIN_VALUE, Short.MAX_VALUE)) {
throw new ConversionException("Supplied number is not of type Short: " + parsed);
}
// now returns property Short
return Short.valueOf(checkInteger(parsed).shortValue());
Expand Down
17 changes: 17 additions & 0 deletions src/test/java/org/apache/commons/beanutils2/converters/ByteConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.math.BigDecimal;
import java.math.BigInteger;

import org.apache.commons.beanutils2.ConversionException;
import org.apache.commons.beanutils2.Converter;
import org.junit.jupiter.api.AfterEach;
Expand Down Expand Up @@ -83,6 +86,20 @@ void testInvalidAmount() {
assertThrows(ConversionException.class, () -> converter.convert(clazz, maxPlusOne), "More than maximum, expected ConversionException");
}

/**
* A {@link BigInteger} or {@link BigDecimal} beyond long range wraps to its low-order 64 bits in {@code longValue()}, so it can slip through a long-based
* bounds check and convert to an unrelated in-range value (2^64 + 5 converted to 5); it must be rejected.
*/
@Test
void testWrappedAmount() {
final Converter<Byte> converter = makeConverter();
final Class<Byte> clazz = Byte.class;
final BigInteger wrapped = BigInteger.ONE.shiftLeft(64).add(BigInteger.valueOf(5));
assertThrows(ConversionException.class, () -> converter.convert(clazz, wrapped), "2^64 + 5, expected ConversionException");
assertThrows(ConversionException.class, () -> converter.convert(clazz, new BigDecimal(wrapped)), "2^64 + 5, expected ConversionException");
assertThrows(ConversionException.class, () -> converter.convert(clazz, wrapped.negate()), "-(2^64 + 5), expected ConversionException");
}

@Test
void testSimpleConversion() throws Exception {
final String[] message = { "from String", "from String", "from String", "from String", "from String", "from String", "from String", "from Byte",
Expand Down
16 changes: 16 additions & 0 deletions src/test/java/org/apache/commons/beanutils2/converters/ByteLocaleConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@

package org.apache.commons.beanutils2.converters;

import static org.junit.jupiter.api.Assertions.assertThrows;

import java.math.BigInteger;

import org.apache.commons.beanutils2.ConversionException;
import org.apache.commons.beanutils2.locale.converters.ByteLocaleConverter;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -205,4 +210,15 @@ void testNonIntegerRejected() {
converter = ByteLocaleConverter.builder().setDefault(defaultValue).setLocale(defaultLocale).get();
convertValueNoPattern(converter, "non-integer", "5.5", defaultValue);
}

/**
* A {@link BigInteger} beyond long range wraps to its low-order 64 bits in {@code longValue()}, so it can slip through a long-based range check and
* convert to an unrelated in-range value (2^64 + 5 converted to 5); it must be rejected.
*/
@Test
void testWrappedOutOfRangeRejected() {
converter = ByteLocaleConverter.builder().setLocale(defaultLocale).get();
final BigInteger wrapped = BigInteger.ONE.shiftLeft(64).add(BigInteger.valueOf(5));
assertThrows(ConversionException.class, () -> converter.convert(wrapped), "2^64 + 5, expected ConversionException");
}
}
15 changes: 15 additions & 0 deletions src/test/java/org/apache/commons/beanutils2/converters/IntegerConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.math.BigInteger;

import org.apache.commons.beanutils2.ConversionException;
import org.apache.commons.beanutils2.Converter;
import org.junit.jupiter.api.AfterEach;
Expand Down Expand Up @@ -83,6 +85,19 @@ void testInvalidAmount() {
assertThrows(ConversionException.class, () -> converter.convert(clazz, maxPlusOne), "More than maximum, expected ConversionException");
}

/**
* A {@link BigInteger} beyond long range wraps to its low-order 64 bits in {@code longValue()}, so it can slip through a long-based bounds check and
* convert to an unrelated in-range value (2^64 + 5 converted to 5); it must be rejected.
*/
@Test
void testWrappedAmount() {
final Converter<Integer> converter = makeConverter();
final Class<?> clazz = Integer.class;
final BigInteger wrapped = BigInteger.ONE.shiftLeft(64).add(BigInteger.valueOf(5));
assertThrows(ConversionException.class, () -> converter.convert(clazz, wrapped), "2^64 + 5, expected ConversionException");
assertThrows(ConversionException.class, () -> converter.convert(clazz, wrapped.negate()), "-(2^64 + 5), expected ConversionException");
}

/**
* Tests whether an invalid default object causes an exception.
*/
Expand Down
15 changes: 15 additions & 0 deletions src/test/java/org/apache/commons/beanutils2/converters/IntegerLocaleConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@
package org.apache.commons.beanutils2.converters;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.math.BigInteger;

import org.apache.commons.beanutils2.ConversionException;
import org.apache.commons.beanutils2.locale.converters.IntegerLocaleConverter;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -201,6 +205,17 @@ void testNonIntegerRejected() {
convertValueNoPattern(converter, "non-integer", "5.5", defaultValue);
}

/**
* A {@link BigInteger} beyond long range wraps to its low-order 64 bits in {@code longValue()}, so it can slip through a long-based range check and
* convert to an unrelated in-range value (2^64 + 5 converted to 5); it must be rejected.
*/
@Test
void testWrappedOutOfRangeRejected() {
converter = IntegerLocaleConverter.builder().setLocale(defaultLocale).get();
final BigInteger wrapped = BigInteger.ONE.shiftLeft(64).add(BigInteger.valueOf(5));
assertThrows(ConversionException.class, () -> converter.convert(wrapped), "2^64 + 5, expected ConversionException");
}

/**
* Test Converting a number
*/
Expand Down
32 changes: 32 additions & 0 deletions src/test/java/org/apache/commons/beanutils2/converters/LongConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.math.BigDecimal;
import java.math.BigInteger;
import java.util.Locale;

Expand Down Expand Up @@ -93,6 +94,37 @@ void testLocaleStringOutOfRange() {
assertThrows(ConversionException.class, () -> converter.convert(Long.class, "99999999999999999999"), "More than maximum, expected ConversionException");
}

/**
* A locale-parsed String one past {@link Long#MAX_VALUE} comes back from {@link java.text.DecimalFormat} as the {@link Double} 2^63, which a double-based
* bounds check cannot distinguish from {@link Long#MAX_VALUE}; it must be rejected rather than clamped.
*/
@Test
void testLocaleStringOutOfRangeBoundary() {
final LongConverter converter = makeConverter();
converter.setLocale(Locale.US);
assertThrows(ConversionException.class, () -> converter.convert(Long.class, "9223372036854775808"), "One more than maximum, expected ConversionException");
}

/**
* Values just past the long range must not wrap or round into range before the bounds check sees them: {@code longValue()} of a {@link BigInteger} keeps
* only the low-order 64 bits (so 2^63 becomes {@link Long#MIN_VALUE}) and {@code doubleValue()} of 2^63 equals the double representation of
* {@link Long#MAX_VALUE}.
*/
@Test
void testOutOfRangeBoundary() {
final Converter<Long> converter = makeConverter();
final Class<?> clazz = Long.class;
final BigInteger maxPlusOne = BigInteger.valueOf(Long.MAX_VALUE).add(BigInteger.ONE);
final BigInteger minMinusOne = BigInteger.valueOf(Long.MIN_VALUE).subtract(BigInteger.ONE);
assertThrows(ConversionException.class, () -> converter.convert(clazz, maxPlusOne), "One more than maximum, expected ConversionException");
assertThrows(ConversionException.class, () -> converter.convert(clazz, minMinusOne), "One less than minimum, expected ConversionException");
assertThrows(ConversionException.class, () -> converter.convert(clazz, new BigDecimal(maxPlusOne)), "One more than maximum, expected ConversionException");
assertThrows(ConversionException.class, () -> converter.convert(clazz, Double.valueOf(9.223372036854775808E18)), "2^63, expected ConversionException");
// Boundaries still convert
assertEquals(Long.valueOf(Long.MAX_VALUE), converter.convert(clazz, BigInteger.valueOf(Long.MAX_VALUE)), "Maximum");
assertEquals(Long.valueOf(Long.MIN_VALUE), converter.convert(clazz, BigInteger.valueOf(Long.MIN_VALUE)), "Minimum");
}

@Test
void testSimpleConversion() throws Exception {
final String[] message = { "from String", "from String", "from String", "from String", "from String", "from String", "from String", "from Byte",
Expand Down
Loading