Hi All,
Whilst browsing Sample code acceptance checklist for IT organizations on Technet, I realised that in several places we as an organisation were not always following best practises. As stated in this article, this probably means that we could do a few things to improve the performance and security of our product - and who doesn't want to write faster, safer code?
Here are the items that rang alarm bells and I thought it might be worth sharing them in case anyone else still has a few BP boxes to check:
- Plain text passwords are not present in Web.config, Machine.config, or any files that contain configuration settings. Utilities such as Aspnet_setreg.exe and Trustee or the identity setting in AppPool on IIS 6.0 or IIS 7.0 are used to encrypt credentials.
This often seems to slip through the net, reason being that administrators tend to think "Users can't get on our servers so what's the point?". The point, I think is that we as developers should always try to implement defence in depth security practises and this is a good example. OK - if an attacker somehow obtains RDP access to your server farm a plain text password for a Web application may well be the least of your worries but this is not a tricky one to fix. Grab Aspnet_setreg.exe and get to work! - If cookies contain sensitive data, they are marked secure.
The severity of missing this item depends on the sensitivity of your application and your supporting infrastructure. If your data must be secure during transit, then you need to either ensure that all cookies are marked secure, or you must ensure the channel is encrypted via some other means. Options that spring to mind are forcing this via IIS (straightforward in version 6.0 and later) and / or forcing an SSL redirect via a reverse proxy server such as ISA Server 2006. In our case, we implemented the last 2 options which ticked the corporate security check box when we conducted an external security review recently. - Input surfaces in Web parts and other customizations include boundary checks, input data integrity checks, and appropriate exception handling to protect from cross-site scripting and SQL injection.
OK, XSS and SQL injection are both well known security problems in Web applications. However, its always worth bearing these two common issues in mind and applying protective measures such restricting input (must be server side - think regular expressions) and using parametrised stored procedures. See How To: Protect From SQL Injection in ASP.NET for more info. - You avoid using AllowUnsafeUpdates. You use ValidateFormDigest() and, if necessary, use elevated privileges to interact with SharePoint objects. In cases where AllowUnsafeUpdates must be used, you ensure that AllowUnsafeUpdates is set to False in your
try-catch-finallyblock, or you use a Dispose() method (as required by the IDisposable interface) to avoid security issues.
In the SharePoint development world, I believe it is relatively common knowledge that using AllowUnsafeUpdates liberally is a big no-no. However, there are places where it is unavoidable (sometimes whilst using SPLimitedWebPartManager) so ensure you utilise that finally block. - The ASP.NET validateRequest option is enabled, if possible.
Based on my experience, this appears to be a favourite item for penetration testers to pick on whilst testing ASP.NET forms applications. Enabling this option is a step toward preventing XSS (see How To: Prevent Cross-Site Scripting in ASP.NET). - The application consistently uses standardized input validation such as RegEx throughout.
I would like to confidently tell you that this requirement is a given and that I haven't come across an example where standardised input validation is neglected in recent years. However, that is simply not the case and I would encourage you to rigorously check and confirm that your ASP.NET controls are all validated using a white list input validation approach. - The application uses a standardized approach to structured error and exception handling throughout.
- The code uses exception handling. The code catches only the exceptions that you know about. For example, do not use
try{} catch(Exception ex){}unless you throw the error again.
I think that although many organisations do use a standardised approach to error and exception handling, it is sometimes an approach that is consistently wrong. For example, I have been guilty of doing this in the past:
catch (Exception ex)
{
throw (ex); // do NOT do this!
}
(this lesson here is that if you want to re-throw the exception currently handled by a parameter-less catch clause, use the throw statement without arguments see here)
The main point here is that you should review your code and ensure that your exception handling procedure is both consistent and follows best practises.
- The code uses exception handling. The code catches only the exceptions that you know about. For example, do not use
- Application errors do not contain sensitive information or information that could be used to exploit the fault.
Quite often, this is viewed as a negative point as far as the end user experience is concerned. If a user fails to logon, they often want to know why they can't logon. As always, the problem here is striking a balance between usability and security. We want to help the user out by providing useful error messages, we really do. But we also want to prevent unauthorised access to your application and that has to take priority.
Throw generic error messages on-screen, but make sure you log as much detail about the error as possible to the log. Use proper exception handling to ensure the stack trace isn't lost. - Customizations are accompanied by a list of all dependencies. This could include account/passwords, Web services, databases, other solutions or Features, patches, tool sets or libraries, and other dependencies.
This might appear to be a relatively obvious point but I'm often surprised at how much information is retained in a developers head rather than correctly documented. The acid test is ensuring that someone outside of the development team can deploy the customisation without any issues. - When using the Count property of a SPListItemCollection, you only call it once and then store it in a variable that you can refer to when looping. You do not call it inside a loop.
I think this one speaks for itself, although this was a recent revelation for me.
I think that just about covers it. Please let me know if these items rang any alarm bells or if you have any comments.
Cheers, Ben.
