The Artima Developer Community
Sponsored Link

Weblogs Forum
Check that Data!

3 replies on 1 page. Most recent reply: May 13, 2003 10:20 AM by Ken Arnold

Welcome Guest
  Sign In

Go back to the topic listing  Back to Topic List Click to reply to this topic  Reply to this Topic Click to search messages in this forum  Search Forum Click for a threaded view of the topic  Threaded View   
Previous Topic   Next Topic
Flat View: This topic has 3 replies on 1 page
Matt Bauer

Posts: 7
Nickname: mbauer
Registered: Mar, 2003

Check that Data! (View in Weblogs)
Posted: Apr 25, 2003 12:16 AM
Reply to this message Reply
Summary
Serialization is a powerful feature of the Java language, but failure to use it properly can cost you.
Advertisement

Pop quiz: how do you set i to -1?

class Bar implements Serializable {
    private int value;

    public Bar(int value) {
        if (value < 0) {
            throw new IllegalArgumentException();
        }
        this.value = value;
    }

    public int getValue() { return value; }
}

Answer:

public class Foo {

    public static void main(String[] args) throws Exception {
        byte[] badBytes = {
            (byte)0xac, (byte)0xed, 0x00, 0x05, 0x73, 0x72, 0x00, 0x03,
            0x42, 0x61, 0x72, (byte)0xeb, (byte)0xd4, (byte)0xdf, 
            (byte)0xb8, 0x54, (byte)0x87, 0x0d, (byte)0xe0, 0x02, 0x00,
            0x01, 0x49, 0x00, 0x05, 0x76, 0x61, 0x6c, 0x75, 0x65,
            0x78, 0x70, (byte)0xff, (byte)0xff, (byte)0xff, (byte)0xff
        };

        ByteArrayInputStream bis = new ByteArrayInputStream(badBytes);
        ObjectInputStream ois = new ObjectInputStream(bis);
        Bar badBar = (Bar)ois.readObject();

        assert (badBar.getValue() >= 0); // will fail
    }
}

Serialization is a significant reason for Java's success. It's a key component of RMI, which is a key component of modern day application servers. It's also a prime target for malicious attacks. More on that later, what happened above?

When an object is marked Serializable but doesn't implement readObject the default is used. As you see above it does no checking of the data it reads. It just reads a field and sets it. A proper version of Bar is this:

class Bar implements Serializable {
    private int value;

    public Bar(int value) {
        if (value < 0) {
            throw new IllegalArgumentException();
        }
        this.value = value;
    }

    public int getValue() { return value; }

    private synchronized void readObject(ObjectInputStream ois)
        throws IOException, ClassNotFoundException {
	ois.defaultReadObject();
        if (value < 0) {
            throw new IllegalStateException();
        }
    }
}

Using this version, no assumptions about the validity of the data are made. Data is checked when the object is created, and it's checked when read in from a stream. All paths to set the value are checked. Making an object Serializable is not just a matter of adding implements Serializable. You should implement writeObject for anything more than a simple object to ensure future compatibilty of the object and to cut down on the size of what is serialized. You definitely need to implement readObject to validate your data. See Effective Java for more information.

What's the problem if you don't? Let's take a customer service representative at a bank. Everyday he or she helps people remove fraudulent charges from their credit card. The application uses the following class to communicate from the client to the application server.

class AccountChangeOrder implements Serializable {
    private String accountId;
    private double balance;

    public AccountChangeOrder(String accountId, double balance) {
        setAccountId(accountId);
        setBalance(balance);
    }

    private void setAccountId(String accountId) {
        if (accountId != null) {
            throw new NullPointerException();
        }
        if (accountId.length() < 0) {
            throw new IllegalArgumentException();
        }
        this.accountId = accountId;
    }

    public String getAccountId() { return accountId; }

    private void setBalance(double balance) {
        if (balance < 0) {
            throw new IllegalArgumentException();
        }
        this.balance = balance;
    }

    public double getBalance() { return balance; }
}

Notice that the intent of the class is to disallow negative balances so the bank never owes the customer money. There isn't a readObject method however. The customer service representative could set any account balance to say $-1000 with just a few bad bytes. While this is a simplistic example, it shows what can happen.

Java may run in a sandbox, but it does no good when the piggy bank is within arms reach. Just like compiled languages, not checking input values is a huge security concern. So remember, any time you have a method, constructor or stream that sets an instance variable, make a defensive copy and then check its validity. Think of it this way - when someone knocks on your door, do you check to see who it is or do you just let him or her in?


Ryan Shriver

Posts: 9
Nickname: rshriver
Registered: Oct, 2002

Re: Check that Data! Posted: Apr 30, 2003 8:55 AM
Reply to this message Reply
Good article, thanks. BTW, shouldn't this:


if (accountId != null) {
throw new NullPointerException();
}


be this:


if (accountId == null) {
throw new NullPointerException();
}


I'd argue to throw an IllegalArgumentException rather than a NullPointerException because the NullPointerException would have been thrown anyway the first time a method was invoked on a null accountId. Throwing an empty NullPointerException doesn't buy much.

-ryan

Matt Bauer

Posts: 7
Nickname: mbauer
Registered: Mar, 2003

Re: Check that Data! Posted: May 1, 2003 8:58 AM
Reply to this message Reply
I guess that's what happens when you focus more on english than the code. Your right about the account == null bit, points for you! I put the NullPointerException in the method to emphasize the method contract. By including the check I explicitly convey to the user my intend of the arguments. That is, null is not allowed here. You could do the same with a comment block, but I feel this is more explicit. For the sake of the post I didn't include any data in the exception. In my own code I pass in more detail.

Matt

Ken Arnold

Posts: 27
Nickname: arnold
Registered: Apr, 2003

Re: Check that Data! Posted: May 13, 2003 10:20 AM
Reply to this message Reply
Isn't the real problem that whoever processes the AccountChangeOrder object doesn't check for validity? The change order is presumably a request that must be processed.

In general, if you are concerned about fraudulent serialization bits, you must be concerned about much more than sanity checks. Setting my balance to $10,000,000 is at least as significant a problem as setting it to -$1,000, isn't it?

To protect against attack what you really need is some notion of securing the stream of incoming requests, possibly by signing each order so you can validate that it comes from a trusted source and hasn't been tampered with in transit. That would seem much more valuable than writing a bunch of readObject methods that will only check for certain sanities.

Flat View: This topic has 3 replies on 1 page
Topic: Why is Poorly Documented Code the Industry Standard? Previous Topic   Next Topic Topic: SARS virus positively identified

Sponsored Links



Google
  Web Artima.com   

Copyright © 1996-2019 Artima, Inc. All Rights Reserved. - Privacy Policy - Terms of Use