[CLOSED] Security vulnerability: Javascript injection attack in grid editors

[Christof]

Hi,

I was surprised to see that I can type html tags into grid cell editors although my website has form violation turned on.

Lets say I modify your "array grid" example so that the company column becomes editable.

<ext:Column ColumnID="Company" Header="Company" Sortable="true" DataIndex="company">
    <Editor>
        <ext:TextField ID = "txtCompany" runat = "server" />
    </Editor>
</ext:Column>

Now I start editing the value and type the following into the editor:

<a href="http://www.ext.net">click me</a>

I was expecting that - once the editor closes - I get a http error 500 (dangerous form value).
Instead, the grid cell gets updated and now shows a hyperlink to your coolite website.
Is this really secure or could some bad guy exploit this and hack into our website?

Please advise.

[Vladimir]

Hi,

You should not get http error 500 because after editor hiding there is no request to the server. You will see that error when try to save grid (of course, if you don't set ValidateRequest="false")


Also you can set AutoEncode="true" for GridPanel

[geoffrey.mcgill]

Yes, this is really a server-side responsibility to ensure the data coming from the client can do no harm.

[Christof]

Is there a way I can make these editors communicate with the server so that validation happens?
I tried AjaxEvents like BeforeHide, BeforeDestroy. These don't seem to be executed at all.
Others like Blur are executed but after the control has been closed [and due to this its text is empty, so form validation does not complain].

[Vladimir]

Hi,

Use the following GridPanel event to validate cell input


validateedit : ( <code style="font-style: normal; font-weight: normal; font-family: 'Lucida Console', 'Courier New', Courier, monospace; font-size: 12px; ">Object e</code> )<div class="mdesc" style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 5px; padding-right: 0px; padding-bottom: 5px; padding-left: 0px; color: rgb(68, 68, 68); "><div class="long" style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; display: block; line-height: 18px; ">Fires after a cell is edited, but before the value is set in the record. Return false to cancel the change. The edit event object has the following properties
<ul style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 5px; padding-right: 5px; padding-bottom: 5px; padding-left: 16px; list-style-type: none; list-style-position: initial; list-style-image: initial; "><li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">grid - This grid<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">record - The record being edited<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">field - The field name being edited<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">value - The value being set<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">originalValue - The original value for the field, before the edit.<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">row - The grid row index<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">column - The grid column index<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">cancel - Set this to true to cancel the edit or return false from your handler.[/list]Usage example showing how to remove the red triangle (dirty record indicator) from some records (not all). By observing the grid's validateedit event, it can be cancelled if the edit occurs on a targeted row (for example) and then setting the field's new value in the Record directly:<code style="font-style: normal; font-weight: normal; font-size: 12px; color: rgb(0, 0, 0); line-height: 16px !important; font-family: 'Lucida Console', 'Courier New', Courier, monospace; ">grid.on(<em style="font-style: normal; font-weight: normal; color: rgb(0, 128, 128); background-color: rgb(238, 238, 238); ">'validateedit'[/i], <b style="font-weight: normal; color: rgb(128, 0, 128); ">function[/b](e) {
<b style="font-weight: normal; color: rgb(128, 0, 128); ">var[/b] myTargetRow = 6;

<b style="font-weight: normal; color: rgb(128, 0, 128); ">if[/b] (e.row == myTargetRow) {
e.cancel = true;
e.record.data[e.field] = e.value;
}
});</code><div class="mdetail-params" style="margin-top: 10px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 12px; font-size: 12px; "><strong style="font-style: normal; font-weight: normal; display: block; margin-bottom: 3px; font-size: 11px; color: rgb(85, 85, 85); ">Listeners will be called with the following arguments:[/b]<ul style="margin-top: 12px; margin-right: 12px; margin-bottom: 12px; margin-left: 12px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; list-style-type: circle; list-style-position: inside; list-style-image: initial; "><li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; list-style-position: inside; list-style-type: circle; list-style-image: initial; "><code style="font-style: normal; font-weight: normal; font-family: 'Lucida Console', 'Courier New', Courier, monospace; font-size: 12px; ">e</code> : Object<div class="sub-desc" style="margin-top: 5px; margin-right: 5px; margin-bottom: 5px; margin-left: 16px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">An edit event (see above for description)[/list]




afteredit : ( <code style="font-style: normal; font-weight: normal; font-family: 'Lucida Console', 'Courier New', Courier, monospace; font-size: 12px; ">Object e</code> )<div class="mdesc" style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 5px; padding-right: 0px; padding-bottom: 5px; padding-left: 0px; color: rgb(68, 68, 68); "><div class="long" style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; display: block; line-height: 18px; ">Fires after a cell is edited. The edit event object has the following properties
<ul style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 5px; padding-right: 5px; padding-bottom: 5px; padding-left: 16px; list-style-type: none; list-style-position: initial; list-style-image: initial; "><li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">grid - This grid<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">record - The record being edited<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">field - The field name being edited<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">value - The value being set<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">originalValue - The original value for the field, before the edit.<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">row - The grid row index<li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">column - The grid column index[/list]<code style="font-style: normal; font-weight: normal; font-size: 12px; color: rgb(0, 0, 0); line-height: 16px !important; font-family: 'Lucida Console', 'Courier New', Courier, monospace; ">grid.on(<em style="font-style: normal; font-weight: normal; color: rgb(0, 128, 128); background-color: rgb(238, 238, 238); ">'afteredit'[/i], afterEdit, this );

