Posted by
Vinay Agarwal on
URL: http://ofbiz.116.s1.nabble.com/Users-Hidden-partyId-Security-Risk-tp137366p137369.html
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