You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@velocity.apache.org by Tom Jenkins <to...@gmail.com> on 2008/07/02 19:49:51 UTC
Results from a security audit
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: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: Results from a security audit
Posted by Adrian Tarau <ad...@gmail.com>.
I agree with Will & Nathan. I don't know what tool they used, but it
doesn't know Java's internals for sure.
Validating user input(when they call the API) is a must but rather than
that is just insane to check what they suggested. Of course, when some
(user) parameters are used to allocate memory you should check if the
value is bellow a reasonable value, just to be sure nobody can bring the
JVM down(once you got OutOfMemory exception you must presume the
application is not stable and must be restarted).
Except for an OutOfMemory prevention, any exception(like
ArrayIndexOutOfBoundsException, NegativeArraySizeException) is welcomed
and it is a standard way to stop the execution flow.
Tom Jenkins 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: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: Results from a security audit
Posted by Nathan Bubna <nb...@gmail.com>.
My thoughts inline...
On Wed, Jul 2, 2008 at 10:49 AM, Tom Jenkins <to...@gmail.com> 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: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: Results from a security audit
Posted by Tom Jenkins <to...@gmail.com>.
On Wed, Jul 2, 2008 at 4:21 PM, Nathan Bubna <nb...@gmail.com> wrote:
> On Wed, Jul 2, 2008 at 12:59 PM, Will Glass-Husain
> <wg...@gmail.com> wrote:
> > 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.
>
I concur with Nathan. Given that these exceptions would be triggered on
theoretically possible circumstances but realistically insane practice I see
no reason to waste anyone's time on it. I have enough information from this
discussion to state that case then turn it around on them and make them
explain their risk assessment.
>
> > 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).
>
No we don't allow calls to main methods. Removing WebMacro from the jar is
easy enough to do. (though not at a security concern, more a client
management concern)
>
> >
> > 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.
>
Nope, we don't use it. Looks like its not used for anything worrisome from
your end so I'm not worried.
>
> >
> > 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.
> >
>
Not a major deal to me. Hope that these lows didn't come across as being
considered a real issue to me
>
> > 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. :)
>
I'll save that as a backup plan. Given the discussion above, I don't see it
having to come to that.
Again, thanks all for taking the time. I appreciate it.
Tom
Re: Results from a security audit
Posted by Nathan Bubna <nb...@gmail.com>.
On Wed, Jul 2, 2008 at 12:59 PM, Will Glass-Husain
<wg...@gmail.com> 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 <to...@gmail.com> 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: dev-unsubscribe@velocity.apache.org
>> For additional commands, e-mail: dev-help@velocity.apache.org
>>
>>
>
>
> --
> Forio Business Simulations
>
> Will Glass-Husain
> wglass@forio.com
> www.forio.com
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: Results from a security audit
Posted by Will Glass-Husain <wg...@gmail.com>.
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.
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.
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). 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 <to...@gmail.com> 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: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
--
Forio Business Simulations
Will Glass-Husain
wglass@forio.com
www.forio.com