What non-language specific security vulnerabilities are present in this code? //

Question

What non-language specific security vulnerabilities are present in this code?

//////////////////////////////////////////////////////////////////////////////////////////////////////

package org.owasp.webgoat.lessons;

import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.List;

/***************************************************************************************************
*This is a java code that performs a certain utility.
*To reduce code size some of the methods and souce codes to higher classes/dependencies have been deleted.
*Most methods used here are self explanatory.
****************************************************************************************************/
public class BlindSqlInjection extends LessonAdapter
{

     private final static String ACCT_NUM = "account_number";

   
     protected Element createContent(WebSession s)
     {
         ElementContainer ec = new ElementContainer();

         try
         {
             Connection connection = DatabaseUtilities.getConnection(s);

             ec.addElement(new P().addElement("Enter your Account Number: "));

             String accountNumber = s.getParser().getRawParameter(ACCT_NUM,"");
             Input input = new Input(Input.TEXT, ACCT_NUM, accountNumber.toString());
             ec.addElement(input);

             Element b = ECSFactory.makeButton("Go!");
             ec.addElement(b);
           //user_data : userid user_name SSN
             String query = "SELECT * FROM user_data WHERE userid = " + accountNumber;
           
             try
             {
                 Statement answer_statement = connection.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE,
                                                                         ResultSet.CONCUR_READ_ONLY);
                
                     Statement statement = connection.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE,
                                                                         ResultSet.CONCUR_READ_ONLY);
                     ResultSet results = statement.executeQuery(query);

                     if ((results != null) && (results.first() == true))
                     {
                         ec.addElement(new P().addElement("Account number is valid"));
                     }
                     else
                     {
                         ec.addElement(new P().addElement("Invalid account number"));
                     }
               
             } catch (SQLException sqle)
             {
                 ec.addElement(new P().addElement("An error occurred, please try again."));
             }
         } catch (Exception e)
         {
             s.setMessage("Error generating " + this.getClass().getName());
             e.printStackTrace();
         }

         return (ec);
     }
public void handleRequest(WebSession s)
     {
         try
         {
             super.handleRequest(s);
         } catch (Exception e)
         {
             //System.out.println("Exception caught: " + e);
             e.printStackTrace(System.out);
         }
     }
}

Explanation / Answer

The following are the vulnerabilities:
- Error messages are popping the name of class that is causing the Exception.
s.setMessage("Error generating " + this.getClass().getName());

- If an exception is caught, the code is printing the entire Stacktrace, this is something that needs to be avoided as it discloses the entire the process flow that goes into servicing the request.

e.printStackTrace(System.out);

- The code is doing all the work from getting the DB connection to forming the query to executing it and then going through the result set. This multifaceted role of this class could cause troubles in the any future if any code change happens. Different classes should be created to delegate all the above work.
- The most important security vulnerability is that the Account number is taken as a String. As we know Strings are immutable i.e. they would continue to be there in memory before the garbage collection process takes place.

private final static String ACCT_NUM = "account_number";

Related Questions

Navigate

• Browse (All)
• Browse W
• Subjects

Integrity-first tutoring: explanations and feedback only — we do not complete graded work. Learn more.