<b style="font-weight: normal; color: rgb(128, 0, 128); ">function[/b] afterEdit(e) {
<i style="font-weight: normal; font-style: normal; color: rgb(153, 153, 153); ">// execute an XHR to send/commit data to the server, <b style="font-weight: normal; color: rgb(153, 153, 153); font-style: normal; ">in[/b] callback <b style="font-weight: normal; color: rgb(153, 153, 153); font-style: normal; ">do[/b] (<b style="font-weight: normal; color: rgb(153, 153, 153); font-style: normal; ">if[/b] successful):[/i]
e.record.commit();
};</code><div class="mdetail-params" style="margin-top: 10px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 12px; font-size: 12px; "><strong style="font-style: normal; font-weight: normal; display: block; margin-bottom: 3px; font-size: 11px; color: rgb(85, 85, 85); ">Listeners will be called with the following arguments:[/b]<ul style="margin-top: 12px; margin-right: 12px; margin-bottom: 12px; margin-left: 12px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; list-style-type: circle; list-style-position: inside; list-style-image: initial; "><li style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; list-style-position: inside; list-style-type: circle; list-style-image: initial; "><code style="font-style: normal; font-weight: normal; font-family: 'Lucida Console', 'Courier New', Courier, monospace; font-size: 12px; ">e</code> : Object<div class="sub-desc" style="margin-top: 5px; margin-right: 5px; margin-bottom: 5px; margin-left: 16px; padding-top: 0px; padding-right: 0px; padding-bottom: 0px; padding-left: 0px; ">An edit event (see above for description)[/list]

[Christof]

Hi,

I did as you told me, but while doing this I noticed another security problem.
Let me first explain the solution and afterwards explain the new issue.

I wanted to have the check server-side, so I added an AJAX event to the grid

                <AjaxEvents>
                    <ValidateEdit OnEvent = "AfterGridCellEdit">
                        <ExtraParams>
                            <ext:Parameter Name = "NewValue" Value = "params[0].value" Mode = "Raw" />
                        </ExtraParams>
                    </ValidateEdit>
                </AjaxEvents>

    protected void AfterGridCellEdit(object sender, AjaxEventArgs e) 
    {
        // this method does not do anything, it just needs to be called so that form validation
        // will detect dangerous input values
    }

Of course, since AJAX works asynchronously in the mean time the grid was updated with the bad value, so I decided to have a client-side check next to it.

                <Listeners>
                    <ValidateEdit Handler = "onInPlaceEdit(e);" />
                </Listeners>

client code:

<script type="text/javascript">
    var cellValueValidated = function(param)
    {
        // now set the actual value in the grid after the server has validated it
        param.record.set(param.field,param.value);
    }
    
    var cellValueNotValidated = function(response,param)
    {
        
        // did we get a http error 500?
        if (response.status != 500)
            alert("An unknown error has occured on the server. The error code is: " + String(response.status));
            
        // in theory we could display a message to the user in this method
        // but since the Ajax event will raise a server-side error this would be a duplicate message
        /*
        else
            alert("A potentially dangerous Request.Form value was detected from the client (HTTP error 500)");
            */
    }
    
    var onInPlaceEdit = function(param)
    {
        // only need to do this if the value is a string and the string contains html tags...
        var t = typeof(param.value);
        if (t == "string")
        {
            if (param.value.indexOf('<')>=0)
            {
                param.cancel = true;
                Coolite.AjaxMethods.OnGridCellValidate(param.value,
                    { success: function(result) { cellValueValidated(param); }, 
                      failure: function(result, response, e, o) { cellValueNotValidated(response, param); } } );
            }
        }
    }
</script>

server code:

    [AjaxMethod]
    public string OnGridCellValidate(string newCellValue)
    {
        // this method simply returns the input
        // the only reason why we need this is because the new cell value needs to be verified
        // by form validation on the server side (so it does not contain insertion attacks)
        // if the function made the round-trip back to the client the value is safe otherwise HTTP error 500 occurs
        return newCellValue;
    }

This changed the behavior of the grid as I wanted it to be. The client calls into the OnGridCellValidate method, and that method fails with HTTP error 500.
Since I remembered from your AjaxMethod example that static methods have better performance I changed my method into public static string OnGridCellValidate...
I was surprised to see that after the change my method was actually called with the dangerous value, form validation effectively bypassed.
This can be a serious issue. Lets assume that I did not bother to check the edited company name after the editor closes and I choose to implement saving the grid by looping over the records in the grid and for every record I call the server side method below.

    [AjaxMethod]
    public static string SaveCompanyRecord(int companyID, string companyName)
    {
        // do the DB stuff here: UPDATE Company SET Name = ... WHERE ID = ...
    }

Since the data is no longer validated, the malicious script is saved in the database and the next visitor to my website will have it executed on his machine.

Don't you think that the arguments of this method should have been validated?

[Vladimir]

Hi,

Please note that validation (dangerous content) performs by page instance (not toolkit). When you use static method Page instance is not created. Therefore there is no such validation for static method. Toolkit should not validate submitted data because for one cases such data is acceptable, for another don't (it is like that DB doesn't check sql injection)


You have to check all data yourself if it is required