My thoughts inline...
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
I don't see how an integer overflow in any of these would cause
anything but an array access error. And i'm quite sure none of these
could be triggered by a third party unless that third party is able to
create their own templates and have the app process them. So unless
you process user input as templates or using the RenderTool, i don't
see anything to worry about. Even if you do have the VelocityEngine
process user-generated VTL somehow, then at worst you have to worry
about an uncaught exception that i doubt could be exploited.
> 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.
heh. that would mean a macro with more than 2,147,483,647 arguments.
i think a NegativeArraySizeException sounds like a fair retort to the
author of such a macro.
> 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.
again, if they have over 2 million child nodes at any point in the
AST, then they deserve an exception. :)
>
>
> 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.
this has its origins in JavaCC. i've never heard of any problems from
it, and the javacc crew usually seem to know what they're doing.
> 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.
over 2.1 million arguments to a method call in a template also seems unlikely.
>
> 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.
as with VelocityCharStream, i've never heard of it happening.
>
> 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?
perhaps they prefer nasty exceptions like NegativeArraySizeException
to be caught and suppressed so that no one ever knows what went wrong?
honestly, it feels like they either don't believe that java actually
prevents access to arbitrary areas of memory via overflows. i'm not
convinced the software that did the audit knows things like array
bounds checking are built into java. it's simply not possible to
address arbitrary memory through out-of-range array indices. trying
to do so causes an exception to be thrown; exploitation is not
possible.
>
>
> 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?
no. just yank it out of the jar. it's only for translating webmacro
templates to velocity templates. it long out-lived its usefulness,
IMHO.
> ++++++++++++++++++++++++++++++
>
> 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
this is a hard-coded html error message. the exception messages
(possibly user-influenced) are escaped as of 1.3. i don't see what
the complaint is about.
> 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.
no thanks. MathTool.random() is for display randomization, not any
sort of hashing or crypto. SecureRandom would be total overkill. if
you're really concerned, then don't use the MathTool. if you really
want to use it, use a custom subclass that overrides random() to kill
it or use SecureRandom.
> 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.
i don't get it. i've never heard of printStackTrace() being a risk.
that's weird. i'd love to hear their rational. i also have no idea
what this has to do with SOAP. that makes zero sense to me.
>
> 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]
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
