Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-6207

Anyone can view any Request or Quote

    Details

      Description

      This is a security bug in the ecommerce application. Anyone can view any quote or request in the system regardless of the associated partyId. They can do this via URL parameter manipulation.

      Reproduction:
      1) Login to the ecommerce application as DemoCustomer.
      2) Navigate to http://demo-stable-ofbiz.apache.org/ecommerce/control/ViewRequest?custRequestId=9000 to view your own request.
      3) Navigate to http://demo-stable-ofbiz.apache.org/ecommerce/control/ViewRequest?custRequestId=9001 to view DemoCustAgent's request.
      4) Navigate to http://demo-stable-ofbiz.apache.org/ecommerce/control/ViewRequest?custRequestId=9002 to view DemoCustomer2's request.

      Same goes for Quotes, although there are no quotes in the Demo data. The attach patch fixes this issue.

      Would like this issue back ported to release 13.07 please.

        Activity

        Hide
        fbr@14x.net Forrest Rae added a comment -

        Fixes issue.

        Show
        fbr@14x.net Forrest Rae added a comment - Fixes issue.
        Hide
        pfm.smits Pierre Smits added a comment -

        If you have confirmed that this issue applies to a r13.07.x, please change the 'affected version, to have it included in the list.

        Show
        pfm.smits Pierre Smits added a comment - If you have confirmed that this issue applies to a r13.07.x, please change the 'affected version, to have it included in the list.
        Hide
        fbr@14x.net Forrest Rae added a comment -

        Turns out my patch isn't correct. The location of the condition in the "ViewRequest" screen is out of place. Please don't apply yet.

        Show
        fbr@14x.net Forrest Rae added a comment - Turns out my patch isn't correct. The location of the condition in the "ViewRequest" screen is out of place. Please don't apply yet.
        Hide
        fbr@14x.net Forrest Rae added a comment -

        I just attached OFBIZ-6207-second-attempt.patch. This patch should work cleaner.

        Show
        fbr@14x.net Forrest Rae added a comment - I just attached OFBIZ-6207 -second-attempt.patch. This patch should work cleaner.
        Hide
        fbr@14x.net Forrest Rae added a comment -

        Although, when applied to the 13.07 branch, it seems to cause an error. Strange because it works great applied to Trunk.

        [java] 2015-03-24 18:06:22,012 |-0.0.0.0-8443-exec-3 |PrimaryKeyFinder |E| Error finding entity value by primary key with entity-one: org.ofbiz.entity.GenericModelException: [GenericDelegator.findOne] Passed primary key is not a valid primary key: [GenericEntity:CustRequest][custRequestId,9000(java.lang.String)][fromPartyId,DemoCustomer(java.lang.String)]
        [java] org.ofbiz.entity.GenericModelException: [GenericDelegator.findOne] Passed primary key is not a valid primary key: [GenericEntity:CustRequest][custRequestId,9000(java.lang.String)][fromPartyId,DemoCustomer(java.lang.String)]
        [java] at org.ofbiz.entity.GenericDelegator.findOne(GenericDelegator.java:1483) ~[ofbiz-entity.jar:?]
        [java] at org.ofbiz.entity.finder.PrimaryKeyFinder.runFind(PrimaryKeyFinder.java:154) [ofbiz-entity.jar:?]
        [java] at org.ofbiz.entity.finder.PrimaryKeyFinder.runFind(PrimaryKeyFinder.java:86) [ofbiz-entity.jar:?]
        [java] at org.ofbiz.widget.ModelWidgetAction$EntityOne.runAction(ModelWidgetAction.java:511) [ofbiz-widget.jar:?]
        [java] at org.ofbiz.widget.ModelWidgetAction.runSubActions(ModelWidgetAction.java:114) [ofbiz-widget.jar:?]
        [java] at org.ofbiz.widget.screen.ModelScreenWidget$Section.renderWidgetString(ModelScreenWidget.java:186) [ofbiz-widget.jar:?]
        [java] at org.ofbiz.widget.screen.ModelScreen.renderScreenString(ModelScreen.java:396) [ofbiz-widget.jar:?]
        [java] at org.ofbiz.widget.screen.ScreenRenderer.render(ScreenRenderer.java:135) [ofbiz-widget.jar:?]
        [java] at org.ofbiz.widget.screen.ScreenRenderer.render(ScreenRenderer.java:97) [ofbiz-widget.jar:?]
        [java] at org.ofbiz.widget.screen.MacroScreenViewHandler.render(MacroScreenViewHandler.java:104) [ofbiz-widget.jar:?]
        [java] at org.ofbiz.webapp.control.RequestHandler.renderView(RequestHandler.java:916) [ofbiz-webapp.jar:?]
        [java] at org.ofbiz.webapp.control.RequestHandler.doRequest(RequestHandler.java:614) [ofbiz-webapp.jar:?]
        [java] at org.ofbiz.webapp.control.ControlServlet.doGet(ControlServlet.java:218) [ofbiz-webapp.jar:?]

        Show
        fbr@14x.net Forrest Rae added a comment - Although, when applied to the 13.07 branch, it seems to cause an error. Strange because it works great applied to Trunk. [java] 2015-03-24 18:06:22,012 |-0.0.0.0-8443-exec-3 |PrimaryKeyFinder |E| Error finding entity value by primary key with entity-one: org.ofbiz.entity.GenericModelException: [GenericDelegator.findOne] Passed primary key is not a valid primary key: [GenericEntity:CustRequest] [custRequestId,9000(java.lang.String)] [fromPartyId,DemoCustomer(java.lang.String)] [java] org.ofbiz.entity.GenericModelException: [GenericDelegator.findOne] Passed primary key is not a valid primary key: [GenericEntity:CustRequest] [custRequestId,9000(java.lang.String)] [fromPartyId,DemoCustomer(java.lang.String)] [java] at org.ofbiz.entity.GenericDelegator.findOne(GenericDelegator.java:1483) ~ [ofbiz-entity.jar:?] [java] at org.ofbiz.entity.finder.PrimaryKeyFinder.runFind(PrimaryKeyFinder.java:154) [ofbiz-entity.jar:?] [java] at org.ofbiz.entity.finder.PrimaryKeyFinder.runFind(PrimaryKeyFinder.java:86) [ofbiz-entity.jar:?] [java] at org.ofbiz.widget.ModelWidgetAction$EntityOne.runAction(ModelWidgetAction.java:511) [ofbiz-widget.jar:?] [java] at org.ofbiz.widget.ModelWidgetAction.runSubActions(ModelWidgetAction.java:114) [ofbiz-widget.jar:?] [java] at org.ofbiz.widget.screen.ModelScreenWidget$Section.renderWidgetString(ModelScreenWidget.java:186) [ofbiz-widget.jar:?] [java] at org.ofbiz.widget.screen.ModelScreen.renderScreenString(ModelScreen.java:396) [ofbiz-widget.jar:?] [java] at org.ofbiz.widget.screen.ScreenRenderer.render(ScreenRenderer.java:135) [ofbiz-widget.jar:?] [java] at org.ofbiz.widget.screen.ScreenRenderer.render(ScreenRenderer.java:97) [ofbiz-widget.jar:?] [java] at org.ofbiz.widget.screen.MacroScreenViewHandler.render(MacroScreenViewHandler.java:104) [ofbiz-widget.jar:?] [java] at org.ofbiz.webapp.control.RequestHandler.renderView(RequestHandler.java:916) [ofbiz-webapp.jar:?] [java] at org.ofbiz.webapp.control.RequestHandler.doRequest(RequestHandler.java:614) [ofbiz-webapp.jar:?] [java] at org.ofbiz.webapp.control.ControlServlet.doGet(ControlServlet.java:218) [ofbiz-webapp.jar:?]
        Hide
        pfm.smits Pierre Smits added a comment -

        Hi Forrest,

        If you are going to have a patch for trunk and a patch for r13.07, please have the filenames of the patches reflect the appropriate version. Otherwise other contributors might apply the wrong patch to the wrong branch.

        Best regards,

        Pierre

        Show
        pfm.smits Pierre Smits added a comment - Hi Forrest, If you are going to have a patch for trunk and a patch for r13.07, please have the filenames of the patches reflect the appropriate version. Otherwise other contributors might apply the wrong patch to the wrong branch. Best regards, Pierre
        Hide
        fbr@14x.net Forrest Rae added a comment -

        Honestly, I am a bit lost as to why this isn't working against 13.07. I didn't intend to have a patch for each.

        Show
        fbr@14x.net Forrest Rae added a comment - Honestly, I am a bit lost as to why this isn't working against 13.07. I didn't intend to have a patch for each.
        Hide
        fbr@14x.net Forrest Rae added a comment -

        So, in 13.07, <entity-one> in the widget will call GenericDelegator.findOne(). This checks that you're only using primary keys for the search. My patch seems to cause a problem as it was using partyId as part of the search criteria. partyId is not a PK of Quote.

        What is the recommended way of checking the Quote.partyId field to make sure it matches userLogin.partyId in the actions section of the specialpurpose/ecommerce/widget/QuoteScreens.xml#ViewQuote screen?

        None of this matters in Trunk as the <entity-one> in the screen never results in a call to GenericDelegator.findOne().

        Show
        fbr@14x.net Forrest Rae added a comment - So, in 13.07, <entity-one> in the widget will call GenericDelegator.findOne(). This checks that you're only using primary keys for the search. My patch seems to cause a problem as it was using partyId as part of the search criteria. partyId is not a PK of Quote. What is the recommended way of checking the Quote.partyId field to make sure it matches userLogin.partyId in the actions section of the specialpurpose/ecommerce/widget/QuoteScreens.xml#ViewQuote screen? None of this matters in Trunk as the <entity-one> in the screen never results in a call to GenericDelegator.findOne().
        Hide
        fbr@14x.net Forrest Rae added a comment -

        Figured it out:

        <entity-one entity-name="Quote" value-field="quote1">
        <field-map field-name="quoteId" from-field="parameters.quoteId"/>
        </entity-one>
        <set field="quote" value="$

        {groovy: if(quote1.partyId == userLogin.partyId) quote = quote1;}

        "/>

        Show
        fbr@14x.net Forrest Rae added a comment - Figured it out: <entity-one entity-name="Quote" value-field="quote1"> <field-map field-name="quoteId" from-field="parameters.quoteId"/> </entity-one> <set field="quote" value="$ {groovy: if(quote1.partyId == userLogin.partyId) quote = quote1;} "/>
        Hide
        fbr@14x.net Forrest Rae added a comment -

        Third attempt at a patch that works in both Trunk and 13.07. I believe this one is final.

        Show
        fbr@14x.net Forrest Rae added a comment - Third attempt at a patch that works in both Trunk and 13.07. I believe this one is final.
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Hi Forrest Rae,

        You can use "condition" tag to check condition in screen widget. IMO we dont need to create new screen "ViewRequestTemplate"

        Show
        deepak.dixit Deepak Dixit added a comment - Hi Forrest Rae, You can use "condition" tag to check condition in screen widget. IMO we dont need to create new screen "ViewRequestTemplate"
        Hide
        fbr@14x.net Forrest Rae added a comment -

        Deepak,

        I'll try again tomorrow AM, but I'm fairly sure I tried that. Can you list an example in this context, or maybe post a different patch? It'd help me with learning how to do these types of things properly in OFBIZ.

        Show
        fbr@14x.net Forrest Rae added a comment - Deepak, I'll try again tomorrow AM, but I'm fairly sure I tried that. Can you list an example in this context, or maybe post a different patch? It'd help me with learning how to do these types of things properly in OFBIZ.
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Sure Forrest,

        BTW you can refer specialpurpose/ecommerce/widget/CustomerScreens.xml#digitalproductlist or specialpurpose/ecommerce/widget/CustomerScreens.xml#digitalproductlist#messagelist-include

        Show
        deepak.dixit Deepak Dixit added a comment - Sure Forrest, BTW you can refer specialpurpose/ecommerce/widget/CustomerScreens.xml#digitalproductlist or specialpurpose/ecommerce/widget/CustomerScreens.xml#digitalproductlist#messagelist-include
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        It would be best to protect the screen with a permission service - preferably the same one that guards the Request/Quote services.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - It would be best to protect the screen with a permission service - preferably the same one that guards the Request/Quote services.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        +1

        Show
        jacques.le.roux Jacques Le Roux added a comment - +1
        Hide
        deepak.dixit Deepak Dixit added a comment -

        +1

        Show
        deepak.dixit Deepak Dixit added a comment - +1
        Hide
        fbr@14x.net Forrest Rae added a comment -

        Adrian, Can you point me to an example and I'll take a look?

        Show
        fbr@14x.net Forrest Rae added a comment - Adrian, Can you point me to an example and I'll take a look?
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Permission services implement the permissionInterface service interface:

        https://localhost:8443/webtools/control/ServiceList?sel_service_name=permissionInterface

        A very good example is the fixedAssetPermissionCheck service. You can see it guarding Fixed Assets services in services_fixedasset.xml and it also guards screens in FixedAssetScreens.xml.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Permission services implement the permissionInterface service interface: https://localhost:8443/webtools/control/ServiceList?sel_service_name=permissionInterface A very good example is the fixedAssetPermissionCheck service. You can see it guarding Fixed Assets services in services_fixedasset.xml and it also guards screens in FixedAssetScreens.xml.
        Hide
        fbr@14x.net Forrest Rae added a comment -

        I got it working using this suggestion, which I agree is much cleaner than dropping down to groovy via the 'set" tag.

        Show
        fbr@14x.net Forrest Rae added a comment - I got it working using this suggestion, which I agree is much cleaner than dropping down to groovy via the 'set" tag.
        Hide
        fbr@14x.net Forrest Rae added a comment -

        Thanks Adrian. I explored this quite a bit. The whole framework looks geared to Application level permissions rather than what I'm trying to address, which is entity level access control.

        Show
        fbr@14x.net Forrest Rae added a comment - Thanks Adrian. I explored this quite a bit. The whole framework looks geared to Application level permissions rather than what I'm trying to address, which is entity level access control.
        Hide
        fbr@14x.net Forrest Rae added a comment -

        If I wanted to leverage permission services in Request/Quote services such as custRequestPermissionCheck, it seems I would need to pass in a "fromPartyId" argument, but can't seem to figure out how to do that:

        <if-service-permission service-name="custRequestPermissionCheck" fromPartyId="CustRequest.fromPartyId"/>

        errors:

        Error message: cvc-complex-type.3.2.2: Attribute 'fromPartyId' is not allowed to appear in element 'if-service-permission'.

        Show
        fbr@14x.net Forrest Rae added a comment - If I wanted to leverage permission services in Request/Quote services such as custRequestPermissionCheck, it seems I would need to pass in a "fromPartyId" argument, but can't seem to figure out how to do that: <if-service-permission service-name="custRequestPermissionCheck" fromPartyId="CustRequest.fromPartyId"/> errors: Error message: cvc-complex-type.3.2.2: Attribute 'fromPartyId' is not allowed to appear in element 'if-service-permission'.
        Hide
        fbr@14x.net Forrest Rae added a comment -

        Fourth patch uses condition tag as suggested by Deepak.

        Show
        fbr@14x.net Forrest Rae added a comment - Fourth patch uses condition tag as suggested by Deepak.
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Hi Forrest Raae,

        You can pass in parameters as context-map.
        Here is exa

        <if-service-permission service-name="workEffortICalendarPermission" main-action="CREATE" context-map="parameters"/>

        Show
        deepak.dixit Deepak Dixit added a comment - Hi Forrest Raae, You can pass in parameters as context-map. Here is exa <if-service-permission service-name="workEffortICalendarPermission" main-action="CREATE" context-map="parameters"/>
        Show
        jacques.le.roux Jacques Le Roux added a comment - Forrest Raae, did you read https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Security+Permissions ?
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Hi Adrian Crum,

        I think this is different case, In this case if any request or quote does not belongs to logged in user then also he can view request/quote by changing the id from url.
        If we add VIEW permission check then also use can able to view others request/quote as well.

        It can't be handle by if-service-permission.
        For order view (ecommerce) logged in party id comparison has been checked in orderstatus.groovy. If logged in party exist in any order role then only user can view the order.

        We can create common service to perform check if order/request/quote belongs to logged in part then only user can view else error message will be displayed.

        Show
        deepak.dixit Deepak Dixit added a comment - Hi Adrian Crum , I think this is different case, In this case if any request or quote does not belongs to logged in user then also he can view request/quote by changing the id from url. If we add VIEW permission check then also use can able to view others request/quote as well. It can't be handle by if-service-permission. For order view (ecommerce) logged in party id comparison has been checked in orderstatus.groovy. If logged in party exist in any order role then only user can view the order. We can create common service to perform check if order/request/quote belongs to logged in part then only user can view else error message will be displayed.
        Hide
        deepakdixit Deepak Dixit (Inactive) added a comment -

        Thanks Forrest Rae for the but report and patch.

        Ps: Please do not mix formatting changes with functional changes, its difficult to review the patch

        Show
        deepakdixit Deepak Dixit (Inactive) added a comment - Thanks Forrest Rae for the but report and patch. Ps: Please do not mix formatting changes with functional changes, its difficult to review the patch
        Hide
        fbr@14x.net Forrest Rae added a comment -

        I looked around on the wiki for guidelines on creating bugs/patches but didn't see anything. Is there guidelines on patch creation that I'm just not seeing?

        Show
        fbr@14x.net Forrest Rae added a comment - I looked around on the wiki for guidelines on creating bugs/patches but didn't see anything. Is there guidelines on patch creation that I'm just not seeing?
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        See "How to Send in Your Contributions (or how to create and apply patches)" section at https://cwiki.apache.org/confluence/display/OFBADMIN/OFBiz+Contributors+Best+Practices

        Show
        jacques.le.roux Jacques Le Roux added a comment - See "How to Send in Your Contributions (or how to create and apply patches)" section at https://cwiki.apache.org/confluence/display/OFBADMIN/OFBiz+Contributors+Best+Practices
        Hide
        fbr@14x.net Forrest Rae added a comment -

        I've read that and don't see anything about splitting patches into multiple files, unless I'm blind (there is always that possibility ). If you'd like me to do that, I will. I'll also update the wiki with instructions once I've been through the process.

        Show
        fbr@14x.net Forrest Rae added a comment - I've read that and don't see anything about splitting patches into multiple files, unless I'm blind (there is always that possibility ). If you'd like me to do that, I will. I'll also update the wiki with instructions once I've been through the process.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Yes, that would be good indeed

        Show
        jacques.le.roux Jacques Le Roux added a comment - Yes, that would be good indeed

          People

          • Assignee:
            deepak.dixit Deepak Dixit
            Reporter:
            fbr@14x.net Forrest Rae
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development