Inderjeet Singh's Blog: January 2007 Archive

Folks at FortifySoftware are running a program where they run their static analysis tool for code checking and security analysis for free on open-source projects. They were kind enough to run their tool on our Web 2.0 Pet Store application and report bugs to us. In this blog, I share my experiences with some of the subtle errors that their tool caught.

Some of the bugs that fortify reported were spurious because, presumably, the tool doesn't know about Java EE 5 annotations yet. For example, it failed to notice the following dependency injection:

@Resource UserTransaction utx;

The tool reported the above statement as uninitialized variable that may result in a null pointer exception or cause other errors. But since this code belongs to a Java EE managed component, the container will initialize the utx object correctly when the component is instantiated.

The next bug is something we all know academically, but sometimes run into because of oversight. This has to do with integer division when the two operands are roughly of the same magnitude. The result can contain significant value in the fraction, so it is a good idea to store it in a double. So, that is what the petstore code tried to do:

Line 1. int w = image.getWidth(); Line 2. int thumbWidth = thumb.getWidth(); Line 3: double powerW = thumbWidth / w;

However, there is a bug in the above code that fortify caught: In line 3, both thumbWidth and w are int, so the expression thumbWidth /w is an integer division with result converted to an int thereby losing any fraction value that might have been generated. The result is then promoted to a double so that it can be assigned to powerW, but it is a little too late.

The solution, of course, is to cast one of the elements to double before the division so that the operation becomes a double division operation. So, here is the corrected code:

Line 1. int w = image.getWidth(); Line 2. int thumbWidth = thumb.getWidth(); Line 3: double powerW = thumbWidth / (double) w;

Lesson re-learnt.

That brings us to the final type of bug that fortify reported. This relates to cross-side scripting attacks. Essentially, in the petstore we were using some user-submitted data without first validating it. Here is an example:

Line 1:String itemId=request.getParameter("itemId"); Line 2:out.print(itemId);



In this code, we are using the value of the user-submitted paramater itemId without validating it first. A malicious user, can insert escape characters in it that may result in a JavaScript injection attack. Similarly, if this value was being used as a database query parameter, it was amenable to a SQL injection attack. The correct fix is to validate the itemId parameter before using to ensure that it does not contain characters that we do not expect to be there. 

Needless to say that we have fixed most of these bugs in the petstore application, and intend to fix the remaining ones soon as well. What are your thoughts on a tool like Fortify? Have you run into similar silly bugs as well? Share your experiences as comments to this blog. Thanks for reading. 








Improving support for generics in the Java Persistence API

Posted by inder on January 03, 2007 at 11:42 AM | Permalink
| Comments (15)






The Java Persistence API comes in handy for creating object relational mapping. I recently came across a warnings that the compiler generates on some code that uses these APIs in our Web 2.0 Pet Store project. Upon a closer look, I concluded that the warning was bogus and came up with a suggestion for the Persistence API expert group to better support generics. Here is the warning in question: 


D:\ws\petstore\ws\apps\petstore\src\java\com\sun\javaee\blueprints\petstore\model\CatalogFacade.java:47: warning: [unchecked] unchecked conversion

found   : java.util.List

required: java.util.List<com.sun.javaee.blueprints.petstore.model.Category>
 

Here is the code that causes this warning to be generated: 

 
Query query = em.createQuery("SELECT c FROM Category c");

List<Category> categories = query.getResultList(); 


Essentially, the compiler is complaining that query.getResultList() returns just List whereas we are expecting it to be List<Categories>. This warning is unfortunate because we know that the query results are going to be list of Category objects, and we would like to express it somehow. Ideally, javax.persistence.Query should allow us to express this by providing an additional method: 

List<T> getResultList(Class<T> c);

This will enable us to write the following code: 

 
List<Category> categories = query.getResultList(Category.class); 

and the warning will get eliminated and we will have improved type-safety. 


I hope the Java Persistence expert group will look at this issue and add a generic version to ensure better type-safety. I hope they will look at other API methods as well to see if better support for generics can be added. 

What is your opinion on this matter? Do you have other suggestions for improving the Java Persistence API? Share your thoughts as comments to this blog. Thanks for reading. 










This work is licensed under a
Creative Commons License.








Powered by

Movable Type 3.01D

java.net RSS Feeds