OFBiz
  1. OFBiz
  2. OFBIZ-1592

Database spikes lead to permanent user privilege loss

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: Trunk
    • Component/s: framework
    • Labels:
      None

      Description

      We found a critical bug in OFBiz security where temporary database spikes can lead to permanent privilege loss for users trying to log in or do something during the spike. The loss lasts until a cache refresh or a restart. A symptom is customers not being able to log in to do a checkout, not being able to create new accounts, and backend users not being able to perform their duties due to privilege loss.

      The reason for the bug was found to be in the caching of UserLoginSecurityGroup in OFBizSecurity. When there's an SQL exception, such as during a lag spike, an empty list will be stored in the cache. Subsequent security checks will retrieve this empty list and never ask the database again what the actual security groups are.

      1. OFBizSecurity.patch
        1 kB
        Adrian Crum
      2. permanent-security-loss.patch
        2 kB
        Leon Torres

        Activity

        Hide
        Leon Torres added a comment -

        This patch is known to fix the issue completely.

        Show
        Leon Torres added a comment - This patch is known to fix the issue completely.
        Hide
        Si Chen added a comment -

        If there are no objections I will commit it.

        Show
        Si Chen added a comment - If there are no objections I will commit it.
        Hide
        Adrian Crum added a comment -

        I think the patch needs more work. At first glance it appears that there will be more DB hits for users who aren't in security groups.

        Show
        Adrian Crum added a comment - I think the patch needs more work. At first glance it appears that there will be more DB hits for users who aren't in security groups.
        Hide
        Si Chen added a comment -

        Why do you think so?

        Show
        Si Chen added a comment - Why do you think so?
        Hide
        Leon Torres added a comment -

        Trying to avoid database hits lead to the problem in the first place. We should rely on the database's native caching ability.

        Show
        Leon Torres added a comment - Trying to avoid database hits lead to the problem in the first place. We should rely on the database's native caching ability.
        Hide
        Leon Torres added a comment -

        Also note if the user doesn't have any security groups, an empty list is returned and cached. So it avoids DB hits for the case you stated Adrian.

        Show
        Leon Torres added a comment - Also note if the user doesn't have any security groups, an empty list is returned and cached. So it avoids DB hits for the case you stated Adrian.
        Hide
        Jacopo Cappellato added a comment -

        Can we move this discussion to the dev list?

        Jacopo

        Show
        Jacopo Cappellato added a comment - Can we move this discussion to the dev list? Jacopo
        Hide
        Adrian Crum added a comment -

        Leon,

        Read your comment in the patch: "// only store in cache if we get something" - so if a user isn't a member of a security group, a DB hit will occur every time that user's permissions are checked.

        Show
        Adrian Crum added a comment - Leon, Read your comment in the patch: "// only store in cache if we get something" - so if a user isn't a member of a security group, a DB hit will occur every time that user's permissions are checked.
        Hide
        Adrian Crum added a comment -

        Si & Leon - take a look at OFBizSecurity.patch.

        Show
        Adrian Crum added a comment - Si & Leon - take a look at OFBizSecurity.patch.
        Hide
        Leon Torres added a comment -

        Sorry Adrian, that patch you proposed does exactly the same thing. Can someone else review it?

        I'm not on the dev list at the moment, further comments should be on this issue.

        Show
        Leon Torres added a comment - Sorry Adrian, that patch you proposed does exactly the same thing. Can someone else review it? I'm not on the dev list at the moment, further comments should be on this issue.
        Hide
        Adrian Crum added a comment -

        No, it doesn't do the same thing. Look again.

        Show
        Adrian Crum added a comment - No, it doesn't do the same thing. Look again.
        Hide
        Jacopo Cappellato added a comment - - edited

        Hi Leon,

        here is the address to subscribe to the dev list:

        dev-subscribe@ofbiz.apache.org

        Jacopo

        Show
        Jacopo Cappellato added a comment - - edited Hi Leon, here is the address to subscribe to the dev list: dev-subscribe@ofbiz.apache.org Jacopo
        Hide
        David E. Jones added a comment -

        I agree that we shouldn't be caching an empty list when there is an error. I don't agree that we should never cache an empty list, that would have pretty annoying performance impact.

        I've committed a variation of Adrian's patch in rev 615722 in the trunk and in the release4.0 branch, well, there I got a conflict.

        Show
        David E. Jones added a comment - I agree that we shouldn't be caching an empty list when there is an error. I don't agree that we should never cache an empty list, that would have pretty annoying performance impact. I've committed a variation of Adrian's patch in rev 615722 in the trunk and in the release4.0 branch, well, there I got a conflict.

          People

          • Assignee:
            David E. Jones
            Reporter:
            Leon Torres
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development