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]
