On Wed, Jul 2, 2008 at 12:59 PM, Will Glass-Husain <[EMAIL PROTECTED]> wrote: > Thanks, Tom. > > That's interesting. Appreciate the time to compile this list and post it. > I've posted some thoughts (fairly adhoc) directed to the community, you, and > your auditors below. > > Personally, I wasn't familiar with Integer Overflow errors in Java. I'd > always assumed the bounds checking took care of this C-like error. I see > however there are still potential issues. Googling turned up a nice > summary of the issue here. (very end of page). > > http://www.javacoffeebreak.com/books/extracts/javanotesv3/c9/s1.html > > Personally, this seems fairly miniscule risk here ("Very High Risk"? hard to > take that seriously). None of these functions are under the direct control > of the user. The calls to functions that access nodes are generally > performed while iterating through a fixed number of nodes. One response to > this audit would be to check the calls made to each function and confirm the > arguments are bounded. > > Should Velocity be sanity checking these type of arguments in internal > functions? I open that question to our other experienced developers. > Henning has advocated null checking in the past, perhaps this is a similar > issue.
no thanks. what would we do if we spotted such an overflow? throw an error, but that's already what will happen. we could perhaps put a more explanatory error message in place, but in most cases described, the user is doing something very extreme to cause it. i expect it should be pretty obvious to them that their 2+ million argument method call or macro or 2+ million node template might be the source of their problem. i also have never heard of anyone running across these in all of my many years reading these lists. i don't think it's worth the effort. the Parser and VelocityCharStream ones are perhaps a bit more likely to occur, but again, i have not heard of that happening. i say leave them be. > There are other much more significant gotchas. Please be sure to read the > article Nathan referenced. In particular, if you have third parties > uploading templates they have the potential to call any Java method on > objects in the context. Without proper configuration, they can even access > the ClassLoader and create instantiate new objects (for example, getting a > File object to access arbitrary files). You need to configure Velocity to > use the SecureUberspector to prevent this. Also, Velocity contains event > handlers to automatically escape all references-- this is recommended if > users enter text. agreed. their static code analysis missed the greatest risks, despite their extreme paranoia. if you allow any third party/user content to be processed by Velocity as VTL (either by user templates or processing their inputs with the RenderTool), then you really must use the SecureUberspector or be an expert in controlling java security permissions at the JVM level. And of course, escaping all user inputs (either via the event handler or something like EscapeTool) is always crucial to app security. > Regarding WebMacro in the velocity.jar. This is a utility class, intended to > help WebMacro users migrate to Velocity. Does your app permit users to call > main methods? If not, doesn't seem significant. If your security concerns > are strong enough to warrant removal of this method, I recommend creating a > simple ant script to do a custom build of the Velocity jar that removes this > class. (There are no dependencies on it). > > Regarding VelocityViewServlet-- I'll let Nathan or one of the other Tools > developers comment. > > Regarding the use of Random in Math.tools. It's simply a pass through to > the Java function. No worse or better than Java. Don't like it? Don't > configure your app to use MathTools. Write your own tool-- it's easy. > > I think it's unlikely we'll remove error messages from the log files. We > find most users of Velocity find these helpful. If this is an issue, > comment them out and recompile the code. Or (if you don't want to fork > the source) create a custom logger that ignores those comments. > > One more suggestion to Tom. It seems that the integer overflow issues are > probably the most troubling to your auditors. It's unlikely we'll rapidly > change these (low priority to the rest of us). However, if you or a > colleague were to go through these 9 methods and add argument-checking code, > then submit in a patch, we'd probably add it in to the base. (Would be > interesting to get other perspectives on this first). i wouldn't veto, but don't expect me to waste any more time on it. :) > Then you wouldn't > have to fork the code. More on how to do this here: > http://wiki.apache.org/velocity/GettingYourPatchCommitted > > Best, WILL > > On Wed, Jul 2, 2008 at 10:49 AM, Tom Jenkins <[EMAIL PROTECTED]> wrote: > >> I posted on the user list asking where I should bring up the results >> of a 3rd party security audit one of our applications had to go >> through. >> >> All of these were found through static analysis; no attack vectors are >> known. I'm also unsure of the validity of these results, but I have >> to do my due diligence. >> >> We're using Velocity 1.5 and Velocity Tools 1.3 >> >> Excuse the formatting... >> >> Very High Risk >> ============ >> Integer Overflow (Wrap or Wraparound) (CWE ID 190) >> >> Description >> An integer overflow condition exists when an integer that has not been >> properly sanity checked is used in the determination of an offset or >> size for memory allocation, copying, concatenation, or similarly. If >> the integer in question is incremented past the maximum possible >> value, it may wrap to become a very small, or negative number, >> therefore providing an unintended value. This occurs most commonly in >> arithmetic operations or loop iterations. Integer overflows can often >> result in buffer overflows or data corruption, both of which may be >> potentially exploited to execute arbitrary code. >> >> Recommendations >> Perform bounds checking to ensure that integers do not exceed the >> maximum possible value. >> >> File Line >> Macro.java 241 >> SimpleNode.java 156 >> VelocityCharStream.java 65 >> VelocityCharStream.java 67 >> VelocityCharStream.java 416 >> VelocityCharStream.java 66 >> ASTMethod.java 143 >> ASTMethod.java 134 >> Parser.java 3298 >> >> My analysis: >> Macro: >> ---------- >> int numArgs = node.jjtGetNumChildren(); >> numArgs--; // avoid the block tree... >> >> --> String argArray[] = new String[numArgs]; >> >> Looks like the flag is because if jjtGetNumChildren ever returns 0, >> numArgs will be -1. But that will just throw a >> NegativeArraySizeException so there isn't any overflowing. May be >> unintended that it go to -1, however. >> >> >> SimpleNode: >> ------------------- >> public void jjtAddChild(Node n, int i) >> { >> if (children == null) >> { >> --> children = new Node[i + 1]; >> } >> else if (i >= children.length) >> >> Theoretically the parameter 'i' could be max int. Not sure if in >> practice this will be an issue. This will wrap, but then we get a >> NegativeArraySizeException. >> >> >> VelocityCharStream: >> ------------------------------ >> private final void ExpandBuff(boolean wrapAround) >> { >> --> char[] newbuffer = new char[bufsize + 2048]; >> --> int newbufline[] = new int[bufsize + 2048]; >> --> int newbufcolumn[] = new int[bufsize + 2048]; >> >> All 3 of these array initializations are flagged. Later on in >> ExpandBuff bufsize is incremented by 2048. So again theoretically it >> could wrap. >> >> >> VelocityCharStream: >> ------------------------------ >> public final char[] GetSuffix(int len) >> { >> --> char[] ret = new char[len]; >> >> No idea why this is flagged. >> >> >> >> ASTMethod: >> ----------------- >> VelMethod method = null; >> >> --> Object [] params = new Object[paramCount]; >> >> try >> { >> /* >> * sadly, we do need recalc the values of the args, as this can >> * change from visit to visit >> */ >> >> --> final Class[] paramClasses = paramCount > 0 ? new >> Class[paramCount] : ArrayUtils.EMPTY_CLASS_ARRAY; >> >> paramCount is set by: >> paramCount = jjtGetNumChildren() - 1; >> so again if that method can return a 0, paramCount will wrap. And >> again a NegativeArraySizeException. >> >> >> Parser: >> ---------- >> private void jj_add_error_token(int kind, int pos) { >> if (pos >= 100) return; >> if (pos == jj_endpos + 1) { >> jj_lasttokens[jj_endpos++] = kind; >> } else if (jj_endpos != 0) { >> --> jj_expentry = new int[jj_endpos]; >> >> Again, theoretically jj_endpos could get so large as to wrap. And >> again a NegativeArraySizeException. >> >> >> Any suggestions on how to position these 9 in my meeting with the >> client and auditors? The fact that an exception is thrown (though I >> haven't specifically tested these methods) has got to alleviate some >> of the client panic. Though what does concern me is that knowing an >> exception should be thrown here, why is the auditor marking these as >> Very High? >> >> >> Medium Risk >> ========== >> >> Leftover Debug Code (CWE ID 489) >> >> Description >> A method may be leftover debug code that creates an unintended entry >> point in a web application. Although this is an acceptable practice >> during product development, classes that are part of a production J2EE >> application should not define a main() method. Whether this method can >> be remotely invoked depends on the configuration of the J2EE container >> and the application itself. >> >> Recommendations >> A method may be leftover debug code that creates an unintended entry >> point in a web application. Although this is an acceptable practice >> during product development, classes that are part of a production J2EE >> application should not define a main() method. Whether this method can >> be remotely invoked depends on the configuration of the J2EE container >> and the application itself. >> >> File Line >> WebMacro.java 297 >> >> Won't post the code; simply put there is a main() method there. We >> don't reference WebMacro anywhere (that I can see) but its in the >> velocity jar so it popped out on the scan. >> >> Also want to note that WebMacro "failed" another test - External >> Control of File Name or Path (CWE ID 73). But as this class isn't >> used that I can see I won't include that "failure". >> >> Does WebMacro have to be in the velocity jar? >> >> ++++++++++++++++++++++++++++++ >> >> Failure to Sanitize Script-Related HTML Tags in a Web Page (Basic XSS) >> (CWE ID 80) >> >> Description >> This call contains a cross-site scripting (XSS) flaw. The application >> populates the HTTP response with user-supplied input, allowing an >> attacker to embed malicious content, such as Javascript code, which >> will be executed in the context of the victim's browser. XSS >> vulnerabilities are commonly exploited to steal or manipulate cookies, >> modify presentation of content, and compromise confidential >> information, with new attack vectors being discovered on a regular >> basis. >> >> Recommendations >> Use HTML entities to encode all non-alphanumeric user-supplied data >> when using it to construct an HTTP response. Always validate >> user-supplied input to ensure that it conforms to the expected format, >> using centralized data validation routines when possible. >> >> File Line >> VelocityViewServlet.java 814 >> >> It looks like a change was made to the servlet to add escapeHtml >> (r480851) in 1.3. Not sure why it was flagged. Either way, it looks >> like an upgrade of velocity tools will solve that one for us. >> >> ++++++++++++++++++++++++++++++ >> >> Insufficient Entropy (CWE ID 331) >> >> Description >> Standard random number generators do not provide a sufficient amount >> of entropy when used for security purposes. Attackers can brute force >> the output of pseudorandom number generators such as rand(). >> >> Recommendations >> If this random number is used where security is a concern, such as >> generating a session key or session identifier, use a trusted >> cryptographic random number generator instead. These can be found on >> the Windows platform in the CryptoAPI or in an open source library >> such as OpenSSL. >> >> File Line >> MathTool.java 361 >> MathTool.java 364 >> >> Both of these lines have Math.random(). The audit tool just doesn't >> like Math.random() at all. Not sure what MathTool.getRandom() or >> MathTool.random() are used for internally. As an aside, I have >> changed (because we got dinged on it also) to using SecureRandom which >> I believe will pass this test. >> >> >> Low Risk >> ========== >> >> Error Message Information Leaks (CWE ID 209) >> >> Description >> Due to the fact that SOAP relies on XML data from the user, it is >> possible for the user to submit an invalid XML document to attack the >> parsing routines which can cause buffer overrun and/or denial of >> service attacks. The SOAP service library is responsible for >> converting the data into language specific data types. Users could >> attack this layer by utilizing an understanding of the back end >> languages limitations and the weaknesses in the SOAP libraries string >> to data type conversion process. After getting past all the SOAP >> specific processes your application logic can be attacked with normal >> input attacks, such as providing a negative number value when your >> software is expecting a standard number value and is ill equipped to >> handle it, or even general SQL Injection attacks. >> >> Recommendations >> Make sure you are using the latest security patched parsing engine >> available and that you are trapping and handling all errors due to >> parsing problems and responding to the user gracefully. Make sure you >> are using the latest security patched SOAP library available and that >> you are trapping and handling all errors due to data type conversion >> problems and responding to the user gracefully. It is important to use >> strong definitions of expected data types to avoid having attacks get >> to the next layer. Your application logic must validate all data >> coming in. Do not rely on the SOAP library to handle this for you, >> unless it has specific features to do so. Like any web application >> development, it is important to consider all input from a user to be >> dirty until it passes validation and filtering to be specifically the >> type of input expected. >> >> File Line >> SystemLogChute.java 142 >> StringUtils.java 376 >> SystemLogChute.java 141 >> Generator.java 215 >> PrintExceptions.java 102 >> FieldMethodizer.java 162 >> VelocityViewServlet.java 807 >> FieldMethodizer.java 92 >> Generator.java 168 >> StringUtils.java 489 >> SystemLogChute.java 151 >> LogSystemCommonsLog.java 156 >> StringUtils.java 389 >> Generator.java 520 >> VelocityServlet.java 705 >> FieldMethodizer.java 114 >> >> I'm not quite sure what to say about these. I haven't gone through >> them all, but in our files that failed it was because of a call to >> printStackTrace(). Every call to printStackTrace() seems to raise a >> flag with the auditors. We're going to change ours to call a logger. >> Given that these are very low, I'm not too worried about these. But >> for completeness I included them. >> >> >> >> Hope you could follow this VERY long post. >> >> If anyone has any thoughts or suggestions on how I should proceed I >> would welcome the input. >> >> Thanks for your time. >> >> Tom >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: [EMAIL PROTECTED] >> For additional commands, e-mail: [EMAIL PROTECTED] >> >> > > > -- > Forio Business Simulations > > Will Glass-Husain > [EMAIL PROTECTED] > www.forio.com > --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
