How to enhance security in my membership site - Hacked

Question

I have a membership site where members are able to add credit using prepaid cards. The add credit page contain only one text field that enable members to enter the card number, then I pass the CardNumber parameter to a function to validate the card and top up the member account through a stored procedure on a MS SQL 2008 database.

A few days ago one attacker successfully used the card to add credit to his account without changing the card usage status and UserId. He was able to use the same card several times to add credit.

I wonder if he use Firebug or Fiddler software, but I know it's not affecting the stored procedure code.

Below is my stored procedure code:

ALTER procedure [dbo].[UseCard]
(@CardNumber varchar(10), @Used bit, @UserId bigint)
as
if (@CardNumber NOT in (SELECT CardNumber FROM CardsList)) and 
    RAISERROR('Card not found',16,1)
else if (@Used in (select Used from CardsList WHERE CardNumber = @CardNumber))
    RAISERROR('Card used before',16,1)
else
begin
    Update CardsList
    set Used = 1, UserId = @UserId
    where (CardNumber = @CardNumber)

    UPDATE [Users]
    SET UserBalance = UserBalance + (select Amount from CardsList where CardNumber = @CardNumber)
    WHERE (ID = @UserId)
end

and my function is

Public Sub UpdateCard(ByVal CardNumber As String, ByVal Used As Boolean)
        Dim Command As New SqlClient.SqlCommand
        With Command
            .CommandText = "UseCard"
            .CommandType = CommandType.StoredProcedure

            .Parameters.AddWithValue("@CardNumber", CardNumber)
.Parameters.AddWithValue("@Used", Used)
            .Parameters.AddWithValue("@UserId", UserId)

        End With

        DBProvider.ExecuteNonQuery(Command)

    End Sub

while my HTML code is:

<h3>Account TopUp</h3>

<telerik:RadAjaxLoadingPanel ID="RadAjaxLoadingPanel1" runat="server" 
    Skin="Web20">
</telerik:RadAjaxLoadingPanel>

<telerik:RadAjaxPanel ID="RadAjaxPanel1" runat="server" Height="200px" 
    Width="300px" HorizontalAlign="NotSet" LoadingPanelID="RadAjaxLoadingPanel1">

<asp:Label ID="lbresult" runat="server"></asp:Label>
    <label for="your_name">Card Number</label><br/>
        <asp:TextBox ID="txtCardNumber" runat="server" Width="200px"></asp:TextBox>
        <asp:RegularExpressionValidator Text=" * " ID="RegularExpressionValidator1" runat="server" ValidationExpression="[a-zA-Z0-9]{8}-[a-zA-Z0-9]{4}-[a-zA-Z0-9]{2}"
                            ControlToValidate="txtCardNumber" />
        <asp:RequiredFieldValidator ID="RequiredFieldValidator2" runat="server" 
                    ControlToValidate="txtCardNumber" ErrorMessage="*" 
                    Font-Bold="True"></asp:RequiredFieldValidator>
        <asp:Button ID="btnAddCredits" runat="server" Text="ADD" />
</telerik:RadAjaxPanel>

Answer 1

You should make sure your SP is inside a transaction, otherwise the query that checks whether the card has been used could be executed by two separate threads before the update.

This will protect your update of the UserBalance so it can only be updated once per card and also if anything fails after your CardsList update but before your UserBalance update, it will be rolled back.

e.g. imagine the following sequence of events with the same card

Request 1                            Request 2
Check card 123 has not been used
Card has not been used 
                                     Check card 123 has not been used
                                     Card has not been used 
Update card to mark as used
                                     Update card to mark as used
Update balance
                                     Update balance

As you can see, depending on the order of events that happen from concurrent requests, the update could happen multiple times for the same card.

A transaction will lock the database tables until the transaction is committed, so the sequence of events will now be:

Request 1                            Request 2
BEGIN TRANSACTION                   
Check card 123 has not been used
Card has not been used 
                                     BEGIN TRANSACTION
                                     Check card 123 has not been used  - SQL
                                         Server will
                                         suspend (pause) this thread as this 
                                         record
                                         has been
                                         locked by the other thread

Update card to mark as used

Update balance
COMMIT TRANSACTION                   Thread resumed
                                     Check card 123 has not been used - 
                                         it has so
                                         RAISERROR
                                     TRANSACTION aborted (rolled back, but as no
                                         changes were made it is simply marked as
                                         complete)

Also Used should also not be a parameter, just a variable within the SP.

    ALTER procedure [dbo].[UseCard]



(@CardNumber varchar (10) , @UserId bigint)

AS

DECLARE @Used bit;


SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
BEGIN TRANSACTION;

if (@CardNumber NOT in (SELECT CardNumber FROM CardsList)) and 
    RAISERROR('Card not found',16,1)

    else if (@Used in (select Used from CardsList WHERE CardNumber = @CardNumber))

    RAISERROR('Card used before',16,1)
    else

begin
Update CardsList

set

Used = 1 ,UserId = @UserId
where (CardNumber = @CardNumber)

UPDATE    [Users]
    SET
UserBalance = UserBalance + (select Amount from CardsList where CardNumber = @CardNumber)

    WHERE     (ID = @UserId)
end
COMMIT TRANSACTION;

Answer 2

If I pass in 0 for the @Used parameter of your stored procedure, I can reuse the same card over and over again.

I would start there - if the @Used parameter is exposed to the web, then you have an SQL Injection vulnerability.