Hello,
While going through credit card entry ftl’s, I came across applications/party/webapp/partymgr/party/editcreditcard.ftl which contains the following line <input type="hidden" name="partyId" value="${partyId}"/> I could be missing something here, but it sure looks like a security risk to me. Granted that this ftl is probably designed to be used only for Party Manager part of Webtools and not for a “public” application, but even that is not a good thing from code reuse point of view.
Regards, Vinay Agarwal _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
Another similar case in applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl which contains <input type="hidden" name="paymentMethodId" value="${paymentMethodId}"> And this application is designed for public use. What am I missing here?
Regards, Vinay Agarwal
-----Original Message-----
Hello,
While going through credit card entry ftl’s, I came across applications/party/webapp/partymgr/party/editcreditcard.ftl which contains the following line <input type="hidden" name="partyId" value="${partyId}"/> I could be missing something here, but it sure looks like a security risk to me. Granted that this ftl is probably designed to be used only for Party Manager part of Webtools and not for a “public” application, but even that is not a good thing from code reuse point of view.
Regards, Vinay Agarwal _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
Vinay,
As a hacker, which I'm sure you're not, what would you actually do with that data? On Fri, 2006-02-10 at 08:23 -0800, Vinay Agarwal wrote: > Another similar case in > applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl > which contains > > <input type="hidden" name="paymentMethodId" > value="${paymentMethodId}"> > > And this application is designed for public use. What am I missing > here? > > > > Regards, > > Vinay Agarwal > > > > -----Original Message----- > From: Vinay Agarwal [mailto:[hidden email]] > Sent: Friday, February 10, 2006 8:17 AM > To: 'OFBiz Users / Usage Discussion' > Subject: Hidden partyId - Security Risk? > > > > Hello, > > > > While going through credit card entry ftl’s, I came across > applications/party/webapp/partymgr/party/editcreditcard.ftl which > contains the following line > > <input type="hidden" name="partyId" value="${partyId}"/> > > I could be missing something here, but it sure looks like a security > risk to me. Granted that this ftl is probably designed to be used only > for Party Manager part of Webtools and not for a “public” application, > but even that is not a good thing from code reuse point of view. > > > > Regards, > > Vinay Agarwal > > > _______________________________________________ > Users mailing list > [hidden email] > http://lists.ofbiz.org/mailman/listinfo/users Kind Regards Andrew Sykes <[hidden email]> Sykes Development Ltd http://www.sykesdevelopment.com _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
In reply to this post by Vinay Agarwal
I think the back end services they call will verify the user and
permissions. If not, then it definitely would be a risk. Si Vinay Agarwal wrote: > Another similar case in > applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl > which contains > > <input type="hidden" name="paymentMethodId" value="${paymentMethodId}"> > > And this application is designed for public use. What am I missing here? > > Regards, > > Vinay Agarwal > > -----Original Message----- > *From:* Vinay Agarwal [mailto:[hidden email]] > *Sent:* Friday, February 10, 2006 8:17 AM > *To:* 'OFBiz Users / Usage Discussion' > *Subject:* Hidden partyId - Security Risk? > > Hello, > > While going through credit card entry ftl’s, I came across > applications/party/webapp/partymgr/party/editcreditcard.ftl which > contains the following line > > <input type="hidden" name="partyId" value="${partyId}"/> > > I could be missing something here, but it sure looks like a security > risk to me. Granted that this ftl is probably designed to be used only > for Party Manager part of Webtools and not for a “public” application, > but even that is not a good thing from code reuse point of view. > > Regards, > > Vinay Agarwal > >------------------------------------------------------------------------ > > >_______________________________________________ >Users mailing list >[hidden email] >http://lists.ofbiz.org/mailman/listinfo/users > _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
In reply to this post by Vinay Agarwal
both of those are ofbiz Id's without the corresponding data, in the
database, this is useless information. if some tried to post this to ofbiz, they would run into the certificate, then User privileges, the Security before any information would be revealed about the cc. The information, since ver 3.1, is encrypted in the DB so it would be difficult, or impossible to retrieve such data. I believe it would pass a security audit. Vinay Agarwal sent the following on 2/10/06 8:23 AM: > Another similar case in > applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl which > contains > > <input type="hidden" name="paymentMethodId" > value="${paymentMethodId}"> > > And this application is designed for public use. What am I missing here? > > > > Regards, > > Vinay Agarwal > > > > -----Original Message----- > From: Vinay Agarwal [mailto:[hidden email]] > Sent: Friday, February 10, 2006 8:17 AM > To: 'OFBiz Users / Usage Discussion' > Subject: Hidden partyId - Security Risk? > > > > Hello, > > > > While going through credit card entry ftl's, I came across > applications/party/webapp/partymgr/party/editcreditcard.ftl which contains > the following line > > <input type="hidden" name="partyId" value="${partyId}"/> > > I could be missing something here, but it sure looks like a security risk to > me. Granted that this ftl is probably designed to be used only for Party > Manager part of Webtools and not for a "public" application, but even that > is not a good thing from code reuse point of view. > > > > Regards, > > Vinay Agarwal > > > > > ------------------------------------------------------------------------ > > > _______________________________________________ > Users mailing list > [hidden email] > http://lists.ofbiz.org/mailman/listinfo/users _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
1. What could be done with it? Let’s take example of applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl file. One could save the html page to local disk. Then one could change the ${paymentMethodId} value using a text editor and post the page back. If the security checks don’t catch this, this will have effect of a user get ability to modify paymentMethods that do not belong to him/her. 2. Does the security check catch it? I am not sure if it catches it. Here are the details. updateCreditCard service is called which calls ServiceUtil.getPartyIdCheckSecurity(userLogin, security, context, result, "PAY_INFO", "_UPDATE"); This checks whether partyId in the incoming map matches the partyId of the userLogin. After that, it is calling if (!security.hasEntityPermission(secEntity, secOperation, userLogin)) { This function is defined in framework/security/src/org/ofbiz/security/OFBizSecurity.java. It checks for group permissions but I don’t know enough to tell whether it does or does not catch this.
Regards, Vinay Agarwal
-----Original Message-----
both of those are ofbiz Id's without the corresponding data, in the database, this is useless information. if some tried to post this to ofbiz, they would run into the certificate, then User privileges, the Security before any information would be revealed about the cc. The information, since ver 3.1, is encrypted in the DB so it would be difficult, or impossible to retrieve such data.
I believe it would pass a security audit.
Vinay Agarwal sent the following on 2/10/06 8:23 AM: > Another similar case in > applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl which > contains > > <input type="hidden" name="paymentMethodId" > value="${paymentMethodId}"> > > And this application is designed for public use. What am I missing here? > > > > Regards, > > Vinay Agarwal > > > > -----Original Message----- > From: Vinay Agarwal [mailto:[hidden email]] > Sent: Friday, February 10, 2006 8:17 AM > To: 'OFBiz Users / Usage Discussion' > Subject: Hidden partyId - Security Risk? > > > > Hello, > > > > While going through credit card entry ftl's, I came across > applications/party/webapp/partymgr/party/editcreditcard.ftl which contains > the following line > > <input type="hidden" name="partyId" value="${partyId}"/> > > I could be missing something here, but it sure looks like a security risk to > me. Granted that this ftl is probably designed to be used only for Party > Manager part of Webtools and not for a "public" application, but even that > is not a good thing from code reuse point of view. > > > > Regards, > > Vinay Agarwal > > > > > ------------------------------------------------------------------------ > > > _______________________________________________ > Users mailing list > http://lists.ofbiz.org/mailman/listinfo/users
_______________________________________________ Users mailing list http://lists.ofbiz.org/mailman/listinfo/users _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
In reply to this post by Vinay Agarwal
It should catch it... why not hack the form? Modify the .ftl, put in a
different paymentMethodId string, and then try it? We always think it should do this or that, but.... :) Vinay Agarwal wrote: > 1. What could be done with it? > > Let’s take example of > applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl > file. One could save the html page to local disk. Then one could > change the ${paymentMethodId} value using a text editor and post the > page back. If the security checks don’t catch this, this will have > effect of a user get ability to modify paymentMethods that do not > belong to him/her. > > 2. Does the security check catch it? I am not sure if it catches it. > Here are the details. > > updateCreditCard service is called which calls > > ServiceUtil.getPartyIdCheckSecurity(userLogin, security, context, > result, "PAY_INFO", "_UPDATE"); > > This checks whether partyId in the incoming map matches the partyId of > the userLogin. After that, it is calling > > if (!security.hasEntityPermission(secEntity, secOperation, userLogin)) { > > This function is defined in > framework/security/src/org/ofbiz/security/OFBizSecurity.java. > > It checks for group permissions but I don’t know enough to tell > whether it does or does not catch this. > > Regards, > > Vinay Agarwal > > -----Original Message----- > From: [hidden email] > [mailto:[hidden email]] On Behalf Of bjfree > Sent: Friday, February 10, 2006 9:39 AM > To: OFBiz Users / Usage Discussion > Subject: Re: [OFBiz] Users - Hidden partyId - Security Risk? > > both of those are ofbiz Id's without the corresponding data, in the > > database, this is useless information. > > if some tried to post this to ofbiz, they would run into the > > certificate, then User privileges, the Security before any information > > would be revealed about the cc. > > The information, since ver 3.1, is encrypted in the DB so it would be > > difficult, or impossible to retrieve such data. > > I believe it would pass a security audit. > > Vinay Agarwal sent the following on 2/10/06 8:23 AM: > >> Another similar case in > >> applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl which > >> contains > >> > >> <input type="hidden" name="paymentMethodId" > >> value="${paymentMethodId}"> > >> > >> And this application is designed for public use. What am I missing here? > >> > >> > >> > >> Regards, > >> > >> Vinay Agarwal > >> > >> > >> > >> -----Original Message----- > >> From: Vinay Agarwal [mailto:[hidden email]] > >> Sent: Friday, February 10, 2006 8:17 AM > >> To: 'OFBiz Users / Usage Discussion' > >> Subject: Hidden partyId - Security Risk? > >> > >> > >> > >> Hello, > >> > >> > >> > >> While going through credit card entry ftl's, I came across > >> applications/party/webapp/partymgr/party/editcreditcard.ftl which > contains > >> the following line > >> > >> <input type="hidden" name="partyId" value="${partyId}"/> > >> > >> I could be missing something here, but it sure looks like a security > risk to > >> me. Granted that this ftl is probably designed to be used only for Party > >> Manager part of Webtools and not for a "public" application, but even > that > >> is not a good thing from code reuse point of view. > >> > >> > >> > >> Regards, > >> > >> Vinay Agarwal > >> > >> > >> > >> > >> ------------------------------------------------------------------------ > >> > >> > >> _______________________________________________ > >> Users mailing list > >> [hidden email] > >> http://lists.ofbiz.org/mailman/listinfo/users > > _______________________________________________ > > Users mailing list > > [hidden email] > > http://lists.ofbiz.org/mailman/listinfo/users > >------------------------------------------------------------------------ > > >_______________________________________________ >Users mailing list >[hidden email] >http://lists.ofbiz.org/mailman/listinfo/users > _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
In reply to this post by Vinay Agarwal
Vinay,
That's a fascinating scenario. My gut response is that this wouldn't work, but like you, I can't be sure. It's certainly worth finding a definitive answer! -- Kind Regards Andrew Sykes <[hidden email]> Sykes Development Ltd http://www.sykesdevelopment.com _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
My testing did find problem with hidden paymentMethodId field that I am describing below. In addition, there are 240 other ftl files that contain hidden fields and may pose security risk although I have not looked at anyone else.
File: applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl Statement: <input type="hidden" name="paymentMethodId" value="${paymentMethodId}"> Theory: A hacked form may change the paymentMethodId and modify data that the user does not have authorization for
Method:
Conclusion: A user is able to modify data that he is not authorized for.
I would like to know if you can reproduce it. I can add it to Jira if needed.
Regards, Vinay Agarwal
_______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
In reply to this post by Vinay Agarwal
Vinay,
My hunch is that: ServiceUtil.getPartyIdCheckSecurity(userLogin, security, context, result, "PAY_INFO", "_UPDATE"); is not enough. This checks if the partyId of the userLogin is the partyId in the context or if the userLogin has PAY_INFO_UPDATE. I think the security check there should be: 1. Does userLogin have PAY_INF_UPDATE? Yes -> good. This can be done with hasEntityPermission 2. If not (1), is the userLogin the partyId in the context AND the partyId of the PaymentMethod? Yes -> good. We may be missing the second part here... Want to try it? :) Si Vinay Agarwal wrote: > My testing did find problem with hidden paymentMethodId field that I > am describing below. In addition, there are 240 other ftl files that > contain hidden fields and may pose security risk although I have not > looked at anyone else. > > File: applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl > > Statement: <input type="hidden" name="paymentMethodId" > value="${paymentMethodId}"> > > Theory: > > A hacked form may change the paymentMethodId and modify data that the > user does not have authorization for > > Method: > > 1. Ecommerce application, signed up as “firstuser” and added a > credit card. Its paymentMethodId came out to be 10000. > 2. Logged out and signed up as “seconduser” and added a credit > card. Its paymentMethodId came out to be 10001. > 3. Logged in as seconduser, clicked on update credit card. Saved > the html page locally. > 4. Edited the saved html page > 1. Changed paymentMethodId from 10001 to 10000. > 2. Added http://localhost:8443 <http://localhost:8443/> to > the action url. > 5. Expected result: firstuser and seconduser each has one credit card. > 6. Actual result: firstuser had no card and the second user had 2 > cards as seen on the profile page. > > Conclusion: > > A user is able to modify data that he is not authorized for. > > I would like to know if you can reproduce it. I can add it to Jira if > needed. > > Regards, > > Vinay Agarwal > >------------------------------------------------------------------------ > > >_______________________________________________ >Users mailing list >[hidden email] >http://lists.ofbiz.org/mailman/listinfo/users > _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
I think there is a general security risk with storing database keys, or any
"important" information, in form fields, hidden or not. Overall security is much easier to achieve by not giving user access to this data than to try to contain the contaminant input. In other words follow these two rules: 1. Not store any important information in user forms, instead save them in session. 2. Save only the data in hidden user form that cannot impact security if changed, e.g., sequence page id for multi-page data entry forms. So, I would simply recommend converting all 241 or so potentially unsafe ftl's and corresponding event handlers to use session and instead of hidden fields. Regards, Vinay Agarwal -----Original Message----- From: [hidden email] [mailto:[hidden email]] On Behalf Of Si Chen Sent: Friday, February 10, 2006 4:10 PM To: OFBiz Users / Usage Discussion Subject: Re: [OFBiz] Users - Hidden partyId - Security Risk? Vinay, My hunch is that: ServiceUtil.getPartyIdCheckSecurity(userLogin, security, context, result, "PAY_INFO", "_UPDATE"); is not enough. This checks if the partyId of the userLogin is the partyId in the context or if the userLogin has PAY_INFO_UPDATE. I think the security check there should be: 1. Does userLogin have PAY_INF_UPDATE? Yes -> good. This can be done with hasEntityPermission 2. If not (1), is the userLogin the partyId in the context AND the partyId of the PaymentMethod? Yes -> good. We may be missing the second part here... Want to try it? :) Si Vinay Agarwal wrote: > My testing did find problem with hidden paymentMethodId field that I > am describing below. In addition, there are 240 other ftl files that > contain hidden fields and may pose security risk although I have not > looked at anyone else. > > File: applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl > > Statement: <input type="hidden" name="paymentMethodId" > value="${paymentMethodId}"> > > Theory: > > A hacked form may change the paymentMethodId and modify data that the > user does not have authorization for > > Method: > > 1. Ecommerce application, signed up as "firstuser" and added a > credit card. Its paymentMethodId came out to be 10000. > 2. Logged out and signed up as "seconduser" and added a credit > card. Its paymentMethodId came out to be 10001. > 3. Logged in as seconduser, clicked on update credit card. Saved > the html page locally. > 4. Edited the saved html page > 1. Changed paymentMethodId from 10001 to 10000. > 2. Added http://localhost:8443 <http://localhost:8443/> to > the action url. > 5. Expected result: firstuser and seconduser each has one credit card. > 6. Actual result: firstuser had no card and the second user had 2 > cards as seen on the profile page. > > Conclusion: > > A user is able to modify data that he is not authorized for. > > I would like to know if you can reproduce it. I can add it to Jira if > needed. > > Regards, > > Vinay Agarwal > >------------------------------------------------------------------------ > > >_______________________________________________ >Users mailing list >[hidden email] >http://lists.ofbiz.org/mailman/listinfo/users > _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
In reply to this post by Vinay Agarwal
I agree, but the other fix is also necessary: what if somebody found
some other way to access the system, ie via Web services? Want to create a couple of JIRA issues for these, maybe post your comments and mine? Si Vinay Agarwal wrote: >I think there is a general security risk with storing database keys, or any >"important" information, in form fields, hidden or not. Overall security is >much easier to achieve by not giving user access to this data than to try to >contain the contaminant input. In other words follow these two rules: > >1. Not store any important information in user forms, instead save them in >session. >2. Save only the data in hidden user form that cannot impact security if >changed, e.g., sequence page id for multi-page data entry forms. > >So, I would simply recommend converting all 241 or so potentially unsafe >ftl's and corresponding event handlers to use session and instead of hidden >fields. > >Regards, >Vinay Agarwal > > >-----Original Message----- >From: [hidden email] [mailto:[hidden email]] >On Behalf Of Si Chen >Sent: Friday, February 10, 2006 4:10 PM >To: OFBiz Users / Usage Discussion >Subject: Re: [OFBiz] Users - Hidden partyId - Security Risk? > >Vinay, > >My hunch is that: >ServiceUtil.getPartyIdCheckSecurity(userLogin, security, context, >result, "PAY_INFO", "_UPDATE"); >is not enough. This checks if the partyId of the userLogin is the >partyId in the context or if the userLogin has PAY_INFO_UPDATE. > >I think the security check there should be: >1. Does userLogin have PAY_INF_UPDATE? Yes -> good. This can be done >with hasEntityPermission >2. If not (1), is the userLogin the partyId in the context AND the >partyId of the PaymentMethod? Yes -> good. > >We may be missing the second part here... > >Want to try it? :) > >Si > >Vinay Agarwal wrote: > > > >>My testing did find problem with hidden paymentMethodId field that I >>am describing below. In addition, there are 240 other ftl files that >>contain hidden fields and may pose security risk although I have not >>looked at anyone else. >> >>File: applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl >> >>Statement: <input type="hidden" name="paymentMethodId" >>value="${paymentMethodId}"> >> >>Theory: >> >>A hacked form may change the paymentMethodId and modify data that the >>user does not have authorization for >> >>Method: >> >> 1. Ecommerce application, signed up as "firstuser" and added a >> credit card. Its paymentMethodId came out to be 10000. >> 2. Logged out and signed up as "seconduser" and added a credit >> card. Its paymentMethodId came out to be 10001. >> 3. Logged in as seconduser, clicked on update credit card. Saved >> the html page locally. >> 4. Edited the saved html page >> 1. Changed paymentMethodId from 10001 to 10000. >> 2. Added http://localhost:8443 <http://localhost:8443/> to >> the action url. >> 5. Expected result: firstuser and seconduser each has one credit card. >> 6. Actual result: firstuser had no card and the second user had 2 >> cards as seen on the profile page. >> >>Conclusion: >> >>A user is able to modify data that he is not authorized for. >> >>I would like to know if you can reproduce it. I can add it to Jira if >>needed. >> >>Regards, >> >>Vinay Agarwal >> >>------------------------------------------------------------------------ >> >> >>_______________________________________________ >>Users mailing list >>[hidden email] >>http://lists.ofbiz.org/mailman/listinfo/users >> >> >> > >_______________________________________________ >Users mailing list >[hidden email] >http://lists.ofbiz.org/mailman/listinfo/users > >_______________________________________________ >Users mailing list >[hidden email] >http://lists.ofbiz.org/mailman/listinfo/users > > > _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
In reply to this post by Vinay Agarwal
I VERY VERY VERY HIGHLY disagree with this approach... It is unreliable for security and putting things in the session causes all sort of flow problems in webapps. It is hard to avoid putting IDs in web pages or URLs and is generally a safe thing to do as long as the security for both displaying and changing data is sufficient, so that is what should be focused on... The only way to properly control security of changes like this is to check things properly in the service implementations that process the incoming data. It sounds like in your example that it is not checking to make sure that if the user has no special permission to make the changes that the partyId on the userLogin matches that on the record being changed, which should be considered a bug and the fix is to change that service to check this... -David On Feb 10, 2006, at 5:46 PM, Vinay Agarwal wrote: > I think there is a general security risk with storing database > keys, or any > "important" information, in form fields, hidden or not. Overall > security is > much easier to achieve by not giving user access to this data than > to try to > contain the contaminant input. In other words follow these two rules: > > 1. Not store any important information in user forms, instead save > them in > session. > 2. Save only the data in hidden user form that cannot impact > security if > changed, e.g., sequence page id for multi-page data entry forms. > > So, I would simply recommend converting all 241 or so potentially > unsafe > ftl's and corresponding event handlers to use session and instead > of hidden > fields. > > Regards, > Vinay Agarwal > > > -----Original Message----- > From: [hidden email] [mailto:users- > [hidden email]] > On Behalf Of Si Chen > Sent: Friday, February 10, 2006 4:10 PM > To: OFBiz Users / Usage Discussion > Subject: Re: [OFBiz] Users - Hidden partyId - Security Risk? > > Vinay, > > My hunch is that: > ServiceUtil.getPartyIdCheckSecurity(userLogin, security, context, > result, "PAY_INFO", "_UPDATE"); > is not enough. This checks if the partyId of the userLogin is the > partyId in the context or if the userLogin has PAY_INFO_UPDATE. > > I think the security check there should be: > 1. Does userLogin have PAY_INF_UPDATE? Yes -> good. This can be done > with hasEntityPermission > 2. If not (1), is the userLogin the partyId in the context AND the > partyId of the PaymentMethod? Yes -> good. > > We may be missing the second part here... > > Want to try it? :) > > Si > > Vinay Agarwal wrote: > >> My testing did find problem with hidden paymentMethodId field that I >> am describing below. In addition, there are 240 other ftl files that >> contain hidden fields and may pose security risk although I have not >> looked at anyone else. >> >> File: applications/ecommerce/webapp/ecommerce/customer/ >> editcreditcard.ftl >> >> Statement: <input type="hidden" name="paymentMethodId" >> value="${paymentMethodId}"> >> >> Theory: >> >> A hacked form may change the paymentMethodId and modify data that the >> user does not have authorization for >> >> Method: >> >> 1. Ecommerce application, signed up as "firstuser" and added a >> credit card. Its paymentMethodId came out to be 10000. >> 2. Logged out and signed up as "seconduser" and added a credit >> card. Its paymentMethodId came out to be 10001. >> 3. Logged in as seconduser, clicked on update credit card. Saved >> the html page locally. >> 4. Edited the saved html page >> 1. Changed paymentMethodId from 10001 to 10000. >> 2. Added http://localhost:8443 <http://localhost:8443/> to >> the action url. >> 5. Expected result: firstuser and seconduser each has one >> credit card. >> 6. Actual result: firstuser had no card and the second user had 2 >> cards as seen on the profile page. >> >> Conclusion: >> >> A user is able to modify data that he is not authorized for. >> >> I would like to know if you can reproduce it. I can add it to Jira if >> needed. >> >> Regards, >> >> Vinay Agarwal >> >> --------------------------------------------------------------------- >> --- >> >> >> _______________________________________________ >> Users mailing list >> [hidden email] >> http://lists.ofbiz.org/mailman/listinfo/users >> > > _______________________________________________ > Users mailing list > [hidden email] > http://lists.ofbiz.org/mailman/listinfo/users > > _______________________________________________ > Users mailing list > [hidden email] > http://lists.ofbiz.org/mailman/listinfo/users _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users smime.p7s (3K) Download Attachment |
I agree with David. I see user ID parameters in URLs in many different websites,
so it's not uncommon to handle sensitive data that way. The services MUST check the parameters to make sure they are valid. OFBiz has many security checks for the most part it is secure. If there is a hole somewhere, then yes, we need to fix it. But, like David, I don't think we need to re-write the entire application suite. I've tried hacking into our OFBiz modifications by just changing parameters in the URLs. As long as there is plenty of parameter checking going on at the service level, our data remains secure. David E. Jones wrote: > > I VERY VERY VERY HIGHLY disagree with this approach... It is unreliable > for security and putting things in the session causes all sort of flow > problems in webapps. It is hard to avoid putting IDs in web pages or > URLs and is generally a safe thing to do as long as the security for > both displaying and changing data is sufficient, so that is what should > be focused on... > > The only way to properly control security of changes like this is to > check things properly in the service implementations that process the > incoming data. > > It sounds like in your example that it is not checking to make sure > that if the user has no special permission to make the changes that the > partyId on the userLogin matches that on the record being changed, > which should be considered a bug and the fix is to change that service > to check this... > > -David > > > On Feb 10, 2006, at 5:46 PM, Vinay Agarwal wrote: > >> I think there is a general security risk with storing database keys, >> or any >> "important" information, in form fields, hidden or not. Overall >> security is >> much easier to achieve by not giving user access to this data than to >> try to >> contain the contaminant input. In other words follow these two rules: >> >> 1. Not store any important information in user forms, instead save >> them in >> session. >> 2. Save only the data in hidden user form that cannot impact security if >> changed, e.g., sequence page id for multi-page data entry forms. >> >> So, I would simply recommend converting all 241 or so potentially unsafe >> ftl's and corresponding event handlers to use session and instead of >> hidden >> fields. >> >> Regards, >> Vinay Agarwal >> >> >> -----Original Message----- >> From: [hidden email] [mailto:users- >> [hidden email]] >> On Behalf Of Si Chen >> Sent: Friday, February 10, 2006 4:10 PM >> To: OFBiz Users / Usage Discussion >> Subject: Re: [OFBiz] Users - Hidden partyId - Security Risk? >> >> Vinay, >> >> My hunch is that: >> ServiceUtil.getPartyIdCheckSecurity(userLogin, security, context, >> result, "PAY_INFO", "_UPDATE"); >> is not enough. This checks if the partyId of the userLogin is the >> partyId in the context or if the userLogin has PAY_INFO_UPDATE. >> >> I think the security check there should be: >> 1. Does userLogin have PAY_INF_UPDATE? Yes -> good. This can be done >> with hasEntityPermission >> 2. If not (1), is the userLogin the partyId in the context AND the >> partyId of the PaymentMethod? Yes -> good. >> >> We may be missing the second part here... >> >> Want to try it? :) >> >> Si >> >> Vinay Agarwal wrote: >> >>> My testing did find problem with hidden paymentMethodId field that I >>> am describing below. In addition, there are 240 other ftl files that >>> contain hidden fields and may pose security risk although I have not >>> looked at anyone else. >>> >>> File: applications/ecommerce/webapp/ecommerce/customer/ >>> editcreditcard.ftl >>> >>> Statement: <input type="hidden" name="paymentMethodId" >>> value="${paymentMethodId}"> >>> >>> Theory: >>> >>> A hacked form may change the paymentMethodId and modify data that the >>> user does not have authorization for >>> >>> Method: >>> >>> 1. Ecommerce application, signed up as "firstuser" and added a >>> credit card. Its paymentMethodId came out to be 10000. >>> 2. Logged out and signed up as "seconduser" and added a credit >>> card. Its paymentMethodId came out to be 10001. >>> 3. Logged in as seconduser, clicked on update credit card. Saved >>> the html page locally. >>> 4. Edited the saved html page >>> 1. Changed paymentMethodId from 10001 to 10000. >>> 2. Added http://localhost:8443 <http://localhost:8443/> to >>> the action url. >>> 5. Expected result: firstuser and seconduser each has one credit >>> card. >>> 6. Actual result: firstuser had no card and the second user had 2 >>> cards as seen on the profile page. >>> >>> Conclusion: >>> >>> A user is able to modify data that he is not authorized for. >>> >>> I would like to know if you can reproduce it. I can add it to Jira if >>> needed. >>> >>> Regards, >>> >>> Vinay Agarwal >>> >>> --------------------------------------------------------------------- >>> --- >>> >>> >>> _______________________________________________ >>> Users mailing list >>> [hidden email] >>> http://lists.ofbiz.org/mailman/listinfo/users >>> >> >> _______________________________________________ >> Users mailing list >> [hidden email] >> http://lists.ofbiz.org/mailman/listinfo/users >> >> _______________________________________________ >> Users mailing list >> [hidden email] >> http://lists.ofbiz.org/mailman/listinfo/users > > > > ------------------------------------------------------------------------ > > > _______________________________________________ > Users mailing list > [hidden email] > http://lists.ofbiz.org/mailman/listinfo/users _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
In reply to this post by Si Chen-2
I was very concerned about issues like these when we started down this path so we decided to put a security proxy between the firewall and the ofbiz server (http://www.modsecurity.org/index.php). mod_security will catch remote form posts and many other issues. I don't have it working properly yet but I will be glad to add the config and documentation to the wiki when it is done. SC Si Chen wrote: >I agree, but the other fix is also necessary: what if somebody found >some other way to access the system, ie via Web services? > >Want to create a couple of JIRA issues for these, maybe post your >comments and mine? > >Si > >Vinay Agarwal wrote: > > > >>I think there is a general security risk with storing database keys, or any >>"important" information, in form fields, hidden or not. Overall security is >>much easier to achieve by not giving user access to this data than to try to >>contain the contaminant input. In other words follow these two rules: >> >>1. Not store any important information in user forms, instead save them in >>session. >>2. Save only the data in hidden user form that cannot impact security if >>changed, e.g., sequence page id for multi-page data entry forms. >> >>So, I would simply recommend converting all 241 or so potentially unsafe >>ftl's and corresponding event handlers to use session and instead of hidden >>fields. >> >>Regards, >>Vinay Agarwal >> >> >>-----Original Message----- >>From: [hidden email] [mailto:[hidden email]] >>On Behalf Of Si Chen >>Sent: Friday, February 10, 2006 4:10 PM >>To: OFBiz Users / Usage Discussion >>Subject: Re: [OFBiz] Users - Hidden partyId - Security Risk? >> >>Vinay, >> >>My hunch is that: >>ServiceUtil.getPartyIdCheckSecurity(userLogin, security, context, >>result, "PAY_INFO", "_UPDATE"); >>is not enough. This checks if the partyId of the userLogin is the >>partyId in the context or if the userLogin has PAY_INFO_UPDATE. >> >>I think the security check there should be: >>1. Does userLogin have PAY_INF_UPDATE? Yes -> good. This can be done >>with hasEntityPermission >>2. If not (1), is the userLogin the partyId in the context AND the >>partyId of the PaymentMethod? Yes -> good. >> >>We may be missing the second part here... >> >>Want to try it? :) >> >>Si >> >>Vinay Agarwal wrote: >> >> >> >> >> >>>My testing did find problem with hidden paymentMethodId field that I >>>am describing below. In addition, there are 240 other ftl files that >>>contain hidden fields and may pose security risk although I have not >>>looked at anyone else. >>> >>>File: applications/ecommerce/webapp/ecommerce/customer/editcreditcard.ftl >>> >>>Statement: <input type="hidden" name="paymentMethodId" >>>value="${paymentMethodId}"> >>> >>>Theory: >>> >>>A hacked form may change the paymentMethodId and modify data that the >>>user does not have authorization for >>> >>>Method: >>> >>> 1. Ecommerce application, signed up as "firstuser" and added a >>> credit card. Its paymentMethodId came out to be 10000. >>> 2. Logged out and signed up as "seconduser" and added a credit >>> card. Its paymentMethodId came out to be 10001. >>> 3. Logged in as seconduser, clicked on update credit card. Saved >>> the html page locally. >>> 4. Edited the saved html page >>> 1. Changed paymentMethodId from 10001 to 10000. >>> 2. Added http://localhost:8443 <http://localhost:8443/> to >>> the action url. >>> 5. Expected result: firstuser and seconduser each has one credit card. >>> 6. Actual result: firstuser had no card and the second user had 2 >>> cards as seen on the profile page. >>> >>>Conclusion: >>> >>>A user is able to modify data that he is not authorized for. >>> >>>I would like to know if you can reproduce it. I can add it to Jira if >>>needed. >>> >>>Regards, >>> >>>Vinay Agarwal >>> >>>------------------------------------------------------------------------ >>> >>> >>>_______________________________________________ >>>Users mailing list >>>[hidden email] >>>http://lists.ofbiz.org/mailman/listinfo/users >>> >>> >>> >>> >>> >>_______________________________________________ >>Users mailing list >>[hidden email] >>http://lists.ofbiz.org/mailman/listinfo/users >> >>_______________________________________________ >>Users mailing list >>[hidden email] >>http://lists.ofbiz.org/mailman/listinfo/users >> >> >> >> >> > >_______________________________________________ >Users mailing list >[hidden email] >http://lists.ofbiz.org/mailman/listinfo/users > > _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
In reply to this post by Adrian Crum
I agree with all of you that all parameters must be checked to ensure
application security. What I am afraid of is that this may not be an isolated issue. There may be other places that contain similar errors and it may be repeated in future. In order to determine the extent of the issues, I started looking at other ftl's that have hidden field in them. There are about 240 ftl's like that. I wasn't even able to rule out that the problem did not exist in the very first ftl one from the search. Its details are below. In both these cases, security check functions are being called but they don't seem to check for the data that will be used for the operation. Should we just expand the security check to include the data that will be used? It seems to me that may solve a large number of potential issues. Regards, Vinay Agarwal File: applications/accounting/webapp/accounting/billingaccount/EditBillingAccount. ftl It has these lines right on the top <form name="billingform" method="post" action="<@ofbizUrl>updateBillingAccount</@ofbizUrl>"> <input type="hidden" name="billingAccountId" value="${billingAccount.billingAccountId}"> Looking at the updateBillingAccount request in its controller.xml, it calls <check-permission permission="ACCOUNTING" action="_UPDATE"><fail-property resource="AccountingUiLabels" property="AccountingUpdateBillingAccountPermissionError"/></check-permission > But I don't think it checks for the billingAccountId. I wasn't able to test because I couldn't find a way to create a billing account from unprivileged account. -----Original Message----- From: [hidden email] [mailto:[hidden email]] On Behalf Of Adrian Crum Sent: Friday, February 10, 2006 5:30 PM To: OFBiz Users / Usage Discussion Subject: Re: [OFBiz] Users - Hidden partyId - Security Risk? I agree with David. I see user ID parameters in URLs in many different websites, so it's not uncommon to handle sensitive data that way. The services MUST check the parameters to make sure they are valid. OFBiz has many security checks for the most part it is secure. If there is a hole somewhere, then yes, we need to fix it. But, like David, I don't think we need to re-write the entire application suite. I've tried hacking into our OFBiz modifications by just changing parameters in the URLs. As long as there is plenty of parameter checking going on at the service level, our data remains secure. David E. Jones wrote: > > I VERY VERY VERY HIGHLY disagree with this approach... It is unreliable > for security and putting things in the session causes all sort of flow > problems in webapps. It is hard to avoid putting IDs in web pages or > URLs and is generally a safe thing to do as long as the security for > both displaying and changing data is sufficient, so that is what should > be focused on... > > The only way to properly control security of changes like this is to > check things properly in the service implementations that process the > incoming data. > > It sounds like in your example that it is not checking to make sure > that if the user has no special permission to make the changes that the > partyId on the userLogin matches that on the record being changed, > which should be considered a bug and the fix is to change that service > to check this... > > -David > > > On Feb 10, 2006, at 5:46 PM, Vinay Agarwal wrote: > >> I think there is a general security risk with storing database keys, >> or any >> "important" information, in form fields, hidden or not. Overall >> security is >> much easier to achieve by not giving user access to this data than to >> try to >> contain the contaminant input. In other words follow these two rules: >> >> 1. Not store any important information in user forms, instead save >> them in >> session. >> 2. Save only the data in hidden user form that cannot impact security if >> changed, e.g., sequence page id for multi-page data entry forms. >> >> So, I would simply recommend converting all 241 or so potentially unsafe >> ftl's and corresponding event handlers to use session and instead of >> hidden >> fields. >> >> Regards, >> Vinay Agarwal >> >> >> -----Original Message----- >> From: [hidden email] [mailto:users- >> [hidden email]] >> On Behalf Of Si Chen >> Sent: Friday, February 10, 2006 4:10 PM >> To: OFBiz Users / Usage Discussion >> Subject: Re: [OFBiz] Users - Hidden partyId - Security Risk? >> >> Vinay, >> >> My hunch is that: >> ServiceUtil.getPartyIdCheckSecurity(userLogin, security, context, >> result, "PAY_INFO", "_UPDATE"); >> is not enough. This checks if the partyId of the userLogin is the >> partyId in the context or if the userLogin has PAY_INFO_UPDATE. >> >> I think the security check there should be: >> 1. Does userLogin have PAY_INF_UPDATE? Yes -> good. This can be done >> with hasEntityPermission >> 2. If not (1), is the userLogin the partyId in the context AND the >> partyId of the PaymentMethod? Yes -> good. >> >> We may be missing the second part here... >> >> Want to try it? :) >> >> Si >> _______________________________________________ Users mailing list [hidden email] http://lists.ofbiz.org/mailman/listinfo/users |
Free forum by Nabble | Edit this page |