1

According to Effective Java Item 24 (Make defensive copies when needed) mutable objects pose a security risk, especially when passed as constructor arguments. One is encouraged to make defensive copies as necessary.

BigDecimal is meant to be immutable, but it is not final. According to Effective Java Item 15 (Minimise mutability), a class cannot be immutable unless it is final or all of its constructors are non-extendable.

To make matters worse, BigDecimal doesn't provide a copy constructor.

So, do BigDecimal arguments pose a security risk? Should one go through the painful process of invoking new BigDecimal(untrusted.toString())?

Gili
  • 86,244
  • 97
  • 390
  • 689

2 Answers2

1

According to https://stackoverflow.com/a/33463772/14731:

As discussed in Effective Java, Item 13, Favor Immutability, this was a design oversight when the class was written

According to https://stackoverflow.com/a/12600683/14731 BigDecimal does pose a security risk and defensive copies should be made as necessary.

Looks like the quickest way to create a defensive copy is:

public static BigDecimal copyOf(BigDecimal value)
{
    if (value == null || value.getClass() == BigDecimal.class)
        return value;
    return new BigDecimal(value.unscaledValue(), value.scale());
}

UPDATE: Apparently this is now explicitly discussed in Effective Java 3rd edition, Item 17 (Minimize Mutability).

Gili
  • 86,244
  • 97
  • 390
  • 689
  • Should `value.getClass() == BigDecimal.class` actually be `value.getClass() != BigDecimal.class`? I think you're meaning to defend against an class that extends BigDecimal, right? – Patrick M Nov 29 '19 at 22:38
  • @PatrickM No. If `value.getClass() == BigDecimal.class` then no extension took place and it is okay to return the existing instance; otherwise, we need to make a copy. – Gili Nov 30 '19 at 03:18
0

Something like this could also work:

BigDecimal newValue = oldValue == null ? null : oldValue.add(BigDecimal.ZERO);.

Abbas
  • 3,228
  • 1
  • 23
  • 27
  • I don't think this is safe. Either you invoke `oldValue.add(BigDecimal.ZERO)` which is unsafe because `oldValue` is untrusted or you invoke `BigDecimal.ZERO.add(oldValue)` but under the hood `add()` trusts the operand when it should not so... – Gili Jun 16 '21 at 03:54