chronister Posted January 4, 2009 Share Posted January 4, 2009 I have created a class that handles form creation and form processing. It takes the Post data submitted by the generated form and returns an array with data validated against a set of rules. The form that is created is fully css based form that uses ordered lists for the form elements. An optional message area displays a summary of errors at the top of the page. Each form element is created by calling class methods and passing data to it, an array is created to define validation "rules" and the processing is actually called with a 1 line method. I would like you folks to take a look at it and give me your opinion of it. Are there any changes you would suggest? If you go to http://ctsqc.com/formClassExamples/ you will find a test form with all the field types that are currently working. You will also find the source of this form, the source of the main form class file, and a tab that shows the current html of the form. There are several css files, a few images, some js files and a few other PHP files that hold other classes or misc functions that are not shown. This is my first real shot at an OOP class that really does something, so if you have suggestions, please let me know. Submit the form so you get errors and give note the highlighting. The error summary is optional. Thanks Nate Link to comment Share on other sites More sharing options...
darkfreaks Posted January 4, 2009 Share Posted January 4, 2009 Please refer to this thread: http://www.phpfreaks.com/forums/index.php/topic,232470.0.html Link to comment Share on other sites More sharing options...
chronister Posted January 6, 2009 Author Share Posted January 6, 2009 Over 120 people have looked at this and no one has an opinion?? Come on folks.... lemme know what you think of it. It works well, but I am wondering if you guys can see anything that could be written better, cleaner or more efficient. I have posted a link to my profile on the index page now to "prove" that it is my site I appreciate any feedback given. Nate Link to comment Share on other sites More sharing options...
darkfreaks Posted January 6, 2009 Share Posted January 6, 2009 XSS String Tests Summary (616 tests executed) Failures: 0 Warnings: 48 Passes: 568 Warnings: The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <!-- -- --><script>document.vulnerable=true;</script><!-- -- --> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <![CDATA[<!--]]<script>document.vulnerable=true;//--></script> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <<script>document.vulnerable=true;</script> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <style><!--</style><script>document.vulnerable=true;//--></script> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <OBJECT classid=clsid:ae24fdae-03c6-11d1-8b76-0080c744f389><param name=url value=javascript:document.vulnerable=true></OBJECT> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <!--[if gte IE 4]><SCRIPT>document.vulnerable=true;</SCRIPT><![endif]--> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: ¼script¾document.vulnerable=true;¼/script¾ The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <BODY ONLOAD=document.vulnerable=true;> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: </TITLE><SCRIPT>document.vulnerable=true;</SCRIPT> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <SCRIPT <B>document.vulnerable=true;</SCRIPT> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <<SCRIPT>document.vulnerable=true;//<</SCRIPT> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <SCRIPT>document.vulnerable=true;</SCRIPT> Solution: mysql_real_escape_string(),strip_tags(),trim() Link to comment Share on other sites More sharing options...
chronister Posted January 7, 2009 Author Share Posted January 7, 2009 Thanks for the reply darkfreaks, what did you use to test this, what page did you test it on, and what exactly is it testing against? The trim & strip tags could be used in several places, however I am trying to limit the class to simply 1. Create Form, 2. Process form 3. Return data as an array where further processing can be done to insert to db, send email, or write to file. I have thought about these functions, but I am not sure where to apply them at and how to trigger them properly to be the most effective. At the very basic of this class, we are simply creating form, checking it against some basic information and returning the data, so I am not sure how applying these in the class will make a difference in it's security. I guess my "vision" for this is those kinds of things get applied after the results array is returned. Does this make sense, or should I add some strip_slashes(), trim() or strip_tags() functions in the actual class?? * I did some testing and found that the strip_tags NEEDS to be there, or at least I need to trigger it when tags are not allowed (which is most always).* Thanks for the help. Nate Link to comment Share on other sites More sharing options...
darkfreaks Posted January 7, 2009 Share Posted January 7, 2009 strip_tags() definitely needs to be called somewhere in the code to avoid JavaScript injection. Link to comment Share on other sites More sharing options...
448191 Posted January 7, 2009 Share Posted January 7, 2009 XSS String Tests Summary (616 tests executed) Failures: 0 Warnings: 48 Passes: 568 Warnings: The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <!-- -- --><script>document.vulnerable=true;</script><!-- -- --> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <![CDATA[<!--]]<script>document.vulnerable=true;//--></script> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <<script>document.vulnerable=true;</script> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <style><!--</style><script>document.vulnerable=true;//--></script> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <OBJECT classid=clsid:ae24fdae-03c6-11d1-8b76-0080c744f389><param name=url value=javascript:document.vulnerable=true></OBJECT> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <!--[if gte IE 4]><SCRIPT>document.vulnerable=true;</SCRIPT><![endif]--> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: ¼script¾document.vulnerable=true;¼/script¾ The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <BODY ONLOAD=document.vulnerable=true;> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: </TITLE><SCRIPT>document.vulnerable=true;</SCRIPT> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <SCRIPT <B>document.vulnerable=true;</SCRIPT> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <<SCRIPT>document.vulnerable=true;//<</SCRIPT> The unencoded attack string was found in the html of the document. Other browsers may be vulnerable to this XSS string. Tested value: <SCRIPT>document.vulnerable=true;</SCRIPT> Solution: mysql_real_escape_string(),strip_tags(),trim() Please refer to this thread: http://www.phpfreaks.com/forums/index.php/topic,232470.0.html Link to comment Share on other sites More sharing options...
darkfreaks Posted January 7, 2009 Share Posted January 7, 2009 my appoligies i didn't see the rule about the providing the scanners link: XSS Me: https://addons.mozilla.org/en-US/firefox/addon/7598 Explanation: Like i said briefly before not using something like strip_tags() to filter Javacript/PHP injection attacks is unsafe, people can just insert any code they wish into the database/site compromising your site and putting it at jeopardy. Link to comment Share on other sites More sharing options...
448191 Posted January 7, 2009 Share Posted January 7, 2009 Explanation: Like i said briefly before not using something like strip_tags() to filter Javacript/PHP injection attacks is unsafe, people can just insert any code they wish into the database/site compromising your site and putting it at jeopardy. Ok, now that is simply not true. Yes, the results you posted indicate a risk of XSS attacks (cookie theft, session hijacking), but not PHP code injection and certainly not SQL injection (in your words: "insert any code they wish into the database"). mysql_real_escape_string() as such is not relevant to these results. trim() is NOT security related AT ALL! My personal conclusion: you have no idea what you are talking about. Link to comment Share on other sites More sharing options...
chronister Posted January 7, 2009 Author Share Posted January 7, 2009 thanks for the help here. I will be adding a flag in the validation array to include strip tags. I am trying to make this as universal as I can, and as such I could see some need to allow tags in the future, however I understand that it needs to be handled with caution and in most cases not needed at all. I found a site about XSS, and was able to trigger a remote .js file through the form, so that tells me right there that I have to deal with that. This particular .js file was just an alert, but next time it could be an ajax call and write the data it can find to a database. But other than that, do you see any other flaws? As I stated, this is my first attempt at a "real" OOP class, and am still learning about how to call things the correct way and such. Does it seem useful to those who have looked over it? Not that I am going to sell it, but would you actually pay for something like this if it were available? As far as sanitizing it for database use, that will be done after the results are returned. I thought about including database functionality to it, but I think it would be easier to get any db results for the form data before it is created, and then do any db inserts after the results are given. Thanks, Nate Link to comment Share on other sites More sharing options...
448191 Posted January 7, 2009 Share Posted January 7, 2009 A note about "sanitizing for database insertion". Forget all the noobs and "out of touch" veterans screaming mysql_real_escape_string() or addslashes() and whatnot. These are messy and shakey "solutions". The only real defense against SQL injection are prepared statements. Look into PDO or the MySQLi extension. Link to comment Share on other sites More sharing options...
fenway Posted January 7, 2009 Share Posted January 7, 2009 A note about "sanitizing for database insertion". Forget all the noobs and "out of touch" veterans screaming mysql_real_escape_string() or addslashes() and whatnot. These are messy and shakey "solutions". The only real defense against SQL injection are prepared statements. Look into PDO or the MySQLi extension. I would have to disagree. Using prepared statements for sanitize data is really overkill -- there is a large overhead in using these, which really only pays off in certain circumstances. There's nothing "magical" about sanitizing sql queries... all you need to do is escape quotes, nothing more. I don't think this is shakey or messy, as long as you use a DB class that handles this for you. Link to comment Share on other sites More sharing options...
448191 Posted January 7, 2009 Share Posted January 7, 2009 It's easy to forget to escape a value you parse into a query. With prepared statements you don't have this issue. It's not about "sanitizing" data. Sanitizing becomes obsolete because SQL and values are completely separated. Overkill? I don't think so. As for performance, this guy begs to differ with your statement. Link to comment Share on other sites More sharing options...
Daniel0 Posted January 7, 2009 Share Posted January 7, 2009 A note about "sanitizing for database insertion". Forget all the noobs and "out of touch" veterans screaming mysql_real_escape_string() or addslashes() and whatnot. These are messy and shakey "solutions". The only real defense against SQL injection are prepared statements. Look into PDO or the MySQLi extension. I would have to disagree. Using prepared statements for sanitize data is really overkill -- there is a large overhead in using these, which really only pays off in certain circumstances. There is also an overhead with running a lot of values through a filtering algorithm before. Link to comment Share on other sites More sharing options...
.josh Posted January 7, 2009 Share Posted January 7, 2009 mysql_real_escape_string is not the catch-all solution many people think it is. But neither is it bad. It just does 1 thing, when you should be checking for 2 or 3 or more things. Simply escaping quotes is not enough to 100% prevent injection. It's just 1 possible checkpoint. The real goal should be to validate input, not pacify it. Make sure it is what it is supposed to be. Trash it if it's not. Conform or die! For instance, if you expect an integer, check to make sure it's an integer. There are several built in functions like is_int, ctype_digit, is_numeric that can be used, depending on the situation. Or you can use regex to validate the format. Or force it to be an integer by casting it as one. (edited to linkify functions) Link to comment Share on other sites More sharing options...
Recommended Posts