security related isuue

Splash Forums Rewrite Users security related isuue

This topic contains 11 replies, has 3 voices, and was last updated by  Christian Kaltepoth 2 years, 9 months ago.

Viewing 12 posts - 1 through 12 (of 12 total)
  • Author
    Posts
  • #25179

    0swald
    Participant

    Hi all,
    I believe this one is serious. Rewrite “swallows” security related exceptions instead of propagating them. In my case it is EJBAccessException which is thrown when a user tries to call an EJB method while not having proper set of roles.
    A short investigation has shown that exceptions being caught in org.ocpsoft.rewrite.el.El, method private static Object executeProviderCallable:

    
        for (ExpressionLanguageProvider provider : getProviders()) {
          try {
            return providerCallable.call(event, context, provider);
          } catch (Exception e) {
            exceptions.add(e);
          }
        }
    
        for (Exception exception : exceptions) {
          log.error("DEFERRED EXCEPTION", exception);
        }
        throw new RewriteException("No registered " + ExpressionLanguageProvider.class.getName()
                + " could handle the Expression [" + providerCallable.getExpression() + "]");
    

    Here all exceptions are logged and the new RewriteException is thrown. I have no idea what it was supposed to mean, but some runtime exceptions thrown by a EE container are essential for error handling (showing Access Denied in my case). To solve the issue I had to rewrite the snippet, the issue has gone, but I’m not sure there are no other similar code fragments in the project.

    
        for (ExpressionLanguageProvider provider : getProviders()) {
          try {
            return providerCallable.call(event, context, provider);
          } catch (RuntimeException e) {
            throw e;
          } catch (Exception e) {
            exceptions.add(e);
          }
        }
    
    #25184

    Hey Oswald,

    This is fairly serious – thanks for pointing it out.

    I’m not exactly sure of the best course of action right now. I’ve been thinking about it for the past 24 hours and I think that it’s possible the best solution is to avoid the ‘El’ class entirely (or, like you said, just copy it use a custom version that behaves as you would like.) It is, after all, just a provided configuration element that encapsulates a little bit of the El behavior for you.

    I think, that using your own custom El class is totally fair. I need to ask, though… how is this occurring? Is this because you are directly using the El element in a ConfigurationProvider or is this occurring as part of some deeper/provided functionality of Rewrite?

    ~Lincoln

    #25185

    0swald
    Participant

    Hi Lincoln,

    Actually it wasn’t my EL, here I’m using the stock one from rewrite-servlet-2.0.6-SNAPSHOT, I’ve removed all of my custom libraries while examining the issue. Actually there’s no need in further study – catching ALL exceptions is generally wrong idea in EE environment, Servlet request isn’t only about pages, it also creates and holds a security context which is propagated along with each and every call to underlying components of an EE server. If a proxy of an EJB finds the security context insufficient for execution, it throws a subclass of an EJBAccessException, which is the subclass of RuntimeException and which finally breaks the execution of a filter/servlet. You could handle such situations via web.xml (error-page) or via exception-handler-factory in JSF, but not when these exceptions are swallowed by the Rewrite.

    I see only two options here:
    1) don’t catch any exceptions at all, let it be the responsibility of an ExpressionLanguageProvider.
    2) don’t catch any runtime exceptions (as I did above), this seems more natural to me, because the RewriteException thrown is also a runtime one. Thus, unless you have a very very special exception handling somewhere deep in your code (which it seems you don’t), there is nothing you lose in this case.

    What do you think?

    Regards,
    Andrew

    #25186

    0swald
    Participant

    Lincoln, there is another option, I just had a look at the RewriteException and, in case you prefer to throw namely this particular one, here is it:

    
        for (ExpressionLanguageProvider provider : getProviders()) {
          try {
            return providerCallable.call(event, context, provider);
          } catch (RuntimeException e) {
            throw new RewriteException(e.getMessage(), e);
          } catch (Exception e) {
            exceptions.add(e);
          }
        }
    

    The above would also make it possible to unwrap the source exception for proper error handling.

    #25187

    I agree with Oswald. I also don’t like that Rewrite catches and wraps ALL exceptions thrown during EL evaluation. See this example:

    .perform(Invoke.binding(El.retrievalMethod("pageBean.loadData")))

    Everything thrown from the EL method (including stuff like NullPointerException in the business code) is catched and wrapped. This doesn’t make sense to me. Especially because this makes it impossible to handle special exception types via web.xml.

    I also think that it would makes sense to simple rethrow runtime exceptions them immediately without wrapping them. I don’t see any downside of this approach.

    But perhaps we could add another checked exception type to the ExpressionLanguageProvider SPI to allow the providers to tell Rewrite something like “I wasn’t able to do it because there was an EL related error”. IMHO this is something completely different from “I was able to invoke the EL, but the invoked method has thrown an exception”.

    #25188

    0swald
    Participant

    Looking forward to you both to decide upon the subject, I’d appreciate any, even temporary solution as we’re now seriously addicted to the Rewrite))
    I could issue a patch if you agree to the re-throwing runtime exceptions as a workaround.

    #25189

    Hey Oswald,

    nice to hear that you are addicted to Rewrite the same way as we are. 🙂

    Yeah, we really should find a way to address this issue. As I said before, I think we should modify the ExpressionLanguageProvider SPI to behave like this.

    1. Throw an UnsupportedOperationException if the provider cannot perform the task for some reason. The SpringExpressionLanguageProvider would for example throw this exception if the Spring ApplicationContext cannot be found. This is already part of the API. No change is required here.
    2. Throw an ExpressionLanguageException (or find some other name) if the provider isn’t able to perform the EL invocation. If the expression is myBean.foo for example and foo doesn’t exist, the provider should throw this exception. This exception should be catched and wrapped by Rewrite as this is usually a problem with the user’s Rewrite configuration.
    3. If the code of the invoked bean method (for example myBean.foo) throws a runtime exception, it should simply be thrown and not be catched or wrapped by Rewrite. I think this makes sense because these exceptions aren’t related to Rewrite at all and a usually bugs in the user’s code or something like that. Rewrite should do anything in this case.

    Does this make sense to you?

    #25190

    0swald
    Participant

    Hi Christian,

    It’s not only that we’re addicted, but we’re also very dependent on the Rewrite now. The behavior you’ve described sounds logical, I totally agree, but at this very moment I’m mostly concerned about RuntimeException being caught where it shouldn’t be. Sorry, it’s critical because it interferes with a significant and security-related part of the Java EE architecture, I vote for an immediate patch so one could use latest official snapshots instead of using own patched versions of Rewrite as we started doing as of yesterday. Hope you understand me.

    Regards,
    /Osw

    #25191

    Yeah, I understand that you want to see this issue fixed as soon as possible. But to be honest, if I were you, I would prefer a manually patched stable version over using the snapshots in production. The current master contains our latest development efforts which MAY contain regressions.

    However, I created a pull request for a fix which is IMHO a good start for addressing this issue. This is basically what your patch looks like I think. The CI server will run our test suite against this pull request shortly. I’m looking forward to see if all tests are still green.

    https://github.com/ocpsoft/rewrite/pull/140

    #25192

    I merged the pull request. So the next snapshot should work fine for you.

    #25197

    0swald
    Participant

    Christian thanks!
    But I cant find snapshots page, have you moved it somewhere?

    #25198

    The snapshot repository is mentioned here:

    http://ocpsoft.org/rewrite/docs/configuration/install

    However, I just saw that the latest snapshots aren’t deployed there any more.

    @lincoln: Any idea?

    @Oswald: Sorry, but I think you have to build the latest snapshot yourself. Sorry about that. Seems like there is something wrong with the CI server.

Viewing 12 posts - 1 through 12 (of 12 total)

You must be logged in to reply to this topic.

Comments are closed.