tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher Schultz <ch...@christopherschultz.net>
Subject Re: svn commit: r1676393 - in /tomcat/trunk: java/org/apache/el/parser/AstValue.java test/org/apache/el/TestMethodExpressionImpl.java
Date Tue, 28 Apr 2015 21:42:20 GMT
Mark,

On 4/28/15 5:26 PM, Mark Thomas wrote:
> On 28/04/2015 22:21, Christopher Schultz wrote:
>> Mark,
>>
>> On 4/27/15 7:01 PM, markt@apache.org wrote:
>>> Author: markt
>>> Date: Mon Apr 27 23:01:16 2015
>>> New Revision: 1676393
>>>
>>> URL: http://svn.apache.org/r1676393
>>> Log:
>>> Add some comments to clarify behaviour.
>>> Review by schultz re object allocation
>>>
>>> Modified:
>>>     tomcat/trunk/java/org/apache/el/parser/AstValue.java
>>>     tomcat/trunk/test/org/apache/el/TestMethodExpressionImpl.java
>>>
>>> Modified: tomcat/trunk/java/org/apache/el/parser/AstValue.java
>>> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/el/parser/AstValue.java?rev=1676393&r1=1676392&r2=1676393&view=diff
>>> ==============================================================================
>>> --- tomcat/trunk/java/org/apache/el/parser/AstValue.java (original)
>>> +++ tomcat/trunk/java/org/apache/el/parser/AstValue.java Mon Apr 27 23:01:16
2015
>>> @@ -41,6 +41,9 @@ import org.apache.el.util.ReflectionUtil
>>>   */
>>>  public final class AstValue extends SimpleNode {
>>>  
>>> +    private static final Object[] EMPTY_ARRAY = new Object[0];
>>> +    private static final Object[] ARRAY_OF_SINGLE_NULL = new Object[1];
>>> +
>>>      protected static class Target {
>>>          protected Object base;
>>>  
>>> @@ -263,7 +266,8 @@ public final class AstValue extends Simp
>>>      private Object[] convertArgs(EvaluationContext ctx, Object[] src, Method
m) {
>>>          Class<?>[] types = m.getParameterTypes();
>>>          if (types.length == 0) {
>>> -            return new Object[0];
>>> +            // Treated as if parameters have been provided so src is ignored
>>> +            return EMPTY_ARRAY;
>>>          }
>>>  
>>>          int paramCount = types.length;
>>> @@ -271,23 +275,24 @@ public final class AstValue extends Simp
>>>          if (m.isVarArgs() && paramCount > 1 && (src == null
|| paramCount > src.length) ||
>>>                  !m.isVarArgs() && (paramCount > 0 && src
== null ||
>>>                          src != null && src.length != paramCount)) {
>>> -            String inputParamCount = null;
>>> +            String srcCount = null;
>>>              if (src != null) {
>>> -                inputParamCount = Integer.toString(src.length);
>>> +                srcCount = Integer.toString(src.length);
>>>              }
>>>              String msg;
>>>              if (m.isVarArgs()) {
>>>                  msg = MessageFactory.get("error.invoke.tooFewParams",
>>> -                        m.getName(), inputParamCount, Integer.toString(paramCount));
>>> +                        m.getName(), srcCount, Integer.toString(paramCount));
>>>              } else {
>>>                  msg = MessageFactory.get("error.invoke.wrongParams",
>>> -                        m.getName(), inputParamCount, Integer.toString(paramCount));
>>> +                        m.getName(), srcCount, Integer.toString(paramCount));
>>>              }
>>>              throw new IllegalArgumentException(msg);
>>>          }
>>>  
>>>          if (src == null) {
>>> -            return new Object[1];
>>> +            // Must be a varargs method with a single parameter.
>>> +            return ARRAY_OF_SINGLE_NULL;
>>
>> Is this safe? I'm not sure of the scope of this array, but client code
>> could potentially mutate the contents. We fully expect
>> ARRAY_OF_SINGLE_NULL[0] to be == null, but some other code could change
>> it and it would probably cause a huge mess.
>>
>> I like the use of a flyweight here, but if that object ever gets out of
>> our control, it could be poisoned.
> 
> Untrusted apps are a minority use case but they do exist. I think you
> are right and I'll use a new array every time.

I wasn't sure how far this object would go, but I guess it might
ultimately end up as the parameter to a method controlled by the
application.

There might be a security vulnerability there, like triggering a call to
an evil method to trigger that array to be passed to it, and then
changing the value. Then calling an important method with a null
argument to maybe pass some argument-checking or something. I can't
think of a use-case off the top of my head but someone more creative
than I might be able to.

Since it would affect other web applications running on the same
container, it wouldn't just be dangerous to a single web app.

This is a relatively unlikely case, so I think object-allocation isn't
too much of a concern.

-chris


Mime
View raw message