CXF
  1. CXF
  2. CXF-4684

SOAPFault message improvement in CXF when there is unchecked NPE

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.6.2
    • Fix Version/s: 2.5.8, 2.6.5, 2.7.2
    • Component/s: WS-* Components
    • Labels:
      None
    • Estimated Complexity:
      Unknown

      Description

      When there is unchecked NPE thrown, the SOAPFault in CXF will only throw the "Fault occurred while processing." message rather than the original NPE message.

      Analysis:
      1. In org.apache.cxf.binding.soap.interceptor.Soap11FaultOutInterceptor and org.apache.cxf.binding.soap.interceptor.Soap12FaultOutInterceptor,
      It will check fault.getMessage() :
      if (fault.getMessage() != null) {
      if (message.get("forced.faultstring") != null)

      { writer.writeCharacters((String) message.get("forced.faultstring")); }

      else

      { writer.writeCharacters(fault.getMessage()); }

      } else

      { writer.writeCharacters("Fault occurred while processing."); }

      But for NPE, the fault.getMessage() will return null instead of the "java.lang.NullPointerException" in the getMessage() in NPE.

      2.
      Fault.getMessage will return null in the NPE scenario while it's super class Throwable will not.
      When there is NPE, the message attribute in Fault is null while the detailMessageAtrribute is "java.lang.NullPointerException".
      Details:
      SoapFault->Fault->UncheckedException->RuntimeException->Exception->Throwable. // SoapFault->Fault means SoapFault class extends Fault class
      UncheckedException.getMessage:
      public String getMessage() {
      if (null != message)

      { return message.toString(); }

      return null;
      }
      Throwable.getMessage:
      public String getMessage()

      { return detailMessage; }

        Activity

        Bin Zhu created issue -
        Aki Yoshida made changes -
        Field Original Value New Value
        Assignee Aki Yoshida [ ay ]
        Hide
        Aki Yoshida added a comment -

        I was not sure if the jaxws spec section 10.2.2 was implying this faultstring rule of ex.toString() to be used only for unexpected service endpoints excetptions or also for handlers exceptions.

        http://mail-archives.apache.org/mod_mbox/cxf-dev/201212.mbox/%3CCAF8t5XtpOovD10k-nVGy%2BGbPH1kzVRhn%3D0qnFXdCs1EhrKKbbQ%40mail.gmail.com%3E

        I didn't get a definitive answer to it.

        I checked what RI 2.2 is doing and it uses toString() for handler exceptions as well.

        So we can resolve this issue by making CXF follow the same rule.

        Show
        Aki Yoshida added a comment - I was not sure if the jaxws spec section 10.2.2 was implying this faultstring rule of ex.toString() to be used only for unexpected service endpoints excetptions or also for handlers exceptions. http://mail-archives.apache.org/mod_mbox/cxf-dev/201212.mbox/%3CCAF8t5XtpOovD10k-nVGy%2BGbPH1kzVRhn%3D0qnFXdCs1EhrKKbbQ%40mail.gmail.com%3E I didn't get a definitive answer to it. I checked what RI 2.2 is doing and it uses toString() for handler exceptions as well. So we can resolve this issue by making CXF follow the same rule.
        Bin Zhu made changes -
        Attachment CXF-4684.patch [ 12560720 ]
        Hide
        Bin Zhu added a comment -

        Thanks Aki for the investigation.
        My assumption is that we can fix the issue in UncheckedException.java.
        I attached the patch and can you help review it?

        Show
        Bin Zhu added a comment - Thanks Aki for the investigation. My assumption is that we can fix the issue in UncheckedException.java. I attached the patch and can you help review it?
        Hide
        Aki Yoshida added a comment -

        Hi Bin,
        Thanks for spotting this issue and providing a patch.

        But I don't think your change in UncheckedException will work if it calls its super's getMessage() because that is null. You wanted to call toString(), no? However, that will also not work, as this getMessage method is overwritten by its extended class like CXF's Fault. Besides, UncheckedException is too deep in the layer (almost like java.lang.RuntimeException) and we shouldn't be touching it to just comply to a rule given for a higher layer like jaxws.

        The change should go into either CXF's Fault itself or where the soap fault is serialized. I had an impression that changing the soap fault serialization part is simpler than changing Fault itself when I commented on this issue sometime ago at users@cxf. I will look at it again today.

        regards, aki

        Show
        Aki Yoshida added a comment - Hi Bin, Thanks for spotting this issue and providing a patch. But I don't think your change in UncheckedException will work if it calls its super's getMessage() because that is null. You wanted to call toString(), no? However, that will also not work, as this getMessage method is overwritten by its extended class like CXF's Fault. Besides, UncheckedException is too deep in the layer (almost like java.lang.RuntimeException) and we shouldn't be touching it to just comply to a rule given for a higher layer like jaxws. The change should go into either CXF's Fault itself or where the soap fault is serialized. I had an impression that changing the soap fault serialization part is simpler than changing Fault itself when I commented on this issue sometime ago at users@cxf. I will look at it again today. regards, aki
        Hide
        Aki Yoshida added a comment -

        Hi Bin,
        I submitted the correction in trunk (2.7.2-SNAPSHOT) and 2.6.x (2.6.5-SNAPSHOT).
        Could you try it out to see if it works for your case?
        Thanks.
        Regards, Aki

        Show
        Aki Yoshida added a comment - Hi Bin, I submitted the correction in trunk (2.7.2-SNAPSHOT) and 2.6.x (2.6.5-SNAPSHOT). Could you try it out to see if it works for your case? Thanks. Regards, Aki
        Hide
        Bin Zhu added a comment -

        Hi Aki,
        I tested the code you submitted and it works in my scenario. Thanks.

        Show
        Bin Zhu added a comment - Hi Aki, I tested the code you submitted and it works in my scenario. Thanks.
        Hide
        Glen Mazza added a comment -

        Wait a second...we're not returning server-side error specifics to the SOAP client are we? That would be a security bug, allowing hackers to see the internal implementation of the web service provider. For unexpected internal errors like NPE's, database failures, etc. SOAP clients should be getting a generic "Fault occurred while processing" while the server-side log should detail what the specific problem is.

        Show
        Glen Mazza added a comment - Wait a second...we're not returning server-side error specifics to the SOAP client are we? That would be a security bug, allowing hackers to see the internal implementation of the web service provider. For unexpected internal errors like NPE's, database failures, etc. SOAP clients should be getting a generic "Fault occurred while processing" while the server-side log should detail what the specific problem is.
        Hide
        Aki Yoshida added a comment -

        Hi Glen,
        Yes. This touches exactly that security point. There was a discussion thread started by Bin.

        http://cxf.547215.n5.nabble.com/SOAPFault-message-improvement-in-CXF-when-there-is-unchecked-NPE-tc5719398.html#a5719568

        Initially I suggested for the exceptionCauseEnabled property. However, this property only wrote the cause message but not the cause itself (e.g. NPE). In addition, Ivan pointed us to the jaxws spec section 10.2.2 that specifies that the faultstring must come from the cause.toString() in some cases.

        So, I asked at dev@cxf asking the interpretation of this rule.
        http://cxf.547215.n5.nabble.com/interpretation-of-jaxws-10-2-2-exception-handling-tc5719946.html#a5720008

        The rest is stated in my comment from Dec 12 above.

        Personally, I think the jaxws spec could have left this toString rule out of its normative part. I am not really concerned about just writing the class name like NPE. But I am more concerned about some unknown user extended runtime exception that overwrites its toString to do more than writing its class name.

        However, this rule is there, so we need to comply to it right? Or supposing the jaxws 10.2.2 itself is a security issue, should we only enable this rule with an option? That was the point of my original question in dev@cxf. Any definitive answers will be appreciated.

        Thanks.
        regards, aki

        Show
        Aki Yoshida added a comment - Hi Glen, Yes. This touches exactly that security point. There was a discussion thread started by Bin. http://cxf.547215.n5.nabble.com/SOAPFault-message-improvement-in-CXF-when-there-is-unchecked-NPE-tc5719398.html#a5719568 Initially I suggested for the exceptionCauseEnabled property. However, this property only wrote the cause message but not the cause itself (e.g. NPE). In addition, Ivan pointed us to the jaxws spec section 10.2.2 that specifies that the faultstring must come from the cause.toString() in some cases. So, I asked at dev@cxf asking the interpretation of this rule. http://cxf.547215.n5.nabble.com/interpretation-of-jaxws-10-2-2-exception-handling-tc5719946.html#a5720008 The rest is stated in my comment from Dec 12 above. Personally, I think the jaxws spec could have left this toString rule out of its normative part. I am not really concerned about just writing the class name like NPE. But I am more concerned about some unknown user extended runtime exception that overwrites its toString to do more than writing its class name. However, this rule is there, so we need to comply to it right? Or supposing the jaxws 10.2.2 itself is a security issue, should we only enable this rule with an option? That was the point of my original question in dev@cxf. Any definitive answers will be appreciated. Thanks. regards, aki
        Hide
        Glen Mazza added a comment -

        Hi Aki, sorry I hadn't gotten to your dev list message, and my knowledge in this area is incomplete. The fact that the original submitter of this bug wrote "The SOAPFault in CXF will only throw the 'Fault occurred while processing.' message rather than the original NPE message." shows he doesn't understand the Pen-testing 101 rule not to pass service-side exception bug details back to the client. He didn't mention anything about the JAX-WS specification, so that wasn't his motivation.

        Why is his motivation important? Right now, he apparently feels that if you have a database space error, somehow the SOAP client should receive a message like "Wizard RDMBS error: Table AdminAccountName.MY_STUFF on server camel07 is full, please expand more space for it." That's a training issue, and once he understands that's a no-no, I suspect he would be likely going to request a return to 'Fault occurred while processing' again, rather than defend the new implementation based on the apparent rule in JAX-WS 10.2.2. Very few people indeed truly want service-side error messages propagated back to the client (incidentally, NPEs are embarrassing for all concerned and developers don't like it when their web service stacks embarrass them to their customers in such a manner.)

        Further, "NullPointerException" is a Java-language construct, while SOAP is language-independent. There shouldn't be any need to return Java-language constructs to the client, as that would incorrectly imply the SOAP client base may base logic on the fact that the SOAP service is implemented in Java.

        I'm confused a little about your statement: "But I am more concerned about some unknown user extended runtime exception that overwrites its toString to do more than writing its class name." Does this new change facilitate that (they can override toString() now?) or hinder that (now if they tried to overwrite it, the client will still get an "NPE" message)? If the latter, perhaps this change does more good than harm, but if the former, I would say CXF should tighten up and not allow this, to protect systems against sloppier developers putting too much info in their error messages back to the client.

        Anyway, CXF has several masters-the WSDL, JAX-WS, JAX-RS and WS-I Basic Profile (and Basic Security Profile) specs-and as you know, we can't serve more than one perfectly. I believe CXF has already tightened itself up in a few places w.r.t. the WS-I profiles, becoming invalid as a consequence with the more-easygoing WSDL and JAX-WS specs, both of which allow more than what WS-I does. This would appear to be something where going the safer route rather than strict compliance with any specific JAX-WS rule would appear justified. I don't think this change is vetoable but my instinct would be to revert it, because I think its going to happen anyway (someone several months down the road will complain about CXF improperly sending internal error messages to the client) but if you wait several months to revert you may have to deal with backwards compatibility issues of many SOAP clients now incorrectly basing logic based on specific Java class exceptions being returned from the web service provider.

        Show
        Glen Mazza added a comment - Hi Aki, sorry I hadn't gotten to your dev list message, and my knowledge in this area is incomplete. The fact that the original submitter of this bug wrote "The SOAPFault in CXF will only throw the 'Fault occurred while processing.' message rather than the original NPE message." shows he doesn't understand the Pen-testing 101 rule not to pass service-side exception bug details back to the client. He didn't mention anything about the JAX-WS specification, so that wasn't his motivation. Why is his motivation important? Right now, he apparently feels that if you have a database space error, somehow the SOAP client should receive a message like "Wizard RDMBS error: Table AdminAccountName.MY_STUFF on server camel07 is full, please expand more space for it." That's a training issue, and once he understands that's a no-no, I suspect he would be likely going to request a return to 'Fault occurred while processing' again, rather than defend the new implementation based on the apparent rule in JAX-WS 10.2.2. Very few people indeed truly want service-side error messages propagated back to the client (incidentally, NPEs are embarrassing for all concerned and developers don't like it when their web service stacks embarrass them to their customers in such a manner.) Further, "NullPointerException" is a Java-language construct, while SOAP is language-independent. There shouldn't be any need to return Java-language constructs to the client, as that would incorrectly imply the SOAP client base may base logic on the fact that the SOAP service is implemented in Java. I'm confused a little about your statement: "But I am more concerned about some unknown user extended runtime exception that overwrites its toString to do more than writing its class name." Does this new change facilitate that (they can override toString() now?) or hinder that (now if they tried to overwrite it, the client will still get an "NPE" message)? If the latter, perhaps this change does more good than harm, but if the former, I would say CXF should tighten up and not allow this, to protect systems against sloppier developers putting too much info in their error messages back to the client. Anyway, CXF has several masters- the WSDL, JAX-WS, JAX-RS and WS-I Basic Profile (and Basic Security Profile) specs -and as you know, we can't serve more than one perfectly. I believe CXF has already tightened itself up in a few places w.r.t. the WS-I profiles, becoming invalid as a consequence with the more-easygoing WSDL and JAX-WS specs, both of which allow more than what WS-I does. This would appear to be something where going the safer route rather than strict compliance with any specific JAX-WS rule would appear justified. I don't think this change is vetoable but my instinct would be to revert it, because I think its going to happen anyway (someone several months down the road will complain about CXF improperly sending internal error messages to the client) but if you wait several months to revert you may have to deal with backwards compatibility issues of many SOAP clients now incorrectly basing logic based on specific Java class exceptions being returned from the web service provider.
        Hide
        Aki Yoshida added a comment -

        Hi Glen,
        I agree with you that there is no point in setting the faultstring with e.toString() except under some debugging purpose and the jaxws spec shouldn't have included this rule. I expressed my concern which you quoted "But I am more ...". The change does not facilitate nor hinder overriding of the exception. It simply applies this faulstring rule that can lead to not only those internal exception message being exposed as you mentioned but also to some unknown consequences depending on how the toString of the relevant runtime exception is implemented.

        So, I am perfectly fine with optionally enabling this rule using a configuration property. In fact, if there is a vote to take, I choose for this option. Maybe we could reuse the enableCause thing to align its behavior with this jaxws rule under some condition. The current behavior of enableCause thing is not practical in some situations (e.g. when cause.getMessage() is null).

        I'll look at it again to see what we can do.
        regards, aki

        Show
        Aki Yoshida added a comment - Hi Glen, I agree with you that there is no point in setting the faultstring with e.toString() except under some debugging purpose and the jaxws spec shouldn't have included this rule. I expressed my concern which you quoted "But I am more ...". The change does not facilitate nor hinder overriding of the exception. It simply applies this faulstring rule that can lead to not only those internal exception message being exposed as you mentioned but also to some unknown consequences depending on how the toString of the relevant runtime exception is implemented. So, I am perfectly fine with optionally enabling this rule using a configuration property. In fact, if there is a vote to take, I choose for this option. Maybe we could reuse the enableCause thing to align its behavior with this jaxws rule under some condition. The current behavior of enableCause thing is not practical in some situations (e.g. when cause.getMessage() is null). I'll look at it again to see what we can do. regards, aki
        Hide
        Glen Mazza added a comment -

        Thanks. A configuration property seems like a very workable idea; if you go that route you might wish to default it to what it was before this patch was applied, lest we wait and end up needing to default it to the less-secure option out of concern for backwards compatibility. Also, by keeping "Fault occurred while processing" as the default we can get people wanting something else to show up on the mailing list, where we can (1) inform them of this new property and (2) make sure they are in fact aware that they shouldn't be sending sensitive internal error messages back to the client (if that is, in fact, their intention.)

        Show
        Glen Mazza added a comment - Thanks. A configuration property seems like a very workable idea; if you go that route you might wish to default it to what it was before this patch was applied, lest we wait and end up needing to default it to the less-secure option out of concern for backwards compatibility. Also, by keeping "Fault occurred while processing" as the default we can get people wanting something else to show up on the mailing list, where we can (1) inform them of this new property and (2) make sure they are in fact aware that they shouldn't be sending sensitive internal error messages back to the client (if that is, in fact, their intention.)
        Hide
        Aki Yoshida added a comment -

        Hi Bin,
        As you probably followed the discussion above, I modified the previous change so that you can still configure CXF to construct the faulstring using cause.toString() for no-message runtime exception without the message but keep us all on the safer side from the security perspective.

        In short, to enable this configuration, you can set the endpoint property exceptionMessageCauseEnabled as described in

        http://cxf.apache.org/docs/debugging-and-logging.html#DebuggingandLogging-Stacktraceinfaultdetails

        Programmatically, you can use the following constant from Message, as in

        ep.getProperties().put(org.apache.cxf.message.Message.EXCEPTION_MESSAGE_CAUSE_ENABLED, "true")

        I hope you are satisfied with solution.

        Thanks.
        regards, aki

        Show
        Aki Yoshida added a comment - Hi Bin, As you probably followed the discussion above, I modified the previous change so that you can still configure CXF to construct the faulstring using cause.toString() for no-message runtime exception without the message but keep us all on the safer side from the security perspective. In short, to enable this configuration, you can set the endpoint property exceptionMessageCauseEnabled as described in http://cxf.apache.org/docs/debugging-and-logging.html#DebuggingandLogging-Stacktraceinfaultdetails Programmatically, you can use the following constant from Message, as in ep.getProperties().put(org.apache.cxf.message.Message.EXCEPTION_MESSAGE_CAUSE_ENABLED, "true") I hope you are satisfied with solution. Thanks. regards, aki
        Hide
        Glen Mazza added a comment -

        Thanks for all your attention to this, Aki. Yoku dekimashita!

        Show
        Glen Mazza added a comment - Thanks for all your attention to this, Aki. Yoku dekimashita!
        Aki Yoshida made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 2.5.8 [ 12323606 ]
        Fix Version/s 2.6.5 [ 12323605 ]
        Fix Version/s 2.7.2 [ 12323604 ]
        Resolution Fixed [ 1 ]
        Hide
        Bin Zhu added a comment -

        Hi Aki,
        Sorry for the late response. The final changes looks fine for me. Many thanks to all of you.

        Show
        Bin Zhu added a comment - Hi Aki, Sorry for the late response. The final changes looks fine for me. Many thanks to all of you.
        Daniel Kulp made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Aki Yoshida
            Reporter:
            Bin Zhu
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